netdev - Re: Uninitialized value in __sk_nulls_add_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1512483322.19682.73.camel@gmail.com>
Date:   Tue, 05 Dec 2017 06:15:22 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        Alexander Potapenko <glider@...gle.com>, cgallek@...gle.com
Cc:     Craig Gallek <kraig@...gle.com>,
        David Miller <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>
Subject: Re: Uninitialized value in __sk_nulls_add_node_rcu()

On Wed, 2017-11-22 at 07:58 -0800, Eric Dumazet wrote:
> On Wed, Nov 22, 2017 at 5:38 AM, Alexander Potapenko <glider@...gle.c
> om> wrote:
> > On Thu, Oct 26, 2017 at 4:56 PM, Alexander Potapenko <glider@...gle
> > .com> wrote:
> > > On Thu, Oct 26, 2017 at 4:52 PM, Eric Dumazet <edumazet@...gle.co
> > > m> wrote:
> > > > On Thu, Oct 26, 2017 at 7:47 AM, Eric Dumazet <edumazet@...gle.
> > > > com> wrote:
> > > > > On Thu, Oct 26, 2017 at 7:20 AM, Alexander Potapenko <glider@
> > > > > google.com> wrote:
> > > > > > On Thu, Oct 26, 2017 at 2:51 PM, Alexander Potapenko <glide
> > > > > > r@...gle.com> wrote:
> > > > > > > Hi David, Eric,
> > > > > > > 
> > > > > > > I've changed KMSAN instrumentation a bit and it's now
> > > > > > > reporting a new
> > > > > > > error (see below) when I SSH into a VM.
> > > > > > 
> > > > > > I've double-checked the old instrumentation and found a bug
> > > > > > in it,
> > > > > > which led to masking some of the errors on uninitialized
> > > > > > bitfields.
> > > > > > I now believe this is a true positive report.
> > > > > 
> > > > > 
> > > > > Please do not top post on netdev
> > > 
> > > Sorry about that.
> > > > > A child is cloned from the listener, check sock_copy()
> > > > > 
> > > > > sk_reuseport is part of the copied fields.
> > > > > 
> > > > > You might have some bug at your side ?
> > > > > 
> > > > 
> > > > Oh these are request socket.
> > > > 
> > > > This is an harmless bug added in commit
> > > > d894ba18d4e449b3a7f6eb491f16c9e02933736e
> > > > ("soreuseport: fix ordering for mixed v4/v6 sockets")
> > > > 
> > > > I will send a patch, but really this has no effect at all.
> > > 
> > > Thanks for clarifying!
> > > For me this was a question of the tool's correctness, because
> > > until
> > > recently I wasn't able to understand whether this is a true bug
> > > or
> > > not.
> > 
> > A friendly ping.
> > Eric, did you find the time to send the patch?
> 
> Not yet, I am still investigating this issue.
> 
> Thanks.


Craig, my take on this (minor) bug is that we should be able to remove 
hlist_nulls_add_tail_rcu()

The original problem only applies to UDP sockets or TCP/DCCP listeners.

They no longer use sk_nulls_node (SLAB_DESTROY_BY_RCU)

Other sockets have unique 4-tuple so we can not have a mismatch at
lookup time.

Any objection with following patch ?

diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
index a328e8181e49f3a0947dd713daeef35b9d7c831f..e4b257ff881bfe439a945d7487f5700f17a26740 100644
--- a/include/linux/rculist_nulls.h
+++ b/include/linux/rculist_nulls.h
@@ -100,44 +100,6 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
 		first->pprev = &n->next;
 }
 
-/**
- * hlist_nulls_add_tail_rcu
- * @n: the element to add to the hash list.
- * @h: the list to add to.
- *
- * Description:
- * Adds the specified element to the end of the specified hlist_nulls,
- * while permitting racing traversals.  NOTE: tail insertion requires
- * list traversal.
- *
- * The caller must take whatever precautions are necessary
- * (such as holding appropriate locks) to avoid racing
- * with another list-mutation primitive, such as hlist_nulls_add_head_rcu()
- * or hlist_nulls_del_rcu(), running on this same list.
- * However, it is perfectly legal to run concurrently with
- * the _rcu list-traversal primitives, such as
- * hlist_nulls_for_each_entry_rcu(), used to prevent memory-consistency
- * problems on Alpha CPUs.  Regardless of the type of CPU, the
- * list-traversal primitive must be guarded by rcu_read_lock().
- */
-static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
-					struct hlist_nulls_head *h)
-{
-	struct hlist_nulls_node *i, *last = NULL;
-
-	for (i = hlist_nulls_first_rcu(h); !is_a_nulls(i);
-	     i = hlist_nulls_next_rcu(i))
-		last = i;
-
-	if (last) {
-		n->next = last->next;
-		n->pprev = &last->next;
-		rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
-	} else {
-		hlist_nulls_add_head_rcu(n, h);
-	}
-}
-
 /**
  * hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
  * @tpos:	the type * to use as a loop cursor.
diff --git a/include/net/sock.h b/include/net/sock.h
index 79e1a2c7912c03d8281d449609d57cc909138a3b..8e8adbb8f971546989b185c9f01593c0c0730431 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -685,11 +685,7 @@ static inline void sk_add_node_rcu(struct sock *sk, struct hlist_head *list)
 
 static inline void __sk_nulls_add_node_rcu(struct sock *sk, struct hlist_nulls_head *list)
 {
-	if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
-	    sk->sk_family == AF_INET6)
-		hlist_nulls_add_tail_rcu(&sk->sk_nulls_node, list);
-	else
-		hlist_nulls_add_head_rcu(&sk->sk_nulls_node, list);
+	hlist_nulls_add_head_rcu(&sk->sk_nulss_node, list);
 }
 
 static inline void sk_nulls_add_node_rcu(struct sock *sk, struct hlist_nulls_head *list)