Message-ID: <CAHC9VhRRuBdnv3u2VjKZCR672p4oj_smA72P-181ysdDXGJ-AA@mail.gmail.com>
Date: Fri, 31 May 2024 11:51:47 -0400
From: Paul Moore <paul@...l-moore.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Fan Wu <wufan@...ux.microsoft.com>, corbet@....net, zohar@...ux.ibm.com, 
	jmorris@...ei.org, serge@...lyn.com, tytso@....edu, axboe@...nel.dk, 
	agk@...hat.com, snitzer@...nel.org, mpatocka@...hat.com, eparis@...hat.com, 
	linux-doc@...r.kernel.org, linux-integrity@...r.kernel.org, 
	linux-security-module@...r.kernel.org, fsverity@...ts.linux.dev, 
	linux-block@...r.kernel.org, dm-devel@...ts.linux.dev, audit@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH v19 15/20] fsverity: expose verified fsverity built-in
 signatures to LSMs

On Thu, May 30, 2024 at 8:43 PM Eric Biggers <ebiggers@...nel.org> wrote:
> On Thu, May 30, 2024 at 04:54:37PM -0400, Paul Moore wrote:
> > On Wed, May 29, 2024 at 11:06 PM Eric Biggers <ebiggers@...nel.org> wrote:
> > > On Wed, May 29, 2024 at 09:46:57PM -0400, Paul Moore wrote:
> > > > On Fri, May 24, 2024 at 4:46 PM Fan Wu <wufan@...ux.microsoft.com> wrote:
> > > > >
> > > > > This patch enhances fsverity's capabilities to support both integrity and
> > > > > authenticity protection by introducing the exposure of built-in
> > > > > signatures through a new LSM hook. This functionality allows LSMs,
> > > > > e.g. IPE, to enforce policies based on the authenticity and integrity of
> > > > > files, specifically focusing on built-in fsverity signatures. It enables
> > > > > a policy enforcement layer within LSMs for fsverity, offering granular
> > > > > control over the usage of authenticity claims. For instance, a policy
> > > > > could be established to permit the execution of all files with verified
> > > > > built-in fsverity signatures while restricting kernel module loading
> > > > > from specified fsverity files via fsverity digests.
> >
> > ...
> >
> > > > Eric, can you give this patch in particular a look to make sure you
> > > > are okay with everything?  I believe Fan has addressed all of your
> > > > previous comments and it would be nice to have your Ack/Review tag if
> > > > you are okay with the current revision.
> > >
> > > Sorry, I've just gotten a bit tired of finding so many basic issues in this
> > > patchset even after years of revisions.
> > >
> > > This patch in particular is finally looking better.  There are a couple issues
> > > that I still see.  (BTW, you're welcome to review it too to help find these
> > > things, given that you seem to have an interest in getting this landed...):
> >
> > I too have been reviewing this patchset across multiple years and have
> > worked with Fan to fix locking issues, parsing issues, the initramfs
> > approach, etc.
>
> Sure, but none of the patches actually have your Reviewed-by.

As a general rule I don't post Acked-by/Reviewed-by tags for patches
that are targeting a subsystem that I maintain.  The logic being that
I'm going to be adding my Signed-off-by tag to the patches and arguing
these in front of Linus, so adding a Acked-by/Reviewed-by simply
creates more work later on where I have to strip them off and replace
them with my sign-off.

If the lack of a Reviewed-by tag is *really* what is preventing you
from reviewing the fs-verity patch, I can post that starting with the
next revision, but I'm guessing the lack of my tag isn't your core
issue (or at least I would argue it shouldn't be).

> > My interest in getting this landed is simply a
> > combination of fulfilling my role as LSM maintainer as well as being
> > Fan's coworker.  While I realize you don't work with Fan, you are
> > listed as the fs-verity maintainer and as such I've been looking to
> > you to help review and authorize the fs-verity related code.  If you
> > are too busy, frustrated, or <fill in the blank> to continue reviewing
> > this patchset it would be helpful if you could identify an authorized
> > fs-verity reviewer.  I don't see any besides you and Ted listed in the
> > MAINTAINERS file, but perhaps the fs-verity entry is dated.
> >
> > Regardless, I appreciate your time and feedback thus far and I'm sure
> > Fan does as well.
>
> Maintainers are expected to do reviews and acks, but not to the extent of
> extensive hand-holding of a half-baked submission.

Considering the current state of this patchset I don't believe that
verdict to be fair, or very considerate.

We clearly have different styles and approaches towards subsystem
maintainer roles.  I've had the good fortune to work with both hostile
and helpful senior developers during the early years of my time
working in the Linux kernel, and it helped reinforce the impact
patience and mentoring can have on contributors who are new to the
Linux kernel or perhaps system programming in general.  While I'm far
from perfect in this regard, I do hope and recommend that all of us in
maintainer, or senior developer, roles remember to exercise some
additional patience and education when working with new contributors.

-- 
paul-moore.com

Powered by blists - more mailing lists