| Message ID | 20240315151722.119628-3-atenart@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | gro: various fixes related to UDP tunnels | expand |
Antoine Tenart wrote: > Issue was found while using rx-gro-list. If fragmented packets are GROed Only if you need to respin: "If packets are GROed with fraglist" A bit pedantic, but this is subtle stuff. These are not IP fragmented packets. Or worse, UDP fragmentation offload. > in skb_gro_receive_list, they might be segmented later on and continue > their journey in the stack. In skb_segment_list those skbs can be reused > as-is. This is an issue as their destructor was removed in > skb_gro_receive_list but not the reference to their socket, and then > they can't be orphaned. Fix this by also removing the reference to the > socket. > > For example this could be observed, > > kernel BUG at include/linux/skbuff.h:3131! (skb_orphan) > RIP: 0010:ip6_rcv_core+0x11bc/0x19a0 > Call Trace: > ipv6_list_rcv+0x250/0x3f0 > __netif_receive_skb_list_core+0x49d/0x8f0 > netif_receive_skb_list_internal+0x634/0xd40 > napi_complete_done+0x1d2/0x7d0 > gro_cell_poll+0x118/0x1f0 > > A similar construction is found in skb_gro_receive, apply the same > change there. > > Fixes: 5e10da5385d2 ("skbuff: allow 'slow_gro' for skb carring sock reference") > Signed-off-by: Antoine Tenart <atenart@kernel.org> Looks fine to me on the understanding that the only GSO packets that arrive with skb->sk are are result of the referenced commit, and thus had sock_wfree as destructor.
Quoting Willem de Bruijn (2024-03-16 16:25:13) > Antoine Tenart wrote: > > Issue was found while using rx-gro-list. If fragmented packets are GROed > > Only if you need to respin: "If packets are GROed with fraglist" > > A bit pedantic, but this is subtle stuff. These are not IP fragmented > packets. Or worse, UDP fragmentation offload. Right, that was only describing which kind of packets were GROed in my test. Looks like that's confusing, I'll remove it. > > in skb_gro_receive_list, they might be segmented later on and continue > > their journey in the stack. In skb_segment_list those skbs can be reused > > as-is. This is an issue as their destructor was removed in > > skb_gro_receive_list but not the reference to their socket, and then > > they can't be orphaned. Fix this by also removing the reference to the > > socket. > > > > For example this could be observed, > > > > kernel BUG at include/linux/skbuff.h:3131! (skb_orphan) > > RIP: 0010:ip6_rcv_core+0x11bc/0x19a0 > > Call Trace: > > ipv6_list_rcv+0x250/0x3f0 > > __netif_receive_skb_list_core+0x49d/0x8f0 > > netif_receive_skb_list_internal+0x634/0xd40 > > napi_complete_done+0x1d2/0x7d0 > > gro_cell_poll+0x118/0x1f0 > > > > A similar construction is found in skb_gro_receive, apply the same > > change there. > > > > Fixes: 5e10da5385d2 ("skbuff: allow 'slow_gro' for skb carring sock reference") > > Signed-off-by: Antoine Tenart <atenart@kernel.org> > > Looks fine to me on the understanding that the only GSO packets that > arrive with skb->sk are are result of the referenced commit, and thus > had sock_wfree as destructor. The root cause of the issue is a disparity between skb->destructor and skb->sk; either skb with skb->{destructor,sk} could arrive there and that was not an issue, or they could not. In both cases the above commit is introducing that behavior. Thanks! Antoine
diff --git a/net/core/gro.c b/net/core/gro.c index ee30d4f0c038..83f35d99a682 100644 --- a/net/core/gro.c +++ b/net/core/gro.c @@ -192,8 +192,9 @@ int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb) } merge: - /* sk owenrship - if any - completely transferred to the aggregated packet */ + /* sk ownership - if any - completely transferred to the aggregated packet */ skb->destructor = NULL; + skb->sk = NULL; delta_truesize = skb->truesize; if (offset > headlen) { unsigned int eat = offset - headlen; diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index e9719afe91cf..3bb69464930b 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -449,8 +449,9 @@ static int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb) NAPI_GRO_CB(p)->count++; p->data_len += skb->len; - /* sk owenrship - if any - completely transferred to the aggregated packet */ + /* sk ownership - if any - completely transferred to the aggregated packet */ skb->destructor = NULL; + skb->sk = NULL; p->truesize += skb->truesize; p->len += skb->len;
Issue was found while using rx-gro-list. If fragmented packets are GROed in skb_gro_receive_list, they might be segmented later on and continue their journey in the stack. In skb_segment_list those skbs can be reused as-is. This is an issue as their destructor was removed in skb_gro_receive_list but not the reference to their socket, and then they can't be orphaned. Fix this by also removing the reference to the socket. For example this could be observed, kernel BUG at include/linux/skbuff.h:3131! (skb_orphan) RIP: 0010:ip6_rcv_core+0x11bc/0x19a0 Call Trace: ipv6_list_rcv+0x250/0x3f0 __netif_receive_skb_list_core+0x49d/0x8f0 netif_receive_skb_list_internal+0x634/0xd40 napi_complete_done+0x1d2/0x7d0 gro_cell_poll+0x118/0x1f0 A similar construction is found in skb_gro_receive, apply the same change there. Fixes: 5e10da5385d2 ("skbuff: allow 'slow_gro' for skb carring sock reference") Signed-off-by: Antoine Tenart <atenart@kernel.org> --- net/core/gro.c | 3 ++- net/ipv4/udp_offload.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)