linux-kernel - Re: [PATCH 3/5] coda: use iterate_dir() in coda

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: <9E062191-C1D7-4E46-8BC8-B5DAE2270CAE@cs.cmu.edu>
Date: Mon, 09 Jun 2025 09:33:26 -0400
From: Jan Harkes <jaharkes@...cmu.edu>
To: Jeff Layton <jlayton@...nel.org>, Jan Kara <jack@...e.cz>
CC: NeilBrown <neil@...wn.name>, Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Chuck Lever <chuck.lever@...cle.com>,
 Amir Goldstein <amir73il@...il.com>, David Howells <dhowells@...hat.com>,
 Tyler Hicks <code@...icks.com>, Miklos Szeredi <miklos@...redi.hu>,
 Carlos Maiolino <cem@...nel.org>, linux-fsdevel@...r.kernel.org,
 coda@...cmu.edu, linux-nfs@...r.kernel.org, netfs@...ts.linux.dev,
 ecryptfs@...r.kernel.org, linux-unionfs@...r.kernel.org,
 linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] coda: use iterate_dir() in coda_readdir()



On June 9, 2025 9:21:20 AM EDT, Jeff Layton <jlayton@...nel.org> wrote:
>On Mon, 2025-06-09 at 09:12 -0400, Jan Harkes wrote:
>> There are definitely still users of Coda at CMU, I don't track who else uses it, but it cannot be too many for sure.
>> 
>> At this point it mostly keeps you honest about little locking details in the vfs. There are some tricky details in how the inode mappings are accessed and such. I think that  is helpful for overlay and user filesystems like fuse, overlayfs, etc. but Coda is quite small so it is easy to reason about how it uses these features.
>> 
>> 
>
>The reason I ask is that it's one of the places that we often have to
>do odd fixups like this when making changes to core VFS APIs. It's also
>not seen any non-collateral changes since 2021. I'm just wondering
>whether it's worth it to keep coda in-tree for so few users.

I have no problem maintaining an external module. I already do that to both make development easier and to support building a custom module in case some distro didn't ship with a prebuilt Coda kernel module.

>IIRC, it's also the only fs in the kernel that changes its
>inode->i_mapping pointer after inode instantiation. If not for coda, we
>could probably replace the i_mapping pointer with some macro wizardry
>and shrink struct inode by 8 bytes.

And that would be why an out-of-tree solution will not work in the long run. A small change like that to shave 8 bytes off the inode structure is a fairly easy call to make when there are no in-tree filesystems that use it anymore.

Ultimately it is up to you guys to decide if the extra burden on VFS maintenance is worth it.

Jan

>> On June 9, 2025 9:00:31 AM EDT, Jan Kara <jack@...e.cz> wrote:
>> > On Mon 09-06-25 08:17:15, Jeff Layton wrote:
>> > > On Mon, 2025-06-09 at 09:09 +1000, NeilBrown wrote:
>> > > > The code in coda_readdir() is nearly identical to iterate_dir().
>> > > > Differences are:
>> > > >  - iterate_dir() is killable
>> > > >  - iterate_dir() adds permission checking and accessing notifications
>> > > > 
>> > > > I believe these are not harmful for coda so it is best to use
>> > > > iterate_dir() directly.  This will allow locking changes without
>> > > > touching the code in coda.
>> > > > 
>> > > > Signed-off-by: NeilBrown <neil@...wn.name>
>> > > > ---
>> > > >  fs/coda/dir.c | 12 ++----------
>> > > >  1 file changed, 2 insertions(+), 10 deletions(-)
>> > > > 
>> > > > diff --git a/fs/coda/dir.c b/fs/coda/dir.c
>> > > > index ab69d8f0cec2..ca9990017265 100644
>> > > > --- a/fs/coda/dir.c
>> > > > +++ b/fs/coda/dir.c
>> > > > @@ -429,17 +429,9 @@ static int coda_readdir(struct file *coda_file, struct dir_context *ctx)
>> > > >  	cfi = coda_ftoc(coda_file);
>> > > >  	host_file = cfi->cfi_container;
>> > > >  
>> > > > -	if (host_file->f_op->iterate_shared) {
>> > > > -		struct inode *host_inode = file_inode(host_file);
>> > > > -		ret = -ENOENT;
>> > > > -		if (!IS_DEADDIR(host_inode)) {
>> > > > -			inode_lock_shared(host_inode);
>> > > > -			ret = host_file->f_op->iterate_shared(host_file, ctx);
>> > > > -			file_accessed(host_file);
>> > > > -			inode_unlock_shared(host_inode);
>> > > > -		}
>> > > > +	ret = iterate_dir(host_file, ctx);
>> > > > +	if (ret != -ENOTDIR)
>> > > >  		return ret;
>> > > > -	}
>> > > >  	/* Venus: we must read Venus dirents from a file */
>> > > >  	return coda_venus_readdir(coda_file, ctx);
>> > > >  }
>> > > 
>> > > 
>> > > Is it already time for my annual ask of "Who the heck is using coda
>> > > these days?" Anyway, this patch looks fine to me.
>> > > 
>> > > Reviewed-by: Jeff Layton <jlayton@...nel.org>
>> > 
>> > Send a patch proposing deprecating it and we might learn that :) Searching
>> > the web seems to suggest it is indeed pretty close to dead.
>> > 
>> > 								Honza
>