Skip to contentSkip to conversation

ipv4: remove warning in ip_recv_error

PATCH In review Willem de Bruijn 2 participants 3 messages May 23, 2018 – May 25, 2018

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Willem de Bruijn

author
#0 lore raw

PATCH 1/1 ipv4: remove warning in ip_recv_error

From: Willem de Bruijn <[email protected]>

A precondition check in ip_recv_error triggered on an otherwise benign race. Remove the warning.

The warning triggers when passing an ipv6 socket to this ipv4 error handling function. RaceFuzzer was able to trigger it due to a race in setsockopt IPV6_ADDRFORM.

  ---
  CPU0
    do_ipv6_setsockopt
      sk->sk_socket->ops = &inet_dgram_ops;

  ---
  CPU1
    sk->sk_prot->recvmsg
      udp_recvmsg
        ip_recv_error
          WARN_ON_ONCE(sk->sk_family == AF_INET6);

  ---
  CPU0
    do_ipv6_setsockopt
      sk->sk_family = PF_INET;

This socket option converts a v6 socket that is connected to a v4 peer to an v4 socket. It updates the socket on the fly, changing fields in sk as well as other structs. This is inherently non-atomic. It races with the lockless udp_recvmsg path.

No other code makes an assumption that these fields are updated atomically. It is benign here, too, as ip_recv_error cares only about the protocol of the skbs enqueued on the error queue, for which sk_family is not a precise predictor (thanks to another isue with IPV6_ADDRFORM).

Link http://lkml.kernel.org/r/[email protected] open here
Fixes ("7ce875e5ecb8 ipv4: warn once on passing AF_INET6 socket to ip_recv_error")
Reported-by DaeRyong Jeong <[email protected]>
Suggested-by Eric Dumazet <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/ipv4/ip_sockglue.c | 2 --
 1 file changed, 2 deletions(-)
net/ipv4/ip_sockglue.c +0 -2
index 5ad2d8ed3a3f..57bbb060faaf 100644
@@ -505,8 +505,6 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
 	int err;
 	int copied;
 
-	WARN_ON_ONCE(sk->sk_family == AF_INET6);
-
 	err = -EAGAIN;
 	skb = sock_dequeue_err_skb(sk);
 	if (!skb)
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
maintainer
#1 lore raw

From: Willem de Bruijn <[email protected]>
Date: Wed, 23 May 2018 14:29:52 -0400

Quoted 8 lines

From: Willem de Bruijn <[email protected]>

A precondition check in ip_recv_error triggered on an otherwise benign race. Remove the warning.

The warning triggers when passing an ipv6 socket to this ipv4 error handling function. RaceFuzzer was able to trigger it due to a race in setsockopt IPV6_ADDRFORM.

...

Quoted 11 lines

This socket option converts a v6 socket that is connected to a v4 peer to an v4 socket. It updates the socket on the fly, changing fields in sk as well as other structs. This is inherently non-atomic. It races with the lockless udp_recvmsg path.

No other code makes an assumption that these fields are updated atomically. It is benign here, too, as ip_recv_error cares only about the protocol of the skbs enqueued on the error queue, for which sk_family is not a precise predictor (thanks to another isue with IPV6_ADDRFORM).

Quoted 5 lines
Link http://lkml.kernel.org/r/[email protected] open here
Fixes ("7ce875e5ecb8 ipv4: warn once on passing AF_INET6 socket to ip_recv_error")
Reported-by DaeRyong Jeong <[email protected]>
Suggested-by Eric Dumazet <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>

Applied and queued up for -stable.

The SHA1_ID doesn't go inside the (" ") of the Fixes tag, I fixed it up this time.

author
#2 lore raw
Quoted 29 lines · David Miller

From: Willem de Bruijn <[email protected]>
Date: Wed, 23 May 2018 14:29:52 -0400

Quoted 8 lines

From: Willem de Bruijn <[email protected]>

A precondition check in ip_recv_error triggered on an otherwise benign race. Remove the warning.

The warning triggers when passing an ipv6 socket to this ipv4 error handling function. RaceFuzzer was able to trigger it due to a race in setsockopt IPV6_ADDRFORM.

...

Quoted 11 lines

This socket option converts a v6 socket that is connected to a v4 peer to an v4 socket. It updates the socket on the fly, changing fields in sk as well as other structs. This is inherently non-atomic. It races with the lockless udp_recvmsg path.

No other code makes an assumption that these fields are updated atomically. It is benign here, too, as ip_recv_error cares only about the protocol of the skbs enqueued on the error queue, for which sk_family is not a precise predictor (thanks to another isue with IPV6_ADDRFORM).

Quoted 5 lines
Link http://lkml.kernel.org/r/[email protected] open here
Fixes ("7ce875e5ecb8 ipv4: warn once on passing AF_INET6 socket to ip_recv_error")
Reported-by DaeRyong Jeong <[email protected]>
Suggested-by Eric Dumazet <[email protected]>
Signed-off-by Willem de Bruijn <[email protected]>
Quoted 4 lines

Applied and queued up for -stable.

The SHA1_ID doesn't go inside the (" ") of the Fixes tag, I fixed it up this time.

Thanks David. Sorry about that.

I'll send a checkpatch.pl patch to catch such typos myself in the future. Something like

+# Check format of Fixes line
+ if ($in_commit_log && $line =~ /^\s*fixes:/i) {
+ if ($line !~ /^fixes:\s[0-9a-f]{12,40}\s\(".*"\)$/i) {
+ WARN("BAD_FIXES",
+ "Fixes tag is not of form
\"Fixes: <12+ chars of sha1> \(\"title line\"\)");
+ }
+ }
+

though it seems that Fixes lines are expressly omitted from strict commit style checking at the moment. I don't immediately see why.