[RFC iproute2] tos: interpret ToS in natural numeral system

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
I get the feeling this was discussed in the past.

Also there's more:

devlink/devlink.c:	val = strtoull(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
lib/utils.c:	res = strtoul(arg, &ptr, base);
lib/utils.c:		n = strtoul(cp, &endp, 16);
lib/utils.c:		tmp = strtoul(tmpstr, &endptr, 16);
lib/utils.c:		tmp = strtoul(arg + i * 3, &endptr, 16);
misc/lnstat_util.c:			unsigned long f = strtoul(ptr, &ptr, 16);
tc/f_u32.c:	htid = strtoul(str, &tmp, 16);
tc/f_u32.c:		hash = strtoul(str, &tmp, 16);
tc/f_u32.c:			nodeid = strtoul(str, &tmp, 16);
tc/tc_util.c:	maj = strtoul(str, &p, 16);
tc/tc_util.c:	maj = strtoul(str, &p, 16);
tc/tc_util.c:		min = strtoul(str, &p, 16);
---
 lib/rt_names.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/rt_names.c b/lib/rt_names.c
index b976471d7979..7eb63dad7d4d 100644
--- a/lib/rt_names.c
+++ b/lib/rt_names.c

@@ -531,7 +531,7 @@ int rtnl_dsfield_a2n(__u32 *id, const char *arg)
 		}
 	}
 
-	res = strtoul(arg, &end, 16);
+	res = strtoul(arg, &end, 0);
 	if (!end || end == arg || *end || res > 255)
 		return -1;
 	*id = res;

-- 
2.34.1

On Wed, Feb 16, 2022 at 11:42:05AM -0800, Jakub Kicinski wrote:

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.

I agree that we shouldn't have forced base 16 in the first place.
But after so many years I find it a bit dangerous to change that.

What about just printing a warning when the value isn't prefixed with
'0x'? Something like (completely untested):

@@ -535,6 +535,12 @@ int rtnl_dsfield_a2n(__u32 *id, const char *arg)
 	if (!end || end == arg || *end || res > 255)
 		return -1;
 	*id = res;
+
+	if (strncmp("0x", arg, 2))
+		fprintf(stderr,
+			"Warning: dsfield and tos parameters are interpreted as hexadecimal values\n"
+			"Use 'dsfield 0x%02x' to avoid this message\n", res);
+
 	return 0;
 }

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
I get the feeling this was discussed in the past.

Also there's more:

devlink/devlink.c:	val = strtoull(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
lib/utils.c:	res = strtoul(arg, &ptr, base);
lib/utils.c:		n = strtoul(cp, &endp, 16);
lib/utils.c:		tmp = strtoul(tmpstr, &endptr, 16);
lib/utils.c:		tmp = strtoul(arg + i * 3, &endptr, 16);
misc/lnstat_util.c:			unsigned long f = strtoul(ptr, &ptr, 16);
tc/f_u32.c:	htid = strtoul(str, &tmp, 16);
tc/f_u32.c:		hash = strtoul(str, &tmp, 16);
tc/f_u32.c:			nodeid = strtoul(str, &tmp, 16);
tc/tc_util.c:	maj = strtoul(str, &p, 16);
tc/tc_util.c:	maj = strtoul(str, &p, 16);
tc/tc_util.c:		min = strtoul(str, &p, 16);

After a very quick look, many of these seem to make sense though. For
example lnstat_util.c parses output from /proc, hexstring_a2n() is used
by ipmacsec.c to parse crypto keys, etc.

But I agree that we should probably audit the strtol() (and variants)
that don't use base 0.

From: Guillaume Nault

Sent: 16 February 2022 22:24

On Wed, Feb 16, 2022 at 11:42:05AM -0800, Jakub Kicinski wrote:

quoted

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.

I agree that we shouldn't have forced base 16 in the first place.
But after so many years I find it a bit dangerous to change that.

Aren't the TOS values made up of several multi-bit fields and
very likely to be documented in hex?
I'm not sure $((TOS * 2)) (or even + 2) makes any sense at all.

What it more horrid that that base 0 treats numbers that start
with a 0 as octal - has anyone really used octal since the 1970s
(except for file permissions).

I have written command line parsers that treat 0tnnn as decimal
while defaulting to hex.
That does make it easier to use shell arithmetic for field (like
addresses) that you would never normally specify in decimal.

Ugg.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On 2/16/22 3:23 PM, Guillaume Nault wrote:

On Wed, Feb 16, 2022 at 11:42:05AM -0800, Jakub Kicinski wrote:

quoted

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.

I agree that we shouldn't have forced base 16 in the first place.
But after so many years I find it a bit dangerous to change that.

I agree. In this case the change in behavior will not be very obvious
and could lead to confusion. I think this is something we have to live with.

That seems reasonable to me to let users know of this behavior.

quoted

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
I get the feeling this was discussed in the past.

Also there's more:

devlink/devlink.c:	val = strtoull(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);
devlink/devlink.c:	val = strtoul(str, &endptr, 10);

I think the devlink ones all check out. I think those functions can be
updated to use get_uNN from lib/utils.c.

quoted

lib/utils.c:	res = strtoul(arg, &ptr, base);

This is get_unsigned and those users outside of tc look ok.

quoted

lib/utils.c:		n = strtoul(cp, &endp, 16);

ipv6 address conversion which is hex based.

quoted

lib/utils.c:		tmp = strtoul(tmpstr, &endptr, 16);
lib/utils.c:		tmp = strtoul(arg + i * 3, &endptr, 16);

both of these are valid hex conversions

quoted

misc/lnstat_util.c:			unsigned long f = strtoul(ptr, &ptr, 16);
tc/f_u32.c:	htid = strtoul(str, &tmp, 16);
tc/f_u32.c:		hash = strtoul(str, &tmp, 16);
tc/f_u32.c:			nodeid = strtoul(str, &tmp, 16);
tc/tc_util.c:	maj = strtoul(str, &p, 16);
tc/tc_util.c:	maj = strtoul(str, &p, 16);
tc/tc_util.c:		min = strtoul(str, &p, 16);

After a very quick look, many of these seem to make sense though. For
example lnstat_util.c parses output from /proc, hexstring_a2n() is used
by ipmacsec.c to parse crypto keys, etc.

But I agree that we should probably audit the strtol() (and variants)
that don't use base 0.

Added Jamal and Cong for the tc references. Hopefully those are
documented to be hex values.

On Wed, 16 Feb 2022 18:52:43 -0700 David Ahern wrote:

On 2/16/22 3:23 PM, Guillaume Nault wrote:

quoted

On Wed, Feb 16, 2022 at 11:42:05AM -0800, Jakub Kicinski wrote:  

quoted

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.  

I agree that we shouldn't have forced base 16 in the first place.
But after so many years I find it a bit dangerous to change that.  

I agree. In this case the change in behavior will not be very obvious
and could lead to confusion. I think this is something we have to live with.

quoted

What about just printing a warning when the value isn't prefixed with
'0x'? Something like (completely untested):

@@ -535,6 +535,12 @@ int rtnl_dsfield_a2n(__u32 *id, const char *arg)
 	if (!end || end == arg || *end || res > 255)
 		return -1;
 	*id = res;
+
+	if (strncmp("0x", arg, 2))
+		fprintf(stderr,
+			"Warning: dsfield and tos parameters are interpreted as hexadecimal values\n"
+			"Use 'dsfield 0x%02x' to avoid this message\n", res);
+
 	return 0;
 }  

