linux-kernel - Re: [PATCH 0/2] nfsd: CB

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] [day] [month] [year] [list]

Message-ID: <ZsygD2pZXZtdN1ZJ@tissot.1015granger.net>
Date: Mon, 26 Aug 2024 11:32:31 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Neil Brown <neilb@...e.de>, Jeff Layton <jlayton@...nel.org>
Cc: NeilBrown <neilb@...e.de>, Dai Ngo <dai.ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/2] nfsd: CB_GETATTR fixes

On Mon, Aug 26, 2024 at 10:47:46AM -0400, Jeff Layton wrote:
> On Mon, 2024-08-26 at 10:37 -0400, Jeff Layton wrote:
> > On Mon, 2024-08-26 at 09:22 +1000, NeilBrown wrote:
> > > On Sun, 25 Aug 2024, Chuck Lever wrote:
> > > > On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> > > > > Fixes for a couple of CB_GETATTR bugs I found while working on the
> > > > > delstid set. Mostly this just ensures that we hold references to the
> > > > > delegation while working with it.
> > > > > 
> > > > > 
> > > > 
> > > > Applied to nfsd-fixes for v6.11-rc, thanks!
> > > > 
> > > > [1/2] nfsd: hold reference to delegation when updating it for cb_getattr
> > > >       commit: 8fceb5f6636bbbf803fe29fff59f138206559964
> > > > [2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
> > > >       commit: 8bc97f9b84c8852fcc56be2382f5115c518de785
> > > > 
> > > > -- 
> > > > Chuck Lever
> > > > 
> > > 
> > > Maybe the following can tidy up that code.  I can split this into
> > > a few separate patches if you like.
> > > Thoughts?
> > > 
> > > Note that the patch is easier to review if you apply it then use "git
> > > diff -b".
> > > 
> > > NeilBrown
> > > 
> > > 
> > > From: NeilBrown <neilb@...e.de>
> > > Subject: [PATCH] nfsd: untangle code in nfsd4_deleg_getattr_conflict()
> > > 
> > > The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.
> > > 
> > > With this patch we:
> > >  - properly handle non-nfsd leases.  We must not assume flc_owner is a
> > >     delegation unless fl_lmops == &nfsd_lease_mng_ops
> > 
> > AFAICT, non-nfsd leases are already properly handled (though I do agree
> > that the "flow" of this code is awkward). What case do you see that's
> > wrong?
> > 
> 
> Doh! Nevermind -- I see it now. It looks like the break_lease tag is
> just in the wrong place. We should definitely fix that.
> 
> In any case, your patch looks reasonable to me, but I couldn't get it
> to apply.

I applied Jeff's weekend CB_GETATTR patches to nfsd-fixes. If
there's an additional bug fix carried in Neil's clean-up, I would
like that to apply to that branch, as a small surgical fix, so it
can go into v6.11-rc.

Seems like these CB_GETATTR fixes need to be applicable to LTS
kernels, so let's keep them narrow.


> Care to send a real PATCH instead?  It's fine if you want to
> drop my patch and just replace it with yours.

Neil, I'd prefer:

1) specific fixes to apply to the nfsd-fixes branch
2) larger clean-ups to apply to the nfsd-next branch

Untangling nfsd4_deleg_getattr_conflict() is a sensible thing to do,
IMO, but I'd bet that Linus would consider that development rather
than an urgent bug fix.

-- 
Chuck Lever