[PATCH] ipv6: Fix regression caused by efe4208 in udp_v6_mcast_next()

[PATCH] ipv6: Fix regression caused by efe4208 in udp_v6_mcast_next()

[Sven Wegener]

I've split the last version of the patch into a fix and cleanup patch.

[PATCH 1/2] ipv6: Fix regression caused by efe4208 in udp_v6_mcast_next()

[Sven Wegener]

Commit efe4208 ("ipv6: make lookups simpler and faster") introduced a
regression in udp_v6_mcast_next(), resulting in multicast packets not
reaching the destination sockets under certain conditions.

The packet's IPv6 addresses are wrongly compared to the IPv6 addresses
from the function's socket argument, which indicates the starting point
for looping, instead of the loop variable. If the addresses from the
first socket do not match the packet's addresses, no socket in the list
will match.

Cc: Eric Dumazet <edumazet@google.com>
Cc: stable@vger.kernel.org # 3.13+
Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
---
 net/ipv6/udp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 1e586d9..20b63d2 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -716,15 +716,15 @@ static struct sock *udp_v6_mcast_next(struct net *net, struct sock *sk,
 				if (inet->inet_dport != rmt_port)
 					continue;
 			}
-			if (!ipv6_addr_any(&sk->sk_v6_daddr) &&
-			    !ipv6_addr_equal(&sk->sk_v6_daddr, rmt_addr))
+			if (!ipv6_addr_any(&s->sk_v6_daddr) &&
+			    !ipv6_addr_equal(&s->sk_v6_daddr, rmt_addr))
 				continue;
 
 			if (s->sk_bound_dev_if && s->sk_bound_dev_if != dif)
 				continue;
 
