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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 4 May 2020 20:49:12 -0700
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     David Ahern <dsahern@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Petr Machata <petrm@...lanox.com>
Subject: Re: [RFC PATCH net-next 1/5] nexthop: support for fdb ecmp nexthops

On Mon, May 4, 2020 at 8:23 PM David Ahern <dsahern@...il.com> wrote:
>
> On 5/4/20 4:28 PM, Roopa Prabhu wrote:
> >  include/net/nexthop.h        |  14 ++++++
> >  include/uapi/linux/nexthop.h |   1 +
> >  net/ipv4/nexthop.c           | 101 +++++++++++++++++++++++++++++++++----------
> >  3 files changed, 93 insertions(+), 23 deletions(-)
>
> pretty cool that you can extend this from routes to fdb entries with
> such a small change stat.

yep, exactly. everything fell into place so neatly.

>
> >
> > diff --git a/include/net/nexthop.h b/include/net/nexthop.h
> > index c440ccc..3ad4e97 100644
> > --- a/include/net/nexthop.h
> > +++ b/include/net/nexthop.h
> > @@ -26,6 +26,7 @@ struct nh_config {
> >       u8              nh_family;
> >       u8              nh_protocol;
> >       u8              nh_blackhole;
> > +     u8              nh_fdb;
> >       u32             nh_flags;
> >
> >       int             nh_ifindex;
> > @@ -52,6 +53,7 @@ struct nh_info {
> >
> >       u8                      family;
> >       bool                    reject_nh;
> > +     bool                    fdb_nh;
> >
> >       union {
> >               struct fib_nh_common    fib_nhc;
> > @@ -80,6 +82,7 @@ struct nexthop {
> >       struct rb_node          rb_node;    /* entry on netns rbtree */
> >       struct list_head        fi_list;    /* v4 entries using nh */
> >       struct list_head        f6i_list;   /* v6 entries using nh */
> > +     struct list_head        fdb_list;   /* fdb entries using this nh */
> >       struct list_head        grp_list;   /* nh group entries using this nh */
> >       struct net              *net;
> >
> > @@ -88,6 +91,7 @@ struct nexthop {
> >       u8                      protocol;   /* app managing this nh */
> >       u8                      nh_flags;
> >       bool                    is_group;
> > +     bool                    is_fdb_nh;
> >
> >       refcount_t              refcnt;
> >       struct rcu_head         rcu;
> > @@ -304,4 +308,14 @@ static inline void nexthop_path_fib6_result(struct fib6_result *res, int hash)
> >  int nexthop_for_each_fib6_nh(struct nexthop *nh,
> >                            int (*cb)(struct fib6_nh *nh, void *arg),
> >                            void *arg);
> > +
> > +static inline struct nh_info *nexthop_path_fdb(struct nexthop *nh,  u32 hash)
>
> this is used in the next patch. Any way to not leak the nh_info struct
> into vxlan code? Right now nh_info is only used in nexthop.{c,h}.
>

yes, sure I think I can do that. I will add an api to get the nh
family and use nh everywhere.

> > +{
> > +     struct nh_info *nhi;
> > +
> > +     nh = nexthop_select_path(nh, hash);
> > +     nhi = rcu_dereference(nh->nh_info);
> > +
> > +     return nhi;
> > +}
> >  #endif
> > diff --git a/include/uapi/linux/nexthop.h b/include/uapi/linux/nexthop.h
> > index 7b61867..19a234a 100644
> > --- a/include/uapi/linux/nexthop.h
> > +++ b/include/uapi/linux/nexthop.h
> > @@ -48,6 +48,7 @@ enum {
> >        */
> >       NHA_GROUPS,     /* flag; only return nexthop groups in dump */
> >       NHA_MASTER,     /* u32;  only return nexthops with given master dev */
> > +     NHA_FDB,        /* nexthop belongs to a bridge fdb */
>
> please add the 'type' to the comment; I tried to make this uapi file
> completely self-documenting. ie., no one should have to consult the code
> to know what kind of attribute NHA_FDB is.
>

yep, will do. not sure how i missed it,


> >
> >       __NHA_MAX,
> >  };
> > diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> > index 3957364..98f8d2a 100644
> > --- a/net/ipv4/nexthop.c
> > +++ b/net/ipv4/nexthop.c
> > @@ -33,6 +33,7 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
> >       [NHA_ENCAP]             = { .type = NLA_NESTED },
> >       [NHA_GROUPS]            = { .type = NLA_FLAG },
> >       [NHA_MASTER]            = { .type = NLA_U32 },
> > +     [NHA_FDB]               = { .type = NLA_FLAG },
> >  };
> >
> >  static unsigned int nh_dev_hashfn(unsigned int val)
> > @@ -107,6 +108,7 @@ static struct nexthop *nexthop_alloc(void)
> >               INIT_LIST_HEAD(&nh->fi_list);
> >               INIT_LIST_HEAD(&nh->f6i_list);
> >               INIT_LIST_HEAD(&nh->grp_list);
> > +             INIT_LIST_HEAD(&nh->fdb_list);
> >       }
> >       return nh;
> >  }
> > @@ -227,6 +229,9 @@ static int nh_fill_node(struct sk_buff *skb, struct nexthop *nh,
> >       if (nla_put_u32(skb, NHA_ID, nh->id))
> >               goto nla_put_failure;
> >
> > +     if (nh->is_fdb_nh && nla_put_flag(skb, NHA_FDB))
> > +             goto nla_put_failure;
> > +
> >       if (nh->is_group) {
> >               struct nh_group *nhg = rtnl_dereference(nh->nh_grp);
> >
> > @@ -241,7 +246,7 @@ static int nh_fill_node(struct sk_buff *skb, struct nexthop *nh,
> >               if (nla_put_flag(skb, NHA_BLACKHOLE))
> >                       goto nla_put_failure;
> >               goto out;
> > -     } else {
> > +     } else if (!nh->is_fdb_nh) {
> >               const struct net_device *dev;
> >
> >               dev = nhi->fib_nhc.nhc_dev;
> > @@ -393,6 +398,7 @@ static int nh_check_attr_group(struct net *net, struct nlattr *tb[],
> >       unsigned int len = nla_len(tb[NHA_GROUP]);
> >       struct nexthop_grp *nhg;
> >       unsigned int i, j;
> > +     u8 nhg_fdb = 0;
> >
> >       if (len & (sizeof(struct nexthop_grp) - 1)) {
> >               NL_SET_ERR_MSG(extack,
> > @@ -421,6 +427,8 @@ static int nh_check_attr_group(struct net *net, struct nlattr *tb[],
> >               }
> >       }
> >
> > +     if (tb[NHA_FDB])
> > +             nhg_fdb = 1;
> >       nhg = nla_data(tb[NHA_GROUP]);
> >       for (i = 0; i < len; ++i) {
> >               struct nexthop *nh;
> > @@ -432,11 +440,16 @@ static int nh_check_attr_group(struct net *net, struct nlattr *tb[],
> >               }
> >               if (!valid_group_nh(nh, len, extack))
> >                       return -EINVAL;
> > +             if (nhg_fdb && !nh->is_fdb_nh) {
> > +                     NL_SET_ERR_MSG(extack, "FDB Multipath group can only have fdb nexthops");
> > +                     return -EINVAL;
> > +             }
>
> you should check the reverse as well -- non-nhg_fdb can not use an fdb
> nh. ie., a group can not be a mix of fdb and route entries.
>
> Make sure the selftests covers the permutations as well.
>

yep, will do

> >       }
> >       for (i = NHA_GROUP + 1; i < __NHA_MAX; ++i) {
> >               if (!tb[i])
> >                       continue;
> > -
> > +             if (tb[NHA_FDB])
> > +                     continue;
> >               NL_SET_ERR_MSG(extack,
> >                              "No other attributes can be set in nexthop groups");
> >               return -EINVAL;
>
>

Powered by blists - more mailing lists