netdev - Re: [PATCHv3 net-next 1/2] net: support ip generic csum processing in skb_csum_hwoffload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0Uchuef=e2w5grNitLM1NzsV--6QGnwvKuffMPisVAR0UA@mail.gmail.com>
Date:   Thu, 28 Jan 2021 11:46:27 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Xin Long <lucien.xin@...il.com>,
        network dev <netdev@...r.kernel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Davide Caratti <dcaratti@...hat.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCHv3 net-next 1/2] net: support ip generic csum processing in skb_csum_hwoffload_help

On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Thu, Jan 28, 2021 at 4:29 AM Xin Long <lucien.xin@...il.com> wrote:
> >
> > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload
> > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload
> > for HW, which includes not only for TCP/UDP csum, but also for other
> > protocols' csum like GRE's.
> >
> > However, in skb_csum_hwoffload_help() it only checks features against
> > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP
> > packet and the features doesn't support NETIF_F_HW_CSUM, but supports
> > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW
> > to do csum.
> >
> > This patch is to support ip generic csum processing by checking
> > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM |
> > NETIF_F_IPV6_CSUM) only for TCP and UDP.
> >
> > Note that we're using skb->csum_offset to check if it's a TCP/UDP
> > proctol, this might be fragile. However, as Alex said, for now we
> > only have a few L4 protocols that are requesting Tx csum offload,
> > we'd better fix this until a new protocol comes with a same csum
> > offset.
> >
> > v1->v2:
> >   - not extend skb->csum_not_inet, but use skb->csum_offset to tell
> >     if it's an UDP/TCP csum packet.
> > v2->v3:
> >   - add a note in the changelog, as Willem suggested.
> >
> > Suggested-by: Alexander Duyck <alexander.duyck@...il.com>
> > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > ---
> >  net/core/dev.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 6df3f1b..aae116d 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
> >                 return !!(features & NETIF_F_SCTP_CRC) ? 0 :
> >                         skb_crc32c_csum_help(skb);
> >
> > -       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
> > +       if (features & NETIF_F_HW_CSUM)
> > +               return 0;
> > +
> > +       if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
>
> Should this check the specific feature flag against skb->protocol? I
> don't know if there are actually instances that only support one of
> the two flags.

The issue is at a certain point we start excluding devices that were
previously working.

All this patch is really doing is using the checksum offset to
identify the cases that were previously UDP or TCP offloads and
letting those through with the legacy path, while any offsets that are
not those two, such as the GRE checksum will now have to be explicitly
caught by the NETIF_F_HW_CSUM case and not accepted by the other
cases.