netdev - Re: virtio_net: suspicious RCU usage with xdp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Apr 2019 10:55:21 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>,
        David Ahern <dsahern@...il.com>
Cc:     makita.toshiaki@....ntt.co.jp,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: virtio_net: suspicious RCU usage with xdp


On 2019/4/25 上午1:37, Michael S. Tsirkin wrote:
> On Wed, Apr 24, 2019 at 11:13:42AM -0600, David Ahern wrote:
>> seeing an RCU warning testing xdp with virtio net. net-next as of commit
>> b2f97f7de2f6a4df8e431330cf467576486651c5. No obvious changes so hoping
>> this rings a bell with someone else.
>>
>>
>> [  121.990304] =============================
>> [  121.991488] WARNING: suspicious RCU usage
>> [  121.992392] 5.1.0-rc5+ #60 Not tainted
>> [  121.993220] -----------------------------
>> [  121.994158] /home/dsa/kernel-3.git/drivers/net/virtio_net.c:516
>> suspicious rcu_dereference_check() usage!
>> [  121.996284]
>>                 other info that might help us debug this:
>>
>> [  121.997988]
>>                 rcu_scheduler_active = 2, debug_locks = 1
>> [  121.999321] no locks held by swapper/1/0.
>> [  122.000328]
>>                 stack backtrace:
>> [  122.001253] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.1.0-rc5+ #60
>> [  122.002474] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS 1.11.1-1 04/01/2014
>> [  122.004141] Call Trace:
>> [  122.004651]  <IRQ>
>> [  122.005082]  dump_stack+0x7e/0xbb
>> [  122.005757]  lockdep_rcu_suspicious+0x102/0x10b
>> [  122.006654]  virtnet_xdp_xmit+0x104/0x4fe
>> [  122.007447]  ? kasan_check_read+0x11/0x13
>> [  122.008267]  ? mergeable_rx_buffer_size_show+0x163/0x163
>> [  122.009299]  ? __asan_loadN+0xf/0x11
>> [  122.010010]  ? pvclock_clocksource_read+0xfa/0x189
>> [  122.010975]  bq_xmit_all+0xdc/0x358
>> [  122.011699]  __dev_map_flush+0xc2/0xef
>> [  122.012472]  xdp_do_flush_map+0x5b/0x74
>> [  122.013238]  virtnet_poll+0x58f/0x679
> Well virtnet_xdp_xmit seems to be called from .ndo_xdp_xmit
> and that isn't in an RCU read-side critical section.
>
> Looks like we just need to add RCU read lock/unlock.
> Like the below perhaps?
>
> This issue was introduced by 8dcc5b0ab0 however I find it
> inelegant that we need to do checks in each driver,
> and add RCU locks just for a startup initialization issue.
> Can't XDP core make sure the callback isn't invoked
> at an inappropriate time instead?
>
> --->
>
> virtio_net: call virtnet_xdp_xmit with RCU lock
>
> This functions uses rcu_dereference so it needs to be
> called in an RCU read-side critical section.
>
> Fixes: 8dcc5b0ab0 ("virtio_net: fix ndo_xdp_xmit crash towards dev not ready for XDP")
> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
>
> --
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9010938e2d71..ccc1bdd1bb1f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -495,8 +495,8 @@ static struct send_queue *virtnet_xdp_sq(struct virtnet_info *vi)
>   	return &vi->sq[qp];
>   }
>   
> -static int virtnet_xdp_xmit(struct net_device *dev,
> -			    int n, struct xdp_frame **frames, u32 flags)
> +static int __virtnet_xdp_xmit(struct net_device *dev,
> +			      int n, struct xdp_frame **frames, u32 flags)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	struct receive_queue *rq = vi->rq;
> @@ -569,6 +569,17 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>   	return ret;
>   }
>   
> +static int virtnet_xdp_xmit(struct net_device *dev,
> +			    int n, struct xdp_frame **frames, u32 flags)
> +{
> +	int r;
> +
> +	rcu_read_lock_bh();
> +	r = __virtnet_xdp_xmit(dev, n, frames, flags);
> +	rcu_read_unlock_bh();
> +	return r;
> +}
> +
>   static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
>   {
>   	return vi->xdp_queue_pairs ? VIRTIO_XDP_HEADROOM : 0;
> @@ -714,7 +725,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>   			xdpf = convert_to_xdp_frame(&xdp);
>   			if (unlikely(!xdpf))
>   				goto err_xdp;
> -			err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> +			err = __virtnet_xdp_xmit(dev, 1, &xdpf, 0);
>   			if (unlikely(err < 0)) {
>   				trace_xdp_exception(vi->dev, xdp_prog, act);
>   				goto err_xdp;
> @@ -887,7 +898,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>   			xdpf = convert_to_xdp_frame(&xdp);
>   			if (unlikely(!xdpf))
>   				goto err_xdp;
> -			err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> +			err = __virtnet_xdp_xmit(dev, 1, &xdpf, 0);
>   			if (unlikely(err < 0)) {
>   				trace_xdp_exception(vi->dev, xdp_prog, act);
>   				if (unlikely(xdp_page != page))
>
>

Acked-by: Jason Wang <jasowang@...hat.com>