linux-kernel - Re: [PATCH] fs/nfsd: fix update of inode attrs in 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4937347328afd67710a8114683a99aa0e0948a11.camel@kernel.org>
Date: Mon, 26 Aug 2024 08:27:10 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Christian Brauner <brauner@...nel.org>
Cc: Chuck Lever <chuck.lever@...cle.com>, Neil Brown <neilb@...e.de>, Olga
 Kornievskaia <kolga@...app.com>, Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey
 <tom@...pey.com>, Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara
 <jack@...e.cz>, linux-nfs@...r.kernel.org,  linux-kernel@...r.kernel.org,
 linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] fs/nfsd: fix update of inode attrs in CB_GETATTR

On Mon, 2024-08-26 at 12:36 +0200, Christian Brauner wrote:
> On Sat, Aug 24, 2024 at 08:46:18AM GMT, Jeff Layton wrote:
> > Currently, we copy the mtime and ctime to the in-core inode and then
> > mark the inode dirty. This is fine for certain types of filesystems, but
> > not all. Some require a real setattr to properly change these values
> > (e.g. ceph or reexported NFS).
> > 
> > Fix this code to call notify_change() instead, which is the proper way
> > to effect a setattr. There is one problem though:
> > 
> > In this case, the client is holding a write delegation and has sent us
> > attributes to update our cache. We don't want to break the delegation
> > for this since that would defeat the purpose. Add a new ATTR_DELEG flag
> > that makes notify_change bypass the try_break_deleg call.
> > 
> > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> > One more CB_GETATTR fix. This one involves a little change at the VFS
> > layer to avoid breaking the delegation.
> > 
> > Christian, unless you have objections, this should probably go in
> > via Chuck's tree as this patch depends on a nfsd patch [1] that I sent
> > yesterday. An A-b or R-b would be welcome though.
> 
> Fwiw, 
> 
> #define ATTR_DELEG	(1 << 18) /* Delegated attrs (don't break) */
> 
> is a bit sparse of a comment for anyone not familiar with leases imo. So
> I would update this to say something similar to what what you say in the
> commit message: "Don't break write delegation while we're updating the
> cache because of the write." or something similar/less clunky.
> 
> Reviewed-by: Christian Brauner <brauner@...nel.org>

Thanks. I'll plan to send a v2 that adds better comments.
-- 
Jeff Layton <jlayton@...nel.org>