diff mbox series

xfrm: fix inbound ipv4/udp/esp packets to UDPv6 dualstack sockets

Message ID 20221026083203.2214468-1-zenczykowski@gmail.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series xfrm: fix inbound ipv4/udp/esp packets to UDPv6 dualstack sockets | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 4
netdev/cc_maintainers warning 7 maintainers not CCed: kuba@kernel.org dsahern@kernel.org davem@davemloft.net herbert@gondor.apana.org.au yoshfuji@linux-ipv6.org edumazet@google.com pabeni@redhat.com
netdev/build_clang fail Errors and warnings before: 0 this patch: 4
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 4
netdev/checkpatch warning WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 0146dca70b87 ("xfrm: add support for UDPv6 encapsulation of ESP")'
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Maciej Żenczykowski Oct. 26, 2022, 8:32 a.m. UTC
From: Maciej Żenczykowski <maze@google.com>

Before Linux v5.8 an AF_INET6 SOCK_DGRAM (udp/udplite) socket
with SOL_UDP, UDP_ENCAP, UDP_ENCAP_ESPINUDP{,_NON_IKE} enabled
would just unconditionally use xfrm4_udp_encap_rcv(), afterwards
such a socket would use the newly added xfrm6_udp_encap_rcv()
which only handles IPv6 packets.

Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Fixes: 0146dca70b87 ('xfrm: add support for UDPv6 encapsulation of ESP')
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/ipv6/xfrm6_input.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Maciej Żenczykowski Oct. 26, 2022, 8:42 a.m. UTC | #1
On Wed, Oct 26, 2022 at 5:32 PM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> From: Maciej Żenczykowski <maze@google.com>
>
> Before Linux v5.8 an AF_INET6 SOCK_DGRAM (udp/udplite) socket
> with SOL_UDP, UDP_ENCAP, UDP_ENCAP_ESPINUDP{,_NON_IKE} enabled
> would just unconditionally use xfrm4_udp_encap_rcv(), afterwards
> such a socket would use the newly added xfrm6_udp_encap_rcv()
> which only handles IPv6 packets.
>
> Cc: Sabrina Dubroca <sd@queasysnail.net>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Fixes: 0146dca70b87 ('xfrm: add support for UDPv6 encapsulation of ESP')
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
>  net/ipv6/xfrm6_input.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
> index 04cbeefd8982..2d1c75b42709 100644
> --- a/net/ipv6/xfrm6_input.c
> +++ b/net/ipv6/xfrm6_input.c
> @@ -86,6 +86,9 @@ int xfrm6_udp_encap_rcv(struct sock *sk, struct sk_buff *skb)
>         __be32 *udpdata32;
>         __u16 encap_type = up->encap_type;
>
> +       if (skb->protocol == htons(ETH_P_IP))
> +               xfrm4_udp_encap_rcv(sk, skb);
> +
>         /* if this is not encapsulated socket, then just return now */
>         if (!encap_type)
>                 return 1;
> --
> 2.38.0.135.g90850a2211-goog

Does this seem reasonable?

I'll admit that so far I've only tested that the code builds.
However, the current code seems very obviously wrong, as it blindly
assumes (later in the function) that there's an ipv6 header on the
packet...

Our current API for creating these sockets specifies the port, but not
the ip version.
I think it would be beneficial if we could just always use AF_INET6
(and thus dualstack) sockets,
instead of how we currently just always use AF_INET udp sockets.

side note: with nat64 network based packet translation you can
actually end up with ipv6/udp/esp talking to ipv4/udp/esp, etc, and
you might not be able to tell that this is the case from looking at
the IP addresses themselves.
Jakub Kicinski Oct. 27, 2022, 1:24 a.m. UTC | #2
On Wed, 26 Oct 2022 17:42:37 +0900 Maciej Żenczykowski wrote:
> I'll admit that so far I've only tested that the code builds.

For _a_ definition of builds ;)

ERROR: modpost: "xfrm4_udp_encap_rcv" [net/ipv6/ipv6.ko] undefined!
Maciej Żenczykowski Oct. 27, 2022, 1:52 a.m. UTC | #3
On Thu, Oct 27, 2022 at 10:24 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 26 Oct 2022 17:42:37 +0900 Maciej Żenczykowski wrote:
> > I'll admit that so far I've only tested that the code builds.
>
> For _a_ definition of builds ;)
>
> ERROR: modpost: "xfrm4_udp_encap_rcv" [net/ipv6/ipv6.ko] undefined!

Oh, it does build - it even boots.

egrep 'XFRM|ESP' .config | egrep -v 'is not set'

CONFIG_NAMESPACES=y
CONFIG_X86_ESPFIX64=y
CONFIG_XFRM=y
CONFIG_XFRM_ALGO=y
CONFIG_XFRM_ESP=y
CONFIG_INET6_ESP=y

Looking at the Kconfigs/Makefiles, perhaps you have ipv6 as a module?
Why would you do that ;-)
I don't think my development systems even support booting with ipv6
modularized...
Jakub Kicinski Oct. 27, 2022, 1:58 a.m. UTC | #4
On Thu, 27 Oct 2022 10:52:03 +0900 Maciej Żenczykowski wrote:
> Looking at the Kconfigs/Makefiles, perhaps you have ipv6 as a module?

Yup, allmodconfig, just a note for when you send a v2.

> Why would you do that ;-)

No joke, IMHO it may be time to stop allowing IPV6=m.
kernel test robot Oct. 27, 2022, 4:47 a.m. UTC | #5
Hi Maciej,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on klassert-ipsec-next/master]
[also build test ERROR on klassert-ipsec/master net/master net-next/master linus/master v6.1-rc2 next-20221026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Maciej-enczykowski/xfrm-fix-inbound-ipv4-udp-esp-packets-to-UDPv6-dualstack-sockets/20221026-163400
base:   https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec-next.git master
patch link:    https://lore.kernel.org/r/20221026083203.2214468-1-zenczykowski%40gmail.com
patch subject: [PATCH] xfrm: fix inbound ipv4/udp/esp packets to UDPv6 dualstack sockets
config: m68k-hp300_defconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/aac18d02a77ba118b8808fde26367f223934fb24
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Maciej-enczykowski/xfrm-fix-inbound-ipv4-udp-esp-packets-to-UDPv6-dualstack-sockets/20221026-163400
        git checkout aac18d02a77ba118b8808fde26367f223934fb24
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "xfrm4_udp_encap_rcv" [net/ipv6/ipv6.ko] undefined!
diff mbox series

Patch

diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
index 04cbeefd8982..2d1c75b42709 100644
--- a/net/ipv6/xfrm6_input.c
+++ b/net/ipv6/xfrm6_input.c
@@ -86,6 +86,9 @@  int xfrm6_udp_encap_rcv(struct sock *sk, struct sk_buff *skb)
 	__be32 *udpdata32;
 	__u16 encap_type = up->encap_type;
 
+	if (skb->protocol == htons(ETH_P_IP))
+		xfrm4_udp_encap_rcv(sk, skb);
+
 	/* if this is not encapsulated socket, then just return now */
 	if (!encap_type)
 		return 1;