| Message ID | 20250226171352.258045-1-atenart@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: gso: fix ownership in __udp_gso_segment | expand |
Quoting Antoine Tenart (2025-02-26 18:13:42) > In __udp_gso_segment the skb destructor is removed before segmenting the > skb but the socket reference is kept as-is. This is an issue if the > original skb is later orphaned as we can hit the following bug: > > kernel BUG at ./include/linux/skbuff.h:3312! (skb_orphan) > RIP: 0010:ip_rcv_core+0x8b2/0xca0 > Call Trace: > ip_rcv+0xab/0x6e0 > __netif_receive_skb_one_core+0x168/0x1b0 > process_backlog+0x384/0x1100 > __napi_poll.constprop.0+0xa1/0x370 > net_rx_action+0x925/0xe50 > > The above can happen following a sequence of events when using > OpenVSwitch, when an OVS_ACTION_ATTR_USERSPACE action precedes an > OVS_ACTION_ATTR_OUTPUT action: > > 1. OVS_ACTION_ATTR_USERSPACE is handled (in do_execute_actions): the skb > goes through queue_gso_packets and then __udp_gso_segment, where its > destructor is removed. > 2. The segments' data are copied and sent to userspace. > 3. OVS_ACTION_ATTR_OUTPUT is handled (in do_execute_actions) and the > same original skb is sent to its path. > 4. If it later hits skb_orphan, we hit the bug. While AFAIU removing the skb destructor while not removing the socket reference is not often wanted (we could add an helper as it's not the first time we hit this?), I also looked at the OvS side for completeness. The following also fixes the above, --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -1407,15 +1407,19 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, break; } - case OVS_ACTION_ATTR_USERSPACE: - output_userspace(dp, skb, key, a, attr, - len, OVS_CB(skb)->cutlen); + case OVS_ACTION_ATTR_USERSPACE: { + struct sk_buff *clone; + + clone = nla_is_last(a, rem) ? skb : skb_clone(skb, GFP_ATOMIC); + if (clone) + output_userspace(dp, clone, key, a, attr, len, + OVS_CB(skb)->cutlen); OVS_CB(skb)->cutlen = 0; - if (nla_is_last(a, rem)) { - consume_skb(skb); + consume_skb(clone); + if (nla_is_last(a, rem)) return 0; - } break; + } output_userspace could be seen as a different path where cloning is wanted, but at the same time it's only copying the packet content and I didn't see a reason not to allow reusing the original skb after calling __skb_gso_segment in the GSO case. Thoughts? Thanks! Antoine
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index a5be6e4ed326..ecfca59f31f1 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -321,13 +321,17 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb, /* clear destructor to avoid skb_segment assigning it to tail */ copy_dtor = gso_skb->destructor == sock_wfree; - if (copy_dtor) + if (copy_dtor) { gso_skb->destructor = NULL; + gso_skb->sk = NULL; + } segs = skb_segment(gso_skb, features); if (IS_ERR_OR_NULL(segs)) { - if (copy_dtor) + if (copy_dtor) { gso_skb->destructor = sock_wfree; + gso_skb->sk = sk; + } return segs; }
In __udp_gso_segment the skb destructor is removed before segmenting the skb but the socket reference is kept as-is. This is an issue if the original skb is later orphaned as we can hit the following bug: kernel BUG at ./include/linux/skbuff.h:3312! (skb_orphan) RIP: 0010:ip_rcv_core+0x8b2/0xca0 Call Trace: ip_rcv+0xab/0x6e0 __netif_receive_skb_one_core+0x168/0x1b0 process_backlog+0x384/0x1100 __napi_poll.constprop.0+0xa1/0x370 net_rx_action+0x925/0xe50 The above can happen following a sequence of events when using OpenVSwitch, when an OVS_ACTION_ATTR_USERSPACE action precedes an OVS_ACTION_ATTR_OUTPUT action: 1. OVS_ACTION_ATTR_USERSPACE is handled (in do_execute_actions): the skb goes through queue_gso_packets and then __udp_gso_segment, where its destructor is removed. 2. The segments' data are copied and sent to userspace. 3. OVS_ACTION_ATTR_OUTPUT is handled (in do_execute_actions) and the same original skb is sent to its path. 4. If it later hits skb_orphan, we hit the bug. Fix this by also removing the reference to the socket in __udp_gso_segment. Fixes: ad405857b174 ("udp: better wmem accounting on gso") Signed-off-by: Antoine Tenart <atenart@kernel.org> --- net/ipv4/udp_offload.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)