Commit 6636b6dc authored by Jianyong Wu's avatar Jianyong Wu Committed by Dominique Martinet
Browse files

9p: add refcount to p9_fid struct 

Fix race issue in fid contention.

Eric's and Greg's patch offer a mechanism to fix open-unlink-f*syscall
bug in 9p. But there is race issue in fid parallel accesses.
As Greg's patch stores all of fids from opened files into according inode,
so all the lookup fid ops can retrieve fid from inode preferentially. But
there is no mechanism to handle the fid contention issue. For example,
there are two threads get the same fid in the same time and one of them
clunk the fid before the other thread ready to discard the fid. In this
scenario, it will lead to some fatal problems, even kernel core dump.

I introduce a mechanism to fix this race issue. A counter field introduced
into p9_fid struct to store the reference counter to the fid. When a fid
is allocated from the inode or dentry, the counter will increase, and
will decrease at the end of its occupation. It is guaranteed that the
fid won't be clunked before the reference counter go down to 0, then
we can avoid the clunked fid to be used.

tests:
race issue test from the old test case:
for file in {01..50}; do touch f.${file}; done
seq 1 1000 | xargs -n 1 -P 50 -I{} cat f.* > /dev/null

open-unlink-f*syscall test:
I have tested for f*syscall include: ftruncate fstat fchown fchmod faccessat.

Link: http://lkml.kernel.org/r/20200923141146.90046-5-jianyong.wu@arm.com


Fixes: 478ba09e ("fs/9p: search open fids first")
Signed-off-by: default avatarJianyong Wu <jianyong.wu@arm.com>
Signed-off-by: default avatarDominique Martinet <asmadeus@codewreck.org>
parent 478ba09e

fs/9p/fid.c

+18 −4
Original line number Diff line number Diff line
@@ -28,6 +28,7 @@ 


static inline void __add_fid(struct dentry *dentry, struct p9_fid *fid)

{

	atomic_set(&fid->count, 1);

	hlist_add_head(&fid->dlist, (struct hlist_head *)&dentry->d_fsdata);

}
@@ -60,6 +61,8 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) 
			break;

		}

	}

	if (ret && !IS_ERR(ret))

		atomic_inc(&ret->count);

	spin_unlock(&inode->i_lock);

	return ret;

}
@@ -74,6 +77,7 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) 
void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid)

{

	spin_lock(&inode->i_lock);

	atomic_set(&fid->count, 1);

	hlist_add_head(&fid->ilist, (struct hlist_head *)&inode->i_private);

	spin_unlock(&inode->i_lock);

}
@@ -106,6 +110,7 @@ static struct p9_fid *v9fs_fid_find(struct dentry *dentry, kuid_t uid, int any) 
		hlist_for_each_entry(fid, h, dlist) {

			if (any || uid_eq(fid->uid, uid)) {

				ret = fid;

				atomic_inc(&ret->count);

				break;

			}

		}
@@ -167,7 +172,10 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, 
	fid = v9fs_fid_find(ds, uid, any);

	if (fid) {

		/* Found the parent fid do a lookup with that */

		fid = p9_client_walk(fid, 1, &dentry->d_name.name, 1);

		struct p9_fid *ofid = fid;



		fid = p9_client_walk(ofid, 1, &dentry->d_name.name, 1);

		p9_client_clunk(ofid);

		goto fid_out;

	}

	up_read(&v9ses->rename_sem);
