udp: Fix kernel panic in UDP GSO path
Subsystems (3)
PATCH 1/1 udp: Fix kernel panic in UDP GSO path
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.
--- net/ipv4/ip_output.c | 8 ++++++++ 1 file changed, 8 insertions(+)
net/ipv4/ip_output.c +8 -0
@@ -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 ) {
Signature · 1 line
1.9.1
Quoted 31 lines · 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:
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
-EINVALbeing returned.Fixes 15e36f5b8e98 ("udp: paged allocation with gso")Signed-off-by Sean Tranchetti <[email protected]>Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>--- net/ipv4/ip_output.c | 8 ++++++++ 1 file changed, 8 insertions(+)net/ipv4/ip_output.c +1
-0@@ -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;
net/ipv4/ip_output.c static int __ip_append_data(struct sock *sk, · line 1069
Quoted 10 lines
net/ipv4/ip_output.c +7
-0+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.
Quoted 48 lines · Eric Dumazet
Quoted 31 lines · 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:
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
-EINVALbeing returned.Fixes 15e36f5b8e98 ("udp: paged allocation with gso")Signed-off-by Sean Tranchetti <[email protected]>Signed-off-by Subash Abhinov Kasiviswanathan <[email protected]>--- net/ipv4/ip_output.c | 8 ++++++++ 1 file changed, 8 insertions(+)net/ipv4/ip_output.c +1
-0@@ -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;net/ipv4/ip_output.c static int __ip_append_data(struct sock *sk, · line 1069
Quoted 10 lines
net/ipv4/ip_output.c +7
-0+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.
Quoted 1 line
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.
net/ipv4/ip_output.c +1 -1
@@ -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 );
Quoted 3 lines · Willem de Bruijn
Quoted 3 lines
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?
Quoted 15 lines
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.
net/ipv4/ip_output.c +0
-0
net/ipv4/ip_output.c static int __ip_append_data(struct sock *sk, · line 887
Quoted 5 lines
net/ipv4/ip_output.c +1
-1@@ -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
Quoted 13 lines
Quoted 13 lines
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.
net/ipv4/ip_output.c +0
-0@@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,
Quoted 10 lines
net/ipv4/ip_output.c static int __ip_append_data(struct sock *sk, · line 887
Quoted 4 lines
net/ipv4/ip_output.c +1
-1exthdrlen = !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.
Quoted 9 lines · On 05/14/2018 03:45 PM, [email protected] wrote:
Quoted 3 lines · Willem de Bruijn
Quoted 3 lines
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.