All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-06-03 17:09 ` Xuan Zhuo
  0 siblings, 0 replies; 16+ messages in thread
From: Xuan Zhuo @ 2021-06-03 17:09 UTC (permalink / raw)
  To: netdev
  Cc: Michael S. Tsirkin, Jason Wang, David S. Miller, Jakub Kicinski,
	Xuan Zhuo, virtualization, Corentin Noël

In virtio-net's large packet mode, there is a hole in the space behind
buf.

    hdr_padded_len - hdr_len

We must take this into account when calculating tailroom.

[   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
[   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
[   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
[   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
[   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
[   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
[   44.548251] __napi_poll (net/core/dev.c:6985)
[   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
[   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
[   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
[   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
[   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
[   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)

Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reported-by: Corentin Noël <corentin.noel@collabora.com>
Tested-by: Corentin Noël <corentin.noel@collabora.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fa407eb8b457..78a01c71a17c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
 	 */
 	truesize = headroom ? PAGE_SIZE : truesize;
-	tailroom = truesize - len - headroom;
+	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
 	buf = p - headroom;
 
 	len -= hdr_len;
-- 
2.31.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-06-03 17:09 ` Xuan Zhuo
  0 siblings, 0 replies; 16+ messages in thread
From: Xuan Zhuo @ 2021-06-03 17:09 UTC (permalink / raw)
  To: netdev
  Cc: Michael S. Tsirkin, Corentin Noël, virtualization,
	Jakub Kicinski, David S. Miller

In virtio-net's large packet mode, there is a hole in the space behind
buf.

    hdr_padded_len - hdr_len

We must take this into account when calculating tailroom.

