[PATCH net-next] udp: Fix kernel panic in UDP GSO path

[PATCH net-next] udp: Fix kernel panic in UDP GSO path

[Sean Tranchetti]

Using GSO in the UDP path on a device with
scatter-gather netdevice feature disabled will result in a kernel
panic with the following call stack:

kernel BUG at net/core/skbuff.c:104!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
PC is at skb_panic+0x4c/0x54
LR is at skb_panic+0x4c/0x54
Process udpgso_bench_tx (pid: 4078, stack limit = 0xffffff8048de8000)
[<ffffff96e8790378>] skb_panic+0x4c/0x54
[<ffffff96e8788b54>] skb_copy_bits+0x0/0x244
[<ffffff96e8836088>] __ip_append_data+0x230/0x814
[<ffffff96e8837090>] ip_make_skb+0xe4/0x178
[<ffffff96e8865444>] udp_sendmsg+0x828/0x888
[<ffffff96e8872818>] inet_sendmsg+0xe4/0x130
[<ffffff96e877c894>] ___sys_sendmsg+0x1d8/0x2c0
[<ffffff96e877ca0c>] SyS_sendmsg+0x90/0xe0

This panic is the result of allocating SKBs with small size
for the newly segmented SKB. If the scatter-gather feature is
disabled, the code attempts to call skb_put() on the small SKB
with an argument of nearly the entire unsegmented SKB length.

