Re: [PATCH v2] xfrm: fix inbound ipv4/udp/esp packets to UDPv6 dualstack sockets

[Simon Horman <simon.horman@corigine.com>]

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