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: <20240826-liest-pusten-70d5645c9959@brauner>
Date: Mon, 26 Aug 2024 12:36:53 +0200
From: Christian Brauner <brauner@...nel.org>
To: Jeff Layton <jlayton@...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 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>