@@ -192,8 +200,10 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, 
		v9fs_fid_add(dentry->d_sb->s_root, fid);

	}

	/* If we are root ourself just return that */

	if (dentry->d_sb->s_root == dentry)

	if (dentry->d_sb->s_root == dentry) {

		atomic_inc(&fid->count);

		return fid;

	}

	/*

	 * Do a multipath walk with attached root.

	 * When walking parent we need to make sure we
@@ -240,6 +250,7 @@ fid_out: 
			fid = ERR_PTR(-ENOENT);

		} else {

			__add_fid(dentry, fid);

			atomic_inc(&fid->count);

			spin_unlock(&dentry->d_lock);

		}

	}
@@ -290,11 +301,14 @@ struct p9_fid *v9fs_fid_lookup(struct dentry *dentry) 
struct p9_fid *v9fs_writeback_fid(struct dentry *dentry)

{

	int err;

	struct p9_fid *fid;

	struct p9_fid *fid, *ofid;



	fid = clone_fid(v9fs_fid_lookup_with_uid(dentry, GLOBAL_ROOT_UID, 0));

	ofid = v9fs_fid_lookup_with_uid(dentry, GLOBAL_ROOT_UID, 0);

	if (ofid && !IS_ERR(ofid))

		fid = clone_fid(ofid);

	if (IS_ERR(fid))

		goto error_out;

	p9_client_clunk(ofid);

	/*

	 * writeback fid will only be used to write back the

	 * dirty pages. We always request for the open fid in read-write

fs/9p/fid.h

+9 −1
Original line number Diff line number Diff line
@@ -22,6 +22,14 @@ static inline struct p9_fid *clone_fid(struct p9_fid *fid) 
}

static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)

{

	return clone_fid(v9fs_fid_lookup(dentry));

	struct p9_fid *fid, *nfid;



	fid = v9fs_fid_lookup(dentry);

	if (!fid || IS_ERR(fid))

		return fid;



	nfid = p9_client_walk(fid, 0, NULL, 1);

	p9_client_clunk(fid);

	return nfid;

}

#endif

fs/9p/vfs_dentry.c

+2 −0
Original line number Diff line number Diff line
@@ -85,6 +85,8 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags) 
			retval = v9fs_refresh_inode_dotl(fid, inode);

		else

			retval = v9fs_refresh_inode(fid, inode);

		p9_client_clunk(fid);



		if (retval == -ENOENT)

			return 0;

		if (retval < 0)

fs/9p/vfs_dir.c

+5 −4
Original line number Diff line number Diff line
@@ -210,11 +210,12 @@ int v9fs_dir_release(struct inode *inode, struct file *filp) 
	fid = filp->private_data;

	p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n",

		 inode, filp, fid ? fid->fid : -1);

	if (fid) {

		spin_lock(&inode->i_lock);

		hlist_del(&fid->ilist);

		spin_unlock(&inode->i_lock);

	if (fid)

		p9_client_clunk(fid);

	}

	return 0;

}

fs/9p/vfs_inode.c

+29 −8
Original line number Diff line number Diff line
@@ -551,6 +551,7 @@ static int v9fs_remove(struct inode *dir, struct dentry *dentry, int flags) 
	if (v9fs_proto_dotl(v9ses))

		retval = p9_client_unlinkat(dfid, dentry->d_name.name,

					    v9fs_at_to_dotl_flags(flags));

	p9_client_clunk(dfid);

	if (retval == -EOPNOTSUPP) {

		/* Try the one based on path */

		v9fid = v9fs_fid_clone(dentry);
@@ -595,14 +596,12 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, 
{

	int err;

	const unsigned char *name;

	struct p9_fid *dfid, *ofid, *fid;

	struct p9_fid *dfid, *ofid = NULL, *fid = NULL;

	struct inode *inode;



	p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry);



	err = 0;

	ofid = NULL;

	fid = NULL;

	name = dentry->d_name.name;

	dfid = v9fs_parent_fid(dentry);

	if (IS_ERR(dfid)) {
@@ -616,12 +615,14 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, 
	if (IS_ERR(ofid)) {

		err = PTR_ERR(ofid);

		p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err);

		p9_client_clunk(dfid);

		return ERR_PTR(err);

	}



	err = p9_client_fcreate(ofid, name, perm, mode, extension);

	if (err < 0) {

		p9_debug(P9_DEBUG_VFS, "p9_client_fcreate failed %d\n", err);

		p9_client_clunk(dfid);

		goto error;

	}
