Re: [dpdk-dev] [PATCH 2/2] virtio: change io privilege level as early as possible

[Neil Horman <nhorman@tuxdriver.com>]

On Mon, Mar 09, 2015 at 03:56:39PM +0100, David Marchand wrote:
> Playing with virtio link status triggers a segfault because of an incorrect io
> privilege level.
> 
> To reproduce the problem, virtio device must be bound to igb_uio to have lsc
> enabled.
> 
> $ lspci |grep Ethernet
> 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> $ modprobe uio
> $ insmod ./x86_64-native-linuxapp-gcc/kmod/igb_uio.ko
> $ echo 0000:00:03.0 > /sys/bus/pci/devices/0000\:00\:03.0/driver/unbind
> $ echo 1af4 1000 > /sys/bus/pci/drivers/igb_uio/new_id
> $ ./x86_64-native-linuxapp-gcc/app/testpmd -c 0x6 -n 3 -w 0000:00:03.0 -- -i --txqflags=0xf01
> [snip]
> EAL: PCI device 0000:00:03.0 on NUMA socket -1
> EAL:   probe driver: 1af4:1000 rte_virtio_pmd
> Interactive-mode selected
> Configuring Port 0 (socket 0)
> Port 0: DE:AD:DE:01:02:03
> Checking link statuses...
> Port 0 Link Up - speed 10000 Mbps - full-duplex
> Done
> testpmd>
> 
> Then, from qemu monitor:
> (qemu) set_link virtio-net-pci.0 off
> 
> testpmd> Segmentation fault
> 
> A call to rte_eal_iopl_init() in a specific constructor has been added so that,
> on Linux, iopl() is called asap:
> - for statically linked virtio pmd, the constructor will be called at binary
>   start,
> - for shared virtio pmd (loaded using -d eal option), it will be called at
>   dlopen() in rte_eal_init().
> 
> On linux, iopl() effects are inherited by children threads, so calling it in a
> constructor will ensure that any thread / process created after rte_eal_init()
> inherits the io privilege level needed by the pmd.
> 
> We keep call to rte_eal_iopl_init() in the pmd init macro so that the pmd will
> not register if the io privilege level is insufficient.
> 
> Besides, I moved rte_virtio_pmd_init() and related structure near its calling
> site, so that all init code can be looked at in a glance.
> The only change here is a new comment.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_pmd_virtio/virtio_ethdev.c |   71 ++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
> index d239083..29332fa 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -1228,34 +1228,6 @@ eth_virtio_dev_init(__rte_unused struct eth_driver *eth_drv,
>  	return 0;
>  }
>  
> -static struct eth_driver rte_virtio_pmd = {
> -	{
> -		.name = "rte_virtio_pmd",
> -		.id_table = pci_id_virtio_map,
> -	},
> -	.eth_dev_init = eth_virtio_dev_init,
> -	.dev_private_size = sizeof(struct virtio_hw),
> -};
> -
> -/*
> - * Driver initialization routine.
> - * Invoked once at EAL init time.
> - * Register itself as the [Poll Mode] Driver of PCI virtio devices.
> - * Returns 0 on success.
> - */
> -static int
> -rte_virtio_pmd_init(const char *name __rte_unused,
> -		    const char *param __rte_unused)
> -{
> -	if (rte_eal_iopl_init() != 0) {
> -		PMD_INIT_LOG(ERR, "IOPL call failed - cannot use virtio PMD");
> -		return -1;
> -	}
> -
> -	rte_eth_driver_register(&rte_virtio_pmd);
> -	return 0;
> -}
> -
>  /*
>   * Only 1 queue is supported, no queue release related operation
>   */
> @@ -1484,6 +1456,49 @@ __rte_unused uint8_t is_rx)
>  	return 0;
>  }
>  
> +/*
> + * Early init function, this is needed so that iopl() on linux is done before
> + * rte_eal_init (since it creates other threads that must inherit from the one
> + * that calls rte_eal_init).
> + */
> +static void __attribute__((constructor, used))
> +rte_virtio_early_init(void)
> +{
> +	/* return value is checked at pmd init time, no need to check here, see
> +	 * below. */
> +	rte_eal_iopl_init();
> +}
> +

I don't see how this works for all cases.  The constructor is called once when
the library is first loaded.  What if you have multiple independent (i.e. not
forked children) processes that are using the dpdk in parallel?  Only the
process that triggered the library load will have io permissions set
appropriately.  I think what you need is to have every application that expects
to call through the transmit path or poll the receive path call iopl, which I
think speaks to having this requirement documented, so each application can call
iopl prior to calling fork/daemonize/etc.

Neil