Patchwork [net-next,2/3] ipv6: use newly introduced __ipv6_addr_needs_scope_id and ipv6_iface_scope_id

login
register
mail settings
Submitter Hannes Frederic Sowa
Date Feb. 12, 2013, 10:16 p.m.
Message ID <20130212221634.GA7212@order.stressinduktion.org>
Download mbox | patch
Permalink /patch/219999/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Hannes Frederic Sowa - Feb. 12, 2013, 10:16 p.m.
This patch requires multicast interface-scoped addresses to supply a
sin6_scope_id. Because the sin6_scope_id is now also correctly used
in case of interface-scoped multicast traffic this enables one to use
interface scoped addresses over interfaces which are not targeted by the
default multicast route (the route has to be put there manually, though).

Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 net/ipv6/af_inet6.c              |  6 +++---
 net/ipv6/datagram.c              | 18 ++++++++++--------
 net/ipv6/icmp.c                  |  2 +-
 net/ipv6/inet6_connection_sock.c |  6 ++----
 net/ipv6/raw.c                   |  9 ++++-----
 net/ipv6/udp.c                   | 14 ++++++++------
 6 files changed, 28 insertions(+), 27 deletions(-)
Hannes Frederic Sowa - Feb. 13, 2013, 12:13 a.m.
On Tue, Feb 12, 2013 at 11:16:34PM +0100, Hannes Frederic Sowa wrote:
> This patch requires multicast interface-scoped addresses to supply a
> sin6_scope_id. Because the sin6_scope_id is now also correctly used
> in case of interface-scoped multicast traffic this enables one to use
> interface scoped addresses over interfaces which are not targeted by the
> default multicast route (the route has to be put there manually, though).
> 
> Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  net/ipv6/af_inet6.c              |  6 +++---
>  net/ipv6/datagram.c              | 18 ++++++++++--------
>  net/ipv6/icmp.c                  |  2 +-
>  net/ipv6/inet6_connection_sock.c |  6 ++----
>  net/ipv6/raw.c                   |  9 ++++-----
>  net/ipv6/udp.c                   | 14 ++++++++------
>  6 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
> index 6b793bf..f56277f 100644
> --- a/net/ipv6/af_inet6.c
> +++ b/net/ipv6/af_inet6.c
> @@ -323,7 +323,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  			struct net_device *dev = NULL;
>  
>  			rcu_read_lock();
> -			if (addr_type & IPV6_ADDR_LINKLOCAL) {
> +			if (__ipv6_addr_needs_scope_id(addr_type)) {
>  				if (addr_len >= sizeof(struct sockaddr_in6) &&
>  				    addr->sin6_scope_id) {
>  					/* Override any existing binding, if another one

By trying to setup the multicast interface scoped routes by default I
just found a bug in this patch essentially breaking ipv6 multicast. I
overlooked that ipv6_addr_type strips off the scopes, thus my check if
a multicast address needs a scope_id always returns true. I'll check
if I can convert the ipv6_addr_type calls to __ipv6_addr_type and will
reroll the patch.  Sorry, my tests were too focused on interface/local
multicast. :(

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Haley - Feb. 13, 2013, 2:51 a.m.
On 02/12/2013 07:13 PM, Hannes Frederic Sowa wrote:
>> --- a/net/ipv6/af_inet6.c
>> +++ b/net/ipv6/af_inet6.c
>> @@ -323,7 +323,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>>  			struct net_device *dev = NULL;
>>  
>>  			rcu_read_lock();
>> -			if (addr_type & IPV6_ADDR_LINKLOCAL) {
>> +			if (__ipv6_addr_needs_scope_id(addr_type)) {
>>  				if (addr_len >= sizeof(struct sockaddr_in6) &&
>>  				    addr->sin6_scope_id) {
>>  					/* Override any existing binding, if another one
> 
> By trying to setup the multicast interface scoped routes by default I
> just found a bug in this patch essentially breaking ipv6 multicast. I
> overlooked that ipv6_addr_type strips off the scopes, thus my check if
> a multicast address needs a scope_id always returns true. I'll check
> if I can convert the ipv6_addr_type calls to __ipv6_addr_type and will
> reroll the patch.  Sorry, my tests were too focused on interface/local
> multicast. :(

I'd always thought of adding helper inlines like these in net/ipv6.h:

static inline bool ipv6_addr_linklocal(const struct in6_addr *a)
{
        return ((a->s6_addr32[0] & htonl(0xFFC00000)) == htonl(0xFE800000));
}

static inline bool ipv6_addr_mc_linklocal(const struct in6_addr *a)
{
        return (((a->s6_addr32[0] & htonl(0xFF000000)) == htonl(0xFF000000)) &&
                ((a->s6_addr32[1] & 0x0F) == IPV6_ADDR_SCOPE_LINKLOCAL));
}

Maybe something like that would help here?

When I saw this in patch 3/3 it just seemed like the long way to determine if
the address was a link-local multicast:

	!__ipv6_addr_needs_scope_id(__ipv6_addr_type(&hdr->daddr))

The helper isn't as generic as your patch, but more direct.

-Brian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa - Feb. 13, 2013, 10:33 a.m.
On Tue, Feb 12, 2013 at 09:51:06PM -0500, Brian Haley wrote:
> On 02/12/2013 07:13 PM, Hannes Frederic Sowa wrote:
> >> --- a/net/ipv6/af_inet6.c
> >> +++ b/net/ipv6/af_inet6.c
> >> @@ -323,7 +323,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >>  			struct net_device *dev = NULL;
> >>  
> >>  			rcu_read_lock();
> >> -			if (addr_type & IPV6_ADDR_LINKLOCAL) {
> >> +			if (__ipv6_addr_needs_scope_id(addr_type)) {
> >>  				if (addr_len >= sizeof(struct sockaddr_in6) &&
> >>  				    addr->sin6_scope_id) {
> >>  					/* Override any existing binding, if another one
> > 
> > By trying to setup the multicast interface scoped routes by default I
> > just found a bug in this patch essentially breaking ipv6 multicast. I
> > overlooked that ipv6_addr_type strips off the scopes, thus my check if
> > a multicast address needs a scope_id always returns true. I'll check
> > if I can convert the ipv6_addr_type calls to __ipv6_addr_type and will
> > reroll the patch.  Sorry, my tests were too focused on interface/local
> > multicast. :(
> 
> I'd always thought of adding helper inlines like these in net/ipv6.h:
> 
> static inline bool ipv6_addr_linklocal(const struct in6_addr *a)
> {
>         return ((a->s6_addr32[0] & htonl(0xFFC00000)) == htonl(0xFE800000));
> }
> 
> static inline bool ipv6_addr_mc_linklocal(const struct in6_addr *a)
> {
>         return (((a->s6_addr32[0] & htonl(0xFF000000)) == htonl(0xFF000000)) &&
>                 ((a->s6_addr32[1] & 0x0F) == IPV6_ADDR_SCOPE_LINKLOCAL));
> }
> 
> Maybe something like that would help here?
> 
> When I saw this in patch 3/3 it just seemed like the long way to determine if
> the address was a link-local multicast:
> 
> 	!__ipv6_addr_needs_scope_id(__ipv6_addr_type(&hdr->daddr))
> 
> The helper isn't as generic as your patch, but more direct.

Yup, that would have prevented the bug. My idea was to introduce an
opaque type to have compiler warnings on misuse of addr_type. I'll have a look
later today on how to proceed with this patch. Thanks!

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
YOSHIFUJI Hideaki / 吉藤英明 - Feb. 13, 2013, 4:47 p.m.
Brian Haley wrote:
> On 02/12/2013 07:13 PM, Hannes Frederic Sowa wrote:
>>> --- a/net/ipv6/af_inet6.c
>>> +++ b/net/ipv6/af_inet6.c
>>> @@ -323,7 +323,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>>>  			struct net_device *dev = NULL;
>>>  
>>>  			rcu_read_lock();
>>> -			if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>> +			if (__ipv6_addr_needs_scope_id(addr_type)) {
>>>  				if (addr_len >= sizeof(struct sockaddr_in6) &&
>>>  				    addr->sin6_scope_id) {
>>>  					/* Override any existing binding, if another one
>>
>> By trying to setup the multicast interface scoped routes by default I
>> just found a bug in this patch essentially breaking ipv6 multicast. I
>> overlooked that ipv6_addr_type strips off the scopes, thus my check if
>> a multicast address needs a scope_id always returns true. I'll check
>> if I can convert the ipv6_addr_type calls to __ipv6_addr_type and will
>> reroll the patch.  Sorry, my tests were too focused on interface/local
>> multicast. :(
> 
> I'd always thought of adding helper inlines like these in net/ipv6.h:
> 
> static inline bool ipv6_addr_linklocal(const struct in6_addr *a)
> {
>         return ((a->s6_addr32[0] & htonl(0xFFC00000)) == htonl(0xFE800000));
> }
> 
> static inline bool ipv6_addr_mc_linklocal(const struct in6_addr *a)
> {
>         return (((a->s6_addr32[0] & htonl(0xFF000000)) == htonl(0xFF000000)) &&
>                 ((a->s6_addr32[1] & 0x0F) == IPV6_ADDR_SCOPE_LINKLOCAL));
> }
> 
> Maybe something like that would help here?

If you have several address checks around, please use ipv6_addr_type()
(or __ipv6_addr_type()).  Above "direct" checks should be used only for
single-shot test.  But well, I have to agree that ipv6_addr_type and
friends is becoming complex.  In mid-term, I would like to take look
at it.  I might think of having addr_type for src/dst in skb->cb
after all.

--yoshfuji
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa - Feb. 13, 2013, 5:21 p.m.
On Thu, Feb 14, 2013 at 01:47:47AM +0900, YOSHIFUJI Hideaki wrote:
> If you have several address checks around, please use ipv6_addr_type()
> (or __ipv6_addr_type()).  Above "direct" checks should be used only for
> single-shot test.  But well, I have to agree that ipv6_addr_type and
> friends is becoming complex.  In mid-term, I would like to take look
> at it.  I might think of having addr_type for src/dst in skb->cb
> after all.

Yes, I had no plan to duplicate addrconf_core tests. I just thought about

a) not using addr_types at all in the new helper functions
b) use something like

union ipv6_addr_type {
	struct {
		__u16 scope;
		__u16 addr_type;
	};
	__u32 type_and_scope;
}

and explicitly use this in __ipv6_addr_type (this function does not seam to be
used as often as ipv6_addr_type).

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 6b793bf..f56277f 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -323,7 +323,7 @@  int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 			struct net_device *dev = NULL;
 
 			rcu_read_lock();
-			if (addr_type & IPV6_ADDR_LINKLOCAL) {
+			if (__ipv6_addr_needs_scope_id(addr_type)) {
 				if (addr_len >= sizeof(struct sockaddr_in6) &&
 				    addr->sin6_scope_id) {
 					/* Override any existing binding, if another one
@@ -471,8 +471,8 @@  int inet6_getname(struct socket *sock, struct sockaddr *uaddr,
 
 		sin->sin6_port = inet->inet_sport;
 	}
-	if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
-		sin->sin6_scope_id = sk->sk_bound_dev_if;
+	sin->sin6_scope_id = ipv6_iface_scope_id(&sin->sin6_addr,
+						 sk->sk_bound_dev_if);
 	*uaddr_len = sizeof(*sin);
 	return 0;
 }
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index f5a5478..e356022 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -124,7 +124,7 @@  ipv4_connected:
 		goto out;
 	}
 
-	if (addr_type&IPV6_ADDR_LINKLOCAL) {
+	if (__ipv6_addr_needs_scope_id(addr_type)) {
 		if (addr_len >= sizeof(struct sockaddr_in6) &&
 		    usin->sin6_scope_id) {
 			if (sk->sk_bound_dev_if &&
@@ -355,18 +355,19 @@  int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len)
 		sin->sin6_family = AF_INET6;
 		sin->sin6_flowinfo = 0;
 		sin->sin6_port = serr->port;
-		sin->sin6_scope_id = 0;
 		if (skb->protocol == htons(ETH_P_IPV6)) {
 			const struct ipv6hdr *ip6h = container_of((struct in6_addr *)(nh + serr->addr_offset),
 								  struct ipv6hdr, daddr);
 			sin->sin6_addr = ip6h->daddr;
 			if (np->sndflow)
 				sin->sin6_flowinfo = ip6_flowinfo(ip6h);
-			if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
-				sin->sin6_scope_id = IP6CB(skb)->iif;
+			sin->sin6_scope_id =
+				ipv6_iface_scope_id(&sin->sin6_addr,
+						    IP6CB(skb)->iif);
 		} else {
 			ipv6_addr_set_v4mapped(*(__be32 *)(nh + serr->addr_offset),
 					       &sin->sin6_addr);
+			sin->sin6_scope_id = 0;
 		}
 	}
 
@@ -376,18 +377,19 @@  int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len)
 	if (serr->ee.ee_origin != SO_EE_ORIGIN_LOCAL) {
 		sin->sin6_family = AF_INET6;
 		sin->sin6_flowinfo = 0;
-		sin->sin6_scope_id = 0;
 		if (skb->protocol == htons(ETH_P_IPV6)) {
 			sin->sin6_addr = ipv6_hdr(skb)->saddr;
 			if (np->rxopt.all)
 				ip6_datagram_recv_ctl(sk, msg, skb);
-			if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
-				sin->sin6_scope_id = IP6CB(skb)->iif;
+			sin->sin6_scope_id =
+				ipv6_iface_scope_id(&sin->sin6_addr,
+						    IP6CB(skb)->iif);
 		} else {
 			struct inet_sock *inet = inet_sk(sk);
 
 			ipv6_addr_set_v4mapped(ip_hdr(skb)->saddr,
 					       &sin->sin6_addr);
+			sin->sin6_scope_id = 0;
 			if (inet->cmsg_flags)
 				ip_cmsg_recv(msg, skb);
 		}
@@ -653,7 +655,7 @@  int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
 					rcu_read_unlock();
 					return -ENODEV;
 				}
-			} else if (addr_type & IPV6_ADDR_LINKLOCAL) {
+			} else if (__ipv6_addr_needs_scope_id(addr_type)) {
 				rcu_read_unlock();
 				return -EINVAL;
 			}
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index fff5bdd..71b900c 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -434,7 +434,7 @@  void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info)
 	 *	Source addr check
 	 */
 
-	if (addr_type & IPV6_ADDR_LINKLOCAL)
+	if (__ipv6_addr_needs_scope_id(addr_type))
 		iif = skb->dev->ifindex;
 
 	/*
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index b386a2c..9f1020b 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -170,10 +170,8 @@  void inet6_csk_addr2sockaddr(struct sock *sk, struct sockaddr * uaddr)
 	sin6->sin6_port	= inet_sk(sk)->inet_dport;
 	/* We do not store received flowlabel for TCP */
 	sin6->sin6_flowinfo = 0;
-	sin6->sin6_scope_id = 0;
-	if (sk->sk_bound_dev_if &&
-	    ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL)
-		sin6->sin6_scope_id = sk->sk_bound_dev_if;
+	sin6->sin6_scope_id = ipv6_iface_scope_id(&sin6->sin6_addr,
+						  sk->sk_bound_dev_if);
 }
 
 EXPORT_SYMBOL_GPL(inet6_csk_addr2sockaddr);
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 70fa814..0a540f2 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -264,7 +264,7 @@  static int rawv6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	if (addr_type != IPV6_ADDR_ANY) {
 		struct net_device *dev = NULL;
 
-		if (addr_type & IPV6_ADDR_LINKLOCAL) {
+		if (__ipv6_addr_needs_scope_id(addr_type)) {
 			if (addr_len >= sizeof(struct sockaddr_in6) &&
 			    addr->sin6_scope_id) {
 				/* Override any existing binding, if another
@@ -499,9 +499,8 @@  static int rawv6_recvmsg(struct kiocb *iocb, struct sock *sk,
 		sin6->sin6_port = 0;
 		sin6->sin6_addr = ipv6_hdr(skb)->saddr;
 		sin6->sin6_flowinfo = 0;
-		sin6->sin6_scope_id = 0;
-		if (ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL)
-			sin6->sin6_scope_id = IP6CB(skb)->iif;
+		sin6->sin6_scope_id = ipv6_iface_scope_id(&sin6->sin6_addr,
+							  IP6CB(skb)->iif);
 	}
 
 	sock_recv_ts_and_drops(msg, sk, skb);
@@ -803,7 +802,7 @@  static int rawv6_sendmsg(struct kiocb *iocb, struct sock *sk,
 
 		if (addr_len >= sizeof(struct sockaddr_in6) &&
 		    sin6->sin6_scope_id &&
-		    ipv6_addr_type(daddr)&IPV6_ADDR_LINKLOCAL)
+		    __ipv6_addr_needs_scope_id(ipv6_addr_scope(daddr)))
 			fl6.flowi6_oif = sin6->sin6_scope_id;
 	} else {
 		if (sk->sk_state != TCP_ESTABLISHED)
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 599e1ba6..ba739d9 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -450,15 +450,16 @@  try_again:
 		sin6->sin6_family = AF_INET6;
 		sin6->sin6_port = udp_hdr(skb)->source;
 		sin6->sin6_flowinfo = 0;
-		sin6->sin6_scope_id = 0;
 
-		if (is_udp4)
+		if (is_udp4) {
 			ipv6_addr_set_v4mapped(ip_hdr(skb)->saddr,
 					       &sin6->sin6_addr);
-		else {
+			sin6->sin6_scope_id = 0;
+		} else {
 			sin6->sin6_addr = ipv6_hdr(skb)->saddr;
-			if (ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL)
-				sin6->sin6_scope_id = IP6CB(skb)->iif;
+			sin6->sin6_scope_id =
+				ipv6_iface_scope_id(&sin6->sin6_addr,
+						    IP6CB(skb)->iif);
 		}
 
 	}
@@ -1118,7 +1119,8 @@  do_udp_sendmsg:
 
 		if (addr_len >= sizeof(struct sockaddr_in6) &&
 		    sin6->sin6_scope_id &&
-		    ipv6_addr_type(daddr)&IPV6_ADDR_LINKLOCAL)
+		    __ipv6_addr_needs_scope_id(
+			    ipv6_addr_scope(&sin6->sin6_addr)))
 			fl6.flowi6_oif = sin6->sin6_scope_id;
 	} else {
 		if (sk->sk_state != TCP_ESTABLISHED)