netdev - RE: [PATCH net] splice: do not checksum AF

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: <aef4869613f6418a9cc01ba4f012520c@AcuMS.aculab.com>
Date: Sat, 30 Nov 2024 12:08:09 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Cong Wang' <xiyou.wangcong@...il.com>, Frederik Deweerdt
	<deweerdt.lkml@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "dhowells@...hat.com"
	<dhowells@...hat.com>
Subject: RE: [PATCH net] splice: do not checksum AF_UNIX sockets

From: Cong Wang <xiyou.wangcong@...il.com>
> Sent: 30 November 2024 01:43
> 
> On Fri, Nov 29, 2024 at 03:20:14PM -0800, Frederik Deweerdt wrote:
> > When `skb_splice_from_iter` was introduced, it inadvertently added
> > checksumming for AF_UNIX sockets. This resulted in significant
> > slowdowns, as when using sendfile over unix sockets.
> >
> > Using the test code [1] in my test setup (2G, single core x86_64 qemu),
> > the client receives a 1000M file in:
> > - without the patch: 1577ms (+/- 36.1ms)
> > - with the patch: 725ms (+/- 28.3ms)
> >
> > This commit skips addresses the issue by skipping checksumming when
> > splice occurs a AF_UNIX socket.
> >
> > [1] https://gist.github.com/deweerdt/a3ee2477d1d87524cf08618d3c179f06
> >
> > Signed-off-by: Frederik Deweerdt <deweerdt.lkml@...il.com>
> > Fixes: 2e910b95329c ("net: Add a function to splice pages into an skbuff for MSG_SPLICE_PAGES")
> > ---
> >  net/core/skbuff.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 6841e61a6bd0..49e4f9ab625f 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -7233,7 +7233,7 @@ ssize_t skb_splice_from_iter(struct sk_buff *skb, struct iov_iter *iter,
> >  				goto out;
> >  			}
> >
> > -			if (skb->ip_summed == CHECKSUM_NONE)
> > +			if (skb->ip_summed == CHECKSUM_NONE && skb->sk->sk_family != AF_UNIX)
> 
> Are you sure it is always safe to dereferene skb->sk here? I am not sure
> about the KCM socket case.
> 
> Instead of checking skb->sk->sk_family, why not just pass an additional
> boolean parameter to this function?

A thought.
It is ever actually worth doing an 'early checksum' in software for either
TCP or UDP?
Most modern ethernet hardware supports transmit checksum offload, so there
is nothing to be gained by doing the checksum during a copy and everything
to be lost because the copy is a lot slower.

I think the code always does a checksum when copying data in send()
pretty much all the time it isn't needed.

The same is true for the delayed checksum of receive UDP.
I'm not sure what the rational for that was (and Linus can't remember),
my guess is a userspace NFS daemon.
But it seriously complicated the code.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)