[PATCH net-next 0/3] net: sysctl: allow dump_cpumask to handle higher numbers of CPUs

[PATCH net-next 0/3] net: sysctl: allow dump_cpumask to handle higher numbers of CPUs

[Antoine Tenart]

Hello,

The main goal of this series is to allow dump_cpumask to handle higher
numbers of CPUs (patch 3). While doing so I had the opportunity to make
the function a bit simpler, which is done in patches 1-2.

None of those is net material IMO.

Thanks,
Antoine

Antoine Tenart (3):
  net: sysctl: remove always-true condition
  net: sysctl: do not reserve an extra char in dump_cpumask temporary
    buffer
  net: sysctl: allow dump_cpumask to handle higher numbers of CPUs

 net/core/sysctl_net_core.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

-- 
2.47.0

[PATCH net-next 1/3] net: sysctl: remove always-true condition

[Antoine Tenart]

Before adding a new line at the end of the temporary buffer in
dump_cpumask, a length check is performed to ensure there is space for
it.

  len = min(sizeof(kbuf) - 1, *lenp);
  len = scnprintf(kbuf, len, ...);
  if (len < *lenp)
          kbuf[len++] = '\n';

Note that the check is currently logically wrong, the written length is
compared against the output buffer, not the temporary one. However this
has no consequence as this is always true, even if fixed: scnprintf
includes a null char at the end of the buffer but the returned length do
not include it and there is always space for overriding it with a
newline.

Remove the condition.

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/core/sysctl_net_core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index b60fac380cec..e7c0121dfaa1 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -69,8 +69,10 @@ static void dump_cpumask(void *buffer, size_t *lenp, loff_t *ppos,
 		return;
 	}
 
-	if (len < *lenp)
-		kbuf[len++] = '\n';
+	/* scnprintf writes a trailing null char not counted in the returned
+	 * length, override it with a newline.
+	 */
+	kbuf[len++] = '\n';
 	memcpy(buffer, kbuf, len);
 	*lenp = len;
 	*ppos += len;
-- 
2.47.0

Re: [PATCH net-next 1/3] net: sysctl: remove always-true condition

[Simon Horman]

On Thu, Oct 17, 2024 at 05:24:17PM +0200, Antoine Tenart wrote:
> Before adding a new line at the end of the temporary buffer in
> dump_cpumask, a length check is performed to ensure there is space for
> it.
> 
>   len = min(sizeof(kbuf) - 1, *lenp);
>   len = scnprintf(kbuf, len, ...);
>   if (len < *lenp)
>           kbuf[len++] = '\n';
> 
> Note that the check is currently logically wrong, the written length is
> compared against the output buffer, not the temporary one. However this
> has no consequence as this is always true, even if fixed: scnprintf
> includes a null char at the end of the buffer but the returned length do
> not include it and there is always space for overriding it with a
> newline.
> 
> Remove the condition.
> 
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

Thanks for separating out this and patch 2/3.
It makes it much easier to reason with these changes.

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 2/3] net: sysctl: do not reserve an extra char in dump_cpumask temporary buffer

[Antoine Tenart]

When computing the length we'll be able to use out of the buffers, one
char is removed from the temporary one to make room for a newline. It
should be removed from the output buffer length too, but in reality this
is not needed as the later call to scnprintf makes sure a null char is
written at the end of the buffer which we override with the newline.

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/core/sysctl_net_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index e7c0121dfaa1..8dc07f7b1772 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -62,7 +62,7 @@ static void dump_cpumask(void *buffer, size_t *lenp, loff_t *ppos,
 		return;
 	}
 
