From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mail-pl0-f68.google.com ([209.85.160.68]:46232 "EHLO
        mail-pl0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752079AbeBTRCA (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 20 Feb 2018 12:02:00 -0500
Received: by mail-pl0-f68.google.com with SMTP id x19so7731277plr.13
        for <netdev@vger.kernel.org>; Tue, 20 Feb 2018 09:01:59 -0800 (PST)
Subject: Re: [net PATCH 4/4] virtio_net: fix ndo_xdp_xmit crash towards dev
 not ready for XDP
To: Jesper Dangaard Brouer <brouer@redhat.com>,
        Jason Wang <jasowang@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>, netdev@vger.kernel.org,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Saeed Mahameed <saeedm@mellanox.com>,
        Daniel Borkmann <borkmann@iogearbox.net>,
        "David S. Miller" <davem@davemloft.net>,
        Tariq Toukan <tariqt@mellanox.com>
References: <151913348634.28247.17519468037896960567.stgit@firesoul>
 <151913354012.28247.1462800242440559823.stgit@firesoul>
From: John Fastabend <john.fastabend@gmail.com>
Message-ID: <d427b64f-062b-809b-cb74-a1005fe4029b@gmail.com>
Date: Tue, 20 Feb 2018 09:01:56 -0800
MIME-Version: 1.0
In-Reply-To: <151913354012.28247.1462800242440559823.stgit@firesoul>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 02/20/2018 05:32 AM, Jesper Dangaard Brouer wrote:
> When a driver implements the ndo_xdp_xmit() function, there is
> (currently) no generic way to determine whether it is safe to call.
> 
> It is e.g. unsafe to call the drivers ndo_xdp_xmit, if it have not
> allocated the needed XDP TX queues yet.  This is the case for
> virtio_net, which first allocates the XDP TX queues once an XDP/bpf
> prog is attached (in virtnet_xdp_set()).
> 
> Thus, a crash will occur for virtio_net when redirecting to another
> virtio_net device's ndo_xdp_xmit, which have not attached a XDP prog.
> The sample xdp_redirect_map tries to attach a dummy XDP prog to take
> this into account, but it can also easily fail if the virtio_net (or
> actually underlying vhost driver) have not allocated enough extra
> queues for the device.
> 
> Allocating more queue this is currently a manual config.
> Hint for libvirt XML add:
> 
>   <driver name='vhost' queues='16'>
>     <host mrg_rxbuf='off'/>
>     <guest tso4='off' tso6='off' ecn='off' ufo='off'/>
>   </driver>
> 
> The solution in this patch is to check that the device have loaded an
> XDP/bpf prog before proceeding.  This is similar to the check
> performed in driver ixgbe.
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---

Yep, nice catch. I caught this in ixgbe but didn't manage to
pull it into virtio.

Acked-by: John Fastabend <john.fastabend@gmail.com>

>  drivers/net/virtio_net.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1e0e0fce3ab2..9bb9e562b893 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -452,8 +452,18 @@ static bool __virtnet_xdp_xmit(struct virtnet_info *vi,
>  static int virtnet_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> -	bool sent = __virtnet_xdp_xmit(vi, xdp);
> +	struct receive_queue *rq = vi->rq;
> +	struct bpf_prog *xdp_prog;
> +	bool sent;
> +
> +	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
> +	 * indicate XDP resources have been successfully allocated.
> +	 */
> +	xdp_prog = rcu_dereference(rq->xdp_prog);
> +	if (!xdp_prog)
> +		return -ENXIO;
>  
> +	sent = __virtnet_xdp_xmit(vi, xdp);
>  	if (!sent)
>  		return -ENOSPC;
>  	return 0;
>