[PATCH net-next v5 0/8] Some pktgen fixes/improvments (part I)

[PATCH net-next v5 0/8] Some pktgen fixes/improvments (part I)

[Peter Seiderer]

While taking a look at '[PATCH net] pktgen: Avoid out-of-range in
get_imix_entries' ([1]) and '[PATCH net v2] pktgen: Avoid out-of-bounds
access in get_imix_entries' ([2], [3]) and doing some tests and code review
I detected that the /proc/net/pktgen/... parsing logic does not honour the
user given buffer bounds (resulting in out-of-bounds access).

This can be observed e.g. by the following simple test (sometimes the
old/'longer' previous value is re-read from the buffer):

        $ echo add_device lo@0 > /proc/net/pktgen/kpktgend_0

        $ echo "min_pkt_size 12345" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
Params: count 1000  min_pkt_size: 12345  max_pkt_size: 0
Result: OK: min_pkt_size=12345

        $ echo -n "min_pkt_size 123" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
Params: count 1000  min_pkt_size: 12345  max_pkt_size: 0
Result: OK: min_pkt_size=12345

        $ echo "min_pkt_size 123" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
Params: count 1000  min_pkt_size: 123  max_pkt_size: 0
Result: OK: min_pkt_size=123

So fix the out-of-bounds access (and some minor findings) and add a simple
proc_net_pktgen selftest...

Patch set splited into part I (this one)

- net: pktgen: replace ENOTSUPP with EOPNOTSUPP
- net: pktgen: enable 'param=value' parsing
- net: pktgen: fix hex32_arg parsing for short reads
- net: pktgen: fix 'rate 0' error handling (return -EINVAL)
- net: pktgen: fix 'ratep 0' error handling (return -EINVAL)
- net: pktgen: fix ctrl interface command parsing
- net: pktgen: fix access outside of user given buffer in pktgen_thread_write()

And part II (will follow):

- net: pktgen: use defines for the various dec/hex number parsing digits lengths
- net: pktgen: fix mix of int/long
- net: pktgen: remove extra tmp variable (re-use len instead)
- net: pktgen: remove some superfluous variable initializing
- net: pktgen: fix mpls maximum labels list parsing
- net: pktgen: fix access outside of user given buffer in pktgen_if_write()
- net: pktgen: fix mpls reset parsing
- net: pktgen: remove all superfluous index assignements
- selftest: net: add proc_net_pktgen

Regards,
Peter

Changes v4 -> v5:
 - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4:
 - add rev-by Simon Horman
 - new patch 'net: pktgen: use defines for the various dec/hex number parsing
   digits lengths' (suggested by Simon Horman)
 - replace C99 comment (suggested by Paolo Abeni)
 - drop available characters check in strn_len() (suggested by Paolo Abeni)
 - factored out patch 'net: pktgen: align some variable declarations to the
   most common pattern' (suggested by Paolo Abeni)
 - factored out patch 'net: pktgen: remove extra tmp variable (re-use len
   instead)' (suggested by Paolo Abeni)
 - factored out patch 'net: pktgen: remove some superfluous variable
   initializing' (suggested by Paolo Abeni)
 - factored out patch 'net: pktgen: fix mpls maximum labels list parsing'
   (suggested by Paolo Abeni)
 - factored out 'net: pktgen: hex32_arg/num_arg error out in case no
   characters are available' (suggested by Paolo Abeni)
 - factored out 'net: pktgen: num_arg error out in case no valid character
   is parsed' (suggested by Paolo Abeni)

Changes v2 -> v3:
 - new patch: 'net: pktgen: fix ctrl interface command parsing'
 - new patch: 'net: pktgen: fix mpls reset parsing'
 - tools/testing/selftests/net/proc_net_pktgen.c:
   - fix typo in change description ('v1 -> v1' and tyop)
   - rename some vars to better match usage
     add_loopback_0 -> thr_cmd_add_loopback_0
     rm_loopback_0 -> thr_cmd_rm_loopback_0
     wrong_ctrl_cmd -> wrong_thr_cmd
     legacy_ctrl_cmd -> legacy_thr_cmd
     ctrl_fd -> thr_fd
   - add ctrl interface tests