@@ -633,6 +634,7 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, 
			p9_debug(P9_DEBUG_VFS,

				   "p9_client_walk failed %d\n", err);

			fid = NULL;

			p9_client_clunk(dfid);

			goto error;

		}

		/*
@@ -643,11 +645,13 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, 
			err = PTR_ERR(inode);

			p9_debug(P9_DEBUG_VFS,

				   "inode creation failed %d\n", err);

			p9_client_clunk(dfid);

			goto error;

		}

		v9fs_fid_add(dentry, fid);

		d_instantiate(dentry, inode);

	}

	p9_client_clunk(dfid);

	return ofid;

error:

	if (ofid)
@@ -760,6 +764,7 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry, 
	 */

	name = dentry->d_name.name;

	fid = p9_client_walk(dfid, 1, &name, 1);

	p9_client_clunk(dfid);

	if (fid == ERR_PTR(-ENOENT))

		inode = NULL;

	else if (IS_ERR(fid))
@@ -910,7 +915,7 @@ v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry, 
	struct inode *old_inode;

	struct inode *new_inode;

	struct v9fs_session_info *v9ses;

	struct p9_fid *oldfid;

	struct p9_fid *oldfid, *dfid;

	struct p9_fid *olddirfid;

	struct p9_fid *newdirfid;

	struct p9_wstat wstat;
@@ -927,13 +932,20 @@ v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry, 
	if (IS_ERR(oldfid))

		return PTR_ERR(oldfid);



	olddirfid = clone_fid(v9fs_parent_fid(old_dentry));

	dfid = v9fs_parent_fid(old_dentry);

	olddirfid = clone_fid(dfid);

	if (dfid && !IS_ERR(dfid))

		p9_client_clunk(dfid);



	if (IS_ERR(olddirfid)) {

		retval = PTR_ERR(olddirfid);

		goto done;

	}



	newdirfid = clone_fid(v9fs_parent_fid(new_dentry));

	dfid = v9fs_parent_fid(new_dentry);

	newdirfid = clone_fid(dfid);

	p9_client_clunk(dfid);



	if (IS_ERR(newdirfid)) {

		retval = PTR_ERR(newdirfid);

		goto clunk_olddir;
@@ -990,6 +1002,7 @@ clunk_olddir: 
	p9_client_clunk(olddirfid);



done:

	p9_client_clunk(oldfid);

	return retval;

}
@@ -1022,6 +1035,7 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat, 
		return PTR_ERR(fid);



	st = p9_client_stat(fid);

	p9_client_clunk(fid);

	if (IS_ERR(st))

		return PTR_ERR(st);
@@ -1042,7 +1056,7 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat, 


static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)

{

	int retval;

	int retval, use_dentry = 0;

	struct v9fs_session_info *v9ses;

	struct p9_fid *fid = NULL;

	struct p9_wstat wstat;
@@ -1058,8 +1072,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) 
		fid = iattr->ia_file->private_data;

		WARN_ON(!fid);

	}

	if (!fid)

	if (!fid) {

		fid = v9fs_fid_lookup(dentry);

		use_dentry = 1;

	}

	if(IS_ERR(fid))

		return PTR_ERR(fid);
@@ -1089,6 +1105,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) 
		filemap_write_and_wait(d_inode(dentry)->i_mapping);



	retval = p9_client_wstat(fid, &wstat);



	if (use_dentry)

		p9_client_clunk(fid);



	if (retval < 0)

		return retval;
@@ -1213,6 +1233,7 @@ static const char *v9fs_vfs_get_link(struct dentry *dentry, 
		return ERR_PTR(-EBADF);



	st = p9_client_stat(fid);

	p9_client_clunk(fid);

	if (IS_ERR(st))

		return ERR_CAST(st);

CRA Git | Maintained and supported by SUSTech CRA and CCSE