| Message ID | 20230605110654.809655-1-maze@google.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] xfrm: fix inbound ipv4/udp/esp packets to UDPv6 dualstack sockets | expand |
On Mon, Jun 05, 2023 at 04:06:54AM -0700, Maciej Żenczykowski wrote: > 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> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Benedict Wong <benedictwong@google.com> > Cc: Yan Yan <evitayan@google.com> > Fixes: 0146dca70b87 ('xfrm: add support for UDPv6 encapsulation of ESP') > Signed-off-by: Maciej Żenczykowski <maze@google.com> Hi Maciej, Does the opposite case also need to be handled in xfrm4_udp_encap_rcv()?
On Mon, Jun 05, 2023 at 04:06:54AM -0700, Maciej Żenczykowski wrote: > 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> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Benedict Wong <benedictwong@google.com> > Cc: Yan Yan <evitayan@google.com> > Fixes: 0146dca70b87 ('xfrm: add support for UDPv6 encapsulation of ESP') Nit, which can possibly fixed without reposting. This should probably be: Fixes: 0146dca70b87 ("xfrm: add support for UDPv6 encapsulation of ESP")
On Mon, Jun 5, 2023 at 9:59 PM Simon Horman <simon.horman@corigine.com> wrote: > Hi Maciej, > > Does the opposite case also need to be handled in xfrm4_udp_encap_rcv()? I believe the answer is no: - ipv4 (AF_INET) sockets only ever receive (native) ipv4 traffic. - ipv6 (AF_INET6) ipv6-only sockets only ever receive (native) ipv6 traffic. - ipv6 (AF_INET6) dualstack (ie. not ipv6-only) sockets can receive both (native) ipv4 and (native) ipv6 traffic. Ipv6 dualstack sockets map the ipv4 address space into the IPv6 "IPv4-mapped" range of ::ffff:0.0.0.0/96, ie. 1.2.3.4 -> ::ffff:1.2.3.4 aka ::ffff:0102:0304 Whether ipv6 sockets default to dualstack or not is controlled by a sysctl (net.ipv6.bindv6only - not entirely well named, it actually affects the socket() system call, and bind() only as a later consequence of that, it thus does also affect whether connect() to ipv4 mapped addresses works or not), but can also be toggled manually via IPV6_V6ONLY socket option. Basically a dualstack ipv6 socket is a more-or-less drop-in replacement for ipv4 sockets (*entirely* so for TCP/UDP, and likely SCTP, DCCP & UDPLITE, though I think there might be some edge cases like ICMP sockets or RAW sockets that do need AF_INET - any such exceptions should probably be considered kernel bugs / missing features -> hence this patch). --- I believe we don't need to test the sk for: !ipv6_only_sock(sk), ie. !sk->sk_ipv6only before we do the dispatch to the v4 code path, because if the socket is ipv6-only then there should [IMHO/AFAICT] be no way for ipv4 packets to arrive here in the first place. --- Note: I can guarantee the currently existing code is wrong, both because we've experimentally discovered AF_INET6 dualstack sockets don't work for v4, and because the code obviously tries to read payload length from the ipv6 header, which of course doesn't exist for skb->protocol ETH_P_IP packets. However, I'm still not entirely sure this patch is 100% bug free... though it seems straightforward enough... --- I'll hold off on re-spinning for the ' -> " unless there's other comments.
On Tue, Jun 06, 2023 at 06:38:04AM +0900, Maciej Żenczykowski wrote: > On Mon, Jun 5, 2023 at 9:59 PM Simon Horman <simon.horman@corigine.com> wrote: > > Hi Maciej, > > > > Does the opposite case also need to be handled in xfrm4_udp_encap_rcv()? > > I believe the answer is no: > - ipv4 (AF_INET) sockets only ever receive (native) ipv4 traffic. > - ipv6 (AF_INET6) ipv6-only sockets only ever receive (native) ipv6 traffic. > - ipv6 (AF_INET6) dualstack (ie. not ipv6-only) sockets can receive > both (native) ipv4 and (native) ipv6 traffic. > > Ipv6 dualstack sockets map the ipv4 address space into the IPv6 > "IPv4-mapped" range of ::ffff:0.0.0.0/96, > ie. 1.2.3.4 -> ::ffff:1.2.3.4 aka ::ffff:0102:0304 > > Whether ipv6 sockets default to dualstack or not is controlled by a > sysctl (net.ipv6.bindv6only - not entirely well named, it actually > affects the socket() system call, and bind() only as a later > consequence of that, it thus does also affect whether connect() to > ipv4 mapped addresses works or not), but can also be toggled manually > via IPV6_V6ONLY socket option. > > Basically a dualstack ipv6 socket is a more-or-less drop-in > replacement for ipv4 sockets (*entirely* so for TCP/UDP, and likely > SCTP, DCCP & UDPLITE, though I think there might be some edge cases > like ICMP sockets or RAW sockets that do need AF_INET - any such > exceptions should probably be considered kernel bugs / missing > features -> hence this patch). > > --- > > I believe we don't need to test the sk for: > !ipv6_only_sock(sk), ie. !sk->sk_ipv6only > before we do the dispatch to the v4 code path, > because if the socket is ipv6-only then there should [IMHO/AFAICT] be > no way for ipv4 packets to arrive here in the first place. > > --- > > Note: I can guarantee the currently existing code is wrong, > both because we've experimentally discovered AF_INET6 dualstack > sockets don't work for v4, > and because the code obviously tries to read payload length from the > ipv6 header, > which of course doesn't exist for skb->protocol ETH_P_IP packets. > > However, I'm still not entirely sure this patch is 100% bug free... > though it seems straightforward enough... Thanks for the thorough explanation. I'm happy with this patch. Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > > I'll hold off on re-spinning for the ' -> " unless there's other comments. Ack
On Tue, Jun 06, 2023 at 06:38:04AM +0900, Maciej Żenczykowski wrote: > > I'll hold off on re-spinning for the ' -> " unless there's other comments. I'll fix that up when applying the patch, no need to resend.
2023-06-06, 06:38:04 +0900, Maciej Żenczykowski wrote: > On Mon, Jun 5, 2023 at 9:59 PM Simon Horman <simon.horman@corigine.com> wrote: > > Hi Maciej, > > > > Does the opposite case also need to be handled in xfrm4_udp_encap_rcv()? > > I believe the answer is no: > - ipv4 (AF_INET) sockets only ever receive (native) ipv4 traffic. > - ipv6 (AF_INET6) ipv6-only sockets only ever receive (native) ipv6 traffic. > - ipv6 (AF_INET6) dualstack (ie. not ipv6-only) sockets can receive > both (native) ipv4 and (native) ipv6 traffic. > > Ipv6 dualstack sockets map the ipv4 address space into the IPv6 > "IPv4-mapped" range of ::ffff:0.0.0.0/96, > ie. 1.2.3.4 -> ::ffff:1.2.3.4 aka ::ffff:0102:0304 > > Whether ipv6 sockets default to dualstack or not is controlled by a > sysctl (net.ipv6.bindv6only - not entirely well named, it actually > affects the socket() system call, and bind() only as a later > consequence of that, it thus does also affect whether connect() to > ipv4 mapped addresses works or not), but can also be toggled manually > via IPV6_V6ONLY socket option. > > Basically a dualstack ipv6 socket is a more-or-less drop-in > replacement for ipv4 sockets (*entirely* so for TCP/UDP, and likely > SCTP, DCCP & UDPLITE, though I think there might be some edge cases > like ICMP sockets or RAW sockets that do need AF_INET - any such > exceptions should probably be considered kernel bugs / missing > features -> hence this patch). > > --- > > I believe we don't need to test the sk for: > !ipv6_only_sock(sk), ie. !sk->sk_ipv6only > before we do the dispatch to the v4 code path, > because if the socket is ipv6-only then there should [IMHO/AFAICT] be > no way for ipv4 packets to arrive here in the first place. > > --- > > Note: I can guarantee the currently existing code is wrong, > both because we've experimentally discovered AF_INET6 dualstack > sockets don't work for v4, > and because the code obviously tries to read payload length from the > ipv6 header, > which of course doesn't exist for skb->protocol ETH_P_IP packets. > > However, I'm still not entirely sure this patch is 100% bug free... > though it seems straightforward enough... Reviewed-by: Sabrina Dubroca <sd@queasysnail.net> Thanks Maciej.
On Mon, Jun 05, 2023 at 04:06:54AM -0700, Maciej Żenczykowski wrote: > 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> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Benedict Wong <benedictwong@google.com> > Cc: Yan Yan <evitayan@google.com> > Fixes: 0146dca70b87 ('xfrm: add support for UDPv6 encapsulation of ESP') > Signed-off-by: Maciej Żenczykowski <maze@google.com> Applied, thanks a lot Maciej!
diff --git a/net/ipv4/xfrm4_input.c b/net/ipv4/xfrm4_input.c index ad2afeef4f10..eac206a290d0 100644 --- a/net/ipv4/xfrm4_input.c +++ b/net/ipv4/xfrm4_input.c @@ -164,6 +164,7 @@ int xfrm4_udp_encap_rcv(struct sock *sk, struct sk_buff *skb) kfree_skb(skb); return 0; } +EXPORT_SYMBOL(xfrm4_udp_encap_rcv); int xfrm4_rcv(struct sk_buff *skb) { diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c index 04cbeefd8982..4907ab241d6b 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)) + return xfrm4_udp_encap_rcv(sk, skb); + /* if this is not encapsulated socket, then just return now */ if (!encap_type) return 1;
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> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Benedict Wong <benedictwong@google.com> Cc: Yan Yan <evitayan@google.com> Fixes: 0146dca70b87 ('xfrm: add support for UDPv6 encapsulation of ESP') Signed-off-by: Maciej Żenczykowski <maze@google.com> --- net/ipv4/xfrm4_input.c | 1 + net/ipv6/xfrm6_input.c | 3 +++ 2 files changed, 4 insertions(+)