Re: [PATCH net-next] net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension

From: Benoît Monin
Date: Mon Oct 21 2024 - 11:26:02 EST


10/10/2024 Benoît Monin wrote:
> 07/10/2024 Willem de Bruijn wrote:
> > Benoît Monin wrote:
> > > 07/10/2024 Willem de Bruijn wrote :
> > > > Benoît Monin wrote:
[...]
> > > > > Signed-off-by: Benoît Monin <benoit.monin@xxxxxx>
> > > > > ---
> > > > > net/core/dev.c | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > > index ea5fbcd133ae..199831d86ec1 100644
> > > > > --- a/net/core/dev.c
> > > > > +++ b/net/core/dev.c
> > > > > @@ -3639,6 +3639,9 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> > > > > return 0;
> > > > >
> > > > > if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
> > > > > + if (ip_hdr(skb)->version == 6 &&
> > > > > + skb_network_header_len(skb) != sizeof(struct ipv6hdr))
> > > > > + goto sw_checksum;
> >
> > This check depends on skb->transport_header and skb->network_header
> > being set. This is likely true for all CHECKSUM_PARTIAL packets that
> > originate in the local stack. As well as for the injected packets and
> > forwarded packets, as far as I see, so Ack.
> >
> > Access to the network header at this point likely requires
> > skb_header_pointer, however. As also used in qdisc_pkt_len_init called
> > from the same __dev_queue_xmit_nit.
> >
> > Perhaps this test should be in can_checksum_protocol, which already
> > checks that the packet is IPv6 when testing NETIF_F_IPV6_CSUM.
> >
> You're right, moving this to can_checksum_protocol() makes more sense. I will
> do that, retest and post a new version of the patch.
>
Looking more into it, can_checksum_protocol() is called from multiple places
where network header length cannot easily extracted, in particular from
vxlan_features_check().

How about keeping the length check in skb_csum_hwoffload_help() but using
vlan_get_protocol() to check for IPv6 instead of ip_hdr(skb)->version?

--
Benoît