Changes v1 -> v2:
 - new patch: 'net: pktgen: fix hex32_arg parsing for short reads'
 - new patch: 'net: pktgen: fix 'rate 0' error handling (return -EINVAL)'
 - new patch: 'net: pktgen: fix 'ratep 0' error handling (return -EINVAL)'
 - net/core/pktgen.c: additional fix get_imix_entries() and get_labels()
 - tools/testing/selftests/net/proc_net_pktgen.c:
   - fix tyop not vs. nod (suggested by Jakub Kicinski)
   - fix misaligned line (suggested by Jakub Kicinski)
   - enable fomerly commented out CONFIG_XFRM dependent test (command spi),
     as CONFIG_XFRM is enabled via tools/testing/selftests/net/config
     CONFIG_XFRM_INTERFACE/CONFIG_XFRM_USER (suggestex by Jakub Kicinski)
   - add CONFIG_NET_PKTGEN=m to tools/testing/selftests/net/config
     (suggested by Jakub Kicinski)
   - add modprobe pktgen to FIXTURE_SETUP() (suggested by Jakub Kicinski)
   - fix some checkpatch warnings (Missing a blank line after declarations)
   - shrink line length by re-naming some variables (command -> cmd,
     device -> dev)
   - add 'rate 0' testcase
   - add 'ratep 0' testcase

[1] https://lore.kernel.org/netdev/20241006221221.3744995-1-artem.chernyshev@red-soft.ru/
[2] https://lore.kernel.org/netdev/20250109083039.14004-1-pchelkin@ispras.ru/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76201b5979768500bca362871db66d77cb4c225e


Peter Seiderer (8):
  net: pktgen: replace ENOTSUPP with EOPNOTSUPP
  net: pktgen: enable 'param=value' parsing
  net: pktgen: fix hex32_arg parsing for short reads
  net: pktgen: fix 'rate 0' error handling (return -EINVAL)
  net: pktgen: fix 'ratep 0' error handling (return -EINVAL)
  net: pktgen: fix ctrl interface command parsing
  net: pktgen: fix access outside of user given buffer in
    pktgen_thread_write()
  net: pktgen: use defines for the various dec/hex number parsing digits
    lengths

 net/core/pktgen.c | 119 +++++++++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 53 deletions(-)

-- 
2.48.1

[PATCH net-next v5 1/8] net: pktgen: replace ENOTSUPP with EOPNOTSUPP

[Peter Seiderer]

Replace ENOTSUPP with EOPNOTSUPP, fixes checkpatch hint

  WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

and e.g.

  $ echo "clone_skb 1" > /proc/net/pktgen/lo\@0
  -bash: echo: write error: Unknown error 524

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4
  - add rev-by Simon Horman

Changes v2 -> v3
  - no changes

Changes v1 -> v2
  - no changes
---
 net/core/pktgen.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 82b6a2c3c141..496aa16773e7 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1198,7 +1198,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		if ((value > 0) &&
 		    ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) ||
 		     !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
-			return -ENOTSUPP;
+			return -EOPNOTSUPP;
 		if (value > 0 && (pkt_dev->n_imix_entries > 0 ||
 				  !(pkt_dev->flags & F_SHARED)))
 			return -EINVAL;
@@ -1258,7 +1258,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		    ((pkt_dev->xmit_mode == M_QUEUE_XMIT) ||
 		     ((pkt_dev->xmit_mode == M_START_XMIT) &&
 		     (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))))
-			return -ENOTSUPP;
+			return -EOPNOTSUPP;
 
 		if (value > 1 && !(pkt_dev->flags & F_SHARED))
 			return -EINVAL;
