Re: [PATCH net-next] xfrm: rework default policy structure

[Leon Romanovsky <leon@kernel.org>]

On Thu, Nov 18, 2021 at 03:29:37PM +0100, Nicolas Dichtel wrote:
> This is a follow up of commit f8d858e607b2 ("xfrm: make user policy API
> complete"). The goal is to align userland API to the internal structures.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> 
> This patch targets ipsec-next, but because ipsec-next has not yet been
> rebased on top of net-next, I based the patch on top of net-next.
> 
>  include/net/netns/xfrm.h |  6 +-----
>  include/net/xfrm.h       | 38 ++++++++---------------------------
>  net/xfrm/xfrm_policy.c   | 10 +++++++---
>  net/xfrm/xfrm_user.c     | 43 +++++++++++++++++-----------------------
>  4 files changed, 34 insertions(+), 63 deletions(-)
> 
> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
> index 947733a639a6..bd7c3be4af5d 100644
> --- a/include/net/netns/xfrm.h
> +++ b/include/net/netns/xfrm.h
> @@ -66,11 +66,7 @@ struct netns_xfrm {
>  	int			sysctl_larval_drop;
>  	u32			sysctl_acq_expires;
>  
> -	u8			policy_default;
> -#define XFRM_POL_DEFAULT_IN	1
> -#define XFRM_POL_DEFAULT_OUT	2
> -#define XFRM_POL_DEFAULT_FWD	4
> -#define XFRM_POL_DEFAULT_MASK	7
> +	u8			policy_default[XFRM_POLICY_MAX];
>  
>  #ifdef CONFIG_SYSCTL
>  	struct ctl_table_header	*sysctl_hdr;
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 2308210793a0..3fd1e052927e 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1075,22 +1075,6 @@ xfrm_state_addr_cmp(const struct xfrm_tmpl *tmpl, const struct xfrm_state *x, un
>  }
>  
>  #ifdef CONFIG_XFRM
> -static inline bool
> -xfrm_default_allow(struct net *net, int dir)
> -{
> -	u8 def = net->xfrm.policy_default;
> -
> -	switch (dir) {
> -	case XFRM_POLICY_IN:
> -		return def & XFRM_POL_DEFAULT_IN ? false : true;
> -	case XFRM_POLICY_OUT:
> -		return def & XFRM_POL_DEFAULT_OUT ? false : true;
> -	case XFRM_POLICY_FWD:
> -		return def & XFRM_POL_DEFAULT_FWD ? false : true;
> -	}
> -	return false;
> -}
> -
>  int __xfrm_policy_check(struct sock *, int dir, struct sk_buff *skb,
>  			unsigned short family);
>  
> @@ -1104,13 +1088,10 @@ static inline int __xfrm_policy_check2(struct sock *sk, int dir,
>  	if (sk && sk->sk_policy[XFRM_POLICY_IN])
>  		return __xfrm_policy_check(sk, ndir, skb, family);
>  
> -	if (xfrm_default_allow(net, dir))
> -		return (!net->xfrm.policy_count[dir] && !secpath_exists(skb)) ||
> -		       (skb_dst(skb) && (skb_dst(skb)->flags & DST_NOPOLICY)) ||
> -		       __xfrm_policy_check(sk, ndir, skb, family);
> -	else
> -		return (skb_dst(skb) && (skb_dst(skb)->flags & DST_NOPOLICY)) ||
> -		       __xfrm_policy_check(sk, ndir, skb, family);
> +	return (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_ACCEPT &&
> +		(!net->xfrm.policy_count[dir] && !secpath_exists(skb))) ||
> +	       (skb_dst(skb) && (skb_dst(skb)->flags & DST_NOPOLICY)) ||
> +	       __xfrm_policy_check(sk, ndir, skb, family);
>  }

This is completely unreadable. What is the advantage of writing like this?

>  
>  static inline int xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, unsigned short family)
> @@ -1162,13 +1143,10 @@ static inline int xfrm_route_forward(struct sk_buff *skb, unsigned short family)
>  {
>  	struct net *net = dev_net(skb->dev);
>  
> -	if (xfrm_default_allow(net, XFRM_POLICY_FWD))
> -		return !net->xfrm.policy_count[XFRM_POLICY_OUT] ||
> -			(skb_dst(skb)->flags & DST_NOXFRM) ||
> -			__xfrm_route_forward(skb, family);
> -	else
> -		return (skb_dst(skb)->flags & DST_NOXFRM) ||
> -			__xfrm_route_forward(skb, family);
> +	return (net->xfrm.policy_default[XFRM_POLICY_FWD] == XFRM_USERPOLICY_ACCEPT &&
> +		!net->xfrm.policy_count[XFRM_POLICY_OUT]) ||
> +	       (skb_dst(skb)->flags & DST_NOXFRM) ||
> +	       __xfrm_route_forward(skb, family);

Ditto.

Thanks