-			if (!ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
-				if (!ipv6_addr_equal(&sk->sk_v6_rcv_saddr, loc_addr))
+			if (!ipv6_addr_any(&s->sk_v6_rcv_saddr)) {
+				if (!ipv6_addr_equal(&s->sk_v6_rcv_saddr, loc_addr))
 					continue;
 			}
 			if (!inet6_mc_check(s, loc_addr, rmt_addr))
-- 
2.0.0

[PATCH 2/2] ipv6: Shrink udp_v6_mcast_next() to one socket variable

[Sven Wegener]

To avoid the confusion of having two variables, shrink the function to
only use the parameter variable for looping.

Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
---
 net/ipv6/udp.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 20b63d2..9b7a99d 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -701,35 +701,34 @@ static struct sock *udp_v6_mcast_next(struct net *net, struct sock *sk,
 				      int dif)
 {
 	struct hlist_nulls_node *node;
-	struct sock *s = sk;
 	unsigned short num = ntohs(loc_port);
 
-	sk_nulls_for_each_from(s, node) {
-		struct inet_sock *inet = inet_sk(s);
+	sk_nulls_for_each_from(sk, node) {
+		struct inet_sock *inet = inet_sk(sk);
 
-		if (!net_eq(sock_net(s), net))
+		if (!net_eq(sock_net(sk), net))
 			continue;
 
-		if (udp_sk(s)->udp_port_hash == num &&
-		    s->sk_family == PF_INET6) {
+		if (udp_sk(sk)->udp_port_hash == num &&
+		    sk->sk_family == PF_INET6) {
 			if (inet->inet_dport) {
 				if (inet->inet_dport != rmt_port)
 					continue;
 			}
-			if (!ipv6_addr_any(&s->sk_v6_daddr) &&
-			    !ipv6_addr_equal(&s->sk_v6_daddr, rmt_addr))
+			if (!ipv6_addr_any(&sk->sk_v6_daddr) &&
+			    !ipv6_addr_equal(&sk->sk_v6_daddr, rmt_addr))
 				continue;
 
-			if (s->sk_bound_dev_if && s->sk_bound_dev_if != dif)
+			if (sk->sk_bound_dev_if && sk->sk_bound_dev_if != dif)
 				continue;
 
-			if (!ipv6_addr_any(&s->sk_v6_rcv_saddr)) {
-				if (!ipv6_addr_equal(&s->sk_v6_rcv_saddr, loc_addr))
+			if (!ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
+				if (!ipv6_addr_equal(&sk->sk_v6_rcv_saddr, loc_addr))
 					continue;
 			}
-			if (!inet6_mc_check(s, loc_addr, rmt_addr))
+			if (!inet6_mc_check(sk, loc_addr, rmt_addr))
 				continue;
-			return s;
+			return sk;
 		}
 	}
 	return NULL;
-- 
2.0.0

Re: [PATCH 1/2] ipv6: Fix regression caused by efe4208 in udp_v6_mcast_next()

[Eric Dumazet]

On Thu, 2014-05-29 at 20:27 +0000, Sven Wegener wrote:
> Commit efe4208 ("ipv6: make lookups simpler and faster") introduced a
> regression in udp_v6_mcast_next(), resulting in multicast packets not
> reaching the destination sockets under certain conditions.
> 
> The packet's IPv6 addresses are wrongly compared to the IPv6 addresses
> from the function's socket argument, which indicates the starting point
> for looping, instead of the loop variable. If the addresses from the
> first socket do not match the packet's addresses, no socket in the list
> will match.
> 
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: stable@vger.kernel.org # 3.13+
> Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
> ---
>  net/ipv6/udp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

I removed the CC stable@ , as you should not have use it.

Thanks

Re: [PATCH 1/2] ipv6: Fix regression caused by efe4208 in udp_v6_mcast_next()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 29 May 2014 13:37:35 -0700

> On Thu, 2014-05-29 at 20:27 +0000, Sven Wegener wrote:
>> Commit efe4208 ("ipv6: make lookups simpler and faster") introduced a
>> regression in udp_v6_mcast_next(), resulting in multicast packets not
>> reaching the destination sockets under certain conditions.
>> 
>> The packet's IPv6 addresses are wrongly compared to the IPv6 addresses
>> from the function's socket argument, which indicates the starting point
>> for looping, instead of the loop variable. If the addresses from the
>> first socket do not match the packet's addresses, no socket in the list
>> will match.
>> 
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: stable@vger.kernel.org # 3.13+
>> Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
>> ---
>>  net/ipv6/udp.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Acked-by: Eric Dumazet <edumazet@google.com>
> 
> I removed the CC stable@ , as you should not have use it.

I'll apply this to 'net' and queue it up for -stable, thanks.

The next time I merge 'net' into 'net-next' I'll add the second
patch there.

Thanks again.

Re: [PATCH] ipv6: Fix regression caused by efe4208 in udp_v6_mcast_next()

[Eric Dumazet]

On Sun, 2014-05-25 at 16:14 +0200, Sven Wegener wrote:
> Commit efe4208 ("ipv6: make lookups simpler and faster") introduced a
> regression in udp_v6_mcast_next(), resulting in multicast packets not
> reaching the destination sockets under certain conditions.
> 
> The packet's IPv6 addresses are wrongly compared to the IPv6 addresses
> from the function's socket argument, which indicates the starting point
> for looping, instead of the loop variable. If the addresses from the
> first socket do not match the packet's addresses, no socket in the list
> will match.
> 
> Fix this by shrinking the function to use one variable for the socket to
> avoid confusion in the future.

Thanks for fixing this bug.

Please do not mix a cleanup and a fix in same patch, especially for
a stable candidate.

Your fix is obvious when reduced to this :

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 1e586d92260e..20b63d2ab70f 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -716,15 +716,15 @@ static struct sock *udp_v6_mcast_next(struct net *net, struct sock *sk,
 				if (inet->inet_dport != rmt_port)
 					continue;
 			}
-			if (!ipv6_addr_any(&sk->sk_v6_daddr) &&
-			    !ipv6_addr_equal(&sk->sk_v6_daddr, rmt_addr))
+			if (!ipv6_addr_any(&s->sk_v6_daddr) &&
+			    !ipv6_addr_equal(&s->sk_v6_daddr, rmt_addr))
 				continue;
 
 			if (s->sk_bound_dev_if && s->sk_bound_dev_if != dif)
 				continue;
 
-			if (!ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
-				if (!ipv6_addr_equal(&sk->sk_v6_rcv_saddr, loc_addr))
+			if (!ipv6_addr_any(&s->sk_v6_rcv_saddr)) {
+				if (!ipv6_addr_equal(&s->sk_v6_rcv_saddr, loc_addr))
 					continue;
 			}
 			if (!inet6_mc_check(s, loc_addr, rmt_addr))

[PATCH] ipv6: Fix regression caused by efe4208 in udp_v6_mcast_next()

[Sven Wegener]

Commit efe4208 ("ipv6: make lookups simpler and faster") introduced a
regression in udp_v6_mcast_next(), resulting in multicast packets not
reaching the destination sockets under certain conditions.

The packet's IPv6 addresses are wrongly compared to the IPv6 addresses
from the function's socket argument, which indicates the starting point
for looping, instead of the loop variable. If the addresses from the
first socket do not match the packet's addresses, no socket in the list
will match.

Fix this by shrinking the function to use one variable for the socket to
avoid confusion in the future.

Cc: Eric Dumazet <edumazet@google.com>
Cc: stable@vger.kernel.org # 3.13+
Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
---
 net/ipv6/udp.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 1e586d9..9b7a99d 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -701,17 +701,16 @@ static struct sock *udp_v6_mcast_next(struct net *net, struct sock *sk,
 				      int dif)
 {
 	struct hlist_nulls_node *node;
-	struct sock *s = sk;
 	unsigned short num = ntohs(loc_port);
 
-	sk_nulls_for_each_from(s, node) {
-		struct inet_sock *inet = inet_sk(s);
+	sk_nulls_for_each_from(sk, node) {
+		struct inet_sock *inet = inet_sk(sk);
 
-		if (!net_eq(sock_net(s), net))
+		if (!net_eq(sock_net(sk), net))
 			continue;
 
-		if (udp_sk(s)->udp_port_hash == num &&
-		    s->sk_family == PF_INET6) {
+		if (udp_sk(sk)->udp_port_hash == num &&
+		    sk->sk_family == PF_INET6) {
 			if (inet->inet_dport) {
 				if (inet->inet_dport != rmt_port)
 					continue;
@@ -720,16 +719,16 @@ static struct sock *udp_v6_mcast_next(struct net *net, struct sock *sk,
 			    !ipv6_addr_equal(&sk->sk_v6_daddr, rmt_addr))
 				continue;
 
-			if (s->sk_bound_dev_if && s->sk_bound_dev_if != dif)
+			if (sk->sk_bound_dev_if && sk->sk_bound_dev_if != dif)
 				continue;
 
 			if (!ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
 				if (!ipv6_addr_equal(&sk->sk_v6_rcv_saddr, loc_addr))
 					continue;
 			}
-			if (!inet6_mc_check(s, loc_addr, rmt_addr))
+			if (!inet6_mc_check(sk, loc_addr, rmt_addr))
 				continue;
-			return s;
+			return sk;
 		}
 	}
 	return NULL;