Patchwork [net-next,v4,4/5] vxlan: add scope_id support for ll addr

login
register
mail settings
Submitter Amerigo Wang
Date April 17, 2013, 5:10 a.m.
Message ID <1366175423-27310-5-git-send-email-amwang@redhat.com>
Download mbox | patch
Permalink /patch/237150/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Amerigo Wang - April 17, 2013, 5:10 a.m.
From: Cong Wang <amwang@redhat.com>

As David suggested, we should support ll addr, which requires
scope id.

Cc: David Stevens <dlstevens@us.ibm.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 drivers/net/vxlan.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)
Amerigo Wang - April 18, 2013, 1:36 p.m.
On Wed, 2013-04-17 at 13:10 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> As David suggested, we should support ll addr, which requires
> scope id.

David, any comments on this patch? I am not sure it is correct.


--
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
David Stevens - April 19, 2013, 11:14 a.m.
Cong Wang <amwang@redhat.com> wrote on 04/17/2013 01:10:21 AM:
 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 43ed40f..531c5e2 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -92,9 +92,10 @@ struct vxlan_addr {
>        struct sockaddr_in6   sin6;
>        struct sockaddr      sa;
>     } u;
> -#define va_sin  u.sin.sin_addr.s_addr
> -#define va_sin6 u.sin6.sin6_addr
> -#define va_sa   u.sa.sa_family
> +#define va_sin      u.sin.sin_addr.s_addr
> +#define va_sin6      u.sin6.sin6_addr
> +#define va_scope_id   u.sin6.sin6_scope_id
> +#define va_sa      u.sa.sa_family
>  };

        As I commented before, you're obscuring the types here,
which makes it less readable. "va_sin6" ought to be a sockaddr_in6,
not a sin6_addr, and the scope id ought to then be 
"va_sin6.sin6_scope_id",
without any other #define necessary for it.



> @@ -1708,6 +1712,17 @@ static int vxlan_newlink(struct net *net, 
> struct net_device *dev,
>        /* update header length based on lower device */
>        dev->hard_header_len = lowerdev->hard_header_len +
>                     VXLAN_HEADROOM;
> +#if IS_ENABLED(CONFIG_IPV6)
> +      dst->remote_ip.va_scope_id = ipv6_iface_scope_id
> (&dst->remote_ip.va_sin6,
> +                         dst->remote_ifindex);
> +      if (ipv6_addr_type(&dst->remote_ip.va_sin6) & 
IPV6_ADDR_LINKLOCAL) {
> +         struct vxlan_net *vn = net_generic(net, vxlan_net_id);
> +         struct sock *sk = vn->sock->sk;
> +
> +                        sk->sk_bound_dev_if = 
dst->remote_ip.va_scope_id;
> +      }
> +#endif
> +
>     }

        The socket is not used for transmits, which is where the 
sin6_scope_id is
relevant-- we need this in vxlan_xmit_one() for the route lookup. It also
permanently binds to an interface, which I believe prevents VXLAN traffic 
from
other interfaces on receive, and this socket is an INADDR_ANY-bound socket 
for
all VXLAN devices on the host.
        So, we shouldn't be doing anything with sk_bound_dev_if, but LL 
addrs need
to use the the sin6_scope_id in the route lookup for transmissions. It 
should
be saved in remote_ifindex per destination and stored in flowi6_oif for 
route
lookup, as the v4 code does now for fdb entries, and LL v6 destinations 
for fdb
entries ought to be done the same way.

        So, no, I don't think this patch is correct or complete.

 +-DLS

--
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
Amerigo Wang - April 19, 2013, 11:56 a.m.
On Fri, 2013-04-19 at 07:14 -0400, David Stevens wrote:
> Cong Wang <amwang@redhat.com> wrote on 04/17/2013 01:10:21 AM:
>  
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index 43ed40f..531c5e2 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -92,9 +92,10 @@ struct vxlan_addr {
> >        struct sockaddr_in6   sin6;
> >        struct sockaddr      sa;
> >     } u;
> > -#define va_sin  u.sin.sin_addr.s_addr
> > -#define va_sin6 u.sin6.sin6_addr
> > -#define va_sa   u.sa.sa_family
> > +#define va_sin      u.sin.sin_addr.s_addr
> > +#define va_sin6      u.sin6.sin6_addr
> > +#define va_scope_id   u.sin6.sin6_scope_id
> > +#define va_sa      u.sa.sa_family
> >  };
> 
>         As I commented before, you're obscuring the types here,
> which makes it less readable. "va_sin6" ought to be a sockaddr_in6,
> not a sin6_addr, and the scope id ought to then be 
> "va_sin6.sin6_scope_id",
> without any other #define necessary for it.

OK, so #define va_sin6  u.sin6?

> 
>         So, no, I don't think this patch is correct or complete.
> 

Then let's defer it, I need more time to make it working. I will discard
the scope_id patch for now, and make a correct one later.

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
Amerigo Wang - April 21, 2013, 1:47 p.m.
On Fri, 2013-04-19 at 07:14 -0400, David Stevens wrote:
> 
>         As I commented before, you're obscuring the types here,
> which makes it less readable. "va_sin6" ought to be a sockaddr_in6,
> not a sin6_addr, and the scope id ought to then be 
> "va_sin6.sin6_scope_id",
> without any other #define necessary for it.

So finally you want something like:

vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr

and

