linux-kernel - Re: [PATCH v2 1/5] 9p: Fix refcounting during full path walks for fid lookups

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: <YqUifCFPTG8Qmn7a@codewreck.org>
Date:   Sun, 12 Jun 2022 08:17:16 +0900
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Tyler Hicks <tyhicks@...ux.microsoft.com>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        Christian Schoenebeck <linux_oss@...debyte.com>,
        Jianyong Wu <jianyong.wu@....com>,
        v9fs-developer@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/5] 9p: Fix refcounting during full path walks for
 fid lookups

Tyler Hicks wrote on Thu, May 26, 2022 at 06:59:59PM -0500:
> Decrement the refcount of the parent dentry's fid after walking
> each path component during a full path walk for a lookup. Failure to do
> so can lead to fids that are not clunked until the filesystem is
> unmounted, as indicated by this warning:
> 
>  9pnet: found fid 3 not clunked
> 
> The improper refcounting after walking resulted in open(2) returning
> -EIO on any directories underneath the mount point when using the virtio
> transport. When using the fd transport, there's no apparent issue until
> the filesytem is unmounted and the warning above is emitted to the logs.
> 
> In some cases, the user may not yet be attached to the filesystem and a
> new root fid, associated with the user, is created and attached to the
> root dentry before the full path walk is performed. Increment the new
> root fid's refcount to two in that situation so that it can be safely
> decremented to one after it is used for the walk operation. The new fid
> will still be attached to the root dentry when
> v9fs_fid_lookup_with_uid() returns so a final refcount of one is
> correct/expected.
> 
> Fixes: 6636b6dcc3db ("9p: add refcount to p9_fid struct")
> Cc: stable@...r.kernel.org
> Signed-off-by: Tyler Hicks <tyhicks@...ux.microsoft.com>
> ---
>  fs/9p/fid.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/9p/fid.c b/fs/9p/fid.c
> index 79df61fe0e59..5a469b79c1ee 100644
> --- a/fs/9p/fid.c
> +++ b/fs/9p/fid.c
> @@ -152,7 +152,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry,
>  	const unsigned char **wnames, *uname;
>  	int i, n, l, clone, access;
>  	struct v9fs_session_info *v9ses;
> -	struct p9_fid *fid, *old_fid = NULL;
> +	struct p9_fid *fid, *old_fid;
>  
>  	v9ses = v9fs_dentry2v9ses(dentry);
>  	access = v9ses->flags & V9FS_ACCESS_MASK;
> @@ -194,13 +194,12 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry,
>  		if (IS_ERR(fid))
>  			return fid;
>  
> +		refcount_inc(&fid->count);
>  		v9fs_fid_add(dentry->d_sb->s_root, fid);
>  	}
>  	/* If we are root ourself just return that */
> -	if (dentry->d_sb->s_root == dentry) {
> -		refcount_inc(&fid->count);
> +	if (dentry->d_sb->s_root == dentry)
>  		return fid;
> -	}
>  	/*
>  	 * Do a multipath walk with attached root.
>  	 * When walking parent we need to make sure we
> @@ -212,6 +211,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry,
>  		fid = ERR_PTR(n);
>  		goto err_out;
>  	}
> +	old_fid = fid;
>  	clone = 1;
>  	i = 0;
>  	while (i < n) {
> @@ -221,15 +221,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry,
>  		 * walk to ensure none of the patch component change
>  		 */
>  		fid = p9_client_walk(fid, l, &wnames[i], clone);
> +		p9_client_clunk(old_fid);

hmm, if we're not cloning then fid == old_fid and the refcount is not
increased? (I think... I didn't even realize/remember that walk had a
no-clone mode, sorry.)

So we'd only need to clunk if old fid here if we're cloning (old fid is
the initial root fid), but I'm not sure how to test this path as I
couldn't think of any pattern that'd trigger a multi-level lookup,
so I'm not 100% sure; I'll try a bit more.

-- 
Dominique