[   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
[   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
[   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
[   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
[   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
[   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
[   44.548251] __napi_poll (net/core/dev.c:6985)
[   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
[   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
[   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
[   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
[   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
[   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)

Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reported-by: Corentin Noël <corentin.noel@collabora.com>
Tested-by: Corentin Noël <corentin.noel@collabora.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fa407eb8b457..78a01c71a17c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
 	 */
 	truesize = headroom ? PAGE_SIZE : truesize;
-	tailroom = truesize - len - headroom;
+	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
 	buf = p - headroom;
 
 	len -= hdr_len;
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-06-03 17:09 ` Xuan Zhuo
  (?)
@ 2021-06-03 22:30 ` patchwork-bot+netdevbpf
  -1 siblings, 0 replies; 16+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-06-03 22:30 UTC (permalink / raw)
  To: Xuan Zhuo
  Cc: netdev, mst, jasowang, davem, kuba, virtualization, corentin.noel

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Fri,  4 Jun 2021 01:09:01 +0800 you wrote:
> In virtio-net's large packet mode, there is a hole in the space behind
> buf.
> 
>     hdr_padded_len - hdr_len
> 
> We must take this into account when calculating tailroom.
> 
> [...]

Here is the summary with links:
  - [net] virtio-net: fix for skb_over_panic inside big mode
    https://git.kernel.org/netdev/net/c/1a8024239dac

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-06-03 17:09 ` Xuan Zhuo
@ 2021-06-04  2:28   ` Jason Wang
  -1 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-06-04  2:28 UTC (permalink / raw)
  To: Xuan Zhuo, netdev
  Cc: Michael S. Tsirkin, David S. Miller, Jakub Kicinski,
	virtualization, Corentin Noël


在 2021/6/4 上午1:09, Xuan Zhuo 写道:
> In virtio-net's large packet mode, there is a hole in the space behind
> buf.


before the buf actually or behind the vnet header?


>
>      hdr_padded_len - hdr_len
>
> We must take this into account when calculating tailroom.
>
> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
> [   44.548251] __napi_poll (net/core/dev.c:6985)
> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>
> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reported-by: Corentin Noël <corentin.noel@collabora.com>
> Tested-by: Corentin Noël <corentin.noel@collabora.com>
> ---
>   drivers/net/virtio_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fa407eb8b457..78a01c71a17c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
>   	 */
>   	truesize = headroom ? PAGE_SIZE : truesize;
> -	tailroom = truesize - len - headroom;
> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);


The patch looks correct and I saw it has been merged.

But I prefer to do that in receive_big() instead of here.

Thanks



>   	buf = p - headroom;
>   
>   	len -= hdr_len;


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-06-04  2:28   ` Jason Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-06-04  2:28 UTC (permalink / raw)
  To: Xuan Zhuo, netdev
  Cc: Jakub Kicinski, virtualization, David S. Miller,
	Corentin Noël, Michael S. Tsirkin


在 2021/6/4 上午1:09, Xuan Zhuo 写道:
> In virtio-net's large packet mode, there is a hole in the space behind
> buf.


before the buf actually or behind the vnet header?


>
>      hdr_padded_len - hdr_len
>
> We must take this into account when calculating tailroom.
>
> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
> [   44.548251] __napi_poll (net/core/dev.c:6985)
> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>
> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reported-by: Corentin Noël <corentin.noel@collabora.com>
> Tested-by: Corentin Noël <corentin.noel@collabora.com>
> ---
>   drivers/net/virtio_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fa407eb8b457..78a01c71a17c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
>   	 */
>   	truesize = headroom ? PAGE_SIZE : truesize;
> -	tailroom = truesize - len - headroom;
> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);


The patch looks correct and I saw it has been merged.

But I prefer to do that in receive_big() instead of here.

Thanks



>   	buf = p - headroom;
>   
>   	len -= hdr_len;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-06-04  2:28   ` Jason Wang
  (?)
@ 2021-06-04  2:30   ` Xuan Zhuo
  2021-06-04  3:00       ` Jason Wang
  -1 siblings, 1 reply; 16+ messages in thread
From: Xuan Zhuo @ 2021-06-04  2:30 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, netdev, Corentin Noël, virtualization,
	Jakub Kicinski, David S. Miller

On Fri, 4 Jun 2021 10:28:41 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2021/6/4 上午1:09, Xuan Zhuo 写道:
> > In virtio-net's large packet mode, there is a hole in the space behind
> > buf.
>
>
> before the buf actually or behind the vnet header?
>
>
> >
> >      hdr_padded_len - hdr_len
> >
> > We must take this into account when calculating tailroom.
> >
> > [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
> > [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
> > [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
> > [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
> > [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
> > [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
> > [   44.548251] __napi_poll (net/core/dev.c:6985)
> > [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
> > [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
> > [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
> > [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
> > [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> > [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> >
> > Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Reported-by: Corentin Noël <corentin.noel@collabora.com>
> > Tested-by: Corentin Noël <corentin.noel@collabora.com>
> > ---
> >   drivers/net/virtio_net.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index fa407eb8b457..78a01c71a17c 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >   	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
> >   	 */
> >   	truesize = headroom ? PAGE_SIZE : truesize;
> > -	tailroom = truesize - len - headroom;
> > +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>
>
> The patch looks correct and I saw it has been merged.
>
> But I prefer to do that in receive_big() instead of here.
>
> Thanks

How?

change truesize or headroom?

I didn't find a good way. Do you have a good way?

Thanks.

>
>
>
> >   	buf = p - headroom;
> >
> >   	len -= hdr_len;
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-06-04  2:30   ` Xuan Zhuo
@ 2021-06-04  3:00       ` Jason Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-06-04  3:00 UTC (permalink / raw)
  To: Xuan Zhuo
  Cc: Michael S. Tsirkin, David S. Miller, Jakub Kicinski,
	virtualization, Corentin Noël, netdev


在 2021/6/4 上午10:30, Xuan Zhuo 写道:
> On Fri, 4 Jun 2021 10:28:41 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2021/6/4 上午1:09, Xuan Zhuo 写道:
>>> In virtio-net's large packet mode, there is a hole in the space behind
>>> buf.
>>
>> before the buf actually or behind the vnet header?
>>
>>
>>>       hdr_padded_len - hdr_len
>>>
>>> We must take this into account when calculating tailroom.
>>>
>>> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
>>> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
>>> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
>>> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
>>> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
>>> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
>>> [   44.548251] __napi_poll (net/core/dev.c:6985)
>>> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
>>> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
>>> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
>>> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
>>> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>>
>>> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> Reported-by: Corentin Noël <corentin.noel@collabora.com>
>>> Tested-by: Corentin Noël <corentin.noel@collabora.com>
>>> ---
>>>    drivers/net/virtio_net.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index fa407eb8b457..78a01c71a17c 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>    	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
>>>    	 */
>>>    	truesize = headroom ? PAGE_SIZE : truesize;
>>> -	tailroom = truesize - len - headroom;
>>> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>>
>> The patch looks correct and I saw it has been merged.
>>
>> But I prefer to do that in receive_big() instead of here.
>>
>> Thanks
> How?
>
> change truesize or headroom?
>
> I didn't find a good way. Do you have a good way?


Something like the following? The API is designed to let the caller to 
pass a correct headroom instead of figure it out by itself.

         struct sk_buff *skb =
                 page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0, 
hdr_padded_len - hdr_len);

Thanks


>
> Thanks.
>
>>
>>
>>>    	buf = p - headroom;
>>>
>>>    	len -= hdr_len;


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-06-04  3:00       ` Jason Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-06-04  3:00 UTC (permalink / raw)
  To: Xuan Zhuo
  Cc: Michael S. Tsirkin, netdev, Corentin Noël, virtualization,
	Jakub Kicinski, David S. Miller


在 2021/6/4 上午10:30, Xuan Zhuo 写道:
> On Fri, 4 Jun 2021 10:28:41 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2021/6/4 上午1:09, Xuan Zhuo 写道:
>>> In virtio-net's large packet mode, there is a hole in the space behind
>>> buf.
>>
>> before the buf actually or behind the vnet header?
>>
>>
>>>       hdr_padded_len - hdr_len
>>>
>>> We must take this into account when calculating tailroom.
>>>
>>> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
>>> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
>>> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
>>> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
>>> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
>>> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
>>> [   44.548251] __napi_poll (net/core/dev.c:6985)
>>> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
>>> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
>>> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
>>> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
>>> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>>
>>> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> Reported-by: Corentin Noël <corentin.noel@collabora.com>
>>> Tested-by: Corentin Noël <corentin.noel@collabora.com>
>>> ---
>>>    drivers/net/virtio_net.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index fa407eb8b457..78a01c71a17c 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>    	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
>>>    	 */
>>>    	truesize = headroom ? PAGE_SIZE : truesize;
>>> -	tailroom = truesize - len - headroom;
>>> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>>
>> The patch looks correct and I saw it has been merged.
>>
>> But I prefer to do that in receive_big() instead of here.
>>
>> Thanks
> How?
>
> change truesize or headroom?
>
> I didn't find a good way. Do you have a good way?


Something like the following? The API is designed to let the caller to 
pass a correct headroom instead of figure it out by itself.

         struct sk_buff *skb =
                 page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0, 
hdr_padded_len - hdr_len);

Thanks


>
> Thanks.
>
>>
>>
>>>    	buf = p - headroom;
>>>
>>>    	len -= hdr_len;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-06-04  3:00       ` Jason Wang
  (?)
@ 2021-06-04  3:05       ` Xuan Zhuo
  2021-06-04  6:00           ` Jason Wang
  -1 siblings, 1 reply; 16+ messages in thread
From: Xuan Zhuo @ 2021-06-04  3:05 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, netdev, Corentin Noël, virtualization,
	Jakub Kicinski, David S. Miller

On Fri, 4 Jun 2021 11:00:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2021/6/4 上午10:30, Xuan Zhuo 写道:
> > On Fri, 4 Jun 2021 10:28:41 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >> 在 2021/6/4 上午1:09, Xuan Zhuo 写道:
> >>> In virtio-net's large packet mode, there is a hole in the space behind
> >>> buf.
> >>
> >> before the buf actually or behind the vnet header?
> >>
> >>
> >>>       hdr_padded_len - hdr_len
> >>>
> >>> We must take this into account when calculating tailroom.
> >>>
> >>> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
> >>> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
> >>> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
> >>> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
> >>> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
> >>> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
> >>> [   44.548251] __napi_poll (net/core/dev.c:6985)
> >>> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
> >>> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
> >>> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
> >>> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
> >>> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> >>> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> >>>
> >>> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> >>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>> Reported-by: Corentin Noël <corentin.noel@collabora.com>
> >>> Tested-by: Corentin Noël <corentin.noel@collabora.com>
> >>> ---
> >>>    drivers/net/virtio_net.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>> index fa407eb8b457..78a01c71a17c 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >>>    	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
> >>>    	 */
> >>>    	truesize = headroom ? PAGE_SIZE : truesize;
> >>> -	tailroom = truesize - len - headroom;
> >>> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
> >>
> >> The patch looks correct and I saw it has been merged.
> >>
> >> But I prefer to do that in receive_big() instead of here.
> >>
> >> Thanks
> > How?
> >
> > change truesize or headroom?
> >
> > I didn't find a good way. Do you have a good way?
>
>
> Something like the following? The API is designed to let the caller to
> pass a correct headroom instead of figure it out by itself.
>
>          struct sk_buff *skb =
>                  page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0,
> hdr_padded_len - hdr_len);
>
> Thanks


This line may be affected.

	buf = p - headroom;

In my opinion, this changes the semantics of the original headroom. The meaning
of headroom in big mode and merge mode has become different. The more confusing
problem is that the parameters of page_to_skb() are getting more and more
chaotic.  So I wrote the previous patch. Of course, I understand your concern.
This patch may bring Here are more questions, although I did a lot of tests.

	"virtio-net: Refactor the code related to page_to_skb"

But I hope that our code development direction is as close to what this patch
realizes. I hope that the meaning of the parameters can be more clear.

Do you think this is ok?

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 78a01c71a17c..6d62bb45a188 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -380,34 +380,20 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 				   struct page *page, unsigned int offset,
 				   unsigned int len, unsigned int truesize,
 				   bool hdr_valid, unsigned int metasize,
-				   unsigned int headroom)
+				   int tailroom, char *buf,
+				   unsigned int hdr_padded_len)
 {
 	struct sk_buff *skb;
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
-	unsigned int copy, hdr_len, hdr_padded_len;
+	unsigned int copy, hdr_len;
 	struct page *page_to_free = NULL;
-	int tailroom, shinfo_size;
-	char *p, *hdr_p, *buf;
+	int shinfo_size;
+	char *p, *hdr_p;

 	p = page_address(page) + offset;
 	hdr_p = p;

 	hdr_len = vi->hdr_len;
-	if (vi->mergeable_rx_bufs)
-		hdr_padded_len = sizeof(*hdr);
-	else
-		hdr_padded_len = sizeof(struct padded_vnet_hdr);
-
-	/* If headroom is not 0, there is an offset between the beginning of the
-	 * data and the allocated space, otherwise the data and the allocated
-	 * space are aligned.
-	 *
-	 * Buffers with headroom use PAGE_SIZE as alloc size, see
-	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
-	 */
-	truesize = headroom ? PAGE_SIZE : truesize;
-	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
-	buf = p - headroom;

 	len -= hdr_len;
 	offset += hdr_padded_len;
@@ -492,6 +478,51 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	return skb;
 }

+static struct sk_buff *merge_page_to_skb(struct virtnet_info *vi,
+					 struct receive_queue *rq,
+					 struct page *page, unsigned int offset,
+					 unsigned int len, unsigned int truesize,
+					 bool hdr_valid, unsigned int metasize,
+					 unsigned int headroom)
+{
+	int tailroom;
+	char *buf;
+
+	/* If headroom is not 0, there is an offset between the beginning of the
+	 * data and the allocated space, otherwise the data and the allocated
+	 * space are aligned.
+	 *
+	 * Buffers with headroom use PAGE_SIZE as alloc size, see
+	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
+	 */
+	truesize = headroom ? PAGE_SIZE : truesize;
+	tailroom = truesize - len - headroom;
+	buf = page_address(page) + offset - headroom;
+
+	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
+		    tailroom, buf, sizeof(struct virtio_net_hdr_mrg_rxbuf))
+
+}
+
+static struct sk_buff *big_page_to_skb(struct virtnet_info *vi,
+				       struct receive_queue *rq,
+				       struct page *page, unsigned int offset,
+				       unsigned int len, unsigned int truesize,
+				       bool hdr_valid, unsigned int metasize,
+				       unsigned int headroom)
+{
+	char *p = page_address(page);
+	int hold;
+	int tailroom;
+
+	hold = sizeof(struct padded_vnet_hdr) - vi->hdr_len;
+
+	tailroom = truesize - len - headroom - hold;
+
+	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
+		    tailroom, p, sizeof(struct padded_vnet_hdr));
+}
+
 static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
 				   struct send_queue *sq,
 				   struct xdp_frame *xdpf)


>
>
> >
> > Thanks.
> >
> >>
> >>
> >>>    	buf = p - headroom;
> >>>
> >>>    	len -= hdr_len;
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-06-04  3:05       ` Xuan Zhuo
@ 2021-06-04  6:00           ` Jason Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-06-04  6:00 UTC (permalink / raw)
  To: Xuan Zhuo
  Cc: Michael S. Tsirkin, David S. Miller, Jakub Kicinski,
	virtualization, Corentin Noël, netdev


在 2021/6/4 上午11:05, Xuan Zhuo 写道:
> On Fri, 4 Jun 2021 11:00:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2021/6/4 上午10:30, Xuan Zhuo 写道:
>>> On Fri, 4 Jun 2021 10:28:41 +0800, Jason Wang <jasowang@redhat.com> wrote:
>>>> 在 2021/6/4 上午1:09, Xuan Zhuo 写道:
>>>>> In virtio-net's large packet mode, there is a hole in the space behind
>>>>> buf.
>>>> before the buf actually or behind the vnet header?
>>>>
>>>>
>>>>>        hdr_padded_len - hdr_len
>>>>>
>>>>> We must take this into account when calculating tailroom.
>>>>>
>>>>> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
>>>>> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
>>>>> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
>>>>> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
>>>>> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
>>>>> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
>>>>> [   44.548251] __napi_poll (net/core/dev.c:6985)
>>>>> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
>>>>> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
>>>>> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
>>>>> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
>>>>> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>>>> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>>>>
>>>>> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
>>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>> Reported-by: Corentin Noël <corentin.noel@collabora.com>
>>>>> Tested-by: Corentin Noël <corentin.noel@collabora.com>
>>>>> ---
>>>>>     drivers/net/virtio_net.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>> index fa407eb8b457..78a01c71a17c 100644
>>>>> --- a/drivers/net/virtio_net.c
>>>>> +++ b/drivers/net/virtio_net.c
>>>>> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>     	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
>>>>>     	 */
>>>>>     	truesize = headroom ? PAGE_SIZE : truesize;
>>>>> -	tailroom = truesize - len - headroom;
>>>>> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>>>> The patch looks correct and I saw it has been merged.
>>>>
>>>> But I prefer to do that in receive_big() instead of here.
>>>>
>>>> Thanks
>>> How?
>>>
>>> change truesize or headroom?
>>>
>>> I didn't find a good way. Do you have a good way?
>>
>> Something like the following? The API is designed to let the caller to
>> pass a correct headroom instead of figure it out by itself.
>>
>>           struct sk_buff *skb =
>>                   page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0,
>> hdr_padded_len - hdr_len);
>>
>> Thanks
>
> This line may be affected.
>
> 	buf = p - headroom;
>
> In my opinion, this changes the semantics of the original headroom. The meaning
> of headroom in big mode and merge mode has become different. The more confusing
> problem is that the parameters of page_to_skb() are getting more and more
> chaotic.  So I wrote the previous patch. Of course, I understand your concern.
> This patch may bring Here are more questions, although I did a lot of tests.
>
> 	"virtio-net: Refactor the code related to page_to_skb"
>
> But I hope that our code development direction is as close to what this patch
> realizes. I hope that the meaning of the parameters can be more clear.


So I don't object to this method, but as I replied, it's better to do 
some benchmark to see if it introduces any regression


>
> Do you think this is ok?


Looks ok, but if we decide to go with your approach, it can be squashed 
into that patch.

Thanks


>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 78a01c71a17c..6d62bb45a188 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -380,34 +380,20 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   				   struct page *page, unsigned int offset,
>   				   unsigned int len, unsigned int truesize,
>   				   bool hdr_valid, unsigned int metasize,
> -				   unsigned int headroom)
> +				   int tailroom, char *buf,
> +				   unsigned int hdr_padded_len)
>   {
>   	struct sk_buff *skb;
>   	struct virtio_net_hdr_mrg_rxbuf *hdr;
> -	unsigned int copy, hdr_len, hdr_padded_len;
> +	unsigned int copy, hdr_len;
>   	struct page *page_to_free = NULL;
> -	int tailroom, shinfo_size;
> -	char *p, *hdr_p, *buf;
> +	int shinfo_size;
> +	char *p, *hdr_p;
>
>   	p = page_address(page) + offset;
>   	hdr_p = p;
>
>   	hdr_len = vi->hdr_len;
> -	if (vi->mergeable_rx_bufs)
> -		hdr_padded_len = sizeof(*hdr);
> -	else
> -		hdr_padded_len = sizeof(struct padded_vnet_hdr);
> -
> -	/* If headroom is not 0, there is an offset between the beginning of the
> -	 * data and the allocated space, otherwise the data and the allocated
> -	 * space are aligned.
> -	 *
> -	 * Buffers with headroom use PAGE_SIZE as alloc size, see
> -	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
> -	 */
> -	truesize = headroom ? PAGE_SIZE : truesize;
> -	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
> -	buf = p - headroom;
>
>   	len -= hdr_len;
>   	offset += hdr_padded_len;
> @@ -492,6 +478,51 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   	return skb;
>   }
>
> +static struct sk_buff *merge_page_to_skb(struct virtnet_info *vi,
> +					 struct receive_queue *rq,
> +					 struct page *page, unsigned int offset,
> +					 unsigned int len, unsigned int truesize,
> +					 bool hdr_valid, unsigned int metasize,
> +					 unsigned int headroom)
> +{
> +	int tailroom;
> +	char *buf;
> +
> +	/* If headroom is not 0, there is an offset between the beginning of the
> +	 * data and the allocated space, otherwise the data and the allocated
> +	 * space are aligned.
> +	 *
> +	 * Buffers with headroom use PAGE_SIZE as alloc size, see
> +	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
> +	 */
> +	truesize = headroom ? PAGE_SIZE : truesize;
> +	tailroom = truesize - len - headroom;
> +	buf = page_address(page) + offset - headroom;
> +
> +	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
> +		    tailroom, buf, sizeof(struct virtio_net_hdr_mrg_rxbuf))
> +
> +}
> +
> +static struct sk_buff *big_page_to_skb(struct virtnet_info *vi,
> +				       struct receive_queue *rq,
> +				       struct page *page, unsigned int offset,
> +				       unsigned int len, unsigned int truesize,
> +				       bool hdr_valid, unsigned int metasize,
> +				       unsigned int headroom)
> +{
> +	char *p = page_address(page);
> +	int hold;
> +	int tailroom;
> +
> +	hold = sizeof(struct padded_vnet_hdr) - vi->hdr_len;
> +
> +	tailroom = truesize - len - headroom - hold;
> +
> +	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
> +		    tailroom, p, sizeof(struct padded_vnet_hdr));
> +}
> +
>   static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>   				   struct send_queue *sq,
>   				   struct xdp_frame *xdpf)
>
>
>>
>>> Thanks.
>>>
>>>>
>>>>>     	buf = p - headroom;
>>>>>
>>>>>     	len -= hdr_len;


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-06-04  6:00           ` Jason Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-06-04  6:00 UTC (permalink / raw)
  To: Xuan Zhuo
  Cc: Michael S. Tsirkin, netdev, Corentin Noël, virtualization,
	Jakub Kicinski, David S. Miller


在 2021/6/4 上午11:05, Xuan Zhuo 写道:
> On Fri, 4 Jun 2021 11:00:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2021/6/4 上午10:30, Xuan Zhuo 写道:
>>> On Fri, 4 Jun 2021 10:28:41 +0800, Jason Wang <jasowang@redhat.com> wrote:
>>>> 在 2021/6/4 上午1:09, Xuan Zhuo 写道:
>>>>> In virtio-net's large packet mode, there is a hole in the space behind
>>>>> buf.
>>>> before the buf actually or behind the vnet header?
>>>>
>>>>
>>>>>        hdr_padded_len - hdr_len
>>>>>
>>>>> We must take this into account when calculating tailroom.
>>>>>
>>>>> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
>>>>> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
>>>>> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
>>>>> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
>>>>> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
>>>>> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
>>>>> [   44.548251] __napi_poll (net/core/dev.c:6985)
>>>>> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
>>>>> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
>>>>> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
>>>>> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
>>>>> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>>>> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
>>>>>
>>>>> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
>>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>> Reported-by: Corentin Noël <corentin.noel@collabora.com>
>>>>> Tested-by: Corentin Noël <corentin.noel@collabora.com>
>>>>> ---
>>>>>     drivers/net/virtio_net.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>> index fa407eb8b457..78a01c71a17c 100644
>>>>> --- a/drivers/net/virtio_net.c
>>>>> +++ b/drivers/net/virtio_net.c
>>>>> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>     	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
>>>>>     	 */
>>>>>     	truesize = headroom ? PAGE_SIZE : truesize;
>>>>> -	tailroom = truesize - len - headroom;
>>>>> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>>>> The patch looks correct and I saw it has been merged.
>>>>
>>>> But I prefer to do that in receive_big() instead of here.
>>>>
>>>> Thanks
>>> How?
>>>
>>> change truesize or headroom?
>>>
>>> I didn't find a good way. Do you have a good way?
>>
>> Something like the following? The API is designed to let the caller to
>> pass a correct headroom instead of figure it out by itself.
>>
>>           struct sk_buff *skb =
>>                   page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0,
>> hdr_padded_len - hdr_len);
>>
>> Thanks
>
> This line may be affected.
>
> 	buf = p - headroom;
>
> In my opinion, this changes the semantics of the original headroom. The meaning
> of headroom in big mode and merge mode has become different. The more confusing
> problem is that the parameters of page_to_skb() are getting more and more
> chaotic.  So I wrote the previous patch. Of course, I understand your concern.
> This patch may bring Here are more questions, although I did a lot of tests.
>
> 	"virtio-net: Refactor the code related to page_to_skb"
>
> But I hope that our code development direction is as close to what this patch
> realizes. I hope that the meaning of the parameters can be more clear.


So I don't object to this method, but as I replied, it's better to do 
some benchmark to see if it introduces any regression


>
> Do you think this is ok?


Looks ok, but if we decide to go with your approach, it can be squashed 
into that patch.

Thanks


>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 78a01c71a17c..6d62bb45a188 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -380,34 +380,20 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   				   struct page *page, unsigned int offset,
>   				   unsigned int len, unsigned int truesize,
>   				   bool hdr_valid, unsigned int metasize,
> -				   unsigned int headroom)
> +				   int tailroom, char *buf,
> +				   unsigned int hdr_padded_len)
>   {
>   	struct sk_buff *skb;
>   	struct virtio_net_hdr_mrg_rxbuf *hdr;
> -	unsigned int copy, hdr_len, hdr_padded_len;
> +	unsigned int copy, hdr_len;
>   	struct page *page_to_free = NULL;
> -	int tailroom, shinfo_size;
> -	char *p, *hdr_p, *buf;
> +	int shinfo_size;
> +	char *p, *hdr_p;
>
>   	p = page_address(page) + offset;
>   	hdr_p = p;
>
>   	hdr_len = vi->hdr_len;
> -	if (vi->mergeable_rx_bufs)
> -		hdr_padded_len = sizeof(*hdr);
> -	else
> -		hdr_padded_len = sizeof(struct padded_vnet_hdr);
> -
> -	/* If headroom is not 0, there is an offset between the beginning of the
> -	 * data and the allocated space, otherwise the data and the allocated
> -	 * space are aligned.
> -	 *
> -	 * Buffers with headroom use PAGE_SIZE as alloc size, see
> -	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
> -	 */
> -	truesize = headroom ? PAGE_SIZE : truesize;
> -	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
> -	buf = p - headroom;
>
>   	len -= hdr_len;
>   	offset += hdr_padded_len;
> @@ -492,6 +478,51 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>   	return skb;
>   }
>
> +static struct sk_buff *merge_page_to_skb(struct virtnet_info *vi,
> +					 struct receive_queue *rq,
> +					 struct page *page, unsigned int offset,
> +					 unsigned int len, unsigned int truesize,
> +					 bool hdr_valid, unsigned int metasize,
> +					 unsigned int headroom)
> +{
> +	int tailroom;
> +	char *buf;
> +
> +	/* If headroom is not 0, there is an offset between the beginning of the
> +	 * data and the allocated space, otherwise the data and the allocated
> +	 * space are aligned.
> +	 *
> +	 * Buffers with headroom use PAGE_SIZE as alloc size, see
> +	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
> +	 */
> +	truesize = headroom ? PAGE_SIZE : truesize;
> +	tailroom = truesize - len - headroom;
> +	buf = page_address(page) + offset - headroom;
> +
> +	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
> +		    tailroom, buf, sizeof(struct virtio_net_hdr_mrg_rxbuf))
> +
> +}
> +
> +static struct sk_buff *big_page_to_skb(struct virtnet_info *vi,
> +				       struct receive_queue *rq,
> +				       struct page *page, unsigned int offset,
> +				       unsigned int len, unsigned int truesize,
> +				       bool hdr_valid, unsigned int metasize,
> +				       unsigned int headroom)
> +{
> +	char *p = page_address(page);
> +	int hold;
> +	int tailroom;
> +
> +	hold = sizeof(struct padded_vnet_hdr) - vi->hdr_len;
> +
> +	tailroom = truesize - len - headroom - hold;
> +
> +	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
> +		    tailroom, p, sizeof(struct padded_vnet_hdr));
> +}
> +
>   static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>   				   struct send_queue *sq,
>   				   struct xdp_frame *xdpf)
>
>
>>
>>> Thanks.
>>>
>>>>
>>>>>     	buf = p - headroom;
>>>>>
>>>>>     	len -= hdr_len;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-10-09  9:17 ` Michael S. Tsirkin
@ 2021-10-11  2:04   ` Jason Wang
  -1 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-10-11  2:04 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Xuan Zhuo, Greg KH, Corentin Noël,
	David S. Miller, Jakub Kicinski, virtualization, netdev

On Sat, Oct 9, 2021 at 5:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> commit 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
> accidentally reverted the effect of
> commit 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
> on drivers/net/virtio_net.c
>
> As a result, users of crosvm (which is using large packet mode)
> are experiencing crashes with 5.14-rc1 and above that do not
> occur with 5.13.
>
> Crash trace:
>
> [   61.346677] skbuff: skb_over_panic: text:ffffffff881ae2c7 len:3762 put:3762 head:ffff8a5ec8c22000 data:ffff8a5ec8c22010 tail:0xec2 end:0xec0 dev:<NULL>
> [   61.369192] kernel BUG at net/core/skbuff.c:111!
> [   61.372840] invalid opcode: 0000 [#1] SMP PTI
> [   61.374892] CPU: 5 PID: 0 Comm: swapper/5 Not tainted 5.14.0-rc1 linux-v5.14-rc1-for-mesa-ci.tar.bz2 #1
> [   61.376450] Hardware name: ChromiumOS crosvm, BIOS 0
>
> ..
>
> [   61.393635] Call Trace:
> [   61.394127]  <IRQ>
> [   61.394488]  skb_put.cold+0x10/0x10
> [   61.395095]  page_to_skb+0xf7/0x410
> [   61.395689]  receive_buf+0x81/0x1660
> [   61.396228]  ? netif_receive_skb_list_internal+0x1ad/0x2b0
> [   61.397180]  ? napi_gro_flush+0x97/0xe0
> [   61.397896]  ? detach_buf_split+0x67/0x120
> [   61.398573]  virtnet_poll+0x2cf/0x420
> [   61.399197]  __napi_poll+0x25/0x150
> [   61.399764]  net_rx_action+0x22f/0x280
> [   61.400394]  __do_softirq+0xba/0x257
> [   61.401012]  irq_exit_rcu+0x8e/0xb0
> [   61.401618]  common_interrupt+0x7b/0xa0
> [   61.402270]  </IRQ>
>
> See
> https://lore.kernel.org/r/5edaa2b7c2fe4abd0347b8454b2ac032b6694e2c.camel%40collabora.com
> for the report.
>
> Apply the original 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
> again, the original logic still holds:
>
> In virtio-net's large packet mode, there is a hole in the space behind
> buf.
>
>     hdr_padded_len - hdr_len
>
> We must take this into account when calculating tailroom.
>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> Fixes: 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reported-by: Corentin Noël <corentin.noel@collabora.com>
> Tested-by: Corentin Noël <corentin.noel@collabora.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

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

>
> David, I think we need this in stable, too.
>
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 096c2ac6b7a6..6b0812f44bbf 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>          * add_recvbuf_mergeable() + get_mergeable_buf_len()
>          */
>         truesize = headroom ? PAGE_SIZE : truesize;
> -       tailroom = truesize - len - headroom;
> +       tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>         buf = p - headroom;
>
>         len -= hdr_len;
> --
> MST
>


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-10-11  2:04   ` Jason Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2021-10-11  2:04 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Greg KH, linux-kernel, Corentin Noël, netdev,
	Jakub Kicinski, virtualization, David S. Miller

On Sat, Oct 9, 2021 at 5:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> commit 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
> accidentally reverted the effect of
> commit 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
> on drivers/net/virtio_net.c
>
> As a result, users of crosvm (which is using large packet mode)
> are experiencing crashes with 5.14-rc1 and above that do not
> occur with 5.13.
>
> Crash trace:
>
> [   61.346677] skbuff: skb_over_panic: text:ffffffff881ae2c7 len:3762 put:3762 head:ffff8a5ec8c22000 data:ffff8a5ec8c22010 tail:0xec2 end:0xec0 dev:<NULL>
> [   61.369192] kernel BUG at net/core/skbuff.c:111!
> [   61.372840] invalid opcode: 0000 [#1] SMP PTI
> [   61.374892] CPU: 5 PID: 0 Comm: swapper/5 Not tainted 5.14.0-rc1 linux-v5.14-rc1-for-mesa-ci.tar.bz2 #1
> [   61.376450] Hardware name: ChromiumOS crosvm, BIOS 0
>
> ..
>
> [   61.393635] Call Trace:
> [   61.394127]  <IRQ>
> [   61.394488]  skb_put.cold+0x10/0x10
> [   61.395095]  page_to_skb+0xf7/0x410
> [   61.395689]  receive_buf+0x81/0x1660
> [   61.396228]  ? netif_receive_skb_list_internal+0x1ad/0x2b0
> [   61.397180]  ? napi_gro_flush+0x97/0xe0
> [   61.397896]  ? detach_buf_split+0x67/0x120
> [   61.398573]  virtnet_poll+0x2cf/0x420
> [   61.399197]  __napi_poll+0x25/0x150
> [   61.399764]  net_rx_action+0x22f/0x280
> [   61.400394]  __do_softirq+0xba/0x257
> [   61.401012]  irq_exit_rcu+0x8e/0xb0
> [   61.401618]  common_interrupt+0x7b/0xa0
> [   61.402270]  </IRQ>
>
> See
> https://lore.kernel.org/r/5edaa2b7c2fe4abd0347b8454b2ac032b6694e2c.camel%40collabora.com
> for the report.
>
> Apply the original 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
> again, the original logic still holds:
>
> In virtio-net's large packet mode, there is a hole in the space behind
> buf.
>
>     hdr_padded_len - hdr_len
>
> We must take this into account when calculating tailroom.
>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> Fixes: 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reported-by: Corentin Noël <corentin.noel@collabora.com>
> Tested-by: Corentin Noël <corentin.noel@collabora.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

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

>
> David, I think we need this in stable, too.
>
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 096c2ac6b7a6..6b0812f44bbf 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>          * add_recvbuf_mergeable() + get_mergeable_buf_len()
>          */
>         truesize = headroom ? PAGE_SIZE : truesize;
> -       tailroom = truesize - len - headroom;
> +       tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>         buf = p - headroom;
>
>         len -= hdr_len;
> --
> MST
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode
  2021-10-09  9:17 ` Michael S. Tsirkin
  (?)
@ 2021-10-09  9:29 ` Xuan Zhuo
  -1 siblings, 0 replies; 16+ messages in thread
From: Xuan Zhuo @ 2021-10-09  9:29 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Greg KH, Corentin Noël, virtualization, netdev,
	Jakub Kicinski, David S. Miller, linux-kernel

On Sat, 9 Oct 2021 05:17:53 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> commit 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
> accidentally reverted the effect of
> commit 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
> on drivers/net/virtio_net.c
>
> As a result, users of crosvm (which is using large packet mode)
> are experiencing crashes with 5.14-rc1 and above that do not
> occur with 5.13.
>
> Crash trace:
>
> [   61.346677] skbuff: skb_over_panic: text:ffffffff881ae2c7 len:3762 put:3762 head:ffff8a5ec8c22000 data:ffff8a5ec8c22010 tail:0xec2 end:0xec0 dev:<NULL>
> [   61.369192] kernel BUG at net/core/skbuff.c:111!
> [   61.372840] invalid opcode: 0000 [#1] SMP PTI
> [   61.374892] CPU: 5 PID: 0 Comm: swapper/5 Not tainted 5.14.0-rc1 linux-v5.14-rc1-for-mesa-ci.tar.bz2 #1
> [   61.376450] Hardware name: ChromiumOS crosvm, BIOS 0
>
> ..
>
> [   61.393635] Call Trace:
> [   61.394127]  <IRQ>
> [   61.394488]  skb_put.cold+0x10/0x10
> [   61.395095]  page_to_skb+0xf7/0x410
> [   61.395689]  receive_buf+0x81/0x1660
> [   61.396228]  ? netif_receive_skb_list_internal+0x1ad/0x2b0
> [   61.397180]  ? napi_gro_flush+0x97/0xe0
> [   61.397896]  ? detach_buf_split+0x67/0x120
> [   61.398573]  virtnet_poll+0x2cf/0x420
> [   61.399197]  __napi_poll+0x25/0x150
> [   61.399764]  net_rx_action+0x22f/0x280
> [   61.400394]  __do_softirq+0xba/0x257
> [   61.401012]  irq_exit_rcu+0x8e/0xb0
> [   61.401618]  common_interrupt+0x7b/0xa0
> [   61.402270]  </IRQ>
>
> See
> https://lore.kernel.org/r/5edaa2b7c2fe4abd0347b8454b2ac032b6694e2c.camel%40collabora.com
> for the report.
>
> Apply the original 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
> again, the original logic still holds:
>
> In virtio-net's large packet mode, there is a hole in the space behind
> buf.
>
>     hdr_padded_len - hdr_len
>
> We must take this into account when calculating tailroom.
>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
> Fixes: 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Reported-by: Corentin Noël <corentin.noel@collabora.com>
> Tested-by: Corentin Noël <corentin.noel@collabora.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

LGTM

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

> ---
>
> David, I think we need this in stable, too.
>
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 096c2ac6b7a6..6b0812f44bbf 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
>  	 */
>  	truesize = headroom ? PAGE_SIZE : truesize;
> -	tailroom = truesize - len - headroom;
> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
>  	buf = p - headroom;
>
>  	len -= hdr_len;
> --
> MST
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-10-09  9:17 ` Michael S. Tsirkin
  0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2021-10-09  9:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: Xuan Zhuo, Greg KH, Corentin Noël, Jason Wang,
	David S. Miller, Jakub Kicinski, virtualization, netdev

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

commit 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
accidentally reverted the effect of
commit 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
on drivers/net/virtio_net.c

As a result, users of crosvm (which is using large packet mode)
are experiencing crashes with 5.14-rc1 and above that do not
occur with 5.13.

Crash trace:

[   61.346677] skbuff: skb_over_panic: text:ffffffff881ae2c7 len:3762 put:3762 head:ffff8a5ec8c22000 data:ffff8a5ec8c22010 tail:0xec2 end:0xec0 dev:<NULL>
[   61.369192] kernel BUG at net/core/skbuff.c:111!
[   61.372840] invalid opcode: 0000 [#1] SMP PTI
[   61.374892] CPU: 5 PID: 0 Comm: swapper/5 Not tainted 5.14.0-rc1 linux-v5.14-rc1-for-mesa-ci.tar.bz2 #1
[   61.376450] Hardware name: ChromiumOS crosvm, BIOS 0

..

[   61.393635] Call Trace:
[   61.394127]  <IRQ>
[   61.394488]  skb_put.cold+0x10/0x10
[   61.395095]  page_to_skb+0xf7/0x410
[   61.395689]  receive_buf+0x81/0x1660
[   61.396228]  ? netif_receive_skb_list_internal+0x1ad/0x2b0
[   61.397180]  ? napi_gro_flush+0x97/0xe0
[   61.397896]  ? detach_buf_split+0x67/0x120
[   61.398573]  virtnet_poll+0x2cf/0x420
[   61.399197]  __napi_poll+0x25/0x150
[   61.399764]  net_rx_action+0x22f/0x280
[   61.400394]  __do_softirq+0xba/0x257
[   61.401012]  irq_exit_rcu+0x8e/0xb0
[   61.401618]  common_interrupt+0x7b/0xa0
[   61.402270]  </IRQ>

See
https://lore.kernel.org/r/5edaa2b7c2fe4abd0347b8454b2ac032b6694e2c.camel%40collabora.com
for the report.

Apply the original 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
again, the original logic still holds:

In virtio-net's large packet mode, there is a hole in the space behind
buf.

    hdr_padded_len - hdr_len

We must take this into account when calculating tailroom.

Cc: Greg KH <gregkh@linuxfoundation.org>
Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
Fixes: 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reported-by: Corentin Noël <corentin.noel@collabora.com>
Tested-by: Corentin Noël <corentin.noel@collabora.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

David, I think we need this in stable, too.

 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 096c2ac6b7a6..6b0812f44bbf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
 	 */
 	truesize = headroom ? PAGE_SIZE : truesize;
-	tailroom = truesize - len - headroom;
+	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
 	buf = p - headroom;
 
 	len -= hdr_len;
-- 
MST


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH net] virtio-net: fix for skb_over_panic inside big mode
@ 2021-10-09  9:17 ` Michael S. Tsirkin
  0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2021-10-09  9:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: Greg KH, Corentin Noël, virtualization, netdev,
	Jakub Kicinski, David S. Miller

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

commit 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
accidentally reverted the effect of
commit 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
on drivers/net/virtio_net.c

As a result, users of crosvm (which is using large packet mode)
are experiencing crashes with 5.14-rc1 and above that do not
occur with 5.13.

Crash trace:

[   61.346677] skbuff: skb_over_panic: text:ffffffff881ae2c7 len:3762 put:3762 head:ffff8a5ec8c22000 data:ffff8a5ec8c22010 tail:0xec2 end:0xec0 dev:<NULL>
[   61.369192] kernel BUG at net/core/skbuff.c:111!
[   61.372840] invalid opcode: 0000 [#1] SMP PTI
[   61.374892] CPU: 5 PID: 0 Comm: swapper/5 Not tainted 5.14.0-rc1 linux-v5.14-rc1-for-mesa-ci.tar.bz2 #1
[   61.376450] Hardware name: ChromiumOS crosvm, BIOS 0

..

[   61.393635] Call Trace:
[   61.394127]  <IRQ>
[   61.394488]  skb_put.cold+0x10/0x10
[   61.395095]  page_to_skb+0xf7/0x410
[   61.395689]  receive_buf+0x81/0x1660
[   61.396228]  ? netif_receive_skb_list_internal+0x1ad/0x2b0
[   61.397180]  ? napi_gro_flush+0x97/0xe0
[   61.397896]  ? detach_buf_split+0x67/0x120
[   61.398573]  virtnet_poll+0x2cf/0x420
[   61.399197]  __napi_poll+0x25/0x150
[   61.399764]  net_rx_action+0x22f/0x280
[   61.400394]  __do_softirq+0xba/0x257
[   61.401012]  irq_exit_rcu+0x8e/0xb0
[   61.401618]  common_interrupt+0x7b/0xa0
[   61.402270]  </IRQ>

See
https://lore.kernel.org/r/5edaa2b7c2fe4abd0347b8454b2ac032b6694e2c.camel%40collabora.com
for the report.

Apply the original 1a8024239da ("virtio-net: fix for skb_over_panic inside big mode")
again, the original logic still holds:

In virtio-net's large packet mode, there is a hole in the space behind
buf.

    hdr_padded_len - hdr_len

We must take this into account when calculating tailroom.

Cc: Greg KH <gregkh@linuxfoundation.org>
Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
Fixes: 126285651b7f ("Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reported-by: Corentin Noël <corentin.noel@collabora.com>
Tested-by: Corentin Noël <corentin.noel@collabora.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

David, I think we need this in stable, too.

 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 096c2ac6b7a6..6b0812f44bbf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
 	 */
 	truesize = headroom ? PAGE_SIZE : truesize;
-	tailroom = truesize - len - headroom;
+	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
 	buf = p - headroom;
 
 	len -= hdr_len;
-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-10-11  2:05 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-03 17:09 [PATCH net] virtio-net: fix for skb_over_panic inside big mode Xuan Zhuo
2021-06-03 17:09 ` Xuan Zhuo
2021-06-03 22:30 ` patchwork-bot+netdevbpf
2021-06-04  2:28 ` Jason Wang
2021-06-04  2:28   ` Jason Wang
2021-06-04  2:30   ` Xuan Zhuo
2021-06-04  3:00     ` Jason Wang
2021-06-04  3:00       ` Jason Wang
2021-06-04  3:05       ` Xuan Zhuo
2021-06-04  6:00         ` Jason Wang
2021-06-04  6:00           ` Jason Wang
2021-10-09  9:17 Michael S. Tsirkin
2021-10-09  9:17 ` Michael S. Tsirkin
2021-10-09  9:29 ` Xuan Zhuo
2021-10-11  2:04 ` Jason Wang
2021-10-11  2:04   ` Jason Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.