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

[Leon Romanovsky <leon@kernel.org>]

On Fri, Nov 19, 2021 at 09:06:01AM +0100, Nicolas Dichtel wrote:
> Le 18/11/2021 à 20:09, Leon Romanovsky a écrit :
> > 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?
> Yeah, I was hesitating. I was hoping that indentation could help.
> At the opposite, I could also arg that having two times the "nearly" same test
> is also unreadable.
> I choose to drop xfrm_default_allow() to remove the negation in
> xfrm_lookup_with_ifid():
> 
> -           !xfrm_default_allow(net, dir)) {
> +           net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) {
> 
> 
> What about:
> 
> static inline bool __xfrm_check_nopolicy(struct net *net, struct sk_buff *skb,
>                                          int dir)
> {
>         if (!net->xfrm.policy_count[dir] && !secpath_exists(skb))
>                 return net->xfrm.policy_default[dir] == XFRM_USERPOLICY_ACCEPT;
> 
>         return false;
> }

It is much better, just extra "!" is not in place.
if (!net->xfrm.policy_count[dir] ... -> if (net->xfrm.policy_count[dir] ...

Thanks