From 58e3ce767307130e911408c75f054d6a6673c8a3 Mon Sep 17 00:00:00 2001 From: Sishuai Gong Date: Tue, 8 Aug 2023 12:44:31 -0400 Subject: [PATCH 1/7] 9p/trans_fd: avoid sending req to a cancelled conn When a connection is cancelled by p9_conn_cancel(), all requests on it should be cancelled---mark req->status as REQ_STATUS_ERROR. However, because a race over m->err between p9_conn_cancel() and p9_fd_request(), p9_fd_request might see the old value of m->err, think that the connection is NOT cancelled, and then add new requests to this cancelled connection. Fixing this issue by lock-protecting the check on m->err. Signed-off-by: Sishuai Gong Message-ID: Signed-off-by: Dominique Martinet Reviewed-by: Christian Schoenebeck --- net/9p/trans_fd.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index c4015f30f9fa..f226953577b2 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -671,10 +671,14 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req) p9_debug(P9_DEBUG_TRANS, "mux %p task %p tcall %p id %d\n", m, current, &req->tc, req->tc.id); - if (m->err < 0) - return m->err; spin_lock(&m->req_lock); + + if (m->err < 0) { + spin_unlock(&m->req_lock); + return m->err; + } + WRITE_ONCE(req->status, REQ_STATUS_UNSENT); list_add_tail(&req->req_list, &m->unsent_req_list); spin_unlock(&m->req_lock); From a321af9dd0957713180e51c4ffc5225fc4cdc4f4 Mon Sep 17 00:00:00 2001 From: Yue Haibing Date: Mon, 7 Aug 2023 22:17:26 +0800 Subject: [PATCH 2/7] fs/9p: Remove unused function declaration v9fs_inode2stat() Commit 531b1094b743 ("[PATCH] v9fs: zero copy implementation") declared but never implemented this. Signed-off-by: Yue Haibing Message-ID: <20230807141726.38860-1-yuehaibing@huawei.com> Signed-off-by: Dominique Martinet --- fs/9p/v9fs_vfs.h | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h index cdf441f22e07..731e3d14b67d 100644 --- a/fs/9p/v9fs_vfs.h +++ b/fs/9p/v9fs_vfs.h @@ -52,7 +52,6 @@ void v9fs_stat2inode_dotl(struct p9_stat_dotl *stat, struct inode *inode, unsigned int flags); int v9fs_dir_release(struct inode *inode, struct file *filp); int v9fs_file_open(struct inode *inode, struct file *file); -void v9fs_inode2stat(struct inode *inode, struct p9_wstat *stat); int v9fs_uflags2omode(int uflags, int extended); void v9fs_blank_wstat(struct p9_wstat *wstat); From 355f074609dbf3042900ea9d30fcd2b0c323a365 Mon Sep 17 00:00:00 2001 From: Marco Elver Date: Wed, 25 Oct 2023 19:34:43 +0900 Subject: [PATCH 3/7] 9p/trans_fd: Annotate data-racy writes to file::f_flags syzbot reported: | BUG: KCSAN: data-race in p9_fd_create / p9_fd_create | | read-write to 0xffff888130fb3d48 of 4 bytes by task 15599 on cpu 0: | p9_fd_open net/9p/trans_fd.c:842 [inline] | p9_fd_create+0x210/0x250 net/9p/trans_fd.c:1092 | p9_client_create+0x595/0xa70 net/9p/client.c:1010 | v9fs_session_init+0xf9/0xd90 fs/9p/v9fs.c:410 | v9fs_mount+0x69/0x630 fs/9p/vfs_super.c:123 | legacy_get_tree+0x74/0xd0 fs/fs_context.c:611 | vfs_get_tree+0x51/0x190 fs/super.c:1519 | do_new_mount+0x203/0x660 fs/namespace.c:3335 | path_mount+0x496/0xb30 fs/namespace.c:3662 | do_mount fs/namespace.c:3675 [inline] | __do_sys_mount fs/namespace.c:3884 [inline] | [...] | | read-write to 0xffff888130fb3d48 of 4 bytes by task 15563 on cpu 1: | p9_fd_open net/9p/trans_fd.c:842 [inline] | p9_fd_create+0x210/0x250 net/9p/trans_fd.c:1092 | p9_client_create+0x595/0xa70 net/9p/client.c:1010 | v9fs_session_init+0xf9/0xd90 fs/9p/v9fs.c:410 | v9fs_mount+0x69/0x630 fs/9p/vfs_super.c:123 | legacy_get_tree+0x74/0xd0 fs/fs_context.c:611 | vfs_get_tree+0x51/0x190 fs/super.c:1519 | do_new_mount+0x203/0x660 fs/namespace.c:3335 | path_mount+0x496/0xb30 fs/namespace.c:3662 | do_mount fs/namespace.c:3675 [inline] | __do_sys_mount fs/namespace.c:3884 [inline] | [...] | | value changed: 0x00008002 -> 0x00008802 Within p9_fd_open(), O_NONBLOCK is added to f_flags of the read and write files. This may happen concurrently if e.g. mounting process modifies the fd in another thread. Mark the plain read-modify-writes as intentional data-races, with the assumption that the result of executing the accesses concurrently will always result in the same result despite the accesses themselves not being atomic. Reported-by: syzbot+e441aeeb422763cc5511@syzkaller.appspotmail.com Signed-off-by: Marco Elver Link: https://lore.kernel.org/r/ZO38mqkS0TYUlpFp@elver.google.com Signed-off-by: Dominique Martinet Message-ID: <20231025103445.1248103-1-asmadeus@codewreck.org> --- net/9p/trans_fd.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index f226953577b2..1a3948b8c493 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -836,14 +836,21 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd) goto out_free_ts; if (!(ts->rd->f_mode & FMODE_READ)) goto out_put_rd; - /* prevent workers from hanging on IO when fd is a pipe */ - ts->rd->f_flags |= O_NONBLOCK; + /* Prevent workers from hanging on IO when fd is a pipe. + * It's technically possible for userspace or concurrent mounts to + * modify this flag concurrently, which will likely result in a + * broken filesystem. However, just having bad flags here should + * not crash the kernel or cause any other sort of bug, so mark this + * particular data race as intentional so that tooling (like KCSAN) + * can allow it and detect further problems. + */ + data_race(ts->rd->f_flags |= O_NONBLOCK); ts->wr = fget(wfd); if (!ts->wr) goto out_put_rd; if (!(ts->wr->f_mode & FMODE_WRITE)) goto out_put_wr; - ts->wr->f_flags |= O_NONBLOCK; + data_race(ts->wr->f_flags |= O_NONBLOCK); client->trans = ts; client->status = Connected; From 9b5c6281838fc84683dd99b47302d81fce399918 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Wed, 25 Oct 2023 19:34:44 +0900 Subject: [PATCH 4/7] 9p: v9fs_listxattr: fix %s null argument warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W=1 warns about null argument to kprintf: In file included from fs/9p/xattr.c:12: In function ‘v9fs_xattr_get’, inlined from ‘v9fs_listxattr’ at fs/9p/xattr.c:142:9: include/net/9p/9p.h:55:2: error: ‘%s’ directive argument is null [-Werror=format-overflow=] 55 | _p9_debug(level, __func__, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Use an empty string instead of : - this is ok 9p-wise because p9pdu_vwritef serializes a null string and an empty string the same way (one '0' word for length) - since this degrades the print statements, add new single quotes for xattr's name delimter (Old: "file = (null)", new: "file = ''") Link: https://lore.kernel.org/r/20231008060138.517057-1-suhui@nfschina.com Suggested-by: Su Hui Signed-off-by: Dominique Martinet Acked-by: Christian Schoenebeck Message-ID: <20231025103445.1248103-2-asmadeus@codewreck.org> --- fs/9p/xattr.c | 5 +++-- net/9p/client.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c index e00cf8109b3f..3c4572ef3a48 100644 --- a/fs/9p/xattr.c +++ b/fs/9p/xattr.c @@ -68,7 +68,7 @@ ssize_t v9fs_xattr_get(struct dentry *dentry, const char *name, struct p9_fid *fid; int ret; - p9_debug(P9_DEBUG_VFS, "name = %s value_len = %zu\n", + p9_debug(P9_DEBUG_VFS, "name = '%s' value_len = %zu\n", name, buffer_size); fid = v9fs_fid_lookup(dentry); if (IS_ERR(fid)) @@ -139,7 +139,8 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) { - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); + /* Txattrwalk with an empty string lists xattrs instead */ + return v9fs_xattr_get(dentry, "", buffer, buffer_size); } static int v9fs_xattr_handler_get(const struct xattr_handler *handler, diff --git a/net/9p/client.c b/net/9p/client.c index 86bbc7147fc1..9c2bc15e3cfa 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -1979,7 +1979,7 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid, goto error; } p9_debug(P9_DEBUG_9P, - ">>> TXATTRWALK file_fid %d, attr_fid %d name %s\n", + ">>> TXATTRWALK file_fid %d, attr_fid %d name '%s'\n", file_fid->fid, attr_fid->fid, attr_name); req = p9_client_rpc(clnt, P9_TXATTRWALK, "dds", From 39763480dd19fae66c0051a5f42d1ee4dfdc18cb Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Wed, 25 Oct 2023 19:34:45 +0900 Subject: [PATCH 5/7] 9p/net: xen: fix false positive printf format overflow warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the constant to make the compiler happy about this warning: net/9p/trans_xen.c: In function ‘xen_9pfs_front_changed’: net/9p/trans_xen.c:444:39: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 8 [-Wformat-overflow=] 444 | sprintf(str, "ring-ref%d", i); | ^~ In function ‘xen_9pfs_front_init’, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:516:8, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:504:13: net/9p/trans_xen.c:444:30: note: directive argument in the range [-2147483644, 2147483646] 444 | sprintf(str, "ring-ref%d", i); | ^~~~~~~~~~~~ net/9p/trans_xen.c:444:17: note: ‘sprintf’ output between 10 and 20 bytes into a destination of size 16 444 | sprintf(str, "ring-ref%d", i); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/9p/trans_xen.c: In function ‘xen_9pfs_front_changed’: net/9p/trans_xen.c:450:45: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 2 [-Wformat-overflow=] 450 | sprintf(str, "event-channel-%d", i); | ^~ In function ‘xen_9pfs_front_init’, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:516:8, inlined from ‘xen_9pfs_front_changed’ at net/9p/trans_xen.c:504:13: net/9p/trans_xen.c:450:30: note: directive argument in the range [-2147483644, 2147483646] 450 | sprintf(str, "event-channel-%d", i); | ^~~~~~~~~~~~~~~~~~ net/9p/trans_xen.c:450:17: note: ‘sprintf’ output between 16 and 26 bytes into a destination of size 16 450 | sprintf(str, "event-channel-%d", i); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There is no change in logic: there only are a constant number of rings, and there also already is a BUILD_BUG_ON that checks if that constant goes over 9 as anything bigger would no longer fit the event-channel-%d destination size. In theory having that size as part of the struct means it could be modified by another thread and makes the compiler lose track of possible values for 'i' here, using the constant directly here makes it work. Signed-off-by: Dominique Martinet Message-ID: <20231025103445.1248103-3-asmadeus@codewreck.org> Reviewed-by: Christian Schoenebeck --- net/9p/trans_xen.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c index 1fffe2bed5b0..dfdbe1ca5338 100644 --- a/net/9p/trans_xen.c +++ b/net/9p/trans_xen.c @@ -54,7 +54,6 @@ struct xen_9pfs_front_priv { char *tag; struct p9_client *client; - int num_rings; struct xen_9pfs_dataring *rings; }; @@ -131,7 +130,7 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req) if (list_entry_is_head(priv, &xen_9pfs_devs, list)) return -EINVAL; - num = p9_req->tc.tag % priv->num_rings; + num = p9_req->tc.tag % XEN_9PFS_NUM_RINGS; ring = &priv->rings[num]; again: @@ -279,7 +278,7 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv) list_del(&priv->list); write_unlock(&xen_9pfs_lock); - for (i = 0; i < priv->num_rings; i++) { + for (i = 0; i < XEN_9PFS_NUM_RINGS; i++) { struct xen_9pfs_dataring *ring = &priv->rings[i]; cancel_work_sync(&ring->work); @@ -408,15 +407,14 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order)) p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2; - priv->num_rings = XEN_9PFS_NUM_RINGS; - priv->rings = kcalloc(priv->num_rings, sizeof(*priv->rings), + priv->rings = kcalloc(XEN_9PFS_NUM_RINGS, sizeof(*priv->rings), GFP_KERNEL); if (!priv->rings) { kfree(priv); return -ENOMEM; } - for (i = 0; i < priv->num_rings; i++) { + for (i = 0; i < XEN_9PFS_NUM_RINGS; i++) { priv->rings[i].priv = priv; ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i], max_ring_order); @@ -434,10 +432,11 @@ static int xen_9pfs_front_init(struct xenbus_device *dev) if (ret) goto error_xenbus; ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", - priv->num_rings); + XEN_9PFS_NUM_RINGS); if (ret) goto error_xenbus; - for (i = 0; i < priv->num_rings; i++) { + + for (i = 0; i < XEN_9PFS_NUM_RINGS; i++) { char str[16]; BUILD_BUG_ON(XEN_9PFS_NUM_RINGS > 9); From e02be6390d6fddae6b4b9053caea9fc5ca011f32 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Thu, 26 Oct 2023 07:27:52 +0900 Subject: [PATCH 6/7] 9p/fs: add MODULE_DESCIPTION Fix modpost warning that MODULE_DESCRIPTION is missing in fs/9p/9p.o Signed-off-by: Dominique Martinet Message-ID: <20231025223107.1274963-1-asmadeus@codewreck.org> Reviewed-by: Christian Schoenebeck --- fs/9p/v9fs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c index d525957594b6..61dbe52bb3a3 100644 --- a/fs/9p/v9fs.c +++ b/fs/9p/v9fs.c @@ -732,4 +732,5 @@ module_exit(exit_v9fs) MODULE_AUTHOR("Latchesar Ionkov "); MODULE_AUTHOR("Eric Van Hensbergen "); MODULE_AUTHOR("Ron Minnich "); +MODULE_DESCRIPTION("9P Client File System"); MODULE_LICENSE("GPL"); From ce07087964208eee2ca2f9ee4a98f8b5d9027fe6 Mon Sep 17 00:00:00 2001 From: Hangyu Hua Date: Fri, 27 Oct 2023 11:03:02 +0800 Subject: [PATCH 7/7] 9p/net: fix possible memory leak in p9_check_errors() When p9pdu_readf() is called with "s?d" attribute, it allocates a pointer that will store a string. But when p9pdu_readf() fails while handling "d" then this pointer will not be freed in p9_check_errors(). Fixes: 51a87c552dfd ("9p: rework client code to use new protocol support functions") Reviewed-by: Christian Schoenebeck Signed-off-by: Hangyu Hua Message-ID: <20231027030302.11927-1-hbh25y@gmail.com> Signed-off-by: Dominique Martinet --- net/9p/client.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/net/9p/client.c b/net/9p/client.c index 9c2bc15e3cfa..e265a0ca6bdd 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -540,12 +540,14 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req) return 0; if (!p9_is_proto_dotl(c)) { - char *ename; + char *ename = NULL; err = p9pdu_readf(&req->rc, c->proto_version, "s?d", &ename, &ecode); - if (err) + if (err) { + kfree(ename); goto out_err; + } if (p9_is_proto_dotu(c) && ecode < 512) err = -ecode;