linux-kernel - Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6

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: <ZeIQr3JWoly4JraV@ghost>
Date: Fri, 1 Mar 2024 09:30:23 -0800
From: Charlie Jenkins <charlie@...osinc.com>
To: David Laight <David.Laight@...lab.com>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>,
	Guenter Roeck <linux@...ck-us.net>,
	Palmer Dabbelt <palmer@...belt.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Helge Deller <deller@....de>,
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
	Parisc List <linux-parisc@...r.kernel.org>,
	Arnd Bergmann <arnd@...db.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Russell King <linux@...linux.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Palmer Dabbelt <palmer@...osinc.com>,
	Linux ARM <linux-arm-kernel@...ts.infradead.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v11] lib: checksum: Use aligned accesses for ip_fast_csum
 and csum_ipv6_magic tests

On Fri, Mar 01, 2024 at 05:24:39PM +0000, David Laight wrote:
> From: Charlie Jenkins
> > Sent: 01 March 2024 17:09
> > 
> > On Fri, Mar 01, 2024 at 07:17:38AM +0000, Christophe Leroy wrote:
> > > +CC netdev ARM Russell
> > >
> > > Le 29/02/2024 à 23:46, Charlie Jenkins a écrit :
> > > > The test cases for ip_fast_csum and csum_ipv6_magic were not properly
> > > > aligning the IP header, which were causing failures on architectures
> > > > that do not support misaligned accesses like some ARM platforms. To
> > > > solve this, align the data along (14 + NET_IP_ALIGN) bytes which is the
> > > > standard alignment of an IP header and must be supported by the
> > > > architecture.
> > >
> > > In your description, please provide more details on platforms that have
> > > a problem, what the problem is exactly (Failed calculation, slowliness,
> > > kernel Oops, panic, ....) on each platform.
> > >
> > > And please copy maintainers and lists of platforms your are specifically
> > > addressing with this change. And as this is network related, netdev list
> > > should have been copied as well.
> > >
> > > I still think that your patch is not the good approach, it looks like
> > > you are ignoring all the discussion. Below is a quote of what Geert said
> > > and I fully agree with that:
> > >
> > > 	IMHO the tests should validate the expected functionality.  If a test
> > > 	fails, either functionality is missing or behaves wrong, or the test
> > > 	is wrong.
> > >
> > > 	What is the point of writing tests for a core functionality like network
> > > 	checksumming that do not match the expected functionality?
> > >
> > >
> > > So we all agree that there is something to fix, because today's test
> > > does odd-address accesses which is unexpected for those functions, but
> > > 2-byte alignments should be supported hence tested by the test. Limiting
> > > the test to a 16-bytes alignment deeply reduces the usefullness of the test.
> > >
> > 
> > Maybe I am lost in the conversations. This isn't limited to 16-bytes
> > alignment? It aligns along 14 + NET_IP_ALIGN. That is 16 on some
> > platforms and 14 on platforms where unaligned accesses are desired.
> > These functions are expected to be called with this offset. Testing with
> > any other alignment is not the expected behavior. These tests are
> > testing the expected functionality.
> 
> Aligned received frames can have a 4 byte VLAN header (or two) removed.
> So the alignment of the IP header is either 4n or 4n+2.
> If the cpu fault misaligned accesses you really want the alignment
> to be 4n.
> 
> You pretty much never want to trap and fixup a misaligned access.
> Especially in the network stack.
> I suspect it is better to do a realignment copy of the entire frame.
> At some point the data will be copied again, although you may want
> a CBU (crystal ball unit) to decide whether to align on an 8n
> or 8n+4 boundary to optimise a later copy.
> 
> CPU that support misaligned transfers just make coders sloppy :-)
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Can you elaborate on how exactly you suggest the tests to be changed to
accomidate what you are saying here? I don't understand how what I have
proposed doesn't represent the use case of these functions.

- Charlie