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

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 19 Nov 2021 09:06:01 +0100
From:   Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
        antony.antony@...unet.com, davem@...emloft.net, kuba@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] xfrm: rework default policy structure

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@...nd.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;
}

...
static inline int __xfrm_policy_check2(struct sock *sk, int dir,
...
        return __xfrm_check_nopolicy(net, skb, dir) ||
               (skb_dst(skb) && (skb_dst(skb)->flags & DST_NOPOLICY)) ||
               __xfrm_policy_check(sk, ndir, skb, family);

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