From: Marco Elver <[email protected]>
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: [email protected]
Signed-off-by: Marco Elver <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Dominique Martinet <[email protected]>
---
v1 -> v2:
- reworded comment as discussed
- adjusted commit subject line to match with other trans_fd patch
(there is no change from the one I sent yesterday, I'm just resending
the whole set properly now there's been feedback on other commits)
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
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 <[email protected]>
Message-ID: <[email protected]>
---
v1->v2:
- use constant directly instead of going through a local variable
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);
--
2.41.0
On Wednesday, October 25, 2023 12:34:45 PM CEST Dominique Martinet wrote:
> 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 <[email protected]>
> Message-ID: <[email protected]>
> ---
Reviewed-by: Christian Schoenebeck <[email protected]>
> v1->v2:
> - use constant directly instead of going through a local variable
>
> 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);
>