netdev - Re: [PATCH v3] vxlan: Allow setting destination to unicast address.

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: <87a9oyr65c.wl%atzm@stratosphere.co.jp>
Date:	Tue, 16 Apr 2013 21:50:39 +0900
From:	Atzm Watanabe <atzm@...atosphere.co.jp>
To:	David Stevens <dlstevens@...ibm.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	netdev-owner@...r.kernel.org,
	Stephen Hemminger <stephen@...workplumber.org>,
	Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [PATCH v3] vxlan: Allow setting destination to unicast address.

At Mon, 15 Apr 2013 16:53:35 -0400,
David Stevens wrote:
> 
> netdev-owner@...r.kernel.org wrote on 04/12/2013 05:09:05 AM:
> 
> > From: Atzm Watanabe <atzm@...atosphere.co.jp>
> 
> > Signed-off-by: Atzm Watanabe <atzm@...atosphere.co.jp>
> > ---
> >  drivers/net/vxlan.c          | 45 +++++++++++++++++++++++++++++++++
> > +----------
> >  include/uapi/linux/if_link.h |  1 +
> >  2 files changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index 9a64715..e468d5a 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -106,7 +106,7 @@ struct vxlan_dev {
> >     struct hlist_node hlist;
> >     struct net_device *dev;
> >     __u32        vni;      /* virtual network id */
> > -   __be32             gaddr;   /* multicast group */
> > +   __be32             daddr;   /* destination address */
> >     __be32        saddr;   /* source address */
> >     unsigned int      link;      /* link to multicast over */
> >     __u16        port_min;   /* source port range */
> 
>         This is effectively a default forwarding destination when
> there is no matching fdb entry. As such, I think a fully general
> patch would convert this to a "vxlan_rdst" and thereby allow alternate
> port, vni, specific outgoing interface and multiple remote destinations
> for copies, as via the fdb. And, in the future, support for this being
> an IPv6 destination, as well. I think the name here, "daddr", ought to
> reflect some of this too -- maybe "default_dst" or some such?

Yes, right.  I'll replace "gaddr" with "default_dst" as vxlan_rdst.

But to support multiple default destinations, it will need more fixes
like Mike Rapoport's patch.  Also it will need careful consideration
about VNI...  I'll try to fix them to support multiple default
destinations, after the patch is merged.


> > @@ -1322,6 +1322,7 @@ static const struct nla_policy vxlan_policy
> > [IFLA_VXLAN_MAX + 1] = {
> >     [IFLA_VXLAN_RSC]   = { .type = NLA_U8 },
> >     [IFLA_VXLAN_L2MISS]   = { .type = NLA_U8 },
> >     [IFLA_VXLAN_L3MISS]   = { .type = NLA_U8 },
> > +   [IFLA_VXLAN_REMOTE]   = { .len = FIELD_SIZEOF(struct iphdr, daddr) 
> },
> >  };
> > 
> >  static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[])
> 
> I like the name change from "GROUP". "REMOTE" is ok, but might consider
> "IFLA_VXLAN_DEFAULT_DST" or some such.

Hmm...  I think that "IFLA_VXLAN_DEFAULT_DST" might be confused with
vxlan_rdst.  So I'll just replace "GROUP" with "REMOTE".

But in the future, to support multiple default destinations,
"DEFAULT_DST" attribute might be used to add a destination entry :)


> > @@ -1347,6 +1348,9 @@ static int vxlan_validate(struct nlattr *tb[],
> > struct nlattr *data[])
> >           return -ERANGE;
> >     }
> > 
> > +   if (data[IFLA_VXLAN_GROUP] && data[IFLA_VXLAN_REMOTE])
> > +      return -EINVAL;
> > +
> >     if (data[IFLA_VXLAN_GROUP]) {
> >        __be32 gaddr = nla_get_be32(data[IFLA_VXLAN_GROUP]);
> >        if (!IN_MULTICAST(ntohl(gaddr))) {
> > @@ -1355,6 +1359,14 @@ static int vxlan_validate(struct nlattr *tb
> > [], struct nlattr *data[])
> >        }
> >     }
> > 
> > +   if (data[IFLA_VXLAN_REMOTE]) {
> > +      __be32 daddr = nla_get_be32(data[IFLA_VXLAN_REMOTE]);
> > +      if (IN_MULTICAST(ntohl(daddr))) {
> > +         pr_debug("remote address is not IPv4 unicast\n");
> > +         return -EADDRNOTAVAIL;
> > +      }
> > +   }
> > +
> >     if (data[IFLA_VXLAN_PORT_RANGE]) {
> >        const struct ifla_vxlan_port_range *p
> >           = nla_data(data[IFLA_VXLAN_PORT_RANGE]);
> 
> If this is turned into a vxlan_rdst, it should check for other
> attributes to fill that-- PORT, VNI, IFINDEX-- with approprate
> default values if not present.
> 
> Overall, this patch looks ok to me as far as it goes, but I think
> it'd be more useful if it were more generic; that is, if it supported
> everything an fdb can.

Thank you for the reviewing!
I'll fix that mentioned above and resend patches.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html