Re: WARNING in ip_recv_error

On Fri, May 18, 2018 at 2:46 PM, Willem de Bruijn
[off-list ref] wrote:

On Fri, May 18, 2018 at 2:44 PM, Willem de Bruijn
[off-list ref] wrote:

quoted

On Fri, May 18, 2018 at 1:09 PM, Willem de Bruijn
[off-list ref] wrote:

quoted

On Fri, May 18, 2018 at 11:44 AM, David Miller [off-list ref] wrote:

quoted

From: Eric Dumazet <redacted>
Date: Fri, 18 May 2018 08:30:43 -0700

quoted

We probably need to revert Willem patch (7ce875e5ecb8562fd44040f69bda96c999e38bbc)

Is it really valid to reach ip_recv_err with an ipv6 socket?

I guess the issue is that setsockopt IPV6_ADDRFORM is not an
atomic operation, so that the socket is neither fully ipv4 nor fully
ipv6 by the time it reaches ip_recv_error.

  sk->sk_socket->ops = &inet_dgram_ops;
  < HERE >
  sk->sk_family = PF_INET;

Even calling inet_recv_error to demux would not necessarily help.

Safest would be to look up by skb->protocol, similar to what
ipv6_recv_error does to handle v4-mapped-v6.

Or to make that function safe with PF_INET and swap the order
of the above two operations.

All sound needlessly complicated for this rare socket option, but
I don't have a better idea yet. Dropping on the floor is not nice,
either.

Ensuring that ip_recv_error correctly handles packets from either
socket and removing the warning should indeed be good.

It is robust against v4-mapped packets from an AF_INET6 socket,
but see caveat on reconnect below.

The code between ipv6_recv_error for v4-mapped addresses and
ip_recv_error is essentially the same, the main difference being
whether to return network headers as sockaddr_in with SOL_IP
or sockaddr_in6 with SOL_IPV6.

There are very few other locations in the stack that explicitly test
sk_family in this way and thus would be vulnerable to races with
IPV6_ADDRFORM.

I'm not sure whether it is possible for a udpv6 socket to queue a
real ipv6 packet on the error queue, disconnect, connect to an
ipv4 address, call IPV6_ADDRFORM and then call ip_recv_error
on a true ipv6 packet. That would return buggy data, e.g., in
msg_name.

In do_ipv6_setsockopt IPV6_ADDRFORM we can test that the
error queue is empty, and then take its lock for the duration of the
operation.

Actually, no reason to hold the lock. This setsockopt holds the socket
lock, which connect would need, too. So testing that the queue
is empty after testing that it is connected to a v4 address is
sufficient to ensure that no ipv6 packets are queued for reception.

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4d780c7f0130..a975d6311341 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c

@@ -199,6 +199,11 @@ static int do_ipv6_setsockopt(struct sock *sk,

int level, int optname,

                        if (ipv6_only_sock(sk) ||
                            !ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
                                retv = -EADDRNOTAVAIL;
                                break;
                        }

+                       if (!skb_queue_empty(&sk->sk_error_queue)) {
+                               retv = -EBUSY;
+                               break;
+                       }
+
                        fl6_free_socklist(sk);
                        __ipv6_sock_mc_close(sk);

After this it should be safe to remove the warning in ip_recv_error.