netdev - Re: [PATCH net] ravb: expand rx descriptor data to accommodate hw checksum

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: <20190111154843.k7pejayjrkwvj4pp@verge.net.au>
Date:   Fri, 11 Jan 2019 16:48:47 +0100
From:   Simon Horman <horms@...ge.net.au>
To:     Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:     David Miller <davem@...emloft.net>,
        Magnus Damm <magnus.damm@...il.com>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH net] ravb: expand rx descriptor data to accommodate hw
 checksum

On Fri, Jan 11, 2019 at 06:22:12PM +0300, Sergei Shtylyov wrote:
> On 01/11/2019 04:35 PM, Simon Horman wrote:
> 
> >>> EtherAVB may provide a checksum of packet data appended to packet data. In
> >>> order to allow this checksum to be received by the host descriptor data
> >>> needs to be enlarged by 2 bytes to accommodate the checksum.
> >>>
> >>> In the case of MTU-sized packets without a VLAN tag the
> >>> checksum were already accommodated by virtue of the space reserved for the
> >>> VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
> >>> packet data space provided by a descriptor leaving no space for the
> >>> trailing checksum.
> >>
> >>    Wait! The gen3 manual is rather clear about the auto-checksumming not working
> >> right in the presence of the VLAN tag. Where do you check for that case?
> > 
> > In my testing on E3 this works correctly. Which portion of
> > the manual are you referring to?
> 
>    Section 50.4.1 in R-Car Series, 3rd Generation User’s Manual: Hardware, Rev 1.00,
> section 45A.3.14.1 in R-Car Series, 2nd Generation User’s Manual: Hardware, Rev 2.00.
>    The problem is that the checksum is always calculated starting at byte
> 14, i.e.  amidst the VLAN header (if present).

Yes, I understand that. What I'm unclear on is why that is a problem.
Empirically I have observed that RX csum offload works in the presence
of a VLAN tag and offers a significant benefit in terms of reduced CPU
utilisation.

> >>> This was not detected by the driver which incorrectly used the last two
> >>> bytes of packet data as the checksum and truncate the packet by two bytes.
> >>> This resulted all such packets being dropped.
> >>>
> >>> A work around is to disable rx checksum offload
> >>>  # ethtool -K eth0 rx off
> >>>
> >>> This patch resolves this problem by increasing the size available for
> >>> packet data in rx descriptors by two bytes. It also introduces
> >>> RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
> >>> over the driver.
> >>
> >>    What about using sizeof(__sum16) instead? That type is declared in
> >> <linux/types.h> and used in 'struct iphdr'...
> > 
> > As in the following?
> > 
> > #define RAVB_CSUM_LEN sizeof(__sum16)
> 
>    No, as I said, I'd prefer to avoid any driver specific #define's.

If that is your preference I can update the patch accordingly.

> >>> Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
> >>>
> >>> Fixes: 4d86d3818627 ("ravb: RX checksum offload")
> >>> Signed-off-by: Simon Horman <horms+renesas@...ge.net.au>
> >> [...]
> 
> MBR, Sergei
>