@@ -1303,7 +1303,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		} else if (strcmp(f, "netif_receive") == 0) {
 			/* clone_skb set earlier, not supported in this mode */
 			if (pkt_dev->clone_skb > 0)
-				return -ENOTSUPP;
+				return -EOPNOTSUPP;
 
 			pkt_dev->xmit_mode = M_NETIF_RECEIVE;
 
-- 
2.48.1

[PATCH net-next v5 2/8] net: pktgen: enable 'param=value' parsing

[Peter Seiderer]

Enable more flexible parameters syntax, allowing 'param=value' in
addition to the already supported 'param value' pattern (additional
this gives the skipping '=' in count_trail_chars() a purpose).

Tested with:

	$ echo "min_pkt_size 999" > /proc/net/pktgen/lo\@0
	$ echo "min_pkt_size=999" > /proc/net/pktgen/lo\@0
	$ echo "min_pkt_size =999" > /proc/net/pktgen/lo\@0
	$ echo "min_pkt_size= 999" > /proc/net/pktgen/lo\@0
	$ echo "min_pkt_size = 999" > /proc/net/pktgen/lo\@0

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4:
  - rephrase commit message (suggested by Paolo Abeni)

Changes v2 -> v3:
  - no changes

Changes v1 -> v2:
  - no changes
---
 net/core/pktgen.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 496aa16773e7..4f8ec6c9bed4 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -823,6 +823,7 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen)
 		case '\r':
 		case '\t':
 		case ' ':
+		case '=':
 			goto done_str;
 		default:
 			break;
-- 
2.48.1

[PATCH net-next v5 3/8] net: pktgen: fix hex32_arg parsing for short reads

[Peter Seiderer]

Fix hex32_arg parsing for short reads (here 7 hex digits instead of the
expected 8), shift result only on successful input parsing.

- before the patch

	$ echo "mpls 0000123" > /proc/net/pktgen/lo\@0
	$ grep mpls /proc/net/pktgen/lo\@0
	     mpls: 00001230
	Result: OK: mpls=00001230

- with patch applied

	$ echo "mpls 0000123" > /proc/net/pktgen/lo\@0
	$ grep mpls /proc/net/pktgen/lo\@0
	     mpls: 00000123
	Result: OK: mpls=00000123

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4
  - add rev-by Simon Horman

Changes v2 -> v3:
  - no changes

Changes v1 -> v2:
  - new patch
---
 net/core/pktgen.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 4f8ec6c9bed4..28dbbf70e142 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -753,14 +753,15 @@ static int hex32_arg(const char __user *user_buffer, unsigned long maxlen,
 	for (; i < maxlen; i++) {
 		int value;
 		char c;
-		*num <<= 4;
 		if (get_user(c, &user_buffer[i]))
 			return -EFAULT;
 		value = hex_to_bin(c);
-		if (value >= 0)
+		if (value >= 0) {
+			*num <<= 4;
 			*num |= value;
-		else
+		} else {
 			break;
+		}
 	}
 	return i;
 }
-- 
2.48.1

[PATCH net-next v5 4/8] net: pktgen: fix 'rate 0' error handling (return -EINVAL)

[Peter Seiderer]

Given an invalid 'rate' command e.g. 'rate 0' the return value is '1',
leading to the following misleading output:

- the good case

	$ echo "rate 100" > /proc/net/pktgen/lo\@0
	$ grep "Result:" /proc/net/pktgen/lo\@0
	Result: OK: rate=100

- the bad case (before the patch)

	$ echo "rate 0" > /proc/net/pktgen/lo\@0"
	-bash: echo: write error: Invalid argument
	$ grep "Result:" /proc/net/pktgen/lo\@0
	Result: No such parameter "ate"