-	len = min(sizeof(kbuf) - 1, *lenp);
+	len = min(sizeof(kbuf), *lenp);
 	len = scnprintf(kbuf, len, "%*pb", cpumask_pr_args(mask));
 	if (!len) {
 		*lenp = 0;
-- 
2.47.0

[PATCH net-next 3/3] net: sysctl: allow dump_cpumask to handle higher numbers of CPUs

[Antoine Tenart]

This fixes the output of rps_default_mask and flow_limit_cpu_bitmap when
the CPU count is > 448, as it was truncated.

The underlying values are actually stored correctly when writing to
these sysctl but displaying them uses a fixed length temporary buffer in
dump_cpumask. This buffer can be too small if the CPU count is > 448.

Fix this by dynamically allocating the buffer in dump_cpumask, using a
guesstimate of what we need.

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 net/core/sysctl_net_core.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index 8dc07f7b1772..cb8d32e5c14e 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -51,22 +51,32 @@ int sysctl_devconf_inherit_init_net __read_mostly;
 EXPORT_SYMBOL(sysctl_devconf_inherit_init_net);
 
 #if IS_ENABLED(CONFIG_NET_FLOW_LIMIT) || IS_ENABLED(CONFIG_RPS)
-static void dump_cpumask(void *buffer, size_t *lenp, loff_t *ppos,
-			 struct cpumask *mask)
+static int dump_cpumask(void *buffer, size_t *lenp, loff_t *ppos,
+			struct cpumask *mask)
 {
-	char kbuf[128];
+	char *kbuf;
 	int len;
 
 	if (*ppos || !*lenp) {
 		*lenp = 0;
-		return;
+		return 0;
+	}
+
+	/* CPUs are displayed as a hex bitmap + a comma between each groups of 8
+	 * nibbles (except the last one which has a newline instead).
+	 * Guesstimate the buffer size at the group granularity level.
+	 */
+	len = min(DIV_ROUND_UP(nr_cpumask_bits, 32) * (8 + 1), *lenp);
+	kbuf = kmalloc(len, GFP_KERNEL);
+	if (!kbuf) {
+		*lenp = 0;
+		return -ENOMEM;
 	}
 
-	len = min(sizeof(kbuf), *lenp);
 	len = scnprintf(kbuf, len, "%*pb", cpumask_pr_args(mask));
 	if (!len) {
 		*lenp = 0;
-		return;
+		goto free_buf;
 	}
 
 	/* scnprintf writes a trailing null char not counted in the returned
@@ -76,6 +86,10 @@ static void dump_cpumask(void *buffer, size_t *lenp, loff_t *ppos,
 	memcpy(buffer, kbuf, len);
 	*lenp = len;
 	*ppos += len;
+
+free_buf:
+	kfree(kbuf);
+	return 0;
 }
 #endif
 
@@ -119,8 +133,8 @@ static int rps_default_mask_sysctl(const struct ctl_table *table, int write,
 		if (err)
 			goto done;
 	} else {
-		dump_cpumask(buffer, lenp, ppos,
-			     net->core.rps_default_mask ? : cpu_none_mask);
+		err = dump_cpumask(buffer, lenp, ppos,
+				   net->core.rps_default_mask ? : cpu_none_mask);
 	}
 
 done:
@@ -249,7 +263,7 @@ static int flow_limit_cpu_sysctl(const struct ctl_table *table, int write,
 		}
 		rcu_read_unlock();
 
-		dump_cpumask(buffer, lenp, ppos, mask);
+		ret = dump_cpumask(buffer, lenp, ppos, mask);
 	}
 
 done:
-- 
2.47.0

Re: [PATCH net-next 3/3] net: sysctl: allow dump_cpumask to handle higher numbers of CPUs

[Simon Horman]

On Thu, Oct 17, 2024 at 05:24:19PM +0200, Antoine Tenart wrote:
> This fixes the output of rps_default_mask and flow_limit_cpu_bitmap when
> the CPU count is > 448, as it was truncated.
> 
> The underlying values are actually stored correctly when writing to
> these sysctl but displaying them uses a fixed length temporary buffer in
> dump_cpumask. This buffer can be too small if the CPU count is > 448.
> 
> Fix this by dynamically allocating the buffer in dump_cpumask, using a
> guesstimate of what we need.
> 
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

+1 for using 'nibbles' in a comment in this patch.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 3/3] net: sysctl: allow dump_cpumask to handle higher numbers of CPUs

[Simon Horman]

On Thu, Oct 17, 2024 at 05:24:19PM +0200, Antoine Tenart wrote:
> This fixes the output of rps_default_mask and flow_limit_cpu_bitmap when
> the CPU count is > 448, as it was truncated.
> 
> The underlying values are actually stored correctly when writing to
> these sysctl but displaying them uses a fixed length temporary buffer in
> dump_cpumask. This buffer can be too small if the CPU count is > 448.
> 
> Fix this by dynamically allocating the buffer in dump_cpumask, using a
> guesstimate of what we need.
> 
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 0/3] net: sysctl: allow dump_cpumask to handle higher numbers of CPUs

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 17 Oct 2024 17:24:16 +0200 you wrote:
> Hello,
> 
> The main goal of this series is to allow dump_cpumask to handle higher
> numbers of CPUs (patch 3). While doing so I had the opportunity to make
> the function a bit simpler, which is done in patches 1-2.
> 
> None of those is net material IMO.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: sysctl: remove always-true condition
    https://git.kernel.org/netdev/net-next/c/d631094e4d20
  - [net-next,2/3] net: sysctl: do not reserve an extra char in dump_cpumask temporary buffer
    https://git.kernel.org/netdev/net-next/c/a8cc8fa14541
  - [net-next,3/3] net: sysctl: allow dump_cpumask to handle higher numbers of CPUs
    https://git.kernel.org/netdev/net-next/c/124afe773b1a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html