That seems reasonable to me to let users know of this behavior.

SGTM!

On Wed, Feb 16, 2022 at 10:44:19PM +0000, David Laight wrote:

From: Guillaume Nault

quoted

Sent: 16 February 2022 22:24

On Wed, Feb 16, 2022 at 11:42:05AM -0800, Jakub Kicinski wrote:

quoted

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.

I agree that we shouldn't have forced base 16 in the first place.
But after so many years I find it a bit dangerous to change that.

Aren't the TOS values made up of several multi-bit fields and
very likely to be documented in hex?

In theory, they are. But as far as the kernel is concerned, they're
just plain integers with no significance (apart from some constraints
preventing the use of some values). Users are free to choose their own
values.

I'm not sure $((TOS * 2)) (or even + 2) makes any sense at all.

What it more horrid that that base 0 treats numbers that start
with a 0 as octal - has anyone really used octal since the 1970s
(except for file permissions).

Right, but that'd be consistent with the rest of iproute2, so users
should be aware of this trap at this point (or most likely, they never
prefix their values with 0). Anyway, I think we agreed that it's now
too late to modify the base.

I have written command line parsers that treat 0tnnn as decimal
while defaulting to hex.
That does make it easier to use shell arithmetic for field (like
addresses) that you would never normally specify in decimal.

quoted

What about just printing a warning when the value isn't prefixed with
'0x'? Something like (completely untested):

@@ -535,6 +535,12 @@ int rtnl_dsfield_a2n(__u32 *id, const char *arg)
 	if (!end || end == arg || *end || res > 255)
 		return -1;
 	*id = res;
+
+	if (strncmp("0x", arg, 2))
+		fprintf(stderr,
+			"Warning: dsfield and tos parameters are interpreted as hexadecimal values\n"
+			"Use 'dsfield 0x%02x' to avoid this message\n", res);

Ugg.

Not nice, I agree. But what else can we do without breaking backward
compatibility?
This is similar to the warning we have when creating a new vxlan device
without specifying the destination port:

  # ip link add type vxlan vni 200 remote 2001:db8::1
  vxlan: destination port not specified
  Will use Linux kernel default (non-standard value)
  Use 'dstport 4789' to get the IANA assigned value
  Use 'dstport 0' to get default and quiet this message

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Wed, 16 Feb 2022 11:42:05 -0800
Jakub Kicinski [off-list ref] wrote:

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
I get the feeling this was discussed in the past.

Also there's more:

misc/lnstat_util.c:			unsigned long f = strtoul(ptr, &ptr, 16);

This is reading /proc/net/stat/XXX files and those files
are generated in kernel without any prefix. So this has to stay.

On Wed, 16 Feb 2022 11:42:05 -0800
Jakub Kicinski [off-list ref] wrote:

Silently forcing a base numeral system is very painful for users.
ip currently interprets tos 10 as 0x10. Imagine user's bash script
does:

  .. tos $((TOS * 2)) ..

or any numerical operation on the ToS.

This patch breaks existing scripts if they expect 10 to be 0x10.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Maybe a warning?? but no one will see it.