Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 13 Jun 2022 14:38:07 -0500
From:   Tyler Hicks <tyhicks@linux.microsoft.com>
To:     Dominique Martinet <asmadeus@codewreck.org>
Cc:     Eric Van Hensbergen <ericvh@gmail.com>,
        Latchesar Ionkov <lucho@ionkov.net>,
        Christian Schoenebeck <linux_oss@crudebyte.com>,
        Jianyong Wu <jianyong.wu@arm.com>,
        v9fs-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/5] 9p: Fix refcounting during full path walks for
 fid lookups
Message-ID: <20220613193807.GF7401@sequoia>
References: <20220527000003.355812-1-tyhicks@linux.microsoft.com>
 <20220527000003.355812-2-tyhicks@linux.microsoft.com>
 <YqUifCFPTG8Qmn7a@codewreck.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YqUifCFPTG8Qmn7a@codewreck.org>
Precedence: bulk

On 2022-06-12 08:17:16, Dominique Martinet wrote:
> 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@vger.kernel.org
> > Signed-off-by: Tyler Hicks <tyhicks@linux.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.

Yes, you're correct. Nice catch!

Tyler

> 
> -- 
> Dominique
>