vxlan->default_dst.remote_ip.va_sin6.sin6_addr

Enjoy the readable code! :)

Again, I don't like this at all from the beginning! I just want to make
David happy. v5 is on the way...

For the record, please don't blame me if you think the code is ugly. :)

--
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
Bjørn Mork - April 21, 2013, 2:46 p.m.
Cong Wang <amwang@redhat.com> writes:
> On Fri, 2013-04-19 at 07:14 -0400, David Stevens wrote:
>> 
>>         As I commented before, you're obscuring the types here,
>> which makes it less readable. "va_sin6" ought to be a sockaddr_in6,
>> not a sin6_addr, and the scope id ought to then be 
>> "va_sin6.sin6_scope_id",
>> without any other #define necessary for it.
>
> So finally you want something like:
>
> vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr
>
> and
>
> vxlan->default_dst.remote_ip.va_sin6.sin6_addr
>
> Enjoy the readable code! :)

There's nothing preventing you from using local variables and/or
functions to shorten those, without obscuring things on the way.
Readability depends on more than just "short".  Obviously correct type
is more important. But you can do both.  Most kernel code does. It's not
hard.



struct vxlan_addr {
	union {
		struct sockaddr_in	sin;
		struct sockaddr_in6	sin6;
		struct sockaddr		sa;
	} u;
#define va_sin  u.sin
#define va_sin6 u.sin6
#define va_sa   u.sa
};
 
struct vxlan_addr *rip = &vxlan->default_dst.remote_ip;

rip->va_sin.sin_addr.s_addr
rip->va_sin6.sin6_addr

etc.


Bjørn
--
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
Amerigo Wang - April 21, 2013, 2:57 p.m.
On Sun, 2013-04-21 at 16:46 +0200, Bjørn Mork wrote:
> struct vxlan_addr *rip = &vxlan->default_dst.remote_ip;
> 
> rip->va_sin.sin_addr.s_addr
> rip->va_sin6.sin6_addr 

I don't see any difference between this with:

vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr
vxlan->default_dst.remote_ip.va_sin6.sin6_addr

except making two lines shorter... People still have to go into 5 levels
to get the final field.


--
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
Bjørn Mork - April 21, 2013, 3:10 p.m.
Cong Wang <amwang@redhat.com> writes:

> On Sun, 2013-04-21 at 16:46 +0200, Bjørn Mork wrote:
>> struct vxlan_addr *rip = &vxlan->default_dst.remote_ip;
>> 
>> rip->va_sin.sin_addr.s_addr
>> rip->va_sin6.sin6_addr 
>
> I don't see any difference between this with:
>
> vxlan->default_dst.remote_ip.va_sin.sin_addr.s_addr
> vxlan->default_dst.remote_ip.va_sin6.sin6_addr
>
> except making two lines shorter... People still have to go into 5 levels
> to get the final field.

Yes, of course.  That's how many levels there is in this data structure.

They still have to go into 5 levels when you define away some of those
levels.  Only difference is that they then have to _remember_ that one
of the labels actually is a macro hiding 3 levels...

You are obscuring the levels, not removing them.  That's the problem.

Noone objects to removing levels if you can do that by moving fields
around.  But if you can't, then don't try to hide it.


Bjørn
--
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/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 43ed40f..531c5e2 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -92,9 +92,10 @@  struct vxlan_addr {
 		struct sockaddr_in6	sin6;
 		struct sockaddr		sa;
 	} u;
-#define va_sin  u.sin.sin_addr.s_addr
-#define va_sin6 u.sin6.sin6_addr
-#define va_sa   u.sa.sa_family
+#define va_sin		u.sin.sin_addr.s_addr
+#define va_sin6		u.sin6.sin6_addr
+#define va_scope_id	u.sin6.sin6_scope_id
+#define va_sa		u.sa.sa_family
 };
 
 struct vxlan_rdst {
@@ -1674,6 +1675,9 @@  static int vxlan_newlink(struct net *net, struct net_device *dev,
 		nla_memcpy(&dst->remote_ip.va_sin6, data[IFLA_VXLAN_REMOTE6],
 			   sizeof(struct in6_addr));
 		dst->remote_ip.va_sa = AF_INET6;
+		if (__ipv6_addr_needs_scope_id(__ipv6_addr_type(&dst->remote_ip.va_sin6))
+			    && !data[IFLA_VXLAN_LINK])
+					return -EINVAL;
 #else
 		return -EPFNOSUPPORT;
 #endif
@@ -1708,6 +1712,17 @@  static int vxlan_newlink(struct net *net, struct net_device *dev,
 		/* update header length based on lower device */
 		dev->hard_header_len = lowerdev->hard_header_len +
 				       VXLAN_HEADROOM;
+#if IS_ENABLED(CONFIG_IPV6)
+		dst->remote_ip.va_scope_id = ipv6_iface_scope_id(&dst->remote_ip.va_sin6,
+								 dst->remote_ifindex);
+		if (ipv6_addr_type(&dst->remote_ip.va_sin6) & IPV6_ADDR_LINKLOCAL) {
+			struct vxlan_net *vn = net_generic(net, vxlan_net_id);
+			struct sock *sk = vn->sock->sk;
+
+                        sk->sk_bound_dev_if = dst->remote_ip.va_scope_id;
+		}
+#endif
+
 	}
 
 	if (data[IFLA_VXLAN_TOS])