Message-ID: <202401101625.3664EA5B@keescook>
Date: Wed, 10 Jan 2024 16:39:22 -0800
From: Kees Cook <keescook@...omium.org>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-bcachefs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [GIT PULL] bcachefs updates for 6.8

On Wed, Jan 10, 2024 at 07:04:47PM -0500, Kent Overstreet wrote:
> On Wed, Jan 10, 2024 at 03:48:43PM -0800, Kees Cook wrote:
> > On Wed, Jan 10, 2024 at 02:36:30PM -0500, Kent Overstreet wrote:
> > > [...]
> > >       bcachefs: %pg is banished
> > 
> > Hi!
> > 
> > Not a PR blocker, but this patch re-introduces users of strlcpy() which
> > has been otherwise removed this cycle. I'll send a patch to replace
> > these new uses, but process-wise, I'd like check on how bcachefs patches
> > are reviewed.
> 
> I'm happy to fix it. Perhaps the declaration could get a depracated
> warning, though?

That's one of checkpatch.pl's purposes, seeing as how deprecation warnings
are ... deprecated. :P
https://docs.kernel.org/process/deprecated.html#id1
This has made treewide changes like this more difficult, but these are
the Rules From Linus. ;)

> > Normally I'd go find the original email that posted the patch and reply
> > there, but I couldn't find a development list where this patch was
> > posted. Where is this happening? (Being posted somewhere is supposed
> > to be a prerequisite for living in -next. E.g. quoting from the -next
> > inclusion boiler-plate: "* posted to the relevant mailing list,") It
> > looks like it was authored 5 days ago, which is cutting it awfully close
> > to the merge window opening:
> > 
> > 	AuthorDate: Fri Jan 5 11:58:50 2024 -0500
> 
> I'm confident in my testing; if it was a patch that needed more soak
> time it would have waited.
> 
> > Actually, it looks like you rebased onto v6.7-rc7? This is normally
> > strongly discouraged. The common merge base is -rc2.
> 
> Is there something special about rc2?

It's what sfr suggested as it's when many subsystem maintainers merge
to when opening their trees for development. Usually it's a good tree
state: after stabilization fixes from any rc1 rough edges.

> I reorder patches fairly often just in the normal course of backporting
> fixes, and if I have to rebase everything for a backport I'll often
> rebase onto a newer kernel so that the people who are running my tree
> are testing something more stable - it does come up.

Okay, gotcha. I personally don't care how maintainers handle rebasing; I
was just confused about the timing and why I couldn't find the original
patch on any lists. :) And to potentially warn about Linus possibly not
liking the rebase too.

> 
> > It also seems it didn't get a run through scripts/checkpatch.pl, which
> > shows 4 warnings, 2 or which point out the strlcpy deprecation:
> > 
> > WARNING: Prefer strscpy over strlcpy - see: https://github.com/KSPP/linux/issues/89
> > #123: FILE: fs/bcachefs/super.c:1389:
> > +               strlcpy(c->name, name.buf, sizeof(c->name));
> > 
> > WARNING: Prefer strscpy over strlcpy - see: https://github.com/KSPP/linux/issues/89
> > #124: FILE: fs/bcachefs/super.c:1390:
> > +       strlcpy(ca->name, name.buf, sizeof(ca->name));
> > 
> > Please make sure you're running checkpatch.pl -- it'll make integration,
> > technical debt reduction, and coding style adjustments much easier. :)
> 
> Well, we do have rather a lot of linters these days.
> 
> That's actually something I've been meaning to raise - perhaps we could
> start thinking about some pluggable way of running linters so that
> they're all run as part of a normal kernel build (and something that
> would be easy to drop new linters in to; I'd like to write some bcachefs
> specific ones).

With no central CI, the best we've got is everyone running the same
"minimum set" of checks. I'm most familiar with netdev's CI which has
such things (and checkpatch.pl is included). For example see:
https://patchwork.kernel.org/project/netdevbpf/patch/20240110110451.5473-3-ptikhomirov@virtuozzo.com/

> The current model of "I have to remember to run these 5 things, and then
> I'm going to get email nags for 3 more that I can't run" is not terribly
> scalable :)

Oh, I hear you. It's positively agonizing for those of us doing treewide
changes. I've got at least 4 CIs I check (in addition to my own) just to
check everyone's various coverage tools.

At the very least, checkpatch.pl is the common denominator:
https://docs.kernel.org/process/submitting-patches.html#style-check-your-changes

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists