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... [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/?i...
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(-)
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;
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;
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
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; }
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);
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);
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);
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;
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;
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.
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.
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?
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.
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
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)
linux-kselftest-mirror@lists.linaro.org
-
Edward Cree -
Jakub Kicinski -
Peter Seiderer -
Simon Horman