netdev - Re: [PATCH net-next v3 10/18] nvme/host: Use sendmsg(MSG_SPLICE

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20230630192825.GA2745548@dev-arch.thelio-3990X>
Date: Fri, 30 Jun 2023 12:28:25 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: David Howells <dhowells@...hat.com>, Aurelien Aptel <aaptel@...dia.com>,
	netdev@...r.kernel.org, Alexander Duyck <alexander.duyck@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	David Ahern <dsahern@...nel.org>,
	Matthew Wilcox <willy@...radead.org>, Jens Axboe <axboe@...nel.dk>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	Sagi Grimberg <sagi@...mberg.me>,
	Willem de Bruijn <willemb@...gle.com>,
	Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...com>,
	Christoph Hellwig <hch@....de>, Chaitanya Kulkarni <kch@...dia.com>,
	linux-nvme@...ts.infradead.org, llvm@...ts.linux.dev
Subject: Re: [PATCH net-next v3 10/18] nvme/host: Use
 sendmsg(MSG_SPLICE_PAGES) rather then sendpage

On Fri, Jun 30, 2023 at 09:14:42AM -0700, Jakub Kicinski wrote:
> On Fri, 30 Jun 2023 09:10:43 -0700 Nathan Chancellor wrote:
> > > Let me CC llvm@ in case someone's there is willing to make 
> > > the compiler warn about this.
> > 
> > Turns out clang already has a warning for this, -Wcomma:
> > 
> >   drivers/nvme/host/tcp.c:1017:38: error: possible misuse of comma operator here [-Werror,-Wcomma]
> >    1017 |                         msg.msg_flags &= ~MSG_SPLICE_PAGES,
> >         |                                                           ^
> >   drivers/nvme/host/tcp.c:1017:4: note: cast expression to void to silence warning
> >    1017 |                         msg.msg_flags &= ~MSG_SPLICE_PAGES,
> >         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >         |                         (void)(                           )
> >   1 error generated.
> > 
> > Let me do some wider build testing to see if it is viable to turn this
> > on for the whole kernel because it seems worth it, at least in this
> > case. There are a lot of cases where a warning won't be emitted (see the
> > original upstream review for a list: https://reviews.llvm.org/D3976) but
> > something is better than nothing, right? :)

Well, that was a pipe dream :/ In ARCH=arm multi_v7_defconfig alone,
there are 289 unique instances of the warning (although a good number
have multiple instances per line, so it is not quite as bad as it seems,
but still bad):

$ rg -- -Wcomma arm-multi_v7_defconfig.log | sort | uniq -c | wc -l
289

https://gist.github.com/nathanchance/907867e0a7adffc877fd39fd08853801

Probably not a good sign of the signal to noise ratio, I looked through
a good handful and all the cases I saw were not interesting... Perhaps
the warning could be tuned further to become useful for the kernel but
in its current form, it is definitely a no-go :/

> Ah, neat. Misleading indentation is another possible angle, I reckon,
> but not sure if that's enabled/possible to enable for the entire kernel

Yeah, I was surprised there was no warning for misleading indentation...
it is a part of -Wall for both clang and GCC, so it is on for the
kernel, it just appears not to trigger in this case.

> either :( We test-build with W=1 in networking, FWIW, so W=1 would be
> enough for us.

Unfortunately, even in its current form, it is way too noisy for W=1, as
the qualifier for W=1 is "do not occur too often". Probably could be
placed under W=2 but it still has the problem of wading through every
instance and it is basically a no-op because nobody tests with W=2.

Cheers,
Nathan