diff mbox series

[ipsec-next,v2] udpencap: Remove Obsolete UDP_ENCAP_ESPINUDP_NON_IKE Support

Message ID c873dc4dcaa0ab84b562f29751996db6bd37d440.1712220541.git.antony.antony@secunet.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [ipsec-next,v2] udpencap: Remove Obsolete UDP_ENCAP_ESPINUDP_NON_IKE Support | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2846 this patch: 2846
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: edumazet@google.com
netdev/build_clang success Errors and warnings before: 990 this patch: 990
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3038 this patch: 3038
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 137 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-04-04--12-00 (tests: 941)

Commit Message

Antony Antony April 4, 2024, 8:51 a.m. UTC 
The UDP_ENCAP_ESPINUDP_NON_IKE mode, introduced into the Linux kernel
in 2004 [2], has remained inactive and obsolete for an extended period.

This mode was originally defined in an early version of an IETF draft
[1] from 2001. By the time it was integrated into the kernel in 2004 [2],
it had already been replaced by UDP_ENCAP_ESPINUDP [3] in later
versions of draft-ietf-ipsec-udp-encaps, particularly in version 06.

Over time, UDP_ENCAP_ESPINUDP_NON_IKE has lost its relevance, with no
known use cases.

With this commit, we remove support for UDP_ENCAP_ESPINUDP_NON_IKE,
simplifying the codebase and eliminating unnecessary complexity.
Actually, we remove the functionality and wrap  UDP_ENCAP_ESPINUDP_NON_IKE
defination in "#ifndef __KERNEL__". If it is used again in kernel code
your build will fail.

References:
[1] https://datatracker.ietf.org/doc/html/draft-ietf-ipsec-udp-encaps-00.txt

[2] Commit that added UDP_ENCAP_ESPINUDP_NON_IKE to the Linux historic
    repository.

    Author: Andreas Gruenbacher <agruen@suse.de>
    Date: Fri Apr 9 01:47:47 2004 -0700

   [IPSEC]: Support draft-ietf-ipsec-udp-encaps-00/01, some ipec impls need it.

[3] Commit that added UDP_ENCAP_ESPINUDP to the Linux historic
    repository.

    Author: Derek Atkins <derek@ihtfp.com>
    Date: Wed Apr 2 13:21:02 2003 -0800

    [IPSEC]: Implement UDP Encapsulation framework.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v1 -> v2
- removed defination wrapped in #ifndef __KERNEL__ It would falsly
  let userspace appliction build and break when running.
RFC -> v1
- keep removed defination wrapped in #ifndef __KERNEL__
---
 include/uapi/linux/udp.h |  1 -
 net/ipv4/esp4.c          | 12 ------------
 net/ipv4/udp.c           |  2 --
 net/ipv4/xfrm4_input.c   | 13 -------------
 net/ipv6/esp6.c          | 12 ------------
 net/ipv6/xfrm6_input.c   | 13 -------------
 6 files changed, 53 deletions(-)

--
2.30.2

Comments

Michael Richardson April 4, 2024, 1:48 p.m. UTC | #1 
Antony Antony via Devel <devel@linux-ipsec.org> wrote:
    > With this commit, we remove support for UDP_ENCAP_ESPINUDP_NON_IKE,
    > simplifying the codebase and eliminating unnecessary complexity.

