| 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
| ||
|
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