| Message ID | 20250225112852.2507709-1-mheib@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: Clear old fragment checksum value in napi_reuse_skb | expand |
On 2/25/2025 3:28 AM, Mohammad Heib wrote: > > In certain cases, napi_get_frags() returns an skb that points to an old > received fragment, This skb may have its skb->ip_summed, csum, and other > fields set from previous fragment handling. > > Some network drivers set skb->ip_summed to either CHECKSUM_COMPLETE or > CHECKSUM_UNNECESSARY when getting skb from napi_get_frags(), while > others only set skb->ip_summed when RX checksum offload is enabled on > the device, and do not set any value for skb->ip_summed when hardware > checksum offload is disabled, assuming that the skb->ip_summed > initiated to zero by napi_reuse_skb, ionic driver for example will > ignore/unset any value for the ip_summed filed if HW checksum offload is > disabled, and if we have a situation where the user disables the > checksum offload during a traffic that could lead to the following > errors shown in the kernel logs: > <IRQ> > dump_stack_lvl+0x34/0x48 > __skb_gro_checksum_complete+0x7e/0x90 > tcp6_gro_receive+0xc6/0x190 > ipv6_gro_receive+0x1ec/0x430 > dev_gro_receive+0x188/0x360 > ? ionic_rx_clean+0x25a/0x460 [ionic] > napi_gro_frags+0x13c/0x300 > ? __pfx_ionic_rx_service+0x10/0x10 [ionic] > ionic_rx_service+0x67/0x80 [ionic] > ionic_cq_service+0x58/0x90 [ionic] > ionic_txrx_napi+0x64/0x1b0 [ionic] > __napi_poll+0x27/0x170 > net_rx_action+0x29c/0x370 > handle_softirqs+0xce/0x270 > __irq_exit_rcu+0xa3/0xc0 > common_interrupt+0x80/0xa0 > </IRQ> > > This inconsistency sometimes leads to checksum validation issues in the > upper layers of the network stack. > > To resolve this, this patch clears the skb->ip_summed value for each > reused skb in by napi_reuse_skb(), ensuring that the caller is responsible > for setting the correct checksum status. This eliminates potential > checksum validation issues caused by improper handling of > skb->ip_summed. > > Fixes: 76620aafd66f ("gro: New frags interface to avoid copying shinfo") > Signed-off-by: Mohammad Heib <mheib@redhat.com> Thanks! Reviewed-by: Shannon Nelson <shannon.nelson@amd.com> > --- > net/core/gro.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/core/gro.c b/net/core/gro.c > index 78b320b63174..0ad549b07e03 100644 > --- a/net/core/gro.c > +++ b/net/core/gro.c > @@ -653,6 +653,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb) > skb->pkt_type = PACKET_HOST; > > skb->encapsulation = 0; > + skb->ip_summed = CHECKSUM_NONE; > skb_shinfo(skb)->gso_type = 0; > skb_shinfo(skb)->gso_size = 0; > if (unlikely(skb->slow_gro)) { > -- > 2.48.1 > >
On Tue, Feb 25, 2025 at 12:29 PM Mohammad Heib <mheib@redhat.com> wrote: > > In certain cases, napi_get_frags() returns an skb that points to an old > received fragment, This skb may have its skb->ip_summed, csum, and other > fields set from previous fragment handling. > > Some network drivers set skb->ip_summed to either CHECKSUM_COMPLETE or > CHECKSUM_UNNECESSARY when getting skb from napi_get_frags(), while > others only set skb->ip_summed when RX checksum offload is enabled on > the device, and do not set any value for skb->ip_summed when hardware > checksum offload is disabled, assuming that the skb->ip_summed > initiated to zero by napi_reuse_skb, ionic driver for example will > ignore/unset any value for the ip_summed filed if HW checksum offload is > disabled, and if we have a situation where the user disables the > checksum offload during a traffic that could lead to the following > errors shown in the kernel logs: It would be nice if you could get symbols with this trace ( scripts/decode_stacktrace.sh ) > <IRQ> > dump_stack_lvl+0x34/0x48 > __skb_gro_checksum_complete+0x7e/0x90 > tcp6_gro_receive+0xc6/0x190 > ipv6_gro_receive+0x1ec/0x430 > dev_gro_receive+0x188/0x360 > ? ionic_rx_clean+0x25a/0x460 [ionic] > napi_gro_frags+0x13c/0x300 > ? __pfx_ionic_rx_service+0x10/0x10 [ionic] > ionic_rx_service+0x67/0x80 [ionic] > ionic_cq_service+0x58/0x90 [ionic] > ionic_txrx_napi+0x64/0x1b0 [ionic] > __napi_poll+0x27/0x170 > net_rx_action+0x29c/0x370 > handle_softirqs+0xce/0x270 > __irq_exit_rcu+0xa3/0xc0 > common_interrupt+0x80/0xa0 > </IRQ> > Note : This suggests ionic driver could add one skb_checksum_none_assert() as other drivers did Reviewed-by: Eric Dumazet <edumazet@google.com>
diff --git a/net/core/gro.c b/net/core/gro.c index 78b320b63174..0ad549b07e03 100644 --- a/net/core/gro.c +++ b/net/core/gro.c @@ -653,6 +653,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb) skb->pkt_type = PACKET_HOST; skb->encapsulation = 0; + skb->ip_summed = CHECKSUM_NONE; skb_shinfo(skb)->gso_type = 0; skb_shinfo(skb)->gso_size = 0; if (unlikely(skb->slow_gro)) {
In certain cases, napi_get_frags() returns an skb that points to an old received fragment, This skb may have its skb->ip_summed, csum, and other fields set from previous fragment handling. Some network drivers set skb->ip_summed to either CHECKSUM_COMPLETE or CHECKSUM_UNNECESSARY when getting skb from napi_get_frags(), while others only set skb->ip_summed when RX checksum offload is enabled on the device, and do not set any value for skb->ip_summed when hardware checksum offload is disabled, assuming that the skb->ip_summed initiated to zero by napi_reuse_skb, ionic driver for example will ignore/unset any value for the ip_summed filed if HW checksum offload is disabled, and if we have a situation where the user disables the checksum offload during a traffic that could lead to the following errors shown in the kernel logs: <IRQ> dump_stack_lvl+0x34/0x48 __skb_gro_checksum_complete+0x7e/0x90 tcp6_gro_receive+0xc6/0x190 ipv6_gro_receive+0x1ec/0x430 dev_gro_receive+0x188/0x360 ? ionic_rx_clean+0x25a/0x460 [ionic] napi_gro_frags+0x13c/0x300 ? __pfx_ionic_rx_service+0x10/0x10 [ionic] ionic_rx_service+0x67/0x80 [ionic] ionic_cq_service+0x58/0x90 [ionic] ionic_txrx_napi+0x64/0x1b0 [ionic] __napi_poll+0x27/0x170 net_rx_action+0x29c/0x370 handle_softirqs+0xce/0x270 __irq_exit_rcu+0xa3/0xc0 common_interrupt+0x80/0xa0 </IRQ> This inconsistency sometimes leads to checksum validation issues in the upper layers of the network stack. To resolve this, this patch clears the skb->ip_summed value for each reused skb in by napi_reuse_skb(), ensuring that the caller is responsible for setting the correct checksum status. This eliminates potential checksum validation issues caused by improper handling of skb->ip_summed. Fixes: 76620aafd66f ("gro: New frags interface to avoid copying shinfo") Signed-off-by: Mohammad Heib <mheib@redhat.com> --- net/core/gro.c | 1 + 1 file changed, 1 insertion(+)