From cba83f47fc0ef52638870890e9e352afcc56dd73 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 26 May 2022 19:00:00 -0500 Subject: [PATCH 01/13] 9p: Track the root fid with its own variable during lookups Improve readability by using a new variable when dealing with the root fid. The root fid requires special handling in comparison to other fids used during fid lookup. Link: https://lkml.kernel.org/r/20220527000003.355812-3-tyhicks@linux.microsoft.com Signed-off-by: Tyler Hicks Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/fs/9p/fid.c b/fs/9p/fid.c index baf2b152229e..056b2a7e1964 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; + struct p9_fid *fid, *root_fid, *old_fid; v9ses = v9fs_dentry2v9ses(dentry); access = v9ses->flags & V9FS_ACCESS_MASK; @@ -178,8 +178,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, up_read(&v9ses->rename_sem); /* start from the root and try to do a lookup */ - fid = v9fs_fid_find(dentry->d_sb->s_root, uid, any); - if (!fid) { + root_fid = v9fs_fid_find(dentry->d_sb->s_root, uid, any); + if (!root_fid) { /* the user is not attached to the fs yet */ if (access == V9FS_ACCESS_SINGLE) return ERR_PTR(-EPERM); @@ -189,17 +189,18 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, else uname = v9ses->uname; - fid = p9_client_attach(v9ses->clnt, NULL, uname, uid, - v9ses->aname); - if (IS_ERR(fid)) - return fid; + root_fid = p9_client_attach(v9ses->clnt, NULL, uname, uid, + v9ses->aname); + if (IS_ERR(root_fid)) + return root_fid; - refcount_inc(&fid->count); - v9fs_fid_add(dentry->d_sb->s_root, fid); + refcount_inc(&root_fid->count); + v9fs_fid_add(dentry->d_sb->s_root, root_fid); } /* If we are root ourself just return that */ if (dentry->d_sb->s_root == dentry) - return fid; + return root_fid; + /* * Do a multipath walk with attached root. * When walking parent we need to make sure we @@ -211,7 +212,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, fid = ERR_PTR(n); goto err_out; } - old_fid = fid; + fid = root_fid; + old_fid = root_fid; clone = 1; i = 0; while (i < n) { @@ -220,7 +222,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, * We need to hold rename lock when doing a multipath * walk to ensure none of the patch component change */ - fid = p9_client_walk(fid, l, &wnames[i], clone); + fid = p9_client_walk(old_fid, l, &wnames[i], clone); /* non-cloning walk will return the same fid */ if (fid != old_fid) { p9_client_clunk(old_fid); From c58c72d301d3d16ba1167d56417af39761829030 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 26 May 2022 19:00:01 -0500 Subject: [PATCH 02/13] 9p: Make the path walk logic more clear about when cloning is required Cloning during a path component walk is only needed when the old fid used for the walk operation is the root fid. Make that clear by comparing the two fids rather than using an additional variable. Link: https://lkml.kernel.org/r/20220527000003.355812-4-tyhicks@linux.microsoft.com Signed-off-by: Tyler Hicks Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 056b2a7e1964..e9cea696329f 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -150,7 +150,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, { struct dentry *ds; const unsigned char **wnames, *uname; - int i, n, l, clone, access; + int i, n, l, access; struct v9fs_session_info *v9ses; struct p9_fid *fid, *root_fid, *old_fid; @@ -214,7 +214,6 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, } fid = root_fid; old_fid = root_fid; - clone = 1; i = 0; while (i < n) { l = min(n - i, P9_MAXWELEM); @@ -222,7 +221,8 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, * We need to hold rename lock when doing a multipath * walk to ensure none of the patch component change */ - fid = p9_client_walk(old_fid, l, &wnames[i], clone); + fid = p9_client_walk(old_fid, l, &wnames[i], + old_fid == root_fid /* clone */); /* non-cloning walk will return the same fid */ if (fid != old_fid) { p9_client_clunk(old_fid); @@ -233,7 +233,6 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, goto err_out; } i += l; - clone = 0; } kfree(wnames); fid_out: From 47b1e3432b06c17b2d123b7ad30f04dd97b6d6f7 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 26 May 2022 19:00:02 -0500 Subject: [PATCH 03/13] 9p: Remove unnecessary variable for old fids while walking from d_parent Remove the ofid variable that's local to the conditional block in favor of the old_fid variable that's local to the entire function. Link: https://lkml.kernel.org/r/20220527000003.355812-5-tyhicks@linux.microsoft.com Signed-off-by: Tyler Hicks Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/9p/fid.c b/fs/9p/fid.c index e9cea696329f..7ddfed9f3c62 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -169,10 +169,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 */ - struct p9_fid *ofid = fid; + old_fid = fid; - fid = p9_client_walk(ofid, 1, &dentry->d_name.name, 1); - p9_client_clunk(ofid); + fid = p9_client_walk(old_fid, 1, &dentry->d_name.name, 1); + p9_client_clunk(old_fid); goto fid_out; } up_read(&v9ses->rename_sem); From b296d05746b7fa0d663e3b11abf0e03eab6e03e6 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Thu, 26 May 2022 19:00:03 -0500 Subject: [PATCH 04/13] 9p: Fix minor typo in code comment Fix s/patch/path/ typo and make it clear that we're talking about multiple path components. Link: https://lkml.kernel.org/r/20220527000003.355812-6-tyhicks@linux.microsoft.com Signed-off-by: Tyler Hicks Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 7ddfed9f3c62..e8fad28fc5bd 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -219,7 +219,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, l = min(n - i, P9_MAXWELEM); /* * We need to hold rename lock when doing a multipath - * walk to ensure none of the patch component change + * walk to ensure none of the path components change */ fid = p9_client_walk(old_fid, l, &wnames[i], old_fid == root_fid /* clone */); From b48dbb998d70b7f48c2ec0a15c3cf47136808e4e Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 12 Jun 2022 13:42:32 +0900 Subject: [PATCH 05/13] 9p fid refcount: add p9_fid_get/put wrappers I was recently reminded that it is not clear that p9_client_clunk() was actually just decrementing refcount and clunking only when that reaches zero: make it clear through a set of helpers. This will also allow instrumenting refcounting better for debugging next patch Link: https://lkml.kernel.org/r/20220612085330.1451496-5-asmadeus@codewreck.org Reviewed-by: Tyler Hicks Reviewed-by: Christian Schoenebeck Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 18 ++++++++-------- fs/9p/fid.h | 2 +- fs/9p/vfs_addr.c | 4 ++-- fs/9p/vfs_dentry.c | 4 ++-- fs/9p/vfs_dir.c | 2 +- fs/9p/vfs_file.c | 4 ++-- fs/9p/vfs_inode.c | 48 ++++++++++++++++++++--------------------- fs/9p/vfs_inode_dotl.c | 42 ++++++++++++++++++------------------ fs/9p/vfs_super.c | 6 +++--- fs/9p/xattr.c | 8 +++---- include/net/9p/client.h | 28 ++++++++++++++++++++++++ net/9p/client.c | 15 +++---------- 12 files changed, 100 insertions(+), 81 deletions(-) diff --git a/fs/9p/fid.c b/fs/9p/fid.c index e8fad28fc5bd..d792499349c4 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -56,7 +56,7 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) h = (struct hlist_head *)&inode->i_private; hlist_for_each_entry(fid, h, ilist) { if (uid_eq(fid->uid, uid)) { - refcount_inc(&fid->count); + p9_fid_get(fid); ret = fid; break; } @@ -104,7 +104,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; - refcount_inc(&ret->count); + p9_fid_get(ret); break; } } @@ -172,7 +172,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, old_fid = fid; fid = p9_client_walk(old_fid, 1, &dentry->d_name.name, 1); - p9_client_clunk(old_fid); + p9_fid_put(old_fid); goto fid_out; } up_read(&v9ses->rename_sem); @@ -194,7 +194,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, if (IS_ERR(root_fid)) return root_fid; - refcount_inc(&root_fid->count); + p9_fid_get(root_fid); v9fs_fid_add(dentry->d_sb->s_root, root_fid); } /* If we are root ourself just return that */ @@ -225,7 +225,7 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, old_fid == root_fid /* clone */); /* non-cloning walk will return the same fid */ if (fid != old_fid) { - p9_client_clunk(old_fid); + p9_fid_put(old_fid); old_fid = fid; } if (IS_ERR(fid)) { @@ -240,11 +240,11 @@ fid_out: spin_lock(&dentry->d_lock); if (d_unhashed(dentry)) { spin_unlock(&dentry->d_lock); - p9_client_clunk(fid); + p9_fid_put(fid); fid = ERR_PTR(-ENOENT); } else { __add_fid(dentry, fid); - refcount_inc(&fid->count); + p9_fid_get(fid); spin_unlock(&dentry->d_lock); } } @@ -301,7 +301,7 @@ struct p9_fid *v9fs_writeback_fid(struct dentry *dentry) fid = clone_fid(ofid); if (IS_ERR(fid)) goto error_out; - p9_client_clunk(ofid); + p9_fid_put(ofid); /* * writeback fid will only be used to write back the * dirty pages. We always request for the open fid in read-write @@ -310,7 +310,7 @@ struct p9_fid *v9fs_writeback_fid(struct dentry *dentry) */ err = p9_client_open(fid, O_RDWR); if (err < 0) { - p9_client_clunk(fid); + p9_fid_put(fid); fid = ERR_PTR(err); goto error_out; } diff --git a/fs/9p/fid.h b/fs/9p/fid.h index f7f33509e169..3168dfad510e 100644 --- a/fs/9p/fid.h +++ b/fs/9p/fid.h @@ -29,7 +29,7 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry) return fid; nfid = clone_fid(fid); - p9_client_clunk(fid); + p9_fid_put(fid); return nfid; } #endif diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c index d0833fa69faf..47b9a1122f34 100644 --- a/fs/9p/vfs_addr.c +++ b/fs/9p/vfs_addr.c @@ -73,7 +73,7 @@ static int v9fs_init_request(struct netfs_io_request *rreq, struct file *file) BUG_ON(!fid); } - refcount_inc(&fid->count); + p9_fid_get(fid); rreq->netfs_priv = fid; return 0; } @@ -86,7 +86,7 @@ static void v9fs_free_request(struct netfs_io_request *rreq) { struct p9_fid *fid = rreq->netfs_priv; - p9_client_clunk(fid); + p9_fid_put(fid); } /** diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c index 1c609e99d280..f89f01734587 100644 --- a/fs/9p/vfs_dentry.c +++ b/fs/9p/vfs_dentry.c @@ -54,7 +54,7 @@ static void v9fs_dentry_release(struct dentry *dentry) p9_debug(P9_DEBUG_VFS, " dentry: %pd (%p)\n", dentry, dentry); hlist_for_each_safe(p, n, (struct hlist_head *)&dentry->d_fsdata) - p9_client_clunk(hlist_entry(p, struct p9_fid, dlist)); + p9_fid_put(hlist_entry(p, struct p9_fid, dlist)); dentry->d_fsdata = NULL; } @@ -85,7 +85,7 @@ 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); + p9_fid_put(fid); if (retval == -ENOENT) return 0; diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c index 958680f7f23e..000fbaae9b18 100644 --- a/fs/9p/vfs_dir.c +++ b/fs/9p/vfs_dir.c @@ -218,7 +218,7 @@ int v9fs_dir_release(struct inode *inode, struct file *filp) spin_lock(&inode->i_lock); hlist_del(&fid->ilist); spin_unlock(&inode->i_lock); - p9_client_clunk(fid); + p9_fid_put(fid); } if ((filp->f_mode & FMODE_WRITE)) { diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c index 2573c08f335c..8276f3af35d7 100644 --- a/fs/9p/vfs_file.c +++ b/fs/9p/vfs_file.c @@ -63,7 +63,7 @@ int v9fs_file_open(struct inode *inode, struct file *file) err = p9_client_open(fid, omode); if (err < 0) { - p9_client_clunk(fid); + p9_fid_put(fid); return err; } if ((file->f_flags & O_APPEND) && @@ -98,7 +98,7 @@ int v9fs_file_open(struct inode *inode, struct file *file) v9fs_open_fid_add(inode, fid); return 0; out_error: - p9_client_clunk(file->private_data); + p9_fid_put(file->private_data); file->private_data = NULL; return err; } diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 3d8297714772..bde18776f0c7 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -400,7 +400,7 @@ void v9fs_evict_inode(struct inode *inode) fscache_relinquish_cookie(v9fs_inode_cookie(v9inode), false); /* clunk the fid stashed in writeback_fid */ if (v9inode->writeback_fid) { - p9_client_clunk(v9inode->writeback_fid); + p9_fid_put(v9inode->writeback_fid); v9inode->writeback_fid = NULL; } } @@ -569,7 +569,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); + p9_fid_put(dfid); if (retval == -EOPNOTSUPP) { /* Try the one based on path */ v9fid = v9fs_fid_clone(dentry); @@ -633,14 +633,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); + p9_fid_put(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); + p9_fid_put(dfid); goto error; } @@ -652,7 +652,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); + p9_fid_put(dfid); goto error; } /* @@ -663,20 +663,20 @@ 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); + p9_fid_put(dfid); goto error; } v9fs_fid_add(dentry, fid); d_instantiate(dentry, inode); } - p9_client_clunk(dfid); + p9_fid_put(dfid); return ofid; error: if (ofid) - p9_client_clunk(ofid); + p9_fid_put(ofid); if (fid) - p9_client_clunk(fid); + p9_fid_put(fid); return ERR_PTR(err); } @@ -708,7 +708,7 @@ v9fs_vfs_create(struct user_namespace *mnt_userns, struct inode *dir, return PTR_ERR(fid); v9fs_invalidate_inode_attr(dir); - p9_client_clunk(fid); + p9_fid_put(fid); return 0; } @@ -744,7 +744,7 @@ static int v9fs_vfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir, } if (fid) - p9_client_clunk(fid); + p9_fid_put(fid); return err; } @@ -785,7 +785,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); + p9_fid_put(dfid); if (fid == ERR_PTR(-ENOENT)) inode = NULL; else if (IS_ERR(fid)) @@ -808,7 +808,7 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry, else if (!IS_ERR(res)) v9fs_fid_add(res, fid); else - p9_client_clunk(fid); + p9_fid_put(fid); } return res; } @@ -891,7 +891,7 @@ out: error: if (fid) - p9_client_clunk(fid); + p9_fid_put(fid); goto out; } @@ -959,7 +959,7 @@ v9fs_vfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, dfid = v9fs_parent_fid(old_dentry); olddirfid = clone_fid(dfid); if (dfid && !IS_ERR(dfid)) - p9_client_clunk(dfid); + p9_fid_put(dfid); if (IS_ERR(olddirfid)) { retval = PTR_ERR(olddirfid); @@ -968,7 +968,7 @@ v9fs_vfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, dfid = v9fs_parent_fid(new_dentry); newdirfid = clone_fid(dfid); - p9_client_clunk(dfid); + p9_fid_put(dfid); if (IS_ERR(newdirfid)) { retval = PTR_ERR(newdirfid); @@ -1020,13 +1020,13 @@ clunk_newdir: d_move(old_dentry, new_dentry); } up_write(&v9ses->rename_sem); - p9_client_clunk(newdirfid); + p9_fid_put(newdirfid); clunk_olddir: - p9_client_clunk(olddirfid); + p9_fid_put(olddirfid); done: - p9_client_clunk(oldfid); + p9_fid_put(oldfid); return retval; } @@ -1060,7 +1060,7 @@ v9fs_vfs_getattr(struct user_namespace *mnt_userns, const struct path *path, return PTR_ERR(fid); st = p9_client_stat(fid); - p9_client_clunk(fid); + p9_fid_put(fid); if (IS_ERR(st)) return PTR_ERR(st); @@ -1136,7 +1136,7 @@ static int v9fs_vfs_setattr(struct user_namespace *mnt_userns, retval = p9_client_wstat(fid, &wstat); if (use_dentry) - p9_client_clunk(fid); + p9_fid_put(fid); if (retval < 0) return retval; @@ -1261,7 +1261,7 @@ static const char *v9fs_vfs_get_link(struct dentry *dentry, return ERR_CAST(fid); st = p9_client_stat(fid); - p9_client_clunk(fid); + p9_fid_put(fid); if (IS_ERR(st)) return ERR_CAST(st); @@ -1308,7 +1308,7 @@ static int v9fs_vfs_mkspecial(struct inode *dir, struct dentry *dentry, return PTR_ERR(fid); v9fs_invalidate_inode_attr(dir); - p9_client_clunk(fid); + p9_fid_put(fid); return 0; } @@ -1364,7 +1364,7 @@ v9fs_vfs_link(struct dentry *old_dentry, struct inode *dir, v9fs_refresh_inode(oldfid, d_inode(old_dentry)); v9fs_invalidate_inode_attr(dir); } - p9_client_clunk(oldfid); + p9_fid_put(oldfid); return retval; } diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index b6eb1160296c..09b124fe349c 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -274,7 +274,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, if (IS_ERR(ofid)) { err = PTR_ERR(ofid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); - p9_client_clunk(dfid); + p9_fid_put(dfid); goto out; } @@ -286,7 +286,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, if (err) { p9_debug(P9_DEBUG_VFS, "Failed to get acl values in creat %d\n", err); - p9_client_clunk(dfid); + p9_fid_put(dfid); goto error; } err = p9_client_create_dotl(ofid, name, v9fs_open_to_dotl_flags(flags), @@ -294,14 +294,14 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, if (err < 0) { p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in creat %d\n", err); - p9_client_clunk(dfid); + p9_fid_put(dfid); goto error; } v9fs_invalidate_inode_attr(dir); /* instantiate inode and assign the unopened fid to the dentry */ fid = p9_client_walk(dfid, 1, &name, 1); - p9_client_clunk(dfid); + p9_fid_put(dfid); if (IS_ERR(fid)) { err = PTR_ERR(fid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); @@ -358,10 +358,10 @@ out: error: if (fid) - p9_client_clunk(fid); + p9_fid_put(fid); err_clunk_old_fid: if (ofid) - p9_client_clunk(ofid); + p9_fid_put(ofid); goto out; } @@ -458,9 +458,9 @@ static int v9fs_vfs_mkdir_dotl(struct user_namespace *mnt_userns, v9fs_invalidate_inode_attr(dir); error: if (fid) - p9_client_clunk(fid); + p9_fid_put(fid); v9fs_put_acl(dacl, pacl); - p9_client_clunk(dfid); + p9_fid_put(dfid); return err; } @@ -489,7 +489,7 @@ v9fs_vfs_getattr_dotl(struct user_namespace *mnt_userns, */ st = p9_client_getattr_dotl(fid, P9_STATS_ALL); - p9_client_clunk(fid); + p9_fid_put(fid); if (IS_ERR(st)) return PTR_ERR(st); @@ -603,7 +603,7 @@ int v9fs_vfs_setattr_dotl(struct user_namespace *mnt_userns, retval = p9_client_setattr(fid, &p9attr); if (retval < 0) { if (use_dentry) - p9_client_clunk(fid); + p9_fid_put(fid); return retval; } @@ -619,12 +619,12 @@ int v9fs_vfs_setattr_dotl(struct user_namespace *mnt_userns, retval = v9fs_acl_chmod(inode, fid); if (retval < 0) { if (use_dentry) - p9_client_clunk(fid); + p9_fid_put(fid); return retval; } } if (use_dentry) - p9_client_clunk(fid); + p9_fid_put(fid); return 0; } @@ -771,9 +771,9 @@ v9fs_vfs_symlink_dotl(struct user_namespace *mnt_userns, struct inode *dir, error: if (fid) - p9_client_clunk(fid); + p9_fid_put(fid); - p9_client_clunk(dfid); + p9_fid_put(dfid); return err; } @@ -803,14 +803,14 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct inode *dir, oldfid = v9fs_fid_lookup(old_dentry); if (IS_ERR(oldfid)) { - p9_client_clunk(dfid); + p9_fid_put(dfid); return PTR_ERR(oldfid); } err = p9_client_link(dfid, oldfid, dentry->d_name.name); - p9_client_clunk(dfid); - p9_client_clunk(oldfid); + p9_fid_put(dfid); + p9_fid_put(oldfid); if (err < 0) { p9_debug(P9_DEBUG_VFS, "p9_client_link failed %d\n", err); return err; @@ -826,7 +826,7 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct inode *dir, return PTR_ERR(fid); v9fs_refresh_inode_dotl(fid, d_inode(old_dentry)); - p9_client_clunk(fid); + p9_fid_put(fid); } ihold(d_inode(old_dentry)); d_instantiate(dentry, d_inode(old_dentry)); @@ -924,9 +924,9 @@ v9fs_vfs_mknod_dotl(struct user_namespace *mnt_userns, struct inode *dir, } error: if (fid) - p9_client_clunk(fid); + p9_fid_put(fid); v9fs_put_acl(dacl, pacl); - p9_client_clunk(dfid); + p9_fid_put(dfid); return err; } @@ -956,7 +956,7 @@ v9fs_vfs_get_link_dotl(struct dentry *dentry, if (IS_ERR(fid)) return ERR_CAST(fid); retval = p9_client_readlink(fid, &target); - p9_client_clunk(fid); + p9_fid_put(fid); if (retval) return ERR_PTR(retval); set_delayed_call(done, kfree_link, target); diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c index 97e23b4e6982..bf350fad9500 100644 --- a/fs/9p/vfs_super.c +++ b/fs/9p/vfs_super.c @@ -190,7 +190,7 @@ static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags, return dget(sb->s_root); clunk_fid: - p9_client_clunk(fid); + p9_fid_put(fid); v9fs_session_close(v9ses); free_session: kfree(v9ses); @@ -203,7 +203,7 @@ release_sb: * attached the fid to dentry so it won't get clunked * automatically. */ - p9_client_clunk(fid); + p9_fid_put(fid); deactivate_locked_super(sb); return ERR_PTR(retval); } @@ -270,7 +270,7 @@ static int v9fs_statfs(struct dentry *dentry, struct kstatfs *buf) } res = simple_statfs(dentry, buf); done: - p9_client_clunk(fid); + p9_fid_put(fid); return res; } diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c index a824441b95a2..1f9298a4bd42 100644 --- a/fs/9p/xattr.c +++ b/fs/9p/xattr.c @@ -44,7 +44,7 @@ ssize_t v9fs_fid_xattr_get(struct p9_fid *fid, const char *name, if (err) retval = err; } - p9_client_clunk(attr_fid); + p9_fid_put(attr_fid); return retval; } @@ -71,7 +71,7 @@ ssize_t v9fs_xattr_get(struct dentry *dentry, const char *name, if (IS_ERR(fid)) return PTR_ERR(fid); ret = v9fs_fid_xattr_get(fid, name, buffer, buffer_size); - p9_client_clunk(fid); + p9_fid_put(fid); return ret; } @@ -98,7 +98,7 @@ int v9fs_xattr_set(struct dentry *dentry, const char *name, if (IS_ERR(fid)) return PTR_ERR(fid); ret = v9fs_fid_xattr_set(fid, name, value, value_len, flags); - p9_client_clunk(fid); + p9_fid_put(fid); return ret; } @@ -128,7 +128,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, retval); else p9_client_write(fid, 0, &from, &retval); - err = p9_client_clunk(fid); + err = p9_fid_put(fid); if (!retval && err) retval = err; return retval; diff --git a/include/net/9p/client.h b/include/net/9p/client.h index ec1d1706f43c..eabb53992350 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -237,6 +237,34 @@ static inline int p9_req_try_get(struct p9_req_t *r) int p9_req_put(struct p9_req_t *r); +/* fid reference counting helpers: + * - fids used for any length of time should always be referenced through + * p9_fid_get(), and released with p9_fid_put() + * - v9fs_fid_lookup() or similar will automatically call get for you + * and also require a put + * - the *_fid_add() helpers will stash the fid in the inode, + * at which point it is the responsibility of evict_inode() + * to call the put + * - the last put will automatically send a clunk to the server + */ +static inline struct p9_fid *p9_fid_get(struct p9_fid *fid) +{ + refcount_inc(&fid->count); + + return fid; +} + +static inline int p9_fid_put(struct p9_fid *fid) +{ + if (!fid || IS_ERR(fid)) + return 0; + + if (!refcount_dec_and_test(&fid->count)) + return 0; + + return p9_client_clunk(fid); +} + void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status); int p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, diff --git a/net/9p/client.c b/net/9p/client.c index 8bba0d9cf975..f3eb280c7d9d 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -1228,7 +1228,7 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, clunk_fid: kfree(wqids); - p9_client_clunk(fid); + p9_fid_put(fid); fid = NULL; error: @@ -1459,15 +1459,6 @@ int p9_client_clunk(struct p9_fid *fid) struct p9_req_t *req; int retries = 0; - if (!fid || IS_ERR(fid)) { - pr_warn("%s (%d): Trying to clunk with invalid fid\n", - __func__, task_pid_nr(current)); - dump_stack(); - return 0; - } - if (!refcount_dec_and_test(&fid->count)) - return 0; - again: p9_debug(P9_DEBUG_9P, ">>> TCLUNK fid %d (try %d)\n", fid->fid, retries); @@ -1519,7 +1510,7 @@ int p9_client_remove(struct p9_fid *fid) p9_tag_remove(clnt, req); error: if (err == -ERESTARTSYS) - p9_client_clunk(fid); + p9_fid_put(fid); else p9_fid_destroy(fid); return err; @@ -2042,7 +2033,7 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid, attr_fid->fid, *attr_size); return attr_fid; clunk_fid: - p9_client_clunk(attr_fid); + p9_fid_put(attr_fid); attr_fid = NULL; error: if (attr_fid && attr_fid != file_fid) From 286c171b86ebc693e18b485dde3a3fc470af37bd Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Mon, 13 Jun 2022 08:32:06 +0900 Subject: [PATCH 06/13] 9p fid refcount: add a 9p_fid_ref tracepoint This adds a tracepoint event for 9p fid lifecycle tracing: when a fid is created, its reference count increased/decreased, and freed. The new 9p_fid_ref tracepoint should help anyone wishing to debug any fid problem such as missing clunk (destroy) or use-after-free. Link: https://lkml.kernel.org/r/20220612085330.1451496-6-asmadeus@codewreck.org Signed-off-by: Dominique Martinet --- include/net/9p/client.h | 13 +++++++++++ include/trace/events/9p.h | 48 +++++++++++++++++++++++++++++++++++++++ net/9p/client.c | 20 +++++++++++++++- 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/include/net/9p/client.h b/include/net/9p/client.h index eabb53992350..8f629f1df865 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -11,6 +11,7 @@ #include #include +#include /* Number of requests per row */ #define P9_ROW_MAXTAG 255 @@ -237,6 +238,12 @@ static inline int p9_req_try_get(struct p9_req_t *r) int p9_req_put(struct p9_req_t *r); +/* We cannot have the real tracepoints in header files, + * use a wrapper function */ +DECLARE_TRACEPOINT(9p_fid_ref); +void do_trace_9p_fid_get(struct p9_fid *fid); +void do_trace_9p_fid_put(struct p9_fid *fid); + /* fid reference counting helpers: * - fids used for any length of time should always be referenced through * p9_fid_get(), and released with p9_fid_put() @@ -249,6 +256,9 @@ int p9_req_put(struct p9_req_t *r); */ static inline struct p9_fid *p9_fid_get(struct p9_fid *fid) { + if (tracepoint_enabled(9p_fid_ref)) + do_trace_9p_fid_get(fid); + refcount_inc(&fid->count); return fid; @@ -259,6 +269,9 @@ static inline int p9_fid_put(struct p9_fid *fid) if (!fid || IS_ERR(fid)) return 0; + if (tracepoint_enabled(9p_fid_ref)) + do_trace_9p_fid_put(fid); + if (!refcount_dec_and_test(&fid->count)) return 0; diff --git a/include/trace/events/9p.h b/include/trace/events/9p.h index 78c5608a1648..4dfa6d7f83ba 100644 --- a/include/trace/events/9p.h +++ b/include/trace/events/9p.h @@ -77,6 +77,13 @@ EM( P9_TWSTAT, "P9_TWSTAT" ) \ EMe(P9_RWSTAT, "P9_RWSTAT" ) + +#define P9_FID_REFTYPE \ + EM( P9_FID_REF_CREATE, "create " ) \ + EM( P9_FID_REF_GET, "get " ) \ + EM( P9_FID_REF_PUT, "put " ) \ + EMe(P9_FID_REF_DESTROY, "destroy" ) + /* Define EM() to export the enums to userspace via TRACE_DEFINE_ENUM() */ #undef EM #undef EMe @@ -84,6 +91,21 @@ #define EMe(a, b) TRACE_DEFINE_ENUM(a); P9_MSG_T +P9_FID_REFTYPE + +/* And also use EM/EMe to define helper enums -- once */ +#ifndef __9P_DECLARE_TRACE_ENUMS_ONLY_ONCE +#define __9P_DECLARE_TRACE_ENUMS_ONLY_ONCE +#undef EM +#undef EMe +#define EM(a, b) a, +#define EMe(a, b) a + +enum p9_fid_reftype { + P9_FID_REFTYPE +} __mode(byte); + +#endif /* * Now redefine the EM() and EMe() macros to map the enums to the strings @@ -96,6 +118,8 @@ P9_MSG_T #define show_9p_op(type) \ __print_symbolic(type, P9_MSG_T) +#define show_9p_fid_reftype(type) \ + __print_symbolic(type, P9_FID_REFTYPE) TRACE_EVENT(9p_client_req, TP_PROTO(struct p9_client *clnt, int8_t type, int tag), @@ -168,6 +192,30 @@ TRACE_EVENT(9p_protocol_dump, __entry->tag, 0, __entry->line, 16, __entry->line + 16) ); + +TRACE_EVENT(9p_fid_ref, + TP_PROTO(struct p9_fid *fid, __u8 type), + + TP_ARGS(fid, type), + + TP_STRUCT__entry( + __field( int, fid ) + __field( int, refcount ) + __field( __u8, type ) + ), + + TP_fast_assign( + __entry->fid = fid->fid; + __entry->refcount = refcount_read(&fid->count); + __entry->type = type; + ), + + TP_printk("%s fid %d, refcount %d", + show_9p_fid_reftype(__entry->type), + __entry->fid, __entry->refcount) +); + + #endif /* _TRACE_9P_H */ /* This part must be outside protection */ diff --git a/net/9p/client.c b/net/9p/client.c index f3eb280c7d9d..dfe8beb864fc 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -907,8 +907,10 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) GFP_NOWAIT); spin_unlock_irq(&clnt->lock); idr_preload_end(); - if (!ret) + if (!ret) { + trace_9p_fid_ref(fid, P9_FID_REF_CREATE); return fid; + } kfree(fid); return NULL; @@ -920,6 +922,7 @@ static void p9_fid_destroy(struct p9_fid *fid) unsigned long flags; p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid); + trace_9p_fid_ref(fid, P9_FID_REF_DESTROY); clnt = fid->clnt; spin_lock_irqsave(&clnt->lock, flags); idr_remove(&clnt->fids, fid->fid); @@ -928,6 +931,21 @@ static void p9_fid_destroy(struct p9_fid *fid) kfree(fid); } +/* We also need to export tracepoint symbols for tracepoint_enabled() */ +EXPORT_TRACEPOINT_SYMBOL(9p_fid_ref); + +void do_trace_9p_fid_get(struct p9_fid *fid) +{ + trace_9p_fid_ref(fid, P9_FID_REF_GET); +} +EXPORT_SYMBOL(do_trace_9p_fid_get); + +void do_trace_9p_fid_put(struct p9_fid *fid) +{ + trace_9p_fid_ref(fid, P9_FID_REF_PUT); +} +EXPORT_SYMBOL(do_trace_9p_fid_put); + static int p9_client_version(struct p9_client *c) { int err = 0; From dafbe689736f62c696ac64809b17bdc752cfbe76 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Sun, 12 Jun 2022 16:05:39 +0900 Subject: [PATCH 07/13] 9p fid refcount: cleanup p9_fid_put calls Simplify p9_fid_put cleanup path in many 9p functions since the function is noop on null or error fids. Also make the *_add_fid() helpers "steal" the fid by nulling its pointer, so put after them will be noop. This should lead to no change of behaviour Link: https://lkml.kernel.org/r/20220612085330.1451496-7-asmadeus@codewreck.org Reviewed-by: Tyler Hicks Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 24 +++++++++++------ fs/9p/fid.h | 4 +-- fs/9p/vfs_file.c | 5 ++-- fs/9p/vfs_inode.c | 61 +++++++++++++++++------------------------- fs/9p/vfs_inode_dotl.c | 58 +++++++++++++-------------------------- fs/9p/vfs_super.c | 2 +- 6 files changed, 64 insertions(+), 90 deletions(-) diff --git a/fs/9p/fid.c b/fs/9p/fid.c index d792499349c4..289a85eae2ae 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -31,11 +31,15 @@ static inline void __add_fid(struct dentry *dentry, struct p9_fid *fid) * @fid: fid to add * */ -void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid) +void v9fs_fid_add(struct dentry *dentry, struct p9_fid **pfid) { + struct p9_fid *fid = *pfid; + spin_lock(&dentry->d_lock); __add_fid(dentry, fid); spin_unlock(&dentry->d_lock); + + *pfid = NULL; } /** @@ -72,11 +76,15 @@ 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) +void v9fs_open_fid_add(struct inode *inode, struct p9_fid **pfid) { + struct p9_fid *fid = *pfid; + spin_lock(&inode->i_lock); hlist_add_head(&fid->ilist, (struct hlist_head *)&inode->i_private); spin_unlock(&inode->i_lock); + + *pfid = NULL; } @@ -189,13 +197,13 @@ static struct p9_fid *v9fs_fid_lookup_with_uid(struct dentry *dentry, else uname = v9ses->uname; - root_fid = p9_client_attach(v9ses->clnt, NULL, uname, uid, - v9ses->aname); - if (IS_ERR(root_fid)) - return root_fid; + fid = p9_client_attach(v9ses->clnt, NULL, uname, uid, + v9ses->aname); + if (IS_ERR(fid)) + return fid; - p9_fid_get(root_fid); - v9fs_fid_add(dentry->d_sb->s_root, root_fid); + root_fid = p9_fid_get(fid); + v9fs_fid_add(dentry->d_sb->s_root, &fid); } /* If we are root ourself just return that */ if (dentry->d_sb->s_root == dentry) diff --git a/fs/9p/fid.h b/fs/9p/fid.h index 3168dfad510e..8a4e8cd12ca2 100644 --- a/fs/9p/fid.h +++ b/fs/9p/fid.h @@ -13,9 +13,9 @@ static inline struct p9_fid *v9fs_parent_fid(struct dentry *dentry) { return v9fs_fid_lookup(dentry->d_parent); } -void v9fs_fid_add(struct dentry *dentry, struct p9_fid *fid); +void v9fs_fid_add(struct dentry *dentry, struct p9_fid **fid); struct p9_fid *v9fs_writeback_fid(struct dentry *dentry); -void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid); +void v9fs_open_fid_add(struct inode *inode, struct p9_fid **fid); static inline struct p9_fid *clone_fid(struct p9_fid *fid) { return IS_ERR(fid) ? fid : p9_client_walk(fid, 0, NULL, 1); diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c index 8276f3af35d7..aec43ba83799 100644 --- a/fs/9p/vfs_file.c +++ b/fs/9p/vfs_file.c @@ -69,9 +69,10 @@ int v9fs_file_open(struct inode *inode, struct file *file) if ((file->f_flags & O_APPEND) && (!v9fs_proto_dotu(v9ses) && !v9fs_proto_dotl(v9ses))) generic_file_llseek(file, 0, SEEK_END); + + file->private_data = fid; } - file->private_data = fid; mutex_lock(&v9inode->v_mutex); if ((v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) && !v9inode->writeback_fid && @@ -95,7 +96,7 @@ int v9fs_file_open(struct inode *inode, struct file *file) if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE); - v9fs_open_fid_add(inode, fid); + v9fs_open_fid_add(inode, &fid); return 0; out_error: p9_fid_put(file->private_data); diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index bde18776f0c7..4d1a4a8d9277 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -399,10 +399,8 @@ void v9fs_evict_inode(struct inode *inode) fscache_relinquish_cookie(v9fs_inode_cookie(v9inode), false); /* clunk the fid stashed in writeback_fid */ - if (v9inode->writeback_fid) { - p9_fid_put(v9inode->writeback_fid); - v9inode->writeback_fid = NULL; - } + p9_fid_put(v9inode->writeback_fid); + v9inode->writeback_fid = NULL; } static int v9fs_test_inode(struct inode *inode, void *data) @@ -633,14 +631,12 @@ 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_fid_put(dfid); - return ERR_PTR(err); + goto error; } 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_fid_put(dfid); goto error; } @@ -651,8 +647,6 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, err = PTR_ERR(fid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); - fid = NULL; - p9_fid_put(dfid); goto error; } /* @@ -663,21 +657,17 @@ 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_fid_put(dfid); goto error; } - v9fs_fid_add(dentry, fid); + v9fs_fid_add(dentry, &fid); d_instantiate(dentry, inode); } p9_fid_put(dfid); return ofid; error: - if (ofid) - p9_fid_put(ofid); - - if (fid) - p9_fid_put(fid); - + p9_fid_put(dfid); + p9_fid_put(ofid); + p9_fid_put(fid); return ERR_PTR(err); } @@ -804,9 +794,9 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry, res = d_splice_alias(inode, dentry); if (!IS_ERR(fid)) { if (!res) - v9fs_fid_add(dentry, fid); + v9fs_fid_add(dentry, &fid); else if (!IS_ERR(res)) - v9fs_fid_add(res, fid); + v9fs_fid_add(res, &fid); else p9_fid_put(fid); } @@ -847,7 +837,6 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, v9fs_proto_dotu(v9ses))); if (IS_ERR(fid)) { err = PTR_ERR(fid); - fid = NULL; goto error; } @@ -882,7 +871,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE); - v9fs_open_fid_add(inode, fid); + v9fs_open_fid_add(inode, &fid); file->f_mode |= FMODE_CREATED; out: @@ -890,8 +879,7 @@ out: return err; error: - if (fid) - p9_fid_put(fid); + p9_fid_put(fid); goto out; } @@ -939,9 +927,9 @@ v9fs_vfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, struct inode *old_inode; struct inode *new_inode; struct v9fs_session_info *v9ses; - struct p9_fid *oldfid, *dfid; - struct p9_fid *olddirfid; - struct p9_fid *newdirfid; + struct p9_fid *oldfid = NULL, *dfid = NULL; + struct p9_fid *olddirfid = NULL; + struct p9_fid *newdirfid = NULL; struct p9_wstat wstat; if (flags) @@ -958,21 +946,22 @@ v9fs_vfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, dfid = v9fs_parent_fid(old_dentry); olddirfid = clone_fid(dfid); - if (dfid && !IS_ERR(dfid)) - p9_fid_put(dfid); + p9_fid_put(dfid); + dfid = NULL; if (IS_ERR(olddirfid)) { retval = PTR_ERR(olddirfid); - goto done; + goto error; } dfid = v9fs_parent_fid(new_dentry); newdirfid = clone_fid(dfid); p9_fid_put(dfid); + dfid = NULL; if (IS_ERR(newdirfid)) { retval = PTR_ERR(newdirfid); - goto clunk_olddir; + goto error; } down_write(&v9ses->rename_sem); @@ -983,7 +972,7 @@ v9fs_vfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, retval = p9_client_rename(oldfid, newdirfid, new_dentry->d_name.name); if (retval != -EOPNOTSUPP) - goto clunk_newdir; + goto error_locked; } if (old_dentry->d_parent != new_dentry->d_parent) { /* @@ -992,14 +981,14 @@ v9fs_vfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, p9_debug(P9_DEBUG_ERROR, "old dir and new dir are different\n"); retval = -EXDEV; - goto clunk_newdir; + goto error_locked; } v9fs_blank_wstat(&wstat); wstat.muid = v9ses->uname; wstat.name = new_dentry->d_name.name; retval = p9_client_wstat(oldfid, &wstat); -clunk_newdir: +error_locked: if (!retval) { if (new_inode) { if (S_ISDIR(new_inode->i_mode)) @@ -1020,12 +1009,10 @@ clunk_newdir: d_move(old_dentry, new_dentry); } up_write(&v9ses->rename_sem); + +error: p9_fid_put(newdirfid); - -clunk_olddir: p9_fid_put(olddirfid); - -done: p9_fid_put(oldfid); return retval; } diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 09b124fe349c..5cfa4b4f070f 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -238,7 +238,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, struct inode *inode; struct p9_fid *fid = NULL; struct v9fs_inode *v9inode; - struct p9_fid *dfid, *ofid, *inode_fid; + struct p9_fid *dfid = NULL, *ofid = NULL, *inode_fid = NULL; struct v9fs_session_info *v9ses; struct posix_acl *pacl = NULL, *dacl = NULL; struct dentry *res = NULL; @@ -274,7 +274,6 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, if (IS_ERR(ofid)) { err = PTR_ERR(ofid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); - p9_fid_put(dfid); goto out; } @@ -286,38 +285,34 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, if (err) { p9_debug(P9_DEBUG_VFS, "Failed to get acl values in creat %d\n", err); - p9_fid_put(dfid); - goto error; + goto out; } err = p9_client_create_dotl(ofid, name, v9fs_open_to_dotl_flags(flags), mode, gid, &qid); if (err < 0) { p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in creat %d\n", err); - p9_fid_put(dfid); - goto error; + goto out; } v9fs_invalidate_inode_attr(dir); /* instantiate inode and assign the unopened fid to the dentry */ fid = p9_client_walk(dfid, 1, &name, 1); - p9_fid_put(dfid); if (IS_ERR(fid)) { err = PTR_ERR(fid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); - fid = NULL; - goto error; + goto out; } inode = v9fs_get_new_inode_from_fid(v9ses, fid, dir->i_sb); if (IS_ERR(inode)) { err = PTR_ERR(inode); p9_debug(P9_DEBUG_VFS, "inode creation failed %d\n", err); - goto error; + goto out; } /* Now set the ACL based on the default value */ v9fs_set_create_acl(inode, fid, dacl, pacl); - v9fs_fid_add(dentry, fid); + v9fs_fid_add(dentry, &fid); d_instantiate(dentry, inode); v9inode = V9FS_I(inode); @@ -336,7 +331,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, if (IS_ERR(inode_fid)) { err = PTR_ERR(inode_fid); mutex_unlock(&v9inode->v_mutex); - goto err_clunk_old_fid; + goto out; } v9inode->writeback_fid = (void *) inode_fid; } @@ -344,25 +339,20 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, /* Since we are opening a file, assign the open fid to the file */ err = finish_open(file, dentry, generic_file_open); if (err) - goto err_clunk_old_fid; + goto out; file->private_data = ofid; if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE) fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE); - v9fs_open_fid_add(inode, ofid); + v9fs_open_fid_add(inode, &ofid); file->f_mode |= FMODE_CREATED; out: + p9_fid_put(dfid); + p9_fid_put(ofid); + p9_fid_put(fid); v9fs_put_acl(dacl, pacl); dput(res); return err; - -error: - if (fid) - p9_fid_put(fid); -err_clunk_old_fid: - if (ofid) - p9_fid_put(ofid); - goto out; } /** @@ -400,7 +390,6 @@ static int v9fs_vfs_mkdir_dotl(struct user_namespace *mnt_userns, if (IS_ERR(dfid)) { err = PTR_ERR(dfid); p9_debug(P9_DEBUG_VFS, "fid lookup failed %d\n", err); - dfid = NULL; goto error; } @@ -422,7 +411,6 @@ static int v9fs_vfs_mkdir_dotl(struct user_namespace *mnt_userns, err = PTR_ERR(fid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); - fid = NULL; goto error; } @@ -435,10 +423,9 @@ static int v9fs_vfs_mkdir_dotl(struct user_namespace *mnt_userns, err); goto error; } - v9fs_fid_add(dentry, fid); + v9fs_fid_add(dentry, &fid); v9fs_set_create_acl(inode, fid, dacl, pacl); d_instantiate(dentry, inode); - fid = NULL; err = 0; } else { /* @@ -457,8 +444,7 @@ static int v9fs_vfs_mkdir_dotl(struct user_namespace *mnt_userns, inc_nlink(dir); v9fs_invalidate_inode_attr(dir); error: - if (fid) - p9_fid_put(fid); + p9_fid_put(fid); v9fs_put_acl(dacl, pacl); p9_fid_put(dfid); return err; @@ -743,7 +729,6 @@ v9fs_vfs_symlink_dotl(struct user_namespace *mnt_userns, struct inode *dir, err = PTR_ERR(fid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); - fid = NULL; goto error; } @@ -755,9 +740,8 @@ v9fs_vfs_symlink_dotl(struct user_namespace *mnt_userns, struct inode *dir, err); goto error; } - v9fs_fid_add(dentry, fid); + v9fs_fid_add(dentry, &fid); d_instantiate(dentry, inode); - fid = NULL; err = 0; } else { /* Not in cached mode. No need to populate inode with stat */ @@ -770,9 +754,7 @@ v9fs_vfs_symlink_dotl(struct user_namespace *mnt_userns, struct inode *dir, } error: - if (fid) - p9_fid_put(fid); - + p9_fid_put(fid); p9_fid_put(dfid); return err; } @@ -866,7 +848,6 @@ v9fs_vfs_mknod_dotl(struct user_namespace *mnt_userns, struct inode *dir, if (IS_ERR(dfid)) { err = PTR_ERR(dfid); p9_debug(P9_DEBUG_VFS, "fid lookup failed %d\n", err); - dfid = NULL; goto error; } @@ -891,7 +872,6 @@ v9fs_vfs_mknod_dotl(struct user_namespace *mnt_userns, struct inode *dir, err = PTR_ERR(fid); p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n", err); - fid = NULL; goto error; } @@ -905,9 +885,8 @@ v9fs_vfs_mknod_dotl(struct user_namespace *mnt_userns, struct inode *dir, goto error; } v9fs_set_create_acl(inode, fid, dacl, pacl); - v9fs_fid_add(dentry, fid); + v9fs_fid_add(dentry, &fid); d_instantiate(dentry, inode); - fid = NULL; err = 0; } else { /* @@ -923,8 +902,7 @@ v9fs_vfs_mknod_dotl(struct user_namespace *mnt_userns, struct inode *dir, d_instantiate(dentry, inode); } error: - if (fid) - p9_fid_put(fid); + p9_fid_put(fid); v9fs_put_acl(dacl, pacl); p9_fid_put(dfid); diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c index bf350fad9500..2d9ee073d12c 100644 --- a/fs/9p/vfs_super.c +++ b/fs/9p/vfs_super.c @@ -184,7 +184,7 @@ static struct dentry *v9fs_mount(struct file_system_type *fs_type, int flags, retval = v9fs_get_acl(inode, fid); if (retval) goto release_sb; - v9fs_fid_add(root, fid); + v9fs_fid_add(root, &fid); p9_debug(P9_DEBUG_VFS, " simple set mount, return 0\n"); return dget(sb->s_root); From e3baced02a521a6f213b29053db76394aa22d5e1 Mon Sep 17 00:00:00 2001 From: Yang Li Date: Wed, 15 Jun 2022 09:20:39 +0800 Subject: [PATCH 08/13] 9p: Fix some kernel-doc comments Remove warnings found by running scripts/kernel-doc, which is caused by using 'make W=1'. fs/9p/fid.c:35: warning: Function parameter or member 'pfid' not described in 'v9fs_fid_add' fs/9p/fid.c:35: warning: Excess function parameter 'fid' description in 'v9fs_fid_add' fs/9p/fid.c:80: warning: Function parameter or member 'pfid' not described in 'v9fs_open_fid_add' fs/9p/fid.c:80: warning: Excess function parameter 'fid' description in 'v9fs_open_fid_add' Link: https://lkml.kernel.org/r/20220615012039.43479-1-yang.lee@linux.alibaba.com Reported-by: Abaci Robot Signed-off-by: Yang Li [Dominique: further adjust comment] Signed-off-by: Dominique Martinet --- fs/9p/fid.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/9p/fid.c b/fs/9p/fid.c index 289a85eae2ae..23cf9b2fbfe4 100644 --- a/fs/9p/fid.c +++ b/fs/9p/fid.c @@ -28,7 +28,7 @@ static inline void __add_fid(struct dentry *dentry, struct p9_fid *fid) /** * v9fs_fid_add - add a fid to a dentry * @dentry: dentry that the fid is being added to - * @fid: fid to add + * @pfid: fid to add, NULLed out * */ void v9fs_fid_add(struct dentry *dentry, struct p9_fid **pfid) @@ -72,7 +72,7 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid) /** * v9fs_open_fid_add - add an open fid to an inode * @inode: inode that the fid is being added to - * @fid: fid to add + * @pfid: fid to add, NULLed out * */ From 6cda12864cb0f99810a5809e11e3ee5b102c9a47 Mon Sep 17 00:00:00 2001 From: Kent Overstreet Date: Sun, 3 Jul 2022 21:02:49 -0400 Subject: [PATCH 09/13] 9p: Drop kref usage An upcoming patch is going to require passing the client through p9_req_put() -> p9_req_free(), but that's awkward with the kref indirection - so this patch switches to using refcount_t directly. Link: https://lkml.kernel.org/r/20220704014243.153050-1-kent.overstreet@gmail.com Signed-off-by: Kent Overstreet Cc: Eric Van Hensbergen Cc: Latchesar Ionkov Signed-off-by: Dominique Martinet --- include/net/9p/client.h | 6 +++--- net/9p/client.c | 19 ++++++++----------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/include/net/9p/client.h b/include/net/9p/client.h index 8f629f1df865..ad38325aaef0 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -77,7 +77,7 @@ enum p9_req_status_t { struct p9_req_t { int status; int t_err; - struct kref refcount; + refcount_t refcount; wait_queue_head_t wq; struct p9_fcall tc; struct p9_fcall rc; @@ -228,12 +228,12 @@ struct p9_req_t *p9_tag_lookup(struct p9_client *c, u16 tag); static inline void p9_req_get(struct p9_req_t *r) { - kref_get(&r->refcount); + refcount_inc(&r->refcount); } static inline int p9_req_try_get(struct p9_req_t *r) { - return kref_get_unless_zero(&r->refcount); + return refcount_inc_not_zero(&r->refcount); } int p9_req_put(struct p9_req_t *r); diff --git a/net/9p/client.c b/net/9p/client.c index dfe8beb864fc..3717d3f21a9f 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -305,7 +305,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) * callback), so p9_client_cb eats the second ref there * as the pointer is duplicated directly by virtqueue_add_sgs() */ - refcount_set(&req->refcount.refcount, 2); + refcount_set(&req->refcount, 2); return req; @@ -370,18 +370,15 @@ static int p9_tag_remove(struct p9_client *c, struct p9_req_t *r) return p9_req_put(r); } -static void p9_req_free(struct kref *ref) -{ - struct p9_req_t *r = container_of(ref, struct p9_req_t, refcount); - - p9_fcall_fini(&r->tc); - p9_fcall_fini(&r->rc); - kmem_cache_free(p9_req_cache, r); -} - int p9_req_put(struct p9_req_t *r) { - return kref_put(&r->refcount, p9_req_free); + if (refcount_dec_and_test(&r->refcount)) { + p9_fcall_fini(&r->tc); + p9_fcall_fini(&r->rc); + kmem_cache_free(p9_req_cache, r); + return 1; + } + return 0; } EXPORT_SYMBOL(p9_req_put); From 8b11ff098af42b1fa57fc817daadd53c8b244a0c Mon Sep 17 00:00:00 2001 From: Kent Overstreet Date: Sun, 3 Jul 2022 21:08:18 -0400 Subject: [PATCH 10/13] 9p: Add client parameter to p9_req_put() This is to aid in adding mempools, in the next patch. Link: https://lkml.kernel.org/r/20220704014243.153050-2-kent.overstreet@gmail.com Signed-off-by: Kent Overstreet Cc: Eric Van Hensbergen Cc: Latchesar Ionkov Signed-off-by: Dominique Martinet --- include/net/9p/client.h | 2 +- net/9p/client.c | 12 ++++++------ net/9p/trans_fd.c | 12 ++++++------ net/9p/trans_rdma.c | 2 +- net/9p/trans_virtio.c | 4 ++-- net/9p/trans_xen.c | 2 +- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/include/net/9p/client.h b/include/net/9p/client.h index ad38325aaef0..78ebcf782ce5 100644 --- a/include/net/9p/client.h +++ b/include/net/9p/client.h @@ -236,7 +236,7 @@ static inline int p9_req_try_get(struct p9_req_t *r) return refcount_inc_not_zero(&r->refcount); } -int p9_req_put(struct p9_req_t *r); +int p9_req_put(struct p9_client *c, struct p9_req_t *r); /* We cannot have the real tracepoints in header files, * use a wrapper function */ diff --git a/net/9p/client.c b/net/9p/client.c index 3717d3f21a9f..0be19ab1f693 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -341,7 +341,7 @@ again: if (!p9_req_try_get(req)) goto again; if (req->tc.tag != tag) { - p9_req_put(req); + p9_req_put(c, req); goto again; } } @@ -367,10 +367,10 @@ static int p9_tag_remove(struct p9_client *c, struct p9_req_t *r) spin_lock_irqsave(&c->lock, flags); idr_remove(&c->reqs, tag); spin_unlock_irqrestore(&c->lock, flags); - return p9_req_put(r); + return p9_req_put(c, r); } -int p9_req_put(struct p9_req_t *r) +int p9_req_put(struct p9_client *c, struct p9_req_t *r) { if (refcount_dec_and_test(&r->refcount)) { p9_fcall_fini(&r->tc); @@ -423,7 +423,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status) wake_up(&req->wq); p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc.tag); - p9_req_put(req); + p9_req_put(c, req); } EXPORT_SYMBOL(p9_client_cb); @@ -706,7 +706,7 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c, reterr: p9_tag_remove(c, req); /* We have to put also the 2nd reference as it won't be used */ - p9_req_put(req); + p9_req_put(c, req); return ERR_PTR(err); } @@ -743,7 +743,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...) err = c->trans_mod->request(c, req); if (err < 0) { /* write won't happen */ - p9_req_put(req); + p9_req_put(c, req); if (err != -ERESTARTSYS && err != -EFAULT) c->status = Disconnected; goto recalc_sigpending; diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 8f8f95e39b03..007c3f45fe05 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -378,7 +378,7 @@ static void p9_read_work(struct work_struct *work) m->rc.sdata = NULL; m->rc.offset = 0; m->rc.capacity = 0; - p9_req_put(m->rreq); + p9_req_put(m->client, m->rreq); m->rreq = NULL; } @@ -492,7 +492,7 @@ static void p9_write_work(struct work_struct *work) m->wpos += err; if (m->wpos == m->wsize) { m->wpos = m->wsize = 0; - p9_req_put(m->wreq); + p9_req_put(m->client, m->wreq); m->wreq = NULL; } @@ -695,7 +695,7 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req) if (req->status == REQ_STATUS_UNSENT) { list_del(&req->req_list); req->status = REQ_STATUS_FLSHD; - p9_req_put(req); + p9_req_put(client, req); ret = 0; } spin_unlock(&client->lock); @@ -722,7 +722,7 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req) list_del(&req->req_list); req->status = REQ_STATUS_FLSHD; spin_unlock(&client->lock); - p9_req_put(req); + p9_req_put(client, req); return 0; } @@ -883,12 +883,12 @@ static void p9_conn_destroy(struct p9_conn *m) p9_mux_poll_stop(m); cancel_work_sync(&m->rq); if (m->rreq) { - p9_req_put(m->rreq); + p9_req_put(m->client, m->rreq); m->rreq = NULL; } cancel_work_sync(&m->wq); if (m->wreq) { - p9_req_put(m->wreq); + p9_req_put(m->client, m->wreq); m->wreq = NULL; } diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index 88e563826674..d817d3745238 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -350,7 +350,7 @@ send_done(struct ib_cq *cq, struct ib_wc *wc) c->busa, c->req->tc.size, DMA_TO_DEVICE); up(&rdma->sq_sem); - p9_req_put(c->req); + p9_req_put(client, c->req); kfree(c); } diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index b24a4fb0f0a2..147972bf2e79 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -199,7 +199,7 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req) /* Reply won't come, so drop req ref */ static int p9_virtio_cancelled(struct p9_client *client, struct p9_req_t *req) { - p9_req_put(req); + p9_req_put(client, req); return 0; } @@ -523,7 +523,7 @@ err_out: kvfree(out_pages); if (!kicked) { /* reply won't come */ - p9_req_put(req); + p9_req_put(client, req); } return err; } diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index 833cd3792c51..227f89cc7237 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -163,7 +163,7 @@ again: ring->intf->out_prod = prod; spin_unlock_irqrestore(&ring->lock, flags); notify_remote_via_irq(ring->irq); - p9_req_put(p9_req); + p9_req_put(client, p9_req); return 0; } From 67dd8e445ee0aa5e8bf1d9d866c914ed11626bbd Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Tue, 12 Jul 2022 15:09:35 +0900 Subject: [PATCH 11/13] 9p: roll p9_tag_remove into p9_req_put mempool prep commit removed the awkward kref usage which didn't allow passing client pointer easily with the ref, so we no longer need a separate function to remove the tag from idr. This has the side benefit that it should be more robust in detecting leaks: umount will now properly catch unfreed requests as they still will be in the idr until the last ref is dropped Link: https://lkml.kernel.org/r/20220712060801.2487140-1-asmadeus@codewreck.org Signed-off-by: Dominique Martinet Reviewed-by: Christian Schoenebeck --- net/9p/client.c | 101 ++++++++++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/net/9p/client.c b/net/9p/client.c index 0be19ab1f693..89c5aeb00076 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -298,7 +298,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) /* Init ref to two because in the general case there is one ref * that is put asynchronously by a writer thread, one ref * temporarily given by p9_tag_lookup and put by p9_client_cb - * in the recv thread, and one ref put by p9_tag_remove in the + * in the recv thread, and one ref put by p9_req_put in the * main thread. The only exception is virtio that does not use * p9_tag_lookup but does not have a writer thread either * (the write happens synchronously in the request/zc_request @@ -358,21 +358,22 @@ EXPORT_SYMBOL(p9_tag_lookup); * * Context: Any context. */ -static int p9_tag_remove(struct p9_client *c, struct p9_req_t *r) +static void p9_tag_remove(struct p9_client *c, struct p9_req_t *r) { unsigned long flags; u16 tag = r->tc.tag; - p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag); + p9_debug(P9_DEBUG_MUX, "freeing clnt %p req %p tag: %d\n", c, r, tag); spin_lock_irqsave(&c->lock, flags); idr_remove(&c->reqs, tag); spin_unlock_irqrestore(&c->lock, flags); - return p9_req_put(c, r); } int p9_req_put(struct p9_client *c, struct p9_req_t *r) { if (refcount_dec_and_test(&r->refcount)) { + p9_tag_remove(c, r); + p9_fcall_fini(&r->tc); p9_fcall_fini(&r->rc); kmem_cache_free(p9_req_cache, r); @@ -397,7 +398,7 @@ static void p9_tag_cleanup(struct p9_client *c) rcu_read_lock(); idr_for_each_entry(&c->reqs, req, id) { pr_info("Tag %d still in use\n", id); - if (p9_tag_remove(c, req) == 0) + if (p9_req_put(c, req) == 0) pr_warn("Packet with tag %d has still references", req->tc.tag); } @@ -670,7 +671,7 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq) c->trans_mod->cancelled(c, oldreq); } - p9_tag_remove(c, req); + p9_req_put(c, req); return 0; } @@ -704,7 +705,7 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c, trace_9p_client_req(c, type, req->tc.tag); return req; reterr: - p9_tag_remove(c, req); + p9_req_put(c, req); /* We have to put also the 2nd reference as it won't be used */ p9_req_put(c, req); return ERR_PTR(err); @@ -716,7 +717,7 @@ reterr: * @type: type of request * @fmt: protocol format string (see protocol.c) * - * Returns request structure (which client must free using p9_tag_remove) + * Returns request structure (which client must free using p9_req_put) */ static struct p9_req_t * @@ -794,7 +795,7 @@ recalc_sigpending: if (!err) return req; reterr: - p9_tag_remove(c, req); + p9_req_put(c, req); return ERR_PTR(safe_errno(err)); } @@ -809,7 +810,7 @@ reterr: * @in_hdrlen: reader header size, This is the size of response protocol data * @fmt: protocol format string (see protocol.c) * - * Returns request structure (which client must free using p9_tag_remove) + * Returns request structure (which client must free using p9_req_put) */ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type, struct iov_iter *uidata, @@ -876,7 +877,7 @@ recalc_sigpending: if (!err) return req; reterr: - p9_tag_remove(c, req); + p9_req_put(c, req); return ERR_PTR(safe_errno(err)); } @@ -1005,7 +1006,7 @@ static int p9_client_version(struct p9_client *c) error: kfree(version); - p9_tag_remove(c, req); + p9_req_put(c, req); return err; } @@ -1159,7 +1160,7 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid, err = p9pdu_readf(&req->rc, clnt->proto_version, "Q", &qid); if (err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); goto error; } @@ -1168,7 +1169,7 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid, memmove(&fid->qid, &qid, sizeof(struct p9_qid)); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return fid; error: @@ -1215,10 +1216,10 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, err = p9pdu_readf(&req->rc, clnt->proto_version, "R", &nwqids, &wqids); if (err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); goto clunk_fid; } - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); p9_debug(P9_DEBUG_9P, "<<< RWALK nwqid %d:\n", nwqids); @@ -1294,7 +1295,7 @@ int p9_client_open(struct p9_fid *fid, int mode) fid->iounit = iounit; free_and_error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1338,7 +1339,7 @@ int p9_client_create_dotl(struct p9_fid *ofid, const char *name, u32 flags, ofid->iounit = iounit; free_and_error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1382,7 +1383,7 @@ int p9_client_fcreate(struct p9_fid *fid, const char *name, u32 perm, int mode, fid->iounit = iounit; free_and_error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1416,7 +1417,7 @@ int p9_client_symlink(struct p9_fid *dfid, const char *name, qid->type, qid->path, qid->version); free_and_error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1436,7 +1437,7 @@ int p9_client_link(struct p9_fid *dfid, struct p9_fid *oldfid, const char *newna return PTR_ERR(req); p9_debug(P9_DEBUG_9P, "<<< RLINK\n"); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return 0; } EXPORT_SYMBOL(p9_client_link); @@ -1460,7 +1461,7 @@ int p9_client_fsync(struct p9_fid *fid, int datasync) p9_debug(P9_DEBUG_9P, "<<< RFSYNC fid %d\n", fid->fid); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; @@ -1488,7 +1489,7 @@ again: p9_debug(P9_DEBUG_9P, "<<< RCLUNK fid %d\n", fid->fid); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: /* Fid is not valid even after a failed clunk * If interrupted, retry once then give up and @@ -1522,7 +1523,7 @@ int p9_client_remove(struct p9_fid *fid) p9_debug(P9_DEBUG_9P, "<<< RREMOVE fid %d\n", fid->fid); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: if (err == -ERESTARTSYS) p9_fid_put(fid); @@ -1549,7 +1550,7 @@ int p9_client_unlinkat(struct p9_fid *dfid, const char *name, int flags) } p9_debug(P9_DEBUG_9P, "<<< RUNLINKAT fid %d %s\n", dfid->fid, name); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1617,7 +1618,7 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to, "D", &count, &dataptr); if (*err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return 0; } if (rsize < count) { @@ -1632,13 +1633,13 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to, if (n != count) { *err = -EFAULT; - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return n; } } else { iov_iter_advance(to, count); } - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return count; } EXPORT_SYMBOL(p9_client_read_once); @@ -1681,7 +1682,7 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err) *err = p9pdu_readf(&req->rc, clnt->proto_version, "d", &count); if (*err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); break; } if (rsize < count) { @@ -1691,7 +1692,7 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err) p9_debug(P9_DEBUG_9P, "<<< RWRITE count %d\n", count); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); iov_iter_advance(from, count); total += count; offset += count; @@ -1726,7 +1727,7 @@ struct p9_wstat *p9_client_stat(struct p9_fid *fid) err = p9pdu_readf(&req->rc, clnt->proto_version, "wS", &ignored, ret); if (err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); goto error; } @@ -1743,7 +1744,7 @@ struct p9_wstat *p9_client_stat(struct p9_fid *fid) from_kgid(&init_user_ns, ret->n_gid), from_kuid(&init_user_ns, ret->n_muid)); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return ret; error: @@ -1779,7 +1780,7 @@ struct p9_stat_dotl *p9_client_getattr_dotl(struct p9_fid *fid, err = p9pdu_readf(&req->rc, clnt->proto_version, "A", ret); if (err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); goto error; } @@ -1805,7 +1806,7 @@ struct p9_stat_dotl *p9_client_getattr_dotl(struct p9_fid *fid, ret->st_btime_sec, ret->st_btime_nsec, ret->st_gen, ret->st_data_version); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return ret; error: @@ -1877,7 +1878,7 @@ int p9_client_wstat(struct p9_fid *fid, struct p9_wstat *wst) p9_debug(P9_DEBUG_9P, "<<< RWSTAT fid %d\n", fid->fid); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1909,7 +1910,7 @@ int p9_client_setattr(struct p9_fid *fid, struct p9_iattr_dotl *p9attr) goto error; } p9_debug(P9_DEBUG_9P, "<<< RSETATTR fid %d\n", fid->fid); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1937,7 +1938,7 @@ int p9_client_statfs(struct p9_fid *fid, struct p9_rstatfs *sb) &sb->files, &sb->ffree, &sb->fsid, &sb->namelen); if (err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); goto error; } @@ -1946,7 +1947,7 @@ int p9_client_statfs(struct p9_fid *fid, struct p9_rstatfs *sb) fid->fid, sb->type, sb->bsize, sb->blocks, sb->bfree, sb->bavail, sb->files, sb->ffree, sb->fsid, sb->namelen); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -1974,7 +1975,7 @@ int p9_client_rename(struct p9_fid *fid, p9_debug(P9_DEBUG_9P, "<<< RRENAME fid %d\n", fid->fid); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -2004,7 +2005,7 @@ int p9_client_renameat(struct p9_fid *olddirfid, const char *old_name, p9_debug(P9_DEBUG_9P, "<<< RRENAMEAT newdirfid %d new name %s\n", newdirfid->fid, new_name); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -2040,10 +2041,10 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid, err = p9pdu_readf(&req->rc, clnt->proto_version, "q", attr_size); if (err) { trace_9p_protocol_dump(clnt, &req->rc); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); goto clunk_fid; } - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); p9_debug(P9_DEBUG_9P, "<<< RXATTRWALK fid %d size %llu\n", attr_fid->fid, *attr_size); return attr_fid; @@ -2077,7 +2078,7 @@ int p9_client_xattrcreate(struct p9_fid *fid, const char *name, goto error; } p9_debug(P9_DEBUG_9P, "<<< RXATTRCREATE fid %d\n", fid->fid); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -2139,11 +2140,11 @@ int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset) if (non_zc) memmove(data, dataptr, count); - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return count; free_and_error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); error: return err; } @@ -2175,7 +2176,7 @@ int p9_client_mknod_dotl(struct p9_fid *fid, const char *name, int mode, qid->type, qid->path, qid->version); error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return err; } EXPORT_SYMBOL(p9_client_mknod_dotl); @@ -2205,7 +2206,7 @@ int p9_client_mkdir_dotl(struct p9_fid *fid, const char *name, int mode, qid->path, qid->version); error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return err; } EXPORT_SYMBOL(p9_client_mkdir_dotl); @@ -2237,7 +2238,7 @@ int p9_client_lock_dotl(struct p9_fid *fid, struct p9_flock *flock, u8 *status) } p9_debug(P9_DEBUG_9P, "<<< RLOCK status %i\n", *status); error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return err; } EXPORT_SYMBOL(p9_client_lock_dotl); @@ -2274,7 +2275,7 @@ int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *glock) glock->type, glock->start, glock->length, glock->proc_id, glock->client_id); error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return err; } EXPORT_SYMBOL(p9_client_getlock_dotl); @@ -2300,7 +2301,7 @@ int p9_client_readlink(struct p9_fid *fid, char **target) } p9_debug(P9_DEBUG_9P, "<<< RREADLINK target %s\n", *target); error: - p9_tag_remove(clnt, req); + p9_req_put(clnt, req); return err; } EXPORT_SYMBOL(p9_client_readlink); From 4ac7573e1f9333073fa8d303acc941c9b7ab7f61 Mon Sep 17 00:00:00 2001 From: Hangyu Hua Date: Wed, 13 Jul 2022 09:55:11 +0900 Subject: [PATCH 12/13] net: 9p: fix refcount leak in p9_read_work() error handling p9_req_put need to be called when m->rreq->rc.sdata is NULL to avoid temporary refcount leak. Link: https://lkml.kernel.org/r/20220712104438.30800-1-hbh25y@gmail.com Fixes: 728356dedeff ("9p: Add refcount to p9_req_t") Signed-off-by: Hangyu Hua [Dominique: commit wording adjustments, p9_req_put argument fixes for rebase] Signed-off-by: Dominique Martinet --- net/9p/trans_fd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 007c3f45fe05..e758978b44be 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -343,6 +343,7 @@ static void p9_read_work(struct work_struct *work) p9_debug(P9_DEBUG_ERROR, "No recv fcall for tag %d (req %p), disconnecting!\n", m->rc.tag, m->rreq); + p9_req_put(m->client, m->rreq); m->rreq = NULL; err = -EIO; goto error; From aa7aeee169480e98cf41d83c01290a37e569be6d Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Sun, 10 Jul 2022 09:14:02 -0500 Subject: [PATCH 13/13] net/9p: Initialize the iounit field during fid creation Ensure that the fid's iounit field is set to zero when a new fid is created. Certain 9P operations, such as OPEN and CREATE, allow the server to reply with an iounit size which the client code assigns to the p9_fid struct shortly after the fid is created by p9_fid_create(). On the other hand, an XATTRWALK operation doesn't allow for the server to specify an iounit value. The iounit field of the newly allocated p9_fid struct remained uninitialized in that case. Depending on allocation patterns, the iounit value could have been something reasonable that was carried over from previously freed fids or, in the worst case, could have been arbitrary values from non-fid related usages of the memory location. The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel after the uninitialized iounit field resulted in the typical sequence of two getxattr(2) syscalls, one to get the size of an xattr and another after allocating a sufficiently sized buffer to fit the xattr value, to hit an unexpected ERANGE error in the second call to getxattr(2). An uninitialized iounit field would sometimes force rsize to be smaller than the xattr value size in p9_client_read_once() and the 9P server in WSL refused to chunk up the READ on the attr_fid and, instead, returned ERANGE to the client. The virtfs server in QEMU seems happy to chunk up the READ and this problem goes undetected there. Link: https://lkml.kernel.org/r/20220710141402.803295-1-tyhicks@linux.microsoft.com Fixes: ebf46264a004 ("fs/9p: Add support user. xattr") Cc: stable@vger.kernel.org Signed-off-by: Tyler Hicks Reviewed-by: Christian Schoenebeck Signed-off-by: Dominique Martinet --- net/9p/client.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/net/9p/client.c b/net/9p/client.c index 89c5aeb00076..3c145a64dc2b 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -887,16 +887,13 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) struct p9_fid *fid; p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt); - fid = kmalloc(sizeof(*fid), GFP_KERNEL); + fid = kzalloc(sizeof(*fid), GFP_KERNEL); if (!fid) return NULL; - memset(&fid->qid, 0, sizeof(fid->qid)); fid->mode = -1; fid->uid = current_fsuid(); fid->clnt = clnt; - fid->rdir = NULL; - fid->fid = 0; refcount_set(&fid->count, 1); idr_preload(GFP_KERNEL);