* [PATCH 0/3] Small patches for 6.7
@ 2023-10-23 23:37 Dominique Martinet
2023-10-23 23:37 ` [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount Dominique Martinet
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Dominique Martinet @ 2023-10-23 23:37 UTC (permalink / raw)
To: v9fs, ericvh, linux_oss; +Cc: lucho, linux-kernel, Dominique Martinet
Some of the patches we got this cycle required a bit of re-work so I'm
posting them here; nothing big but please have a look when you can.
I'll also include these two patches:
https://lore.kernel.org/r/20230807141726.38860-1-yuehaibing@huawei.com
https://lore.kernel.org/r/AA2DB53B-DFC7-4B88-9515-E4C9AFA6435D@gmail.com
This set passes my quick checks and honestly can't break much, so I'll
be pushing them to next later today.
I don't think I'll be submitting anything else to Linus this time, but
please point out if you notice anything I forgot!
Dominique Martinet (2):
9p: v9fs_listxattr: fix %s null argument warning
9p/net: xen: fix false positive printf format overflow warning
Marco Elver (1):
9p: Annotate data-racy writes to file::f_flags on fd mount
fs/9p/xattr.c | 4 ++--
net/9p/client.c | 2 +-
net/9p/trans_fd.c | 8 +++++---
net/9p/trans_xen.c | 13 +++++++------
4 files changed, 15 insertions(+), 12 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount
2023-10-23 23:37 [PATCH 0/3] Small patches for 6.7 Dominique Martinet
@ 2023-10-23 23:37 ` Dominique Martinet
2023-10-24 7:12 ` Marco Elver
2023-10-24 11:58 ` [PATCH v2] 9p/trans_fd: Annotate data-racy writes to file::f_flags Dominique Martinet
2023-10-23 23:37 ` [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning Dominique Martinet
2023-10-23 23:37 ` [PATCH 3/3] 9p/net: xen: fix false positive printf format overflow warning Dominique Martinet
2 siblings, 2 replies; 11+ messages in thread
From: Dominique Martinet @ 2023-10-23 23:37 UTC (permalink / raw)
To: v9fs, ericvh, linux_oss
Cc: lucho, linux-kernel, Marco Elver, syzbot+e441aeeb422763cc5511,
Dominique Martinet
From: Marco Elver <elver@google.com>
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 <elver@google.com>
Link: https://lore.kernel.org/r/ZO38mqkS0TYUlpFp@elver.google.com
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
Hi Marco, sorry for taking ages to process this patch. I've reworded the
commit message a bit and added a comment, so given this has your name on
it please have a look.
I'm planning to send this to Linus during the merge window next week
net/9p/trans_fd.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index f226953577b2..d89c88986950 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -836,14 +836,16 @@ 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
+ * We don't support userspace messing with the fd after passing it
+ * to mount, so flag possible data race for KCSAN */
+ 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;
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning
2023-10-23 23:37 [PATCH 0/3] Small patches for 6.7 Dominique Martinet
2023-10-23 23:37 ` [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount Dominique Martinet
@ 2023-10-23 23:37 ` Dominique Martinet
2023-10-24 5:16 ` Dan Carpenter
2023-10-24 12:29 ` Christian Schoenebeck
2023-10-23 23:37 ` [PATCH 3/3] 9p/net: xen: fix false positive printf format overflow warning Dominique Martinet
2 siblings, 2 replies; 11+ messages in thread
From: Dominique Martinet @ 2023-10-23 23:37 UTC (permalink / raw)
To: v9fs, ericvh, linux_oss
Cc: lucho, linux-kernel, Dominique Martinet, Su Hui, Dan Carpenter
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 = ''")
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
Link: https://lore.kernel.org/r/20231008060138.517057-1-suhui@nfschina.com
Suggested-by: Su Hui <suhui@nfschina.com>
Cc: Dan Carpenter <dan.carpenter@linaro.org>
---
I've checked this works as expected (getfattr listing all user.* xattrs
after setting some), so let's fix this warning.
As pointed out by Dan this makes the message les clear, so I added
single quotes to make it clear we're dealing with an empty string; I
think it's good enough.
Su, I made you only Suggested-by because of the extra legwork and
format changes, but happy to give you authorship if it's something you
care about; I'd just like to get it out during the next merge window
in a couple of weeks so please say the word.
This makes fs/9p build warning-free with W=1 on gcc 12
fs/9p/xattr.c | 4 ++--
net/9p/client.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
index e00cf8109b3f..d29907c378fd 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,7 @@ 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);
+ 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",
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] 9p/net: xen: fix false positive printf format overflow warning
2023-10-23 23:37 [PATCH 0/3] Small patches for 6.7 Dominique Martinet
2023-10-23 23:37 ` [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount Dominique Martinet
2023-10-23 23:37 ` [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning Dominique Martinet
@ 2023-10-23 23:37 ` Dominique Martinet
2023-10-24 12:52 ` Christian Schoenebeck
2 siblings, 1 reply; 11+ messages in thread
From: Dominique Martinet @ 2023-10-23 23:37 UTC (permalink / raw)
To: v9fs, ericvh, linux_oss; +Cc: lucho, linux-kernel, Dominique Martinet
Use a local variable 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 a local variable makes it happy.
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
While looking at warnings with W=1, I noticed this one in net/9p.
This is silly but shouldn't hurt, num_rings is never changed so there is
no risk of introducing a race here, it's just helping the compiler a
bit.
net/9p is also now warning-free at W=1
net/9p/trans_xen.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 1fffe2bed5b0..79e91f31a84a 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -382,7 +382,7 @@ static int xen_9pfs_front_init(struct xenbus_device *dev)
struct xenbus_transaction xbt;
struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
char *versions, *v;
- unsigned int max_rings, max_ring_order, len = 0;
+ unsigned int num_rings, max_rings, max_ring_order, len = 0;
versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
if (IS_ERR(versions))
@@ -408,15 +408,15 @@ 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),
+ num_rings = priv->num_rings = XEN_9PFS_NUM_RINGS;
+ priv->rings = kcalloc(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 < num_rings; i++) {
priv->rings[i].priv = priv;
ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i],
max_ring_order);
@@ -434,10 +434,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);
+ num_rings);
if (ret)
goto error_xenbus;
- for (i = 0; i < priv->num_rings; i++) {
+
+ for (i = 0; i < num_rings; i++) {
char str[16];
BUILD_BUG_ON(XEN_9PFS_NUM_RINGS > 9);
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning
2023-10-23 23:37 ` [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning Dominique Martinet
@ 2023-10-24 5:16 ` Dan Carpenter
2023-10-24 12:29 ` Christian Schoenebeck
1 sibling, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2023-10-24 5:16 UTC (permalink / raw)
To: Dominique Martinet; +Cc: v9fs, ericvh, linux_oss, lucho, linux-kernel, Su Hui
On Tue, Oct 24, 2023 at 08:37:03AM +0900, Dominique Martinet wrote:
> 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 = ''")
>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> Link: https://lore.kernel.org/r/20231008060138.517057-1-suhui@nfschina.com
> Suggested-by: Su Hui <suhui@nfschina.com>
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I've checked this works as expected (getfattr listing all user.* xattrs
> after setting some), so let's fix this warning.
Awesome. Thanks!
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount
2023-10-23 23:37 ` [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount Dominique Martinet
@ 2023-10-24 7:12 ` Marco Elver
2023-10-24 7:44 ` Dominique Martinet
2023-10-24 11:58 ` [PATCH v2] 9p/trans_fd: Annotate data-racy writes to file::f_flags Dominique Martinet
1 sibling, 1 reply; 11+ messages in thread
From: Marco Elver @ 2023-10-24 7:12 UTC (permalink / raw)
To: Dominique Martinet
Cc: v9fs, ericvh, linux_oss, lucho, linux-kernel,
syzbot+e441aeeb422763cc5511
On Tue, 24 Oct 2023 at 01:37, Dominique Martinet <asmadeus@codewreck.org> wrote:
>
> From: Marco Elver <elver@google.com>
>
> 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 <elver@google.com>
> Link: https://lore.kernel.org/r/ZO38mqkS0TYUlpFp@elver.google.com
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
>
> Hi Marco, sorry for taking ages to process this patch. I've reworded the
> commit message a bit and added a comment, so given this has your name on
> it please have a look.
> I'm planning to send this to Linus during the merge window next week
No worries, and thank you!
> net/9p/trans_fd.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index f226953577b2..d89c88986950 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -836,14 +836,16 @@ 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
Add '.' at end of sentence(s)?
> + * We don't support userspace messing with the fd after passing it
> + * to mount, so flag possible data race for KCSAN */
The comment should explain why the data race is safe, independent of
KCSAN. I still don't quite get why it's safe.
The case that syzbot found was 2 concurrent mount. Is that also disallowed?
Maybe something like: "We don't support userspace messing with the fd
after passing it to the first mount. While it's not officially
supported, concurrent modification of flags is unlikely to break this
code. So that tooling (like KCSAN) knows about it, mark them as
intentional data races."
> + 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;
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount
2023-10-24 7:12 ` Marco Elver
@ 2023-10-24 7:44 ` Dominique Martinet
2023-10-24 7:49 ` Marco Elver
0 siblings, 1 reply; 11+ messages in thread
From: Dominique Martinet @ 2023-10-24 7:44 UTC (permalink / raw)
To: Marco Elver
Cc: v9fs, ericvh, linux_oss, lucho, linux-kernel,
syzbot+e441aeeb422763cc5511
Marco Elver wrote on Tue, Oct 24, 2023 at 09:12:56AM +0200:
> > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> > index f226953577b2..d89c88986950 100644
> > --- a/net/9p/trans_fd.c
> > +++ b/net/9p/trans_fd.c
> > @@ -836,14 +836,16 @@ 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
>
> Add '.' at end of sentence(s)?
I don't think we have any rule about this in the 9p part of the tree,
looking around there seem to be more comments without '.' than with, but
it's never too late to start... I'll add some in a v2 after we've agreed
with the rest.
>
> > + * We don't support userspace messing with the fd after passing it
> > + * to mount, so flag possible data race for KCSAN */
>
> The comment should explain why the data race is safe, independent of
> KCSAN. I still don't quite get why it's safe.
I guess it depends on what we call 'safe' here: if we agree the worst
thing that can happen is weird flags being set when we didn't request
them and socket operations behaving oddly (of the level of block when
they shouldn't), we don't care because there's no way to make concurrent
usage of the fd work anyway.
If it's possible to get an invalid value there such that a socket
operation ends up executing user-controlled code somewhere, then we've
got a bigger problem and we should take some lock (presumably the same
lock fcntl(F_SETFD) is taking, as that's got more potential for breakage
than another mount in my opinon)
> The case that syzbot found was 2 concurrent mount. Is that also disallowed?
Yes, there's no way you'll get a working filesystem out of two mounts
using the same fd as the protocol has no muxing
> Maybe something like: "We don't support userspace messing with the fd
> after passing it to the first mount. While it's not officially
> supported, concurrent modification of flags is unlikely to break this
> code. So that tooling (like KCSAN) knows about it, mark them as
> intentional data races."
I'd word this as much likely to break, how about something like this?
/* 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.
*/
Thanks,
--
Dominique Martinet | Asmadeus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount
2023-10-24 7:44 ` Dominique Martinet
@ 2023-10-24 7:49 ` Marco Elver
0 siblings, 0 replies; 11+ messages in thread
From: Marco Elver @ 2023-10-24 7:49 UTC (permalink / raw)
To: Dominique Martinet
Cc: v9fs, ericvh, linux_oss, lucho, linux-kernel,
syzbot+e441aeeb422763cc5511
On Tue, 24 Oct 2023 at 09:44, Dominique Martinet <asmadeus@codewreck.org> wrote:
>
> Marco Elver wrote on Tue, Oct 24, 2023 at 09:12:56AM +0200:
> > > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> > > index f226953577b2..d89c88986950 100644
> > > --- a/net/9p/trans_fd.c
> > > +++ b/net/9p/trans_fd.c
> > > @@ -836,14 +836,16 @@ 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
> >
> > Add '.' at end of sentence(s)?
>
> I don't think we have any rule about this in the 9p part of the tree,
> looking around there seem to be more comments without '.' than with, but
> it's never too late to start... I'll add some in a v2 after we've agreed
> with the rest.
Sounds good.
I think if there's 1 short sentence (1 line) comment, it's more or
less optional. But I'd insist on punctuation as soon as there are 2 or
more sentences.
> >
> > > + * We don't support userspace messing with the fd after passing it
> > > + * to mount, so flag possible data race for KCSAN */
> >
> > The comment should explain why the data race is safe, independent of
> > KCSAN. I still don't quite get why it's safe.
>
> I guess it depends on what we call 'safe' here: if we agree the worst
> thing that can happen is weird flags being set when we didn't request
> them and socket operations behaving oddly (of the level of block when
> they shouldn't), we don't care because there's no way to make concurrent
> usage of the fd work anyway.
Yes, that's reasonable.
> If it's possible to get an invalid value there such that a socket
> operation ends up executing user-controlled code somewhere, then we've
> got a bigger problem and we should take some lock (presumably the same
> lock fcntl(F_SETFD) is taking, as that's got more potential for breakage
> than another mount in my opinon)
>
> > The case that syzbot found was 2 concurrent mount. Is that also disallowed?
>
> Yes, there's no way you'll get a working filesystem out of two mounts
> using the same fd as the protocol has no muxing
>
> > Maybe something like: "We don't support userspace messing with the fd
> > after passing it to the first mount. While it's not officially
> > supported, concurrent modification of flags is unlikely to break this
> > code. So that tooling (like KCSAN) knows about it, mark them as
> > intentional data races."
>
> I'd word this as much likely to break, how about something like this?
>
> /* 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.
> */
I think this sounds much clearer. Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] 9p/trans_fd: Annotate data-racy writes to file::f_flags
2023-10-23 23:37 ` [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount Dominique Martinet
2023-10-24 7:12 ` Marco Elver
@ 2023-10-24 11:58 ` Dominique Martinet
1 sibling, 0 replies; 11+ messages in thread
From: Dominique Martinet @ 2023-10-24 11:58 UTC (permalink / raw)
To: v9fs
Cc: ericvh, linux_oss, lucho, linux-kernel, Marco Elver,
syzbot+e441aeeb422763cc5511, Dominique Martinet
From: Marco Elver <elver@google.com>
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 <elver@google.com>
Link: https://lore.kernel.org/r/ZO38mqkS0TYUlpFp@elver.google.com
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
v1 -> v2:
- reworded comment as discussed
- adjusted commit subject line to match with other trans_fd patch
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;
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning
2023-10-23 23:37 ` [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning Dominique Martinet
2023-10-24 5:16 ` Dan Carpenter
@ 2023-10-24 12:29 ` Christian Schoenebeck
1 sibling, 0 replies; 11+ messages in thread
From: Christian Schoenebeck @ 2023-10-24 12:29 UTC (permalink / raw)
To: v9fs, ericvh, Dominique Martinet
Cc: lucho, linux-kernel, Dominique Martinet, Su Hui, Dan Carpenter
On Tuesday, October 24, 2023 1:37:03 AM CEST Dominique Martinet wrote:
> 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 = ''")
>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> Link: https://lore.kernel.org/r/20231008060138.517057-1-suhui@nfschina.com
> Suggested-by: Su Hui <suhui@nfschina.com>
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I've checked this works as expected (getfattr listing all user.* xattrs
> after setting some), so let's fix this warning.
>
> As pointed out by Dan this makes the message les clear, so I added
> single quotes to make it clear we're dealing with an empty string; I
> think it's good enough.
> Su, I made you only Suggested-by because of the extra legwork and
> format changes, but happy to give you authorship if it's something you
> care about; I'd just like to get it out during the next merge window
> in a couple of weeks so please say the word.
>
> This makes fs/9p build warning-free with W=1 on gcc 12
>
> fs/9p/xattr.c | 4 ++--
> net/9p/client.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
> index e00cf8109b3f..d29907c378fd 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,7 @@ 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);
> + return v9fs_xattr_get(dentry, "", buffer, buffer_size);
> }
As from the previous discussions on this, it might be worth to add a short
comment here that the magical empty string in the subsequent 'Txattrwalk' 9p
request causes 9p server to return a list of xattrs instead of one specific
xattr.
Up to you though:
Acked-by: Christian Schoenebeck <linux_oss@crudebyte.com>
>
> 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",
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] 9p/net: xen: fix false positive printf format overflow warning
2023-10-23 23:37 ` [PATCH 3/3] 9p/net: xen: fix false positive printf format overflow warning Dominique Martinet
@ 2023-10-24 12:52 ` Christian Schoenebeck
0 siblings, 0 replies; 11+ messages in thread
From: Christian Schoenebeck @ 2023-10-24 12:52 UTC (permalink / raw)
To: v9fs, ericvh, Dominique Martinet; +Cc: lucho, linux-kernel, Dominique Martinet
On Tuesday, October 24, 2023 1:37:04 AM CEST Dominique Martinet wrote:
> Use a local variable 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 a local variable makes it happy.
Or ... what about dropping struct's 'num_rings' member and using the constant
XEN_9PFS_NUM_RINGS everywhere instead? As this is really a compile-time value
after all.
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> While looking at warnings with W=1, I noticed this one in net/9p.
>
> This is silly but shouldn't hurt, num_rings is never changed so there is
> no risk of introducing a race here, it's just helping the compiler a
> bit.
>
> net/9p is also now warning-free at W=1
>
> net/9p/trans_xen.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index 1fffe2bed5b0..79e91f31a84a 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -382,7 +382,7 @@ static int xen_9pfs_front_init(struct xenbus_device *dev)
> struct xenbus_transaction xbt;
> struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
> char *versions, *v;
> - unsigned int max_rings, max_ring_order, len = 0;
> + unsigned int num_rings, max_rings, max_ring_order, len = 0;
>
> versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
> if (IS_ERR(versions))
> @@ -408,15 +408,15 @@ 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),
> + num_rings = priv->num_rings = XEN_9PFS_NUM_RINGS;
> + priv->rings = kcalloc(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 < num_rings; i++) {
> priv->rings[i].priv = priv;
> ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i],
> max_ring_order);
> @@ -434,10 +434,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);
> + num_rings);
> if (ret)
> goto error_xenbus;
> - for (i = 0; i < priv->num_rings; i++) {
> +
> + for (i = 0; i < num_rings; i++) {
> char str[16];
>
> BUILD_BUG_ON(XEN_9PFS_NUM_RINGS > 9);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-10-24 12:53 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-23 23:37 [PATCH 0/3] Small patches for 6.7 Dominique Martinet
2023-10-23 23:37 ` [PATCH 1/3] 9p: Annotate data-racy writes to file::f_flags on fd mount Dominique Martinet
2023-10-24 7:12 ` Marco Elver
2023-10-24 7:44 ` Dominique Martinet
2023-10-24 7:49 ` Marco Elver
2023-10-24 11:58 ` [PATCH v2] 9p/trans_fd: Annotate data-racy writes to file::f_flags Dominique Martinet
2023-10-23 23:37 ` [PATCH 2/3] 9p: v9fs_listxattr: fix %s null argument warning Dominique Martinet
2023-10-24 5:16 ` Dan Carpenter
2023-10-24 12:29 ` Christian Schoenebeck
2023-10-23 23:37 ` [PATCH 3/3] 9p/net: xen: fix false positive printf format overflow warning Dominique Martinet
2023-10-24 12:52 ` Christian Schoenebeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox