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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 4 May 2020 21:23:32 -0600
From:   David Ahern <dsahern@...il.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, nikolay@...ulusnetworks.com,
        idosch@...lanox.com, jiri@...lanox.com, petrm@...lanox.com
Subject: Re: [RFC PATCH net-next 1/5] nexthop: support for fdb ecmp nexthops

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.

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

> +{
> +	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.

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

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