- with patch applied

	$ echo "rate 0" > /proc/net/pktgen/lo\@0
	-bash: echo: write error: Invalid argument
	$ grep "Result:" /proc/net/pktgen/lo\@0
	Result: Idle

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4
      - add rev-by Simon Horman

Changes v2 -> v3:
  - no changes

Changes v1 -> v2:
  - new patch
---
 net/core/pktgen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 28dbbf70e142..75c7511bf492 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1115,7 +1115,7 @@ static ssize_t pktgen_if_write(struct file *file,
 
 		i += len;
 		if (!value)
-			return len;
+			return -EINVAL;
 		pkt_dev->delay = pkt_dev->min_pkt_size*8*NSEC_PER_USEC/value;
 		if (debug)
 			pr_info("Delay set at: %llu ns\n", pkt_dev->delay);
-- 
2.48.1

[PATCH net-next v5 5/8] net: pktgen: fix 'ratep 0' error handling (return -EINVAL)

[Peter Seiderer]

Given an invalid 'ratep' command e.g. 'ratep 0' the return value is '1',
leading to the following misleading output:

- the good case

	$ echo "ratep 100" > /proc/net/pktgen/lo\@0
	$ grep "Result:" /proc/net/pktgen/lo\@0
	Result: OK: ratep=100

- the bad case (before the patch)

	$ echo "ratep 0" > /proc/net/pktgen/lo\@0"
	-bash: echo: write error: Invalid argument
	$ grep "Result:" /proc/net/pktgen/lo\@0
	Result: No such parameter "atep"

- with patch applied

	$ echo "ratep 0" > /proc/net/pktgen/lo\@0
	-bash: echo: write error: Invalid argument
	$ grep "Result:" /proc/net/pktgen/lo\@0
	Result: Idle

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4
  - add rev-by Simon Horman

Changes v2 -> v3:
  - no changes

Changes v1 -> v2:
  - new patch
---
 net/core/pktgen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 75c7511bf492..c8a5b4d17407 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1130,7 +1130,7 @@ static ssize_t pktgen_if_write(struct file *file,
 
 		i += len;
 		if (!value)
-			return len;
+			return -EINVAL;
 		pkt_dev->delay = NSEC_PER_SEC/value;
 		if (debug)
 			pr_info("Delay set at: %llu ns\n", pkt_dev->delay);
-- 
2.48.1

[PATCH net-next v5 6/8] net: pktgen: fix ctrl interface command parsing

[Peter Seiderer]

Enable command writing without trailing '\n':

- the good case

	$ echo "reset" > /proc/net/pktgen/pgctrl

- the bad case (before the patch)

	$ echo -n "reset" > /proc/net/pktgen/pgctrl
	-bash: echo: write error: Invalid argument

- with patch applied

	$ echo -n "reset" > /proc/net/pktgen/pgctrl

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4
  - add rev-by Simon Horman

Changes v2 -> v3:
      - new patch
---
 net/core/pktgen.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index c8a5b4d17407..f6e35ba035c7 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -517,21 +517,23 @@ static ssize_t pgctrl_write(struct file *file, const char __user *buf,
 			    size_t count, loff_t *ppos)
 {
 	char data[128];
+	size_t max;
 	struct pktgen_net *pn = net_generic(current->nsproxy->net_ns, pg_net_id);
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	if (count == 0)
+	if (count < 1)
 		return -EINVAL;
 
-	if (count > sizeof(data))
-		count = sizeof(data);
-
-	if (copy_from_user(data, buf, count))
+	max = min(count, sizeof(data) - 1);
+	if (copy_from_user(data, buf, max))
 		return -EFAULT;
 
-	data[count - 1] = 0;	/* Strip trailing '\n' and terminate string */
+	if (data[max - 1] == '\n')
+		data[max - 1] = 0; /* strip trailing '\n', terminate string */
+	else
+		data[max] = 0; /* terminate string */
 
 	if (!strcmp(data, "stop"))
 		pktgen_stop_all_threads(pn);
-- 
2.48.1

[PATCH net-next v5 7/8] net: pktgen: fix access outside of user given buffer in pktgen_thread_write()

[Peter Seiderer]

Honour the user given buffer size for the strn_len() calls (otherwise
strn_len() will access memory outside of the user given buffer).

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)

Changes v3 -> v4
  - add rev-by Simon Horman

Changes v2 -> v3:
  - no changes

Changes v1 -> v2:
  - no changes
---
 net/core/pktgen.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f6e35ba035c7..55064713223e 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1900,8 +1900,8 @@ static ssize_t pktgen_thread_write(struct file *file,
 	i = len;
 
 	/* Read variable name */
-
-	len = strn_len(&user_buffer[i], sizeof(name) - 1);
+	max = min(sizeof(name) - 1, count - i);
+	len = strn_len(&user_buffer[i], max);
 	if (len < 0)
 		return len;
 
@@ -1931,7 +1931,8 @@ static ssize_t pktgen_thread_write(struct file *file,
 	if (!strcmp(name, "add_device")) {
 		char f[32];
 		memset(f, 0, 32);
-		len = strn_len(&user_buffer[i], sizeof(f) - 1);
+		max = min(sizeof(f) - 1, count - i);
+		len = strn_len(&user_buffer[i], max);
 		if (len < 0) {
 			ret = len;
 			goto out;
-- 
2.48.1

[PATCH net-next v5 8/8] net: pktgen: use defines for the various dec/hex number parsing digits lengths

[Peter Seiderer]

Use defines for the various dec/hex number parsing digits lengths
(hex32_arg/num_arg calls).

Signed-off-by: Peter Seiderer <ps.report@gmx.net>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changes v4 -> v5
  - split up patchset into part i/ii (suggested by Simon Horman)
  - add rev-by Simon Horman

Changes v3 -> v4
  - new patch (suggested by Simon Horman)
---
 net/core/pktgen.c | 80 ++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 55064713223e..4f201a2db2dc 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -179,6 +179,15 @@
 #define MAX_IMIX_ENTRIES 20
 #define IMIX_PRECISION 100 /* Precision of IMIX distribution */
 
+#define DEC_1_DIGITS 1
+#define DEC_4_DIGITS 4
+#define DEC_5_DIGITS 5
+#define DEC_9_DIGITS 9
+#define DEC_10_DIGITS 10
+
+#define HEX_2_DIGITS 2
+#define HEX_8_DIGITS 8
+
 #define func_enter() pr_debug("entering %s\n", __func__);
 
 #define PKT_FLAGS							\
@@ -844,7 +853,6 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen)
 static ssize_t get_imix_entries(const char __user *buffer,
 				struct pktgen_dev *pkt_dev)
 {
-	const int max_digits = 10;
 	int i = 0;
 	long len;
 	char c;
@@ -858,7 +866,7 @@ static ssize_t get_imix_entries(const char __user *buffer,
 		if (pkt_dev->n_imix_entries >= MAX_IMIX_ENTRIES)
 			return -E2BIG;
 
-		len = num_arg(&buffer[i], max_digits, &size);
+		len = num_arg(&buffer[i], DEC_10_DIGITS, &size);
 		if (len < 0)
 			return len;
 		i += len;
@@ -872,7 +880,7 @@ static ssize_t get_imix_entries(const char __user *buffer,
 		if (size < 14 + 20 + 8)
 			size = 14 + 20 + 8;
 
-		len = num_arg(&buffer[i], max_digits, &weight);
+		len = num_arg(&buffer[i], DEC_10_DIGITS, &weight);
 		if (len < 0)
 			return len;
 		if (weight <= 0)
@@ -902,7 +910,7 @@ static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev)
 	pkt_dev->nr_labels = 0;
 	do {
 		__u32 tmp;
-		len = hex32_arg(&buffer[i], 8, &tmp);
+		len = hex32_arg(&buffer[i], HEX_8_DIGITS, &tmp);
 		if (len <= 0)
 			return len;
 		pkt_dev->labels[n] = htonl(tmp);
@@ -1008,7 +1016,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "min_pkt_size")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1025,7 +1033,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "max_pkt_size")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1044,7 +1052,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	/* Shortcut for min = max */
 
 	if (!strcmp(name, "pkt_size")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1075,7 +1083,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "debug")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1086,7 +1094,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "frags")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1096,7 +1104,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "delay")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1111,7 +1119,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "rate")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1126,7 +1134,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "ratep")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1141,7 +1149,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "udp_src_min")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1154,7 +1162,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "udp_dst_min")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1167,7 +1175,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "udp_src_max")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1180,7 +1188,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "udp_dst_max")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1193,7 +1201,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "clone_skb")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 		/* clone_skb is not supported for netif_receive xmit_mode and
@@ -1214,7 +1222,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "count")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1225,7 +1233,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "src_mac_count")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1239,7 +1247,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "dst_mac_count")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1253,7 +1261,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "burst")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1272,7 +1280,7 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "node")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1592,7 +1600,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "flows")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1606,7 +1614,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 #ifdef CONFIG_XFRM
 	if (!strcmp(name, "spi")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1617,7 +1625,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 #endif
 	if (!strcmp(name, "flowlen")) {
-		len = num_arg(&user_buffer[i], 10, &value);
+		len = num_arg(&user_buffer[i], DEC_10_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1628,7 +1636,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "queue_map_min")) {
-		len = num_arg(&user_buffer[i], 5, &value);
+		len = num_arg(&user_buffer[i], DEC_5_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1639,7 +1647,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "queue_map_max")) {
-		len = num_arg(&user_buffer[i], 5, &value);
+		len = num_arg(&user_buffer[i], DEC_5_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1673,7 +1681,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "vlan_id")) {
-		len = num_arg(&user_buffer[i], 4, &value);
+		len = num_arg(&user_buffer[i], DEC_4_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1700,7 +1708,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "vlan_p")) {
-		len = num_arg(&user_buffer[i], 1, &value);
+		len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1715,7 +1723,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "vlan_cfi")) {
-		len = num_arg(&user_buffer[i], 1, &value);
+		len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1730,7 +1738,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "svlan_id")) {
-		len = num_arg(&user_buffer[i], 4, &value);
+		len = num_arg(&user_buffer[i], DEC_4_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1757,7 +1765,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "svlan_p")) {
-		len = num_arg(&user_buffer[i], 1, &value);
+		len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1772,7 +1780,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "svlan_cfi")) {
-		len = num_arg(&user_buffer[i], 1, &value);
+		len = num_arg(&user_buffer[i], DEC_1_DIGITS, &value);
 		if (len < 0)
 			return len;
 
@@ -1788,7 +1796,7 @@ static ssize_t pktgen_if_write(struct file *file,
 
 	if (!strcmp(name, "tos")) {
 		__u32 tmp_value = 0;
-		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
+		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);
 		if (len < 0)
 			return len;
 
@@ -1804,7 +1812,7 @@ static ssize_t pktgen_if_write(struct file *file,
 
 	if (!strcmp(name, "traffic_class")) {
 		__u32 tmp_value = 0;
-		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
+		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);
 		if (len < 0)
 			return len;
 
@@ -1819,7 +1827,7 @@ static ssize_t pktgen_if_write(struct file *file,
 	}
 
 	if (!strcmp(name, "skb_priority")) {
-		len = num_arg(&user_buffer[i], 9, &value);
+		len = num_arg(&user_buffer[i], DEC_9_DIGITS, &value);
 		if (len < 0)
 			return len;
 
-- 
2.48.1

Re: [PATCH net-next v5 8/8] net: pktgen: use defines for the various dec/hex number parsing digits lengths

[Jakub Kicinski]

On Thu, 13 Feb 2025 12:00:25 +0100 Peter Seiderer wrote:
> Use defines for the various dec/hex number parsing digits lengths
> (hex32_arg/num_arg calls).

I don't understand the value of this patch, TBH.

Example:

+#define HEX_2_DIGITS 2

-		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
+		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);

The word hex is already there.
There is still a two.
I don't think the new define has any explanatory power?

Previous 7 patches look ready indeed.
-- 
pw-bot: cr

Re: [PATCH net-next v5 8/8] net: pktgen: use defines for the various dec/hex number parsing digits lengths

[Simon Horman]

On Fri, Feb 14, 2025 at 08:11:45PM -0800, Jakub Kicinski wrote:
> On Thu, 13 Feb 2025 12:00:25 +0100 Peter Seiderer wrote:
> > Use defines for the various dec/hex number parsing digits lengths
> > (hex32_arg/num_arg calls).
> 
> I don't understand the value of this patch, TBH.
> 
> Example:
> 
> +#define HEX_2_DIGITS 2
> 
> -		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
> +		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);
> 
> The word hex is already there.
> There is still a two.
> I don't think the new define has any explanatory power?
> 
> Previous 7 patches look ready indeed.

Hi Jakub,

This one is on me. I felt the magic number 2 and so on
was unclear. But if you prefer the code as-is that is fine by me too.

Re: [PATCH net-next v5 2/8] net: pktgen: enable 'param=value' parsing

[Simon Horman]

On Thu, Feb 13, 2025 at 12:00:19PM +0100, Peter Seiderer wrote:
> Enable more flexible parameters syntax, allowing 'param=value' in
> addition to the already supported 'param value' pattern (additional
> this gives the skipping '=' in count_trail_chars() a purpose).
> 
> Tested with:
> 
> 	$ echo "min_pkt_size 999" > /proc/net/pktgen/lo\@0
> 	$ echo "min_pkt_size=999" > /proc/net/pktgen/lo\@0
> 	$ echo "min_pkt_size =999" > /proc/net/pktgen/lo\@0
> 	$ echo "min_pkt_size= 999" > /proc/net/pktgen/lo\@0
> 	$ echo "min_pkt_size = 999" > /proc/net/pktgen/lo\@0
> 
> Signed-off-by: Peter Seiderer <ps.report@gmx.net>

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

Re: [PATCH net-next v5 8/8] net: pktgen: use defines for the various dec/hex number parsing digits lengths

[Jakub Kicinski]

On Sun, 16 Feb 2025 09:17:39 +0000 Simon Horman wrote:
> On Fri, Feb 14, 2025 at 08:11:45PM -0800, Jakub Kicinski wrote:
> > On Thu, 13 Feb 2025 12:00:25 +0100 Peter Seiderer wrote:  
> > > Use defines for the various dec/hex number parsing digits lengths
> > > (hex32_arg/num_arg calls).  
> > 
> > I don't understand the value of this patch, TBH.
> > 
> > Example:
> > 
> > +#define HEX_2_DIGITS 2
> > 
> > -		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
> > +		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);
> > 
> > The word hex is already there.
> > There is still a two.
> > I don't think the new define has any explanatory power?
> > 
> > Previous 7 patches look ready indeed.  
> 
> This one is on me. I felt the magic number 2 and so on
> was unclear. But if you prefer the code as-is that is fine by me too.

I agree that it's a bit hard to guess what the call does and what 
the arguments are. To me at least, the constants as named don't help. 
We can get a third opinion, or if none is provided skip the patch for
now?

Re: [PATCH net-next v5 8/8] net: pktgen: use defines for the various dec/hex number parsing digits lengths

[Edward Cree]

On 15/02/2025 04:11, Jakub Kicinski wrote:
> On Thu, 13 Feb 2025 12:00:25 +0100 Peter Seiderer wrote:
>> Use defines for the various dec/hex number parsing digits lengths
>> (hex32_arg/num_arg calls).
> 
> I don't understand the value of this patch, TBH.
> 
> Example:
> 
> +#define HEX_2_DIGITS 2
> 
> -		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
> +		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);
> 
> The word hex is already there.
> There is still a two.
> I don't think the new define has any explanatory power?
Seems like the intent of the code is that the argument is one byte,
 which just happens to take two digits to represent in hex.
Perhaps that would help to come up with more meaningful names for
 these constants?  (Can't think of good ones off the top of my head)

Re: [PATCH net-next v5 8/8] net: pktgen: use defines for the various dec/hex number parsing digits lengths

[Simon Horman]

On Mon, Feb 17, 2025 at 09:47:40AM -0800, Jakub Kicinski wrote:
> On Sun, 16 Feb 2025 09:17:39 +0000 Simon Horman wrote:
> > On Fri, Feb 14, 2025 at 08:11:45PM -0800, Jakub Kicinski wrote:
> > > On Thu, 13 Feb 2025 12:00:25 +0100 Peter Seiderer wrote:  
> > > > Use defines for the various dec/hex number parsing digits lengths
> > > > (hex32_arg/num_arg calls).  
> > > 
> > > I don't understand the value of this patch, TBH.
> > > 
> > > Example:
> > > 
> > > +#define HEX_2_DIGITS 2
> > > 
> > > -		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
> > > +		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);
> > > 
> > > The word hex is already there.
> > > There is still a two.
> > > I don't think the new define has any explanatory power?
> > > 
> > > Previous 7 patches look ready indeed.  
> > 
> > This one is on me. I felt the magic number 2 and so on
> > was unclear. But if you prefer the code as-is that is fine by me too.
> 
> I agree that it's a bit hard to guess what the call does and what 
> the arguments are. To me at least, the constants as named don't help. 
> We can get a third opinion, or if none is provided skip the patch for
> now?

Yes, I see your point.
No objections from me to skipping this patch.

Re: [PATCH net-next v5 8/8] net: pktgen: use defines for the various dec/hex number parsing digits lengths

[Peter Seiderer]

Hello *,

On Tue, 18 Feb 2025 13:29:05 +0000, Simon Horman <horms@kernel.org> wrote:

> On Mon, Feb 17, 2025 at 09:47:40AM -0800, Jakub Kicinski wrote:
> > On Sun, 16 Feb 2025 09:17:39 +0000 Simon Horman wrote:
> > > On Fri, Feb 14, 2025 at 08:11:45PM -0800, Jakub Kicinski wrote:
> > > > On Thu, 13 Feb 2025 12:00:25 +0100 Peter Seiderer wrote:
> > > > > Use defines for the various dec/hex number parsing digits lengths
> > > > > (hex32_arg/num_arg calls).
> > > >
> > > > I don't understand the value of this patch, TBH.
> > > >
> > > > Example:
> > > >
> > > > +#define HEX_2_DIGITS 2
> > > >
> > > > -		len = hex32_arg(&user_buffer[i], 2, &tmp_value);
> > > > +		len = hex32_arg(&user_buffer[i], HEX_2_DIGITS, &tmp_value);
> > > >
> > > > The word hex is already there.
> > > > There is still a two.
> > > > I don't think the new define has any explanatory power?
> > > >
> > > > Previous 7 patches look ready indeed.
> > >
> > > This one is on me. I felt the magic number 2 and so on
> > > was unclear. But if you prefer the code as-is that is fine by me too.
> >
> > I agree that it's a bit hard to guess what the call does and what
> > the arguments are. To me at least, the constants as named don't help.
> > We can get a third opinion, or if none is provided skip the patch for
> > now?
>
> Yes, I see your point.
> No objections from me to skipping this patch.

O.k., will re-send the patch set without this one and the
rev-by for patch 2 added...

Regards,
Peter