linux-kernel - Re: [PATCH v2 3/7] fs: FS_IOC

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: <cm4wbdmpuq6mlyfqrb3qqwyysa3qao6t5sc2eq3ykmgb4ptpab@qkyberqtvrtt>
Date: Tue, 6 Feb 2024 17:37:22 -0500
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Dave Chinner <david@...morbit.com>
Cc: brauner@...nel.org, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Jan Kara <jack@...e.cz>, Dave Chinner <dchinner@...hat.com>, 
	"Darrick J. Wong" <djwong@...nel.org>, Theodore Ts'o <tytso@....edu>, linux-fsdevel@...r.kernel.or
Subject: Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > +{
> > +	struct super_block *sb = file_inode(file)->i_sb;
> > +
> > +	if (!sb->s_uuid_len)
> > +		return -ENOIOCTLCMD;
> > +
> > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > +
> > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > +}
> 
> Can we please keep the declarations separate from the code? I always
> find this sort of implicit scoping of variables both difficult to
> read (especially in larger functions) and a landmine waiting to be
> tripped over. This could easily just be:
> 
> static int ioctl_getfsuuid(struct file *file, void __user *argp)
> {
> 	struct super_block *sb = file_inode(file)->i_sb;
> 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> 
> 	....
> 
> and then it's consistent with all the rest of the code...

The way I'm doing it here is actually what I'm transitioning my own code
to - the big reason being that always declaring variables at the tops of
functions leads to separating declaration and initialization, and worse
it leads people to declaring a variable once and reusing it for multiple
things (I've seen that be a source of real bugs too many times).

But I won't push that in this patch, we can just keep the style
consistent for now.

> > +/* Returns the external filesystem UUID, the same one blkid returns */
> > +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> > +
> 
> Can you add a comment somewhere in the file saying that new VFS
> ioctls should use the "0x12" namespace in the range 142-255, and
> mention that BLK ioctls should be kept within the 0x12 {0-141}
> range?

Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in
separate ranges, then FS_IOC_ needs to move to something else becasue
otherwise BLK_ won't have a way to expand.

So what else -

ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12
appears to be the only one without conflicts - all the other ranges seem
to have originated with other filesystems.

So perhaps I will take Darrick's nak (0x15) suggestion after all.