netdev - Re: [PATCH ipsec-next v2] udpencap: Remove Obsolete UDP_ENCAP_ESPINUDP_NON

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zhgh0HsEx0uRiEtd@moon.secunet.de>
Date: Thu, 11 Apr 2024 19:45:52 +0200
From: Antony Antony <antony.antony@...unet.com>
To: Steffen Klassert <steffen.klassert@...unet.com>
CC: Antony Antony <antony.antony@...unet.com>, Florian Westphal
	<fw@...len.de>, Herbert Xu <herbert@...dor.apana.org.au>, Willem de Bruijn
	<willemdebruijn.kernel@...il.com>, "David S. Miller" <davem@...emloft.net>,
	David Ahern <dsahern@...nel.org>, Jakub Kicinski <kuba@...nel.org>, "Paolo
 Abeni" <pabeni@...hat.com>, Andreas Gruenbacher <agruenba@...hat.com>,
	<devel@...ux-ipsec.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH ipsec-next v2] udpencap: Remove Obsolete
 UDP_ENCAP_ESPINUDP_NON_IKE Support

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@...e.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@...fp.com>
> >     Date: Wed Apr 2 13:21:02 2003 -0800
> > 
> >     [IPSEC]: Implement UDP Encapsulation framework.
> > 
> > Signed-off-by: Antony Antony <antony.antony@...unet.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