netdev - RE: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented packets

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] [day] [month] [year] [list]

Message-ID: <BL3PR02MB81874EDDEB16BFEE8E3C8405C9CB9@BL3PR02MB8187.namprd02.prod.outlook.com>
Date:   Thu, 12 May 2022 17:09:17 +0000
From:   Harini Katakam <harinik@...inx.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Nicolas Ferre <nicolas.ferre@...rochip.com>,
        David Miller <davem@...emloft.net>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        "dumazet@...gle.com" <dumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Michal Simek <michals@...inx.com>,
        Radhey Shyam Pandey <radheys@...inx.com>
Subject: RE: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented
 packets

Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Thursday, May 12, 2022 9:15 PM
> To: Harini Katakam <harinik@...inx.com>
> Cc: Harini Katakam <harinik@...inx.com>; Nicolas Ferre
> <nicolas.ferre@...rochip.com>; David Miller <davem@...emloft.net>;
> Claudiu Beznea <claudiu.beznea@...rochip.com>; dumazet@...gle.com;
> Paolo Abeni <pabeni@...hat.com>; netdev <netdev@...r.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@...r.kernel.org>; Michal Simek
> <michals@...inx.com>; Radhey Shyam Pandey <radheys@...inx.com>
> Subject: Re: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented
> packets
> 
> On Thu, 12 May 2022 12:26:15 +0530 Harini Katakam wrote:
> > On Thu, May 12, 2022 at 4:10 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > > On Tue, 10 May 2022 21:58:09 +0530 Harini Katakam wrote:
> > > > data_len in skbuff represents bytes resident in fragment lists or
> > > > unmapped page buffers. For such packets, when data_len is
> > > > non-zero, skb_put cannot be used - this will throw a kernel bug.
> > > > Hence do not use macb_pad_and_fcs for such fragments.
> > > >
> > > > Fixes: 653e92a9175e ("net: macb: add support for padding and fcs
> > > > computation")
> > > > Signed-off-by: Harini Katakam <harini.katakam@...inx.com>
> > > > Signed-off-by: Michal Simek <michal.simek@...inx.com>
> > > > Signed-off-by: Radhey Shyam Pandey
> > > > <radhey.shyam.pandey@...inx.com>
> > > > Reviewed-by: Claudiu Beznea <claudiu.beznea@...rochip.com>
> > >
> > > I'm confused. When do we *have to* compute the FCS?
> > >
> > > This commit seems to indicate that we can't put the FCS so it's okay
> > > to ask the HW to do it. But that's backwards. We should ask the HW
> > > to compute the FCS whenever possible, to save the CPU cycles.
> > >
> > > Is there an unstated HW limitation here?
> >
> > Thanks for the review. The top level summary is that there CSUM
> > offload is enabled by via NETIF_F_HW_CSUM (and universally in IP
> > registers) and then selectively disabled for certain packets (using
> > NOCRC bit in buffer descriptors) where the application intentionally
> > performs CSUM and HW should not replace it, for ex. forwarding usecases.
> > I'm modifying this list of exceptions with this patch.
> >
> > This was due to HW limitation (see
> > https://www.spinics.net/lists/netdev/msg505065.html).
> > Further to this, Claudiu added macb_pad_and_fcs support. Please see
> > comment starting with "It was reported in" below:
> > https://lists.openwall.net/netdev/2018/10/30/76
> >
> > Hope this helps.
> > I'll fix the nit and send another version.
> 
> So the NOCRC bit controls both ethernet and transport protocol checksums?
> The CRC in the name is a little confusing.

Yes

> 
> Are you sure commit 403dc16796f5 ("cadence: force nonlinear buffers to be
> cloned") does not fix the case you're trying to address?

Thanks for this pointer. Yes, this patch already addresses the bug on linear skb buffers
and solves the issue I was hitting.

Apologies for the inconvenience. Please ignore this patch.

I had to unfortunately work on an tightly coupled older kernel and app to reproduce this
particular issue and hence could only verify whether my patch *works* on 5.18, not whether
it was *needed*. I backported 403dc16796f5 ("cadence: force nonlinear buffers to be cloned")
and it works as expected.

Regards,
Harini