Good.
Steffen Klassert April 11, 2024, 8:22 a.m. UTC | #2 
On Thu, Apr 04, 2024 at 10:51:31AM +0200, Antony Antony wrote:
> The UDP_ENCAP_ESPINUDP_NON_IKE mode, introduced into the Linux kernel
> in 2004 [2], has remained inactive and obsolete for an extended period.
> 
> This mode was originally defined in an early version of an IETF draft
> [1] from 2001. By the time it was integrated into the kernel in 2004 [2],
> it had already been replaced by UDP_ENCAP_ESPINUDP [3] in later
> versions of draft-ietf-ipsec-udp-encaps, particularly in version 06.
> 
> Over time, UDP_ENCAP_ESPINUDP_NON_IKE has lost its relevance, with no
> known use cases.
> 
> With this commit, we remove support for UDP_ENCAP_ESPINUDP_NON_IKE,
> simplifying the codebase and eliminating unnecessary complexity.
> Actually, we remove the functionality and wrap  UDP_ENCAP_ESPINUDP_NON_IKE
> defination in "#ifndef __KERNEL__". If it is used again in kernel code
> your build will fail.
> 
> References:
> [1] https://datatracker.ietf.org/doc/html/draft-ietf-ipsec-udp-encaps-00.txt
> 
> [2] Commit that added UDP_ENCAP_ESPINUDP_NON_IKE to the Linux historic
>     repository.
> 
>     Author: Andreas Gruenbacher <agruen@suse.de>
>     Date: Fri Apr 9 01:47:47 2004 -0700
> 
>    [IPSEC]: Support draft-ietf-ipsec-udp-encaps-00/01, some ipec impls need it.
> 
> [3] Commit that added UDP_ENCAP_ESPINUDP to the Linux historic
>     repository.
> 
>     Author: Derek Atkins <derek@ihtfp.com>
>     Date: Wed Apr 2 13:21:02 2003 -0800
> 
>     [IPSEC]: Implement UDP Encapsulation framework.
> 
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
> v1 -> v2
> - removed defination wrapped in #ifndef __KERNEL__ It would falsly
>   let userspace appliction build and break when running.
> RFC -> v1
> - keep removed defination wrapped in #ifndef __KERNEL__
> ---
>  include/uapi/linux/udp.h |  1 -
>  net/ipv4/esp4.c          | 12 ------------
>  net/ipv4/udp.c           |  2 --
>  net/ipv4/xfrm4_input.c   | 13 -------------
>  net/ipv6/esp6.c          | 12 ------------
>  net/ipv6/xfrm6_input.c   | 13 -------------
>  6 files changed, 53 deletions(-)
> 
> diff --git a/include/uapi/linux/udp.h b/include/uapi/linux/udp.h
> index 4828794efcf8..1516f53698e0 100644
> --- a/include/uapi/linux/udp.h
> +++ b/include/uapi/linux/udp.h
> @@ -36,7 +36,6 @@ struct udphdr {
>  #define UDP_GRO		104	/* This socket can receive UDP GRO packets */
> 
>  /* UDP encapsulation types */
> -#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* draft-ietf-ipsec-nat-t-ike-00/01 */

Please don't remove that, it is part of the ABI.
Typically this is left in and marked as: /* unused */
Antony Antony April 11, 2024, 5:45 p.m. UTC | #3 
On Thu, Apr 11, 2024 at 10:22:32 +0200, Steffen Klassert wrote:
> On Thu, Apr 04, 2024 at 10:51:31AM +0200, Antony Antony wrote:
> > The UDP_ENCAP_ESPINUDP_NON_IKE mode, introduced into the Linux kernel
> > in 2004 [2], has remained inactive and obsolete for an extended period.
> > 
> > This mode was originally defined in an early version of an IETF draft
> > [1] from 2001. By the time it was integrated into the kernel in 2004 [2],
> > it had already been replaced by UDP_ENCAP_ESPINUDP [3] in later
> > versions of draft-ietf-ipsec-udp-encaps, particularly in version 06.
> > 
> > Over time, UDP_ENCAP_ESPINUDP_NON_IKE has lost its relevance, with no
> > known use cases.
> > 
> > With this commit, we remove support for UDP_ENCAP_ESPINUDP_NON_IKE,
> > simplifying the codebase and eliminating unnecessary complexity.
> > Actually, we remove the functionality and wrap  UDP_ENCAP_ESPINUDP_NON_IKE
> > defination in "#ifndef __KERNEL__". If it is used again in kernel code
> > your build will fail.
> > 
> > References:
> > [1] https://datatracker.ietf.org/doc/html/draft-ietf-ipsec-udp-encaps-00.txt
> > 
> > [2] Commit that added UDP_ENCAP_ESPINUDP_NON_IKE to the Linux historic
> >     repository.
> > 
> >     Author: Andreas Gruenbacher <agruen@suse.de>
> >     Date: Fri Apr 9 01:47:47 2004 -0700
> > 
> >    [IPSEC]: Support draft-ietf-ipsec-udp-encaps-00/01, some ipec impls need it.
> > 
> > [3] Commit that added UDP_ENCAP_ESPINUDP to the Linux historic
> >     repository.
> > 
> >     Author: Derek Atkins <derek@ihtfp.com>
> >     Date: Wed Apr 2 13:21:02 2003 -0800
> > 
> >     [IPSEC]: Implement UDP Encapsulation framework.
> > 
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > ---
> > v1 -> v2
> > - removed defination wrapped in #ifndef __KERNEL__ It would falsly
> >   let userspace appliction build and break when running.
> > RFC -> v1
> > - keep removed defination wrapped in #ifndef __KERNEL__
> > ---
> >  include/uapi/linux/udp.h |  1 -
> >  net/ipv4/esp4.c          | 12 ------------
> >  net/ipv4/udp.c           |  2 --
> >  net/ipv4/xfrm4_input.c   | 13 -------------
> >  net/ipv6/esp6.c          | 12 ------------
> >  net/ipv6/xfrm6_input.c   | 13 -------------
> >  6 files changed, 53 deletions(-)
> > 
> > diff --git a/include/uapi/linux/udp.h b/include/uapi/linux/udp.h
> > index 4828794efcf8..1516f53698e0 100644
> > --- a/include/uapi/linux/udp.h
> > +++ b/include/uapi/linux/udp.h
> > @@ -36,7 +36,6 @@ struct udphdr {
> >  #define UDP_GRO		104	/* This socket can receive UDP GRO packets */
> > 
> >  /* UDP encapsulation types */
> > -#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* draft-ietf-ipsec-nat-t-ike-00/01 */
> 
> Please don't remove that, it is part of the ABI.
> Typically this is left in and marked as: /* unused */

Where exactly should I add /* unused */ ?

I agree, it is part of the ABI. We should be careful when removing. However, the solution to obselete is not clear to me. Would you please elaborte on the solution? I see 3 options. Which one do you prefer? Or is there 4th option?

1. change the comment and leave the definition

-#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* draft-ietf-ipsec-nat-t-ike-00/01 */
+#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* unused  draft-ietf-ipsec-nat-t-ike-00/01 */

Paul brought up a probable issue with this solution. This means user space would build without error while at runtime user space would fail.
Also kernel would build if someone add it.

2: wrap it in ifndef __KERNEL__ as Florian suggested 
-#define UDP_ENCAP_ESPINUDP_NON_IKE     1 /* draft-ietf-ipsec-nat-t-ike-00/01 */
+#ifndef __KERNEL__
+#define UDP_ENCAP_ESPINUDP_NON_IKE     1 /* (obsolete) draft-ietf-ipsec-nat-t-ike-00/01 */
+#endif
+

Kernel would not build, however, usersapce would build. This is v2. It was not accepeted.

3. comment it completly?
-#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* draft-ietf-ipsec-nat-t-ike-00/01 */
+ /* #define UDP_ENCAP_ESPINUDP_NON_IKE	1 unused draft-ietf-ipsec-nat-t-ike-00/01 */

This means kernel and userspace would fail to build when is used.
What is your preference? Then I can send a v3.

-antony
Steffen Klassert April 12, 2024, 6:24 a.m. UTC | #4 
On Thu, Apr 11, 2024 at 07:45:52PM +0200, Antony Antony wrote:
> On Thu, Apr 11, 2024 at 10:22:32 +0200, Steffen Klassert wrote:
> > On Thu, Apr 04, 2024 at 10:51:31AM +0200, Antony Antony wrote:
> > > 
> > > diff --git a/include/uapi/linux/udp.h b/include/uapi/linux/udp.h
> > > index 4828794efcf8..1516f53698e0 100644
> > > --- a/include/uapi/linux/udp.h
> > > +++ b/include/uapi/linux/udp.h
> > > @@ -36,7 +36,6 @@ struct udphdr {
> > >  #define UDP_GRO		104	/* This socket can receive UDP GRO packets */
> > > 
> > >  /* UDP encapsulation types */
> > > -#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* draft-ietf-ipsec-nat-t-ike-00/01 */
> > 
> > Please don't remove that, it is part of the ABI.
> > Typically this is left in and marked as: /* unused */
> 
> Where exactly should I add /* unused */ ?
> 
> I agree, it is part of the ABI. We should be careful when removing. However, the solution to obselete is not clear to me. Would you please elaborte on the solution? I see 3 options. Which one do you prefer? Or is there 4th option?
> 
> 1. change the comment and leave the definition
> 
> -#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* draft-ietf-ipsec-nat-t-ike-00/01 */
> +#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* unused  draft-ietf-ipsec-nat-t-ike-00/01 */

I prefer this option. The kernel will return -ENOPROTOOPT if userspace
tries to set that option.

> 
> Paul brought up a probable issue with this solution. This means user space would build without error while at runtime user space would fail.

If this still has users, it is not unused :-)

> Also kernel would build if someone add it.

Hopefully that will be catched by the review process.
diff mbox series

Patch

diff --git a/include/uapi/linux/udp.h b/include/uapi/linux/udp.h
index 4828794efcf8..1516f53698e0 100644
--- a/include/uapi/linux/udp.h
+++ b/include/uapi/linux/udp.h
@@ -36,7 +36,6 @@  struct udphdr {
 #define UDP_GRO		104	/* This socket can receive UDP GRO packets */

 /* UDP encapsulation types */
-#define UDP_ENCAP_ESPINUDP_NON_IKE	1 /* draft-ietf-ipsec-nat-t-ike-00/01 */
 #define UDP_ENCAP_ESPINUDP	2 /* draft-ietf-ipsec-udp-encaps-06 */
 #define UDP_ENCAP_L2TPINUDP	3 /* rfc2661 */
 #define UDP_ENCAP_GTP0		4 /* GSM TS 09.60 */
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index d33d12421814..695f775640ac 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -347,7 +347,6 @@  static struct ip_esp_hdr *esp_output_udp_encap(struct sk_buff *skb,
 					       __be16 dport)
 {
 	struct udphdr *uh;
-	__be32 *udpdata32;
 	unsigned int len;

 	len = skb->len + esp->tailen - skb_transport_offset(skb);
@@ -362,12 +361,6 @@  static struct ip_esp_hdr *esp_output_udp_encap(struct sk_buff *skb,

 	*skb_mac_header(skb) = IPPROTO_UDP;

-	if (encap_type == UDP_ENCAP_ESPINUDP_NON_IKE) {
-		udpdata32 = (__be32 *)(uh + 1);
-		udpdata32[0] = udpdata32[1] = 0;
-		return (struct ip_esp_hdr *)(udpdata32 + 2);
-	}
-
 	return (struct ip_esp_hdr *)(uh + 1);
 }

@@ -423,7 +416,6 @@  static int esp_output_encap(struct xfrm_state *x, struct sk_buff *skb,
 	switch (encap_type) {
 	default:
 	case UDP_ENCAP_ESPINUDP:
-	case UDP_ENCAP_ESPINUDP_NON_IKE:
 		esph = esp_output_udp_encap(skb, encap_type, esp, sport, dport);
 		break;
 	case TCP_ENCAP_ESPINTCP:
@@ -775,7 +767,6 @@  int esp_input_done2(struct sk_buff *skb, int err)
 			source = th->source;
 			break;
 		case UDP_ENCAP_ESPINUDP:
-		case UDP_ENCAP_ESPINUDP_NON_IKE:
 			source = uh->source;
 			break;
 		default:
@@ -1179,9 +1170,6 @@  static int esp_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)
 		case UDP_ENCAP_ESPINUDP:
 			x->props.header_len += sizeof(struct udphdr);
 			break;
-		case UDP_ENCAP_ESPINUDP_NON_IKE:
-			x->props.header_len += sizeof(struct udphdr) + 2 * sizeof(u32);
-			break;
 #ifdef CONFIG_INET_ESPINTCP
 		case TCP_ENCAP_ESPINTCP:
 			/* only the length field, TCP encap is done by
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 661d0e0d273f..16b6a9d59b02 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2688,8 +2688,6 @@  int udp_lib_setsockopt(struct sock *sk, int level, int optname,
 #ifdef CONFIG_XFRM
 		case UDP_ENCAP_ESPINUDP:
 			set_xfrm_gro_udp_encap_rcv(val, sk->sk_family, sk);
-			fallthrough;
-		case UDP_ENCAP_ESPINUDP_NON_IKE:
 #if IS_ENABLED(CONFIG_IPV6)
 			if (sk->sk_family == AF_INET6)
 				WRITE_ONCE(up->encap_rcv,
diff --git a/net/ipv4/xfrm4_input.c b/net/ipv4/xfrm4_input.c
index dae35101d189..0918b0682174 100644
--- a/net/ipv4/xfrm4_input.c
+++ b/net/ipv4/xfrm4_input.c
@@ -113,19 +113,6 @@  static int __xfrm4_udp_encap_rcv(struct sock *sk, struct sk_buff *skb, bool pull
 			/* Must be an IKE packet.. pass it through */
 			return 1;
 		break;
-	case UDP_ENCAP_ESPINUDP_NON_IKE:
-		/* Check if this is a keepalive packet.  If so, eat it. */
-		if (len == 1 && udpdata[0] == 0xff) {
-			return -EINVAL;
-		} else if (len > 2 * sizeof(u32) + sizeof(struct ip_esp_hdr) &&
-			   udpdata32[0] == 0 && udpdata32[1] == 0) {
-
-			/* ESP Packet with Non-IKE marker */
-			len = sizeof(struct udphdr) + 2 * sizeof(u32);
-		} else
-			/* Must be an IKE packet.. pass it through */
-			return 1;
-		break;
 	}

 	/* At this point we are sure that this is an ESPinUDP packet,
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 7371886d4f9f..cafb5746f4c7 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -383,7 +383,6 @@  static struct ip_esp_hdr *esp6_output_udp_encap(struct sk_buff *skb,
 					       __be16 dport)
 {
 	struct udphdr *uh;
-	__be32 *udpdata32;
 	unsigned int len;

 	len = skb->len + esp->tailen - skb_transport_offset(skb);
@@ -398,12 +397,6 @@  static struct ip_esp_hdr *esp6_output_udp_encap(struct sk_buff *skb,

 	*skb_mac_header(skb) = IPPROTO_UDP;

-	if (encap_type == UDP_ENCAP_ESPINUDP_NON_IKE) {
-		udpdata32 = (__be32 *)(uh + 1);
-		udpdata32[0] = udpdata32[1] = 0;
-		return (struct ip_esp_hdr *)(udpdata32 + 2);
-	}
-
 	return (struct ip_esp_hdr *)(uh + 1);
 }

@@ -459,7 +452,6 @@  static int esp6_output_encap(struct xfrm_state *x, struct sk_buff *skb,
 	switch (encap_type) {
 	default:
 	case UDP_ENCAP_ESPINUDP:
-	case UDP_ENCAP_ESPINUDP_NON_IKE:
 		esph = esp6_output_udp_encap(skb, encap_type, esp, sport, dport);
 		break;
 	case TCP_ENCAP_ESPINTCP:
@@ -822,7 +814,6 @@  int esp6_input_done2(struct sk_buff *skb, int err)
 			source = th->source;
 			break;
 		case UDP_ENCAP_ESPINUDP:
-		case UDP_ENCAP_ESPINUDP_NON_IKE:
 			source = uh->source;
 			break;
 		default:
@@ -1232,9 +1223,6 @@  static int esp6_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)
 		case UDP_ENCAP_ESPINUDP:
 			x->props.header_len += sizeof(struct udphdr);
 			break;
-		case UDP_ENCAP_ESPINUDP_NON_IKE:
-			x->props.header_len += sizeof(struct udphdr) + 2 * sizeof(u32);
-			break;
 #ifdef CONFIG_INET6_ESPINTCP
 		case TCP_ENCAP_ESPINTCP:
 			/* only the length field, TCP encap is done by
diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
index a17d783dc7c0..2c6aeb090b7a 100644
--- a/net/ipv6/xfrm6_input.c
+++ b/net/ipv6/xfrm6_input.c
@@ -109,19 +109,6 @@  static int __xfrm6_udp_encap_rcv(struct sock *sk, struct sk_buff *skb, bool pull
 			/* Must be an IKE packet.. pass it through */
 			return 1;
 		break;
-	case UDP_ENCAP_ESPINUDP_NON_IKE:
-		/* Check if this is a keepalive packet.  If so, eat it. */
-		if (len == 1 && udpdata[0] == 0xff) {
-			return -EINVAL;
-		} else if (len > 2 * sizeof(u32) + sizeof(struct ip_esp_hdr) &&
-			   udpdata32[0] == 0 && udpdata32[1] == 0) {
-
-			/* ESP Packet with Non-IKE marker */
-			len = sizeof(struct udphdr) + 2 * sizeof(u32);
-		} else
-			/* Must be an IKE packet.. pass it through */
-			return 1;
-		break;
 	}

 	/* At this point we are sure that this is an ESPinUDP packet,