After this patch, attempting to use GSO with scatter-gather
disabled will result in -EINVAL being returned.

Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 net/ipv4/ip_output.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index b5e21eb..0d63690 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1054,8 +1054,16 @@ static int __ip_append_data(struct sock *sk,
 			copy = length;
 
 		if (!(rt->dst.dev->features&NETIF_F_SG)) {
+			struct sk_buff *tmp;
 			unsigned int off;
 
+			if (paged) {
+				err = -EINVAL;
+				while ((tmp = __skb_dequeue(queue)) != NULL)
+					kfree(tmp);
+				goto error;
+			}
+
 			off = skb->len;
 			if (getfrag(from, skb_put(skb, copy),
 					offset, copy, off, skb) < 0) {
-- 
1.9.1

Re: [PATCH net-next] udp: Fix kernel panic in UDP GSO path

[Eric Dumazet]

On 05/10/2018 05:38 PM, Sean Tranchetti wrote:
> Using GSO in the UDP path on a device with
> scatter-gather netdevice feature disabled will result in a kernel
> panic with the following call stack:
>
> This panic is the result of allocating SKBs with small size
> for the newly segmented SKB. If the scatter-gather feature is
> disabled, the code attempts to call skb_put() on the small SKB
> with an argument of nearly the entire unsegmented SKB length.
> 
> After this patch, attempting to use GSO with scatter-gather
> disabled will result in -EINVAL being returned.
> 
> Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
> Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
>  net/ipv4/ip_output.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index b5e21eb..0d63690 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1054,8 +1054,16 @@ static int __ip_append_data(struct sock *sk,
>  			copy = length;
>  
>  		if (!(rt->dst.dev->features&NETIF_F_SG)) {
> +			struct sk_buff *tmp;
>  			unsigned int off;
>  
> +			if (paged) {
> +				err = -EINVAL;
> +				while ((tmp = __skb_dequeue(queue)) != NULL)
> +					kfree(tmp);
> +				goto error;
> +			}
> +
>  			off = skb->len;
>  			if (getfrag(from, skb_put(skb, copy),
>  					offset, copy, off, skb) < 0) {
> 


Hmm, no, we absolutely need to fix GSO instead.

Think of a bonding device (or any virtual devices), your patch wont avoid the crash.

Re: [PATCH net-next] udp: Fix kernel panic in UDP GSO path

[Willem de Bruijn]

On Thu, May 10, 2018 at 8:51 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/10/2018 05:38 PM, Sean Tranchetti wrote:
>> Using GSO in the UDP path on a device with
>> scatter-gather netdevice feature disabled will result in a kernel
>> panic with the following call stack:
>>
>> This panic is the result of allocating SKBs with small size
>> for the newly segmented SKB. If the scatter-gather feature is
>> disabled, the code attempts to call skb_put() on the small SKB
>> with an argument of nearly the entire unsegmented SKB length.
>>
>> After this patch, attempting to use GSO with scatter-gather
>> disabled will result in -EINVAL being returned.
>>
>> Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
>> Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
>> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
>> ---
>>  net/ipv4/ip_output.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
>> index b5e21eb..0d63690 100644
>> --- a/net/ipv4/ip_output.c
>> +++ b/net/ipv4/ip_output.c
>> @@ -1054,8 +1054,16 @@ static int __ip_append_data(struct sock *sk,
>>                       copy = length;
>>
>>               if (!(rt->dst.dev->features&NETIF_F_SG)) {
>> +                     struct sk_buff *tmp;
>>                       unsigned int off;
>>
>> +                     if (paged) {
>> +                             err = -EINVAL;
>> +                             while ((tmp = __skb_dequeue(queue)) != NULL)
>> +                                     kfree(tmp);
>> +                             goto error;
>> +                     }
>> +
>>                       off = skb->len;
>>                       if (getfrag(from, skb_put(skb, copy),
>>                                       offset, copy, off, skb) < 0) {
>>
>
>
> Hmm, no, we absolutely need to fix GSO instead.
>
> Think of a bonding device (or any virtual devices), your patch wont avoid the crash.

Thanks for reporting the issue.

Paged skbuffs is an optimization for gso, but the feature should
continue to work even if gso skbs are linear, indeed (if at the cost
of copying during skb_segment).

We need to make paged contingent on scatter-gather. Rough
patch below. That is for ipv4 only, the same will be needed for ipv6.

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index b5e21eb198d8..b38731d8a44f 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,

        exthdrlen = !skb ? rt->dst.header_len : 0;
        mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
-       paged = !!cork->gso_size;
+       paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);

Re: [PATCH net-next] udp: Fix kernel panic in UDP GSO path

[stranche]

On 2018-05-11 17:16, Willem de Bruijn wrote:

>> Hmm, no, we absolutely need to fix GSO instead.
>> 
>> Think of a bonding device (or any virtual devices), your patch wont 
>> avoid the crash.

Hi Eric. Can you clarify what you mean by "fix GSO?" Is that just having 
the GSO path work
regardless of whether or not SG is enabled for the device?

> 
> Thanks for reporting the issue.
> 
> Paged skbuffs is an optimization for gso, but the feature should
> continue to work even if gso skbs are linear, indeed (if at the cost
> of copying during skb_segment).
> 
> We need to make paged contingent on scatter-gather. Rough
> patch below. That is for ipv4 only, the same will be needed for ipv6.
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index b5e21eb198d8..b38731d8a44f 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,
> 
>         exthdrlen = !skb ? rt->dst.header_len : 0;
>         mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
> -       paged = !!cork->gso_size;
> +       paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);

Hi Willem. That's definitely a much cleaner patch than ours since it 
allows the GSO to continue without failure.
We tried it on both the IPv4 and IPv6 path and didn't see the crash in 
either case.

-----
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

Re: [PATCH net-next] udp: Fix kernel panic in UDP GSO path

[Willem de Bruijn]

>> Paged skbuffs is an optimization for gso, but the feature should
>> continue to work even if gso skbs are linear, indeed (if at the cost
>> of copying during skb_segment).
>>
>> We need to make paged contingent on scatter-gather. Rough
>> patch below. That is for ipv4 only, the same will be needed for ipv6.
>>
>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
>> index b5e21eb198d8..b38731d8a44f 100644
>> --- a/net/ipv4/ip_output.c
>> +++ b/net/ipv4/ip_output.c
>> @@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,
>>
>>         exthdrlen = !skb ? rt->dst.header_len : 0;
>>         mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
>> -       paged = !!cork->gso_size;
>> +       paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);
>
>
> Hi Willem. That's definitely a much cleaner patch than ours since it allows
> the GSO to continue without failure.
> We tried it on both the IPv4 and IPv6 path and didn't see the crash in
> either case.

Thanks for testing. I have a small set of fixes to udp gso, including
this one. Let me send them right away.

Re: [PATCH net-next] udp: Fix kernel panic in UDP GSO path

[Eric Dumazet]

On 05/14/2018 03:45 PM, stranche@codeaurora.org wrote:
> On 2018-05-11 17:16, Willem de Bruijn wrote:
> 
>>> Hmm, no, we absolutely need to fix GSO instead.
>>>
>>> Think of a bonding device (or any virtual devices), your patch wont avoid the crash.
> 
> Hi Eric. Can you clarify what you mean by "fix GSO?" Is that just having the GSO path work
> regardless of whether or not SG is enabled for the device?
>

Yes. GSO is a fallback, and must work all the time, not panic.