linux-kernel - Re: [PATCH v2 2/2] fs: add the ability for statmount() to report the mount devicename

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: <20241107115701.ueeykzbel22sdr2f@quack3>
Date: Thu, 7 Nov 2024 12:57:01 +0100
From: Jan Kara <jack@...e.cz>
To: Jeff Layton <jlayton@...nel.org>
Cc: Jan Kara <jack@...e.cz>, Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>,
	Josef Bacik <josef@...icpanda.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] fs: add the ability for statmount() to report the
 mount devicename

On Thu 07-11-24 06:15:40, Jeff Layton wrote:
> On Thu, 2024-11-07 at 10:40 +0100, Jan Kara wrote:
> > On Wed 06-11-24 14:53:06, Jeff Layton wrote:
> > > /proc/self/mountinfo displays the devicename for the mount, but
> > > statmount() doesn't yet have a way to return it. Add a new
> > > STATMOUNT_MNT_DEVNAME flag, claim the 32-bit __spare1 field to hold the
> > > offset into the str[] array. STATMOUNT_MNT_DEVNAME will only be set in
> > > the return mask if there is a device string.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > 
> > Just one question below:
> > 
> > > @@ -5078,6 +5091,12 @@ static int statmount_string(struct kstatmount *s, u64 flag)
> > >  		if (seq->count == sm->fs_subtype)
> > >  			return 0;
> > >  		break;
> > > +	case STATMOUNT_MNT_DEVNAME:
> > > +		sm->mnt_devname = seq->count;
> > > +		ret = statmount_mnt_devname(s, seq);
> > > +		if (seq->count == sm->mnt_devname)
> > 
> > Why this odd check? Why don't you rather do:
> > 		if (ret)
> > ?
> >
> 
> statmount_mnt_devname() can return without emitting anything to the seq
> if ->show_devname and r->mnt_devname are both NULL. In that case, we
> don't want statmount_string() to return an error, but we also don't
> want to do any further manipulation of the seq. So, the above handles
> either the case where show_devname returned an error and the case where
> there was nothing to emit.
> 
> I did consider having statmount_mnt_devname() return -ENOBUFS if there
> was nothing to emit, and then handle that in the caller, but checking
> to see whether the seq has advanced seemed like a cleaner solution.

OK, but don't we want to emit an empty string - i.e., just \0 character in
case r->mnt_devname is NULL and there's no ->show_devname? Because now we
literaly emit nothing and it's going to be very confusing for userspace to
parse this when this happens in the middle of other strings in the seq.
And emitting \0 is exactly what will happen if we don't do anything special
in STATMOUNT_MNT_DEVNAME case...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR