[PATCH net-next] icmp: standardize naming of RFC 8335 PROBE constants

[PATCH net-next] icmp: standardize naming of RFC 8335 PROBE constants

[Andreas Roeseler]

The current definitions of constants for PROBE are inconsistent, with
some beginning with ICMP and others with simply EXT. This patch
attempts to standardize the naming conventions of the constants for
PROBE, and update the relevant definitions in net/ipv4/icmp.c.

Similarly, the definitions for the code field (previously
ICMP_EXT_MAL_QUERY, etc) use the same prefixes as the type field. This
patch adds _CODE_ to the prefix to clarify the distinction of these
constants.

Signed-off-by: Andreas Roeseler <andreas.a.roeseler@gmail.com>
---
 include/uapi/linux/icmp.h | 28 ++++++++++++++--------------
 net/ipv4/icmp.c           | 16 ++++++++--------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
index 222325d1d80e..c1da8244c5e1 100644
--- a/include/uapi/linux/icmp.h
+++ b/include/uapi/linux/icmp.h
@@ -70,22 +70,22 @@
 #define ICMP_EXC_FRAGTIME	1	/* Fragment Reass time exceeded	*/
 
 /* Codes for EXT_ECHO (PROBE) */
-#define ICMP_EXT_ECHO		42
-#define ICMP_EXT_ECHOREPLY	43
-#define ICMP_EXT_MAL_QUERY	1	/* Malformed Query */
-#define ICMP_EXT_NO_IF		2	/* No such Interface */
-#define ICMP_EXT_NO_TABLE_ENT	3	/* No such Table Entry */
-#define ICMP_EXT_MULT_IFS	4	/* Multiple Interfaces Satisfy Query */
+#define ICMP_EXT_ECHO			42
+#define ICMP_EXT_ECHOREPLY		43
+#define ICMP_EXT_CODE_MAL_QUERY		1	/* Malformed Query */
+#define ICMP_EXT_CODE_NO_IF		2	/* No such Interface */
+#define ICMP_EXT_CODE_NO_TABLE_ENT	3	/* No such Table Entry */
+#define ICMP_EXT_CODE_MULT_IFS		4	/* Multiple Interfaces Satisfy Query */
 
 /* Constants for EXT_ECHO (PROBE) */
-#define EXT_ECHOREPLY_ACTIVE	(1 << 2)/* active bit in reply message */
-#define EXT_ECHOREPLY_IPV4	(1 << 1)/* ipv4 bit in reply message */
-#define EXT_ECHOREPLY_IPV6	1	/* ipv6 bit in reply message */
-#define EXT_ECHO_CTYPE_NAME	1
-#define EXT_ECHO_CTYPE_INDEX	2
-#define EXT_ECHO_CTYPE_ADDR	3
-#define ICMP_AFI_IP		1	/* Address Family Identifier for ipv4 */
-#define ICMP_AFI_IP6		2	/* Address Family Identifier for ipv6 */
+#define ICMP_EXT_ECHOREPLY_ACTIVE	(1 << 2)/* active bit in reply message */
+#define ICMP_EXT_ECHOREPLY_IPV4		(1 << 1)/* ipv4 bit in reply message */
+#define ICMP_EXT_ECHOREPLY_IPV6		1	/* ipv6 bit in reply message */
+#define ICMP_EXT_ECHO_CTYPE_NAME	1
+#define ICMP_EXT_ECHO_CTYPE_INDEX	2
+#define ICMP_EXT_ECHO_CTYPE_ADDR	3
+#define ICMP_AFI_IP			1	/* Address Family Identifier for ipv4 */
+#define ICMP_AFI_IP6			2	/* Address Family Identifier for ipv6 */
 
 struct icmphdr {
   __u8		type;
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 8bd988fbcb31..7b6931a4d775 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1033,7 +1033,7 @@ static bool icmp_echo(struct sk_buff *skb)
 	status = 0;
 	dev = NULL;
 	switch (iio->extobj_hdr.class_type) {
-	case EXT_ECHO_CTYPE_NAME:
+	case ICMP_EXT_ECHO_CTYPE_NAME:
 		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
 		if (ident_len >= IFNAMSIZ)
 			goto send_mal_query;
@@ -1041,14 +1041,14 @@ static bool icmp_echo(struct sk_buff *skb)
 		memcpy(buff, &iio->ident.name, ident_len);
 		dev = dev_get_by_name(net, buff);
 		break;
-	case EXT_ECHO_CTYPE_INDEX:
+	case ICMP_EXT_ECHO_CTYPE_INDEX:
 		iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(iio->extobj_hdr) +
 					 sizeof(iio->ident.ifindex), &_iio);
 		if (ident_len != sizeof(iio->ident.ifindex))
 			goto send_mal_query;
 		dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
 		break;
-	case EXT_ECHO_CTYPE_ADDR:
+	case ICMP_EXT_ECHO_CTYPE_ADDR:
 		if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
 				 iio->ident.addr.ctype3_hdr.addrlen)
 			goto send_mal_query;
@@ -1080,23 +1080,23 @@ static bool icmp_echo(struct sk_buff *skb)
 		goto send_mal_query;
 	}
 	if (!dev) {
-		icmp_param.data.icmph.code = ICMP_EXT_NO_IF;
+		icmp_param.data.icmph.code = ICMP_EXT_CODE_NO_IF;
 		goto send_reply;
 	}
 	/* Fill bits in reply message */
 	if (dev->flags & IFF_UP)
-		status |= EXT_ECHOREPLY_ACTIVE;
+		status |= ICMP_EXT_ECHOREPLY_ACTIVE;
 	if (__in_dev_get_rcu(dev) && __in_dev_get_rcu(dev)->ifa_list)
-		status |= EXT_ECHOREPLY_IPV4;
+		status |= ICMP_EXT_ECHOREPLY_IPV4;
 	if (!list_empty(&rcu_dereference(dev->ip6_ptr)->addr_list))
-		status |= EXT_ECHOREPLY_IPV6;
+		status |= ICMP_EXT_ECHOREPLY_IPV6;
 	dev_put(dev);
 	icmp_param.data.icmph.un.echo.sequence |= htons(status);
 send_reply:
 	icmp_reply(&icmp_param, skb);
 		return true;
 send_mal_query:
-	icmp_param.data.icmph.code = ICMP_EXT_MAL_QUERY;
+	icmp_param.data.icmph.code = ICMP_EXT_CODE_MAL_QUERY;
 	goto send_reply;
 }
 
-- 
2.31.1

Re: [PATCH net-next] icmp: standardize naming of RFC 8335 PROBE constants

[Stephen Hemminger]

On Mon, 26 Apr 2021 22:40:02 -0500
Andreas Roeseler <andreas.a.roeseler@gmail.com> wrote:

> The current definitions of constants for PROBE are inconsistent, with
> some beginning with ICMP and others with simply EXT. This patch
> attempts to standardize the naming conventions of the constants for
> PROBE, and update the relevant definitions in net/ipv4/icmp.c.
> 
> Similarly, the definitions for the code field (previously
> ICMP_EXT_MAL_QUERY, etc) use the same prefixes as the type field. This
> patch adds _CODE_ to the prefix to clarify the distinction of these
> constants.
> 
> Signed-off-by: Andreas Roeseler <andreas.a.roeseler@gmail.com>
> ---
>  include/uapi/linux/icmp.h | 28 ++++++++++++++--------------
>  net/ipv4/icmp.c           | 16 ++++++++--------
>  2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
> index 222325d1d80e..c1da8244c5e1 100644
> --- a/include/uapi/linux/icmp.h
> +++ b/include/uapi/linux/icmp.h
> @@ -70,22 +70,22 @@
>  #define ICMP_EXC_FRAGTIME	1	/* Fragment Reass time exceeded	*/
>  
>  /* Codes for EXT_ECHO (PROBE) */
> -#define ICMP_EXT_ECHO		42
> -#define ICMP_EXT_ECHOREPLY	43
> -#define ICMP_EXT_MAL_QUERY	1	/* Malformed Query */
> -#define ICMP_EXT_NO_IF		2	/* No such Interface */
> -#define ICMP_EXT_NO_TABLE_ENT	3	/* No such Table Entry */
> -#define ICMP_EXT_MULT_IFS	4	/* Multiple Interfaces Satisfy Query */
> +#define ICMP_EXT_ECHO			42
> +#define ICMP_EXT_ECHOREPLY		43
> +#define ICMP_EXT_CODE_MAL_QUERY		1	/* Malformed Query */
> +#define ICMP_EXT_CODE_NO_IF		2	/* No such Interface */
> +#define ICMP_EXT_CODE_NO_TABLE_ENT	3	/* No such Table Entry */
> +#define ICMP_EXT_CODE_MULT_IFS		4	/* Multiple Interfaces Satisfy Query */
>  
>  /* Constants for EXT_ECHO (PROBE) */
> -#define EXT_ECHOREPLY_ACTIVE	(1 << 2)/* active bit in reply message */
> -#define EXT_ECHOREPLY_IPV4	(1 << 1)/* ipv4 bit in reply message */
> -#define EXT_ECHOREPLY_IPV6	1	/* ipv6 bit in reply message */
> -#define EXT_ECHO_CTYPE_NAME	1
> -#define EXT_ECHO_CTYPE_INDEX	2
> -#define EXT_ECHO_CTYPE_ADDR	3
> -#define ICMP_AFI_IP		1	/* Address Family Identifier for ipv4 */
> -#define ICMP_AFI_IP6		2	/* Address Family Identifier for ipv6 */
> +#define ICMP_EXT_ECHOREPLY_ACTIVE	(1 << 2)/* active bit in reply message */
> +#define ICMP_EXT_ECHOREPLY_IPV4		(1 << 1)/* ipv4 bit in reply message */
> +#define ICMP_EXT_ECHOREPLY_IPV6		1	/* ipv6 bit in reply message */
> +#define ICMP_EXT_ECHO_CTYPE_NAME	1
> +#define ICMP_EXT_ECHO_CTYPE_INDEX	2
> +#define ICMP_EXT_ECHO_CTYPE_ADDR	3
> +#define ICMP_AFI_IP			1	/* Address Family Identifier for ipv4 */
> +#define ICMP_AFI_IP6			2	/* Address Family Identifier for ipv6 */

You can't just remove the old constants. They have to stay there.
The #defines are part of the Linux API by now.

Re: [PATCH net-next] icmp: standardize naming of RFC 8335 PROBE constants

[Andreas Roeseler]

On Mon, 2021-04-26 at 20:54 -0700, Stephen Hemminger wrote:
> On Mon, 26 Apr 2021 22:40:02 -0500
> Andreas Roeseler <andreas.a.roeseler@gmail.com> wrote:
> 
> > The current definitions of constants for PROBE are inconsistent,
> > with
> > some beginning with ICMP and others with simply EXT. This patch
> > attempts to standardize the naming conventions of the constants for
> > PROBE, and update the relevant definitions in net/ipv4/icmp.c.
> > 
> > Similarly, the definitions for the code field (previously
> > ICMP_EXT_MAL_QUERY, etc) use the same prefixes as the type field.
> > This
> > patch adds _CODE_ to the prefix to clarify the distinction of these
> > constants.
> > 
> > Signed-off-by: Andreas Roeseler <andreas.a.roeseler@gmail.com>
> > ---
> >  include/uapi/linux/icmp.h | 28 ++++++++++++++--------------
> >  net/ipv4/icmp.c           | 16 ++++++++--------
> >  2 files changed, 22 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
> > index 222325d1d80e..c1da8244c5e1 100644
> > --- a/include/uapi/linux/icmp.h
> > +++ b/include/uapi/linux/icmp.h
> > @@ -70,22 +70,22 @@
> >  #define ICMP_EXC_FRAGTIME      1       /* Fragment Reass time
> > exceeded */
> >  
> >  /* Codes for EXT_ECHO (PROBE) */
> > -#define ICMP_EXT_ECHO          42
> > -#define ICMP_EXT_ECHOREPLY     43
> > -#define ICMP_EXT_MAL_QUERY     1       /* Malformed Query */
> > -#define ICMP_EXT_NO_IF         2       /* No such Interface */
> > -#define ICMP_EXT_NO_TABLE_ENT  3       /* No such Table Entry */
> > -#define ICMP_EXT_MULT_IFS      4       /* Multiple Interfaces
> > Satisfy Query */
> > +#define ICMP_EXT_ECHO                  42
> > +#define ICMP_EXT_ECHOREPLY             43
> > +#define ICMP_EXT_CODE_MAL_QUERY                1       /*
> > Malformed Query */
> > +#define ICMP_EXT_CODE_NO_IF            2       /* No such
> > Interface */
> > +#define ICMP_EXT_CODE_NO_TABLE_ENT     3       /* No such Table
> > Entry */
> > +#define ICMP_EXT_CODE_MULT_IFS         4       /* Multiple
> > Interfaces Satisfy Query */
> >  
> >  /* Constants for EXT_ECHO (PROBE) */
> > -#define EXT_ECHOREPLY_ACTIVE   (1 << 2)/* active bit in reply
> > message */
> > -#define EXT_ECHOREPLY_IPV4     (1 << 1)/* ipv4 bit in reply
> > message */
> > -#define EXT_ECHOREPLY_IPV6     1       /* ipv6 bit in reply
> > message */
> > -#define EXT_ECHO_CTYPE_NAME    1
> > -#define EXT_ECHO_CTYPE_INDEX   2
> > -#define EXT_ECHO_CTYPE_ADDR    3
> > -#define ICMP_AFI_IP            1       /* Address Family
> > Identifier for ipv4 */
> > -#define ICMP_AFI_IP6           2       /* Address Family
> > Identifier for ipv6 */
> > +#define ICMP_EXT_ECHOREPLY_ACTIVE      (1 << 2)/* active bit in
> > reply message */
> > +#define ICMP_EXT_ECHOREPLY_IPV4                (1 << 1)/* ipv4 bit
> > in reply message */
> > +#define ICMP_EXT_ECHOREPLY_IPV6                1       /* ipv6 bit
> > in reply message */
> > +#define ICMP_EXT_ECHO_CTYPE_NAME       1
> > +#define ICMP_EXT_ECHO_CTYPE_INDEX      2
> > +#define ICMP_EXT_ECHO_CTYPE_ADDR       3
> > +#define ICMP_AFI_IP                    1       /* Address Family
> > Identifier for ipv4 */
> > +#define ICMP_AFI_IP6                   2       /* Address Family
> > Identifier for ipv6 */
> 
> You can't just remove the old constants. They have to stay there.
> The #defines are part of the Linux API by now.

Even in the case where these constants were added less than a month ago
(3/30/2021) and are not used elsewhere in the kernel? I agree with your
statement in the general sense, but I thought I could get ahead of it
in this case and update them.

For reference, they were added in commit
2b246b2569cd2ac6ff700d0dce56b8bae29b1842

Re: [PATCH net-next] icmp: standardize naming of RFC 8335 PROBE constants

[Andrew Lunn]

> > You can't just remove the old constants. They have to stay there.
> > The #defines are part of the Linux API by now.
> 
> Even in the case where these constants were added less than a month ago
> (3/30/2021) and are not used elsewhere in the kernel? I agree with your
> statement in the general sense, but I thought I could get ahead of it
> in this case and update them.
> 
> For reference, they were added in commit
> 2b246b2569cd2ac6ff700d0dce56b8bae29b1842
 
You need to make this very clear in the commit message, that the code
you are changing is currently only in net-next, no released Kernels
are using these symbols, and you are not break the API.

    Andrew