Re: [PATCH net-next v2 5/5] ipv6: route: dissect flow in input path if fib rules need it

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2018-02-27 at 19:52 -0800, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> Dissect flow in fwd path if fib rules require it. Controlled by
> a flag to avoid penatly for the common case. Flag is set when fib
> rules with sport, dport and proto match that require flow dissect
> are installed. Also passes the dissected hash keys to the multipath
> hash function when applicable to avoid dissecting the flow again.
> icmp packets will continue to use inner header for hash
> calculations.
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  include/net/ip6_fib.h    | 25 +++++++++++++++++++++++++
>  include/net/ip6_route.h  |  4 +++-
>  include/net/netns/ipv6.h |  3 ++-
>  net/ipv6/fib6_rules.c    | 16 ++++++++++++++++
>  net/ipv6/icmp.c          |  2 +-
>  net/ipv6/route.c         | 34 +++++++++++++++++++++++++---------
>  6 files changed, 72 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 34ec321d..8d906a3 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -415,6 +415,24 @@ void fib6_rules_cleanup(void);
>  bool fib6_rule_default(const struct fib_rule *rule);
>  int fib6_rules_dump(struct net *net, struct notifier_block *nb);
>  unsigned int fib6_rules_seq_read(struct net *net);
> +
> +static inline bool fib6_rules_early_flow_dissect(struct net *net,
> +						 struct sk_buff *skb,
> +						 struct flowi6 *fl6,
> +						 struct flow_keys *flkeys)
> +{
> +	unsigned int flag = FLOW_DISSECTOR_F_STOP_AT_ENCAP;
> +
> +	if (!net->ipv6.fib6_rules_require_fldissect)
> +		return false;
> +
> +	skb_flow_dissect_flow_keys(skb, flkeys, flag);
> +	fl6->fl6_sport = flkeys->ports.src;
> +	fl6->fl6_dport = flkeys->ports.dst;
> +	fl6->flowi6_proto = flkeys->basic.ip_proto;
> +
> +	return true;
> +}
>  #else
>  static inline int               fib6_rules_init(void)
>  {
> @@ -436,5 +454,12 @@ static inline unsigned int fib6_rules_seq_read(struct net *net)
>  {
>  	return 0;
>  }
> +static inline bool fib6_rules_early_flow_dissect(struct net *net,
> +						 struct sk_buff *skb,
> +						 struct flowi6 *fl6,
> +						 struct flow_keys *flkeys)
> +{
> +	return false;
> +}
>  #endif
>  #endif
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 27d23a6..da2bde5 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -127,7 +127,8 @@ static inline int ip6_route_get_saddr(struct net *net, struct rt6_info *rt,
>  
>  struct rt6_info *rt6_lookup(struct net *net, const struct in6_addr *daddr,
>  			    const struct in6_addr *saddr, int oif, int flags);
> -u32 rt6_multipath_hash(const struct flowi6 *fl6, const struct sk_buff *skb);
> +u32 rt6_multipath_hash(const struct flowi6 *fl6, const struct sk_buff *skb,
> +		       struct flow_keys *hkeys);
>  
>  struct dst_entry *icmp6_dst_alloc(struct net_device *dev, struct flowi6 *fl6);
>  
> @@ -266,4 +267,5 @@ static inline bool rt6_duplicate_nexthop(struct rt6_info *a, struct rt6_info *b)
>  	       ipv6_addr_equal(&a->rt6i_gateway, &b->rt6i_gateway) &&
>  	       !lwtunnel_cmp_encap(a->dst.lwtstate, b->dst.lwtstate);
>  }
> +
>  #endif
> diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
> index 987cc45..2b91942 100644
> --- a/include/net/netns/ipv6.h
> +++ b/include/net/netns/ipv6.h
> @@ -71,7 +71,8 @@ struct netns_ipv6 {
>  	unsigned int		 ip6_rt_gc_expire;
>  	unsigned long		 ip6_rt_last_gc;
>  #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> -	bool			 fib6_has_custom_rules;
> +	unsigned int		fib6_rules_require_fldissect;
> +	bool			fib6_has_custom_rules;
>  	struct rt6_info         *ip6_prohibit_entry;
>  	struct rt6_info         *ip6_blk_hole_entry;
>  	struct fib6_table       *fib6_local_tbl;
> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index bcd1f22..04e5f52 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -269,12 +269,26 @@ static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
>  	rule6->dst.plen = frh->dst_len;
>  	rule6->tclass = frh->tos;
>  
> +	if (fib_rule_requires_fldissect(rule))
> +		net->ipv6.fib6_rules_require_fldissect++;
> +
>  	net->ipv6.fib6_has_custom_rules = true;
>  	err = 0;
>  errout:
>  	return err;
>  }
>  
> +static int fib6_rule_delete(struct fib_rule *rule)
> +{
> +	struct net *net = rule->fr_net;
> +
> +	if (net->ipv6.fib6_rules_require_fldissect &&
> +	    fib_rule_requires_fldissect(rule))
> +		net->ipv6.fib6_rules_require_fldissect--;
> +
> +	return 0;
> +}
> +
>  static int fib6_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh,
>  			     struct nlattr **tb)
>  {
> @@ -334,6 +348,7 @@ static const struct fib_rules_ops __net_initconst fib6_rules_ops_template = {
>  	.match			= fib6_rule_match,
>  	.suppress		= fib6_rule_suppress,
>  	.configure		= fib6_rule_configure,
> +	.delete			= fib6_rule_delete,
>  	.compare		= fib6_rule_compare,
>  	.fill			= fib6_rule_fill,
>  	.nlmsg_payload		= fib6_rule_nlmsg_payload,
> @@ -361,6 +376,7 @@ static int __net_init fib6_rules_net_init(struct net *net)
>  		goto out_fib6_rules_ops;
>  
>  	net->ipv6.fib6_rules_ops = ops;
> +	net->ipv6.fib6_rules_require_fldissect = 0;
>  out:
>  	return err;
>  
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index 4fa4f1b..b0778d3 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -522,7 +522,7 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
>  	fl6.fl6_icmp_type = type;
>  	fl6.fl6_icmp_code = code;
>  	fl6.flowi6_uid = sock_net_uid(net, NULL);
> -	fl6.mp_hash = rt6_multipath_hash(&fl6, skb);
> +	fl6.mp_hash = rt6_multipath_hash(&fl6, skb, NULL);
>  	security_skb_classify_flow(skb, flowi6_to_flowi(&fl6));
>  
>  	sk = icmpv6_xmit_lock(net);
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index aa709b6..e2bb408 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -460,7 +460,7 @@ static struct rt6_info *rt6_multipath_select(struct rt6_info *match,
>  	 * case it will always be non-zero. Otherwise now is the time to do it.
>  	 */
>  	if (!fl6->mp_hash)
> -		fl6->mp_hash = rt6_multipath_hash(fl6, NULL);
> +		fl6->mp_hash = rt6_multipath_hash(fl6, NULL, NULL);
>  
>  	if (fl6->mp_hash <= atomic_read(&match->rt6i_nh_upper_bound))
>  		return match;
> @@ -1786,10 +1786,12 @@ struct dst_entry *ip6_route_input_lookup(struct net *net,
>  EXPORT_SYMBOL_GPL(ip6_route_input_lookup);
>  
>  static void ip6_multipath_l3_keys(const struct sk_buff *skb,
> -				  struct flow_keys *keys)
> +				  struct flow_keys *keys,
> +				  struct flow_keys *flkeys)
>  {
>  	const struct ipv6hdr *outer_iph = ipv6_hdr(skb);
>  	const struct ipv6hdr *key_iph = outer_iph;
> +	struct flow_keys *_flkeys = flkeys;
>  	const struct ipv6hdr *inner_iph;
>  	const struct icmp6hdr *icmph;
>  	struct ipv6hdr _inner_iph;
> @@ -1811,22 +1813,31 @@ static void ip6_multipath_l3_keys(const struct sk_buff *skb,
>  		goto out;
>  
>  	key_iph = inner_iph;
> +	_flkeys = NULL;
>  out:
>  	memset(keys, 0, sizeof(*keys));
>  	keys->control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> -	keys->addrs.v6addrs.src = key_iph->saddr;
> -	keys->addrs.v6addrs.dst = key_iph->daddr;
> -	keys->tags.flow_label = ip6_flowinfo(key_iph);
> -	keys->basic.ip_proto = key_iph->nexthdr;
> +	if (_flkeys) {
> +		keys->addrs.v6addrs.src = _flkeys->addrs.v6addrs.src;
> +		keys->addrs.v6addrs.dst = _flkeys->addrs.v6addrs.dst;
> +		keys->tags.flow_label = _flkeys->tags.flow_label;
> +		keys->basic.ip_proto = _flkeys->basic.ip_proto;
> +	} else {
> +		keys->addrs.v6addrs.src = key_iph->saddr;
> +		keys->addrs.v6addrs.dst = key_iph->daddr;
> +		keys->tags.flow_label = ip6_flowinfo(key_iph);
> +		keys->basic.ip_proto = key_iph->nexthdr;
> +	}
>  }
>  
>  /* if skb is set it will be used and fl6 can be NULL */
> -u32 rt6_multipath_hash(const struct flowi6 *fl6, const struct sk_buff *skb)
> +u32 rt6_multipath_hash(const struct flowi6 *fl6, const struct sk_buff *skb,
> +		       struct flow_keys *flkeys)
>  {
>  	struct flow_keys hash_keys;
>  
>  	if (skb) {
> -		ip6_multipath_l3_keys(skb, &hash_keys);
> +		ip6_multipath_l3_keys(skb, &hash_keys, flkeys);
>  		return flow_hash_from_keys(&hash_keys) >> 1;
>  	}
>  
> @@ -1847,12 +1858,17 @@ void ip6_route_input(struct sk_buff *skb)
>  		.flowi6_mark = skb->mark,
>  		.flowi6_proto = iph->nexthdr,
>  	};
> +	struct flow_keys *flkeys = NULL, _flkeys;
>  
>  	tun_info = skb_tunnel_info(skb);
>  	if (tun_info && !(tun_info->mode & IP_TUNNEL_INFO_TX))
>  		fl6.flowi6_tun_key.tun_id = tun_info->key.tun_id;
> +
> +	if (fib6_rules_early_flow_dissect(net, skb, &fl6, &_flkeys))
> +		flkeys = &_flkeys;
> +
>  	if (unlikely(fl6.flowi6_proto == IPPROTO_ICMPV6))
> -		fl6.mp_hash = rt6_multipath_hash(&fl6, skb);
> +		fl6.mp_hash = rt6_multipath_hash(&fl6, skb, flkeys);
>  	skb_dst_drop(skb);
>  	skb_dst_set(skb, ip6_route_input_lookup(net, skb->dev, &fl6, flags));
>  }

LGTM

Acked-by: Paolo Abeni <pabeni@redhat.com>