virtio_net: suspicious RCU usage with xdp

virtio_net: suspicious RCU usage with xdp

[David Ahern]

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

Re: virtio_net: suspicious RCU usage with xdp

[Michael S. Tsirkin]

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@redhat.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))


-- 
MST

Re: virtio_net: suspicious RCU usage with xdp

[David Ahern]

On 4/24/19 11:37 AM, Michael S. Tsirkin wrote:
> 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,

receive_small takes the rcu lock at the beginning.


>  			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,

same here.
>  			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))
> 
> 

bq_xmit_all is invoking the xdp_xmit callback without taking the rcu
lock, but from the git history does not appear to have any recent changes.

Re: virtio_net: suspicious RCU usage with xdp

[Jason Wang]

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@redhat.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@redhat.com>

Re: virtio_net: suspicious RCU usage with xdp

[Jason Wang]

On 2019/4/25 上午1:40, David Ahern wrote:
> On 4/24/19 11:37 AM, Michael S. Tsirkin wrote:
>> 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,
> receive_small takes the rcu lock at the beginning.


Yes, so the code call __virtnet_xdp_xmit() that won't try to take rcu lock.

Thanks


>
>
>>   			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,
> same here.
>>   			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))
>>
>>
> bq_xmit_all is invoking the xdp_xmit callback without taking the rcu
> lock, but from the git history does not appear to have any recent changes.

Re: virtio_net: suspicious RCU usage with xdp

[Toshiaki Makita]

On 2019/04/25 2: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

Probably not 8dcc5b0ab0, but 5d053f9da431 ("bpf: devmap prepare xdp
frames for bulking").

> 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?

Before commit 5d053f9da431, .ndo_xdp_xmit() should have always been
called under RCU. After the commit, xdp_do_flush_map() also can trigger
.ndo_xdp_xmit() but we forgot to add RCU read lock there?
I guess veth has the same problem and I feel like it should be fixed in
__dev_map_flush(). dev_map_flush_old() needs to be cared too.

-- 
Toshiaki Makita

Re: virtio_net: suspicious RCU usage with xdp

[Michael S. Tsirkin]

On Thu, Apr 25, 2019 at 01:58:48PM +0900, Toshiaki Makita wrote:
> On 2019/04/25 2: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
> 
> Probably not 8dcc5b0ab0, but 5d053f9da431 ("bpf: devmap prepare xdp
> frames for bulking").
> 
> > 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?
> 
> Before commit 5d053f9da431, .ndo_xdp_xmit() should have always been
> called under RCU. After the commit, xdp_do_flush_map() also can trigger
> .ndo_xdp_xmit() but we forgot to add RCU read lock there?
> I guess veth has the same problem and I feel like it should be fixed in
> __dev_map_flush(). dev_map_flush_old() needs to be cared too.

I don't have a problem either way.  Jesper, what do you think?

> -- 
> Toshiaki Makita

Re: virtio_net: suspicious RCU usage with xdp

[Jesper Dangaard Brouer]

On Thu, 25 Apr 2019 13:03:39 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Apr 25, 2019 at 01:58:48PM +0900, Toshiaki Makita wrote:
> > On 2019/04/25 2: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  
> > 
> > Probably not 8dcc5b0ab0, but 5d053f9da431 ("bpf: devmap prepare xdp
> > frames for bulking").
> >   
> > > 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?  
> > 
> > Before commit 5d053f9da431, .ndo_xdp_xmit() should have always been
> > called under RCU. After the commit, xdp_do_flush_map() also can trigger
> > .ndo_xdp_xmit() but we forgot to add RCU read lock there?
> > I guess veth has the same problem and I feel like it should be fixed in
> > __dev_map_flush(). dev_map_flush_old() needs to be cared too.  
> 
> I don't have a problem either way.  Jesper, what do you think?

It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
frames for bulking") introduced this issue.  I guess we can add the RCU
section to xdp_do_flush_map(), and then also verify that the devmap
(and cpumap) take-down code also have appropriate RCU sections (which
they should have).

Another requirement for calling .ndo_xdp_xmit is running under NAPI
protection, is that still satisifed for veth?
(even when invoked via xdp_do_flush_map()).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: virtio_net: suspicious RCU usage with xdp

[David Ahern]

On 4/25/19 11:41 AM, Jesper Dangaard Brouer wrote:
> On Thu, 25 Apr 2019 13:03:39 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Thu, Apr 25, 2019 at 01:58:48PM +0900, Toshiaki Makita wrote:
>>> On 2019/04/25 2: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  
>>>
>>> Probably not 8dcc5b0ab0, but 5d053f9da431 ("bpf: devmap prepare xdp
>>> frames for bulking").
>>>   
>>>> 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?  
>>>
>>> Before commit 5d053f9da431, .ndo_xdp_xmit() should have always been
>>> called under RCU. After the commit, xdp_do_flush_map() also can trigger
>>> .ndo_xdp_xmit() but we forgot to add RCU read lock there?
>>> I guess veth has the same problem and I feel like it should be fixed in
>>> __dev_map_flush(). dev_map_flush_old() needs to be cared too.  
>>
>> I don't have a problem either way.  Jesper, what do you think?
> 
> It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
> frames for bulking") introduced this issue.  I guess we can add the RCU
> section to xdp_do_flush_map(), and then also verify that the devmap
> (and cpumap) take-down code also have appropriate RCU sections (which
> they should have).
> 
> Another requirement for calling .ndo_xdp_xmit is running under NAPI
> protection, is that still satisifed for veth?
> (even when invoked via xdp_do_flush_map()).
> 

virtio_net hits this because of:
   xdp_prog = rcu_dereference(rq->xdp_prog);

in its ndo_xdp_xmit. Scanning ndo_xdp_xmit for other nics does not show
this same check. Is it really required? If so, why don't other drivers
do it?

Re: virtio_net: suspicious RCU usage with xdp

[Maciej Fijalkowski]

On Thu, 25 Apr 2019 11:44:27 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 4/25/19 11:41 AM, Jesper Dangaard Brouer wrote:
> > On Thu, 25 Apr 2019 13:03:39 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> >> On Thu, Apr 25, 2019 at 01:58:48PM +0900, Toshiaki Makita wrote:  
> >>> On 2019/04/25 2: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    
> >>>
> >>> Probably not 8dcc5b0ab0, but 5d053f9da431 ("bpf: devmap prepare xdp
> >>> frames for bulking").
> >>>     
> >>>> 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?    
> >>>
> >>> Before commit 5d053f9da431, .ndo_xdp_xmit() should have always been
> >>> called under RCU. After the commit, xdp_do_flush_map() also can trigger
> >>> .ndo_xdp_xmit() but we forgot to add RCU read lock there?
> >>> I guess veth has the same problem and I feel like it should be fixed in
> >>> __dev_map_flush(). dev_map_flush_old() needs to be cared too.    
> >>
> >> I don't have a problem either way.  Jesper, what do you think?  
> > 
> > It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
> > frames for bulking") introduced this issue.  I guess we can add the RCU
> > section to xdp_do_flush_map(), and then also verify that the devmap
> > (and cpumap) take-down code also have appropriate RCU sections (which
> > they should have).
> > 
> > Another requirement for calling .ndo_xdp_xmit is running under NAPI
> > protection, is that still satisifed for veth?
> > (even when invoked via xdp_do_flush_map()).
> >   
> 
> virtio_net hits this because of:
>    xdp_prog = rcu_dereference(rq->xdp_prog);
> 
> in its ndo_xdp_xmit. Scanning ndo_xdp_xmit for other nics does not show
> this same check. Is it really required? If so, why don't other drivers
> do it?

That was my initial thought as well. Can't we just check that
vi->xdp_queue_pairs was set in virtnet_xdp_xmit? It is being done just before
assigning the XDP prog pointers for rqs. Haven't looked at veth though.

Re: virtio_net: suspicious RCU usage with xdp

[David Ahern]

On 4/25/19 12:54 PM, Maciej Fijalkowski wrote:
>>
>> virtio_net hits this because of:
>>    xdp_prog = rcu_dereference(rq->xdp_prog);
>>
>> in its ndo_xdp_xmit. Scanning ndo_xdp_xmit for other nics does not show
>> this same check. Is it really required? If so, why don't other drivers
>> do it?
> 
> That was my initial thought as well. Can't we just check that
> vi->xdp_queue_pairs was set in virtnet_xdp_xmit? It is being done just before
> assigning the XDP prog pointers for rqs. Haven't looked at veth though.
> 

or change the dereference to just rcu_access_pointer. The xdp_prog is
not needed; it only checks if set.

Re: virtio_net: suspicious RCU usage with xdp

[Jesper Dangaard Brouer]

On Thu, 25 Apr 2019 20:54:11 +0200
Maciej Fijalkowski <maciejromanfijalkowski@gmail.com> wrote:

> On Thu, 25 Apr 2019 11:44:27 -0600
> David Ahern <dsahern@gmail.com> wrote:
> 
> > On 4/25/19 11:41 AM, Jesper Dangaard Brouer wrote:  
> > > On Thu, 25 Apr 2019 13:03:39 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >     
> > >> On Thu, Apr 25, 2019 at 01:58:48PM +0900, Toshiaki Makita wrote:    
> > >>> On 2019/04/25 2: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      
> > >>>
> > >>> Probably not 8dcc5b0ab0, but 5d053f9da431 ("bpf: devmap prepare xdp
> > >>> frames for bulking").
> > >>>       
> > >>>> 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?      
> > >>>
> > >>> Before commit 5d053f9da431, .ndo_xdp_xmit() should have always been
> > >>> called under RCU. After the commit, xdp_do_flush_map() also can trigger
> > >>> .ndo_xdp_xmit() but we forgot to add RCU read lock there?
> > >>> I guess veth has the same problem and I feel like it should be fixed in
> > >>> __dev_map_flush(). dev_map_flush_old() needs to be cared too.      
> > >>
> > >> I don't have a problem either way.  Jesper, what do you think?    
> > > 
> > > It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
> > > frames for bulking") introduced this issue.  I guess we can add the RCU
> > > section to xdp_do_flush_map(), and then also verify that the devmap
> > > (and cpumap) take-down code also have appropriate RCU sections (which
> > > they should have).
> > > 
> > > Another requirement for calling .ndo_xdp_xmit is running under NAPI
> > > protection, is that still satisifed for veth?
> > > (even when invoked via xdp_do_flush_map()).
> > >     
> > 
> > virtio_net hits this because of:
> >    xdp_prog = rcu_dereference(rq->xdp_prog);
> > 
> > in its ndo_xdp_xmit. Scanning ndo_xdp_xmit for other nics does not show
> > this same check. Is it really required? If so, why don't other drivers
> > do it?  
> 
> That was my initial thought as well. Can't we just check that
> vi->xdp_queue_pairs was set in virtnet_xdp_xmit? It is being done
> just before assigning the XDP prog pointers for rqs. Haven't looked
> at veth though.

Ah, I see, the reason virtio_net is doing this is documented in a
comment:

	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
	 * indicate XDP resources have been successfully allocated.
	 */

As we have discussed before, this is a bad assumption. We even have a
TODO[1] in the XDP-project named: Better ndo_xdp_xmit resource management.
This area need to be improved. 

I can see other drivers also end-up checking for if xdp_prog pointer
exist, but they don't need to dereference the pointer.  Even if we find
some other resource management check, then that check also need some
protection that likely want to leverage RCU as well(?).

The mlx5 driver also had issues in this area, and is doing some RCU
sync/barrier tricks, which also indicate a RCU dependency.


[1] https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#better-ndo_xdp_xmit-resource-management

IHMO this tying XDP ndo_xdp_xmit resources to whether there is a XDP RX
prog loaded is wrong... and cause the very ugly trick of loading dummy
XDP program on TX devices.
Fixing this is a larger effort... a work-around for now would be to do
a RCU read-section in xdp_do_flush_map().
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: virtio_net: suspicious RCU usage with xdp

[Toshiaki Makita]

On 2019/04/26 2:41, Jesper Dangaard Brouer wrote:
> On Thu, 25 Apr 2019 13:03:39 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Thu, Apr 25, 2019 at 01:58:48PM +0900, Toshiaki Makita wrote:
>>> On 2019/04/25 2: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  
>>>
>>> Probably not 8dcc5b0ab0, but 5d053f9da431 ("bpf: devmap prepare xdp
>>> frames for bulking").
>>>   
>>>> 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?  
>>>
>>> Before commit 5d053f9da431, .ndo_xdp_xmit() should have always been
>>> called under RCU. After the commit, xdp_do_flush_map() also can trigger
>>> .ndo_xdp_xmit() but we forgot to add RCU read lock there?
>>> I guess veth has the same problem and I feel like it should be fixed in
>>> __dev_map_flush(). dev_map_flush_old() needs to be cared too.  
>>
>> I don't have a problem either way.  Jesper, what do you think?
> 
> It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
> frames for bulking") introduced this issue.  I guess we can add the RCU
> section to xdp_do_flush_map(), and then also verify that the devmap
> (and cpumap) take-down code also have appropriate RCU sections (which
> they should have).
> 
> Another requirement for calling .ndo_xdp_xmit is running under NAPI
> protection, is that still satisifed for veth?
> (even when invoked via xdp_do_flush_map()).

veth_xdp_xmit() assumes it's called from NAPI handler, and veth calls
xdp_do_flush_map() only from within NAPI context. Not sure what happens
when .ndo_xdp_xmit is called from dev_map_flush_old().

-- 
Toshiaki Makita

Re: virtio_net: suspicious RCU usage with xdp

[Jason Wang]

On 2019/4/26 上午1:41, Jesper Dangaard Brouer wrote:
> It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
> frames for bulking") introduced this issue.  I guess we can add the RCU
> section to xdp_do_flush_map(), and then also verify that the devmap
> (and cpumap) take-down code also have appropriate RCU sections (which
> they should have).
>
> Another requirement for calling .ndo_xdp_xmit is running under NAPI
> protection,


May I know the reason for this? I'm asking since if the packet was 
redirected from tuntap, ndo_xdp_xmit()  won't be called under the 
protection of NAPI (but bh is disabled).

Thanks


> is that still satisifed for veth?
> (even when invoked via xdp_do_flush_map()).

Re: virtio_net: suspicious RCU usage with xdp

[Jesper Dangaard Brouer]

On Fri, 26 Apr 2019 16:00:28 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2019/4/26 上午1:41, Jesper Dangaard Brouer wrote:
> > It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
> > frames for bulking") introduced this issue.  I guess we can add the RCU
> > section to xdp_do_flush_map(), and then also verify that the devmap
> > (and cpumap) take-down code also have appropriate RCU sections (which
> > they should have).
> >
> > Another requirement for calling .ndo_xdp_xmit is running under NAPI
> > protection,  
> 
> 
> May I know the reason for this? I'm asking since if the packet was 
> redirected from tuntap, ndo_xdp_xmit()  won't be called under the 
> protection of NAPI (but bh is disabled).

There are a number of things that rely on this NAPI/softirq protection.

One is preempt-free access per-cpu struct bpf_redirect_info. Which is
at the core of the XDP and TC redirect feature.

  DEFINE_PER_CPU(struct bpf_redirect_info, bpf_redirect_info);
  EXPORT_PER_CPU_SYMBOL_GPL(bpf_redirect_info);
  struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);

And devmap and cpumap also have per-cpu variables, that we don't use
preempt-disable around.

Another is xdp_return_frame_rx_napi() that when page_pool is active,
can store frames to be recycled directly into an array, in function
__page_pool_recycle_direct() (but as I don't trust every driver getting
this correct I've added a safe-guard in page-pool via
in_serving_softirq().

I guess, disable_bh is sufficient protection, as we are mostly
optimizing away a preempt-disable when accessing per-cpu variables.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: virtio_net: suspicious RCU usage with xdp

[Jason Wang]

On 2019/4/26 下午7:05, Jesper Dangaard Brouer wrote:
> On Fri, 26 Apr 2019 16:00:28 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2019/4/26 上午1:41, Jesper Dangaard Brouer wrote:
>>> It does sound like my commit 5d053f9da431 ("bpf: devmap prepare xdp
>>> frames for bulking") introduced this issue.  I guess we can add the RCU
>>> section to xdp_do_flush_map(), and then also verify that the devmap
>>> (and cpumap) take-down code also have appropriate RCU sections (which
>>> they should have).
>>>
>>> Another requirement for calling .ndo_xdp_xmit is running under NAPI
>>> protection,
>>
>> May I know the reason for this? I'm asking since if the packet was
>> redirected from tuntap, ndo_xdp_xmit()  won't be called under the
>> protection of NAPI (but bh is disabled).
> There are a number of things that rely on this NAPI/softirq protection.
>
> One is preempt-free access per-cpu struct bpf_redirect_info. Which is
> at the core of the XDP and TC redirect feature.
>
>    DEFINE_PER_CPU(struct bpf_redirect_info, bpf_redirect_info);
>    EXPORT_PER_CPU_SYMBOL_GPL(bpf_redirect_info);
>    struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>
> And devmap and cpumap also have per-cpu variables, that we don't use
> preempt-disable around.
>
> Another is xdp_return_frame_rx_napi() that when page_pool is active,
> can store frames to be recycled directly into an array, in function
> __page_pool_recycle_direct() (but as I don't trust every driver getting
> this correct I've added a safe-guard in page-pool via
> in_serving_softirq().


I see, if I want to use page pool for tap for VM2VM traffic, this 
probably means I can only recycle through ptr_ring.


>
> I guess, disable_bh is sufficient protection, as we are mostly
> optimizing away a preempt-disable when accessing per-cpu variables.
>

Yes, that's what I want for confirm.

Thanks