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/[email protected]
https://lore.kernel.org/r/[email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Suggested-by: Su Hui <[email protected]>
Cc: Dan Carpenter <[email protected]>
---
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
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 <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Suggested-by: Su Hui <[email protected]>
> Cc: Dan Carpenter <[email protected]>
> ---
> 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
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 <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Suggested-by: Su Hui <[email protected]>
> Cc: Dan Carpenter <[email protected]>
> ---
> 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 <[email protected]>
>
> 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",
>
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 <[email protected]>
> ---
> 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);
>