* [PATCH net-next] xfrm: rework default policy structure
@ 2021-11-18 14:29 Nicolas Dichtel
2021-11-18 19:09 ` Leon Romanovsky
0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Dichtel @ 2021-11-18 14:29 UTC (permalink / raw)
To: steffen.klassert, herbert, antony.antony
Cc: davem, kuba, netdev, Nicolas Dichtel
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);
}
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);
}
static inline int xfrm4_route_forward(struct sk_buff *skb)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 1a06585022ab..1a3bdc3521cb 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3156,7 +3156,7 @@ struct dst_entry *xfrm_lookup_with_ifid(struct net *net,
nopol:
if (!(dst_orig->dev->flags & IFF_LOOPBACK) &&
- !xfrm_default_allow(net, dir)) {
+ net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) {
err = -EPERM;
goto error;
}
@@ -3548,7 +3548,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
}
if (!pol) {
- if (!xfrm_default_allow(net, dir)) {
+ if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) {
XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
return 0;
}
@@ -3608,7 +3608,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
}
xfrm_nr = ti;
- if (!xfrm_default_allow(net, dir) && !xfrm_nr) {
+ if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK &&
+ !xfrm_nr) {
XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOSTATES);
goto reject;
}
@@ -4097,6 +4098,9 @@ static int __net_init xfrm_net_init(struct net *net)
spin_lock_init(&net->xfrm.xfrm_policy_lock);
seqcount_spinlock_init(&net->xfrm.xfrm_policy_hash_generation, &net->xfrm.xfrm_policy_lock);
mutex_init(&net->xfrm.xfrm_cfg_mutex);
+ net->xfrm.policy_default[XFRM_POLICY_IN] = XFRM_USERPOLICY_ACCEPT;
+ net->xfrm.policy_default[XFRM_POLICY_FWD] = XFRM_USERPOLICY_ACCEPT;
+ net->xfrm.policy_default[XFRM_POLICY_OUT] = XFRM_USERPOLICY_ACCEPT;
rv = xfrm_statistics_init(net);
if (rv < 0)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 7c36cc1f3d79..a13161111cf4 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1980,12 +1980,9 @@ static int xfrm_notify_userpolicy(struct net *net)
}
up = nlmsg_data(nlh);
- up->in = net->xfrm.policy_default & XFRM_POL_DEFAULT_IN ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- up->fwd = net->xfrm.policy_default & XFRM_POL_DEFAULT_FWD ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- up->out = net->xfrm.policy_default & XFRM_POL_DEFAULT_OUT ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
+ up->in = net->xfrm.policy_default[XFRM_POLICY_IN];
+ up->fwd = net->xfrm.policy_default[XFRM_POLICY_FWD];
+ up->out = net->xfrm.policy_default[XFRM_POLICY_OUT];
nlmsg_end(skb, nlh);
@@ -1996,26 +1993,26 @@ static int xfrm_notify_userpolicy(struct net *net)
return err;
}
+static bool xfrm_userpolicy_is_valid(__u8 policy)
+{
+ return policy == XFRM_USERPOLICY_BLOCK ||
+ policy == XFRM_USERPOLICY_ACCEPT;
+}
+
static int xfrm_set_default(struct sk_buff *skb, struct nlmsghdr *nlh,
struct nlattr **attrs)
{
struct net *net = sock_net(skb->sk);
struct xfrm_userpolicy_default *up = nlmsg_data(nlh);
- if (up->in == XFRM_USERPOLICY_BLOCK)
- net->xfrm.policy_default |= XFRM_POL_DEFAULT_IN;
- else if (up->in == XFRM_USERPOLICY_ACCEPT)
- net->xfrm.policy_default &= ~XFRM_POL_DEFAULT_IN;
+ if (xfrm_userpolicy_is_valid(up->in))
+ net->xfrm.policy_default[XFRM_POLICY_IN] = up->in;
- if (up->fwd == XFRM_USERPOLICY_BLOCK)
- net->xfrm.policy_default |= XFRM_POL_DEFAULT_FWD;
- else if (up->fwd == XFRM_USERPOLICY_ACCEPT)
- net->xfrm.policy_default &= ~XFRM_POL_DEFAULT_FWD;
+ if (xfrm_userpolicy_is_valid(up->fwd))
+ net->xfrm.policy_default[XFRM_POLICY_FWD] = up->fwd;
- if (up->out == XFRM_USERPOLICY_BLOCK)
- net->xfrm.policy_default |= XFRM_POL_DEFAULT_OUT;
- else if (up->out == XFRM_USERPOLICY_ACCEPT)
- net->xfrm.policy_default &= ~XFRM_POL_DEFAULT_OUT;
+ if (xfrm_userpolicy_is_valid(up->out))
+ net->xfrm.policy_default[XFRM_POLICY_OUT] = up->out;
rt_genid_bump_all(net);
@@ -2045,13 +2042,9 @@ static int xfrm_get_default(struct sk_buff *skb, struct nlmsghdr *nlh,
}
r_up = nlmsg_data(r_nlh);
-
- r_up->in = net->xfrm.policy_default & XFRM_POL_DEFAULT_IN ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- r_up->fwd = net->xfrm.policy_default & XFRM_POL_DEFAULT_FWD ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- r_up->out = net->xfrm.policy_default & XFRM_POL_DEFAULT_OUT ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
+ r_up->in = net->xfrm.policy_default[XFRM_POLICY_IN];
+ r_up->fwd = net->xfrm.policy_default[XFRM_POLICY_FWD];
+ r_up->out = net->xfrm.policy_default[XFRM_POLICY_OUT];
nlmsg_end(r_skb, r_nlh);
return nlmsg_unicast(net->xfrm.nlsk, r_skb, portid);
--
2.33.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH net-next] xfrm: rework default policy structure
2021-11-18 14:29 [PATCH net-next] xfrm: rework default policy structure Nicolas Dichtel
@ 2021-11-18 19:09 ` Leon Romanovsky
2021-11-19 8:06 ` Nicolas Dichtel
0 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2021-11-18 19:09 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: steffen.klassert, herbert, antony.antony, davem, kuba, netdev
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next] xfrm: rework default policy structure
2021-11-18 19:09 ` Leon Romanovsky
@ 2021-11-19 8:06 ` Nicolas Dichtel
2021-11-19 15:41 ` Leon Romanovsky
0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Dichtel @ 2021-11-19 8:06 UTC (permalink / raw)
To: Leon Romanovsky
Cc: steffen.klassert, herbert, antony.antony, davem, kuba, netdev
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;
}
...
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next] xfrm: rework default policy structure
2021-11-19 8:06 ` Nicolas Dichtel
@ 2021-11-19 15:41 ` Leon Romanovsky
2021-11-19 17:31 ` Nicolas Dichtel
0 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2021-11-19 15:41 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: steffen.klassert, herbert, antony.antony, davem, kuba, netdev
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next] xfrm: rework default policy structure
2021-11-19 15:41 ` Leon Romanovsky
@ 2021-11-19 17:31 ` Nicolas Dichtel
2021-11-21 14:07 ` Leon Romanovsky
0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Dichtel @ 2021-11-19 17:31 UTC (permalink / raw)
To: Leon Romanovsky
Cc: steffen.klassert, herbert, antony.antony, davem, kuba, netdev
Le 19/11/2021 à 16:41, Leon Romanovsky a écrit :
[snip]
>> 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.
Ok, I will send a v2 with that.
> if (!net->xfrm.policy_count[dir] ... -> if (net->xfrm.policy_count[dir] ...
Hmm, are you sure?
If "there is no policy configured" and "there is no secpath"
then "return the default policy"
The original statement is:
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);
Thank you,
Nicolas
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH net-next] xfrm: rework default policy structure
2021-11-19 17:31 ` Nicolas Dichtel
@ 2021-11-21 14:07 ` Leon Romanovsky
2022-03-14 10:38 ` [PATCH ipsec-next v2] " Nicolas Dichtel
0 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2021-11-21 14:07 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: steffen.klassert, herbert, antony.antony, davem, kuba, netdev
On Fri, Nov 19, 2021 at 06:31:18PM +0100, Nicolas Dichtel wrote:
> Le 19/11/2021 à 16:41, Leon Romanovsky a écrit :
> [snip]
> >> 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.
> Ok, I will send a v2 with that.
>
> > if (!net->xfrm.policy_count[dir] ... -> if (net->xfrm.policy_count[dir] ...
> Hmm, are you sure?
Not sure at all, maybe wrong.
Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH ipsec-next v2] xfrm: rework default policy structure
2021-11-21 14:07 ` Leon Romanovsky
@ 2022-03-14 10:38 ` Nicolas Dichtel
2022-03-15 8:32 ` Antony Antony
2022-03-18 6:27 ` Steffen Klassert
0 siblings, 2 replies; 9+ messages in thread
From: Nicolas Dichtel @ 2022-03-14 10:38 UTC (permalink / raw)
To: steffen.klassert, herbert, antony.antony
Cc: leon, davem, kuba, netdev, Nicolas Dichtel
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>
---
v1 -> v2: introduce __xfrm_check_nopolicy()
include/net/netns/xfrm.h | 6 +----
include/net/xfrm.h | 48 +++++++++++++++-------------------------
net/xfrm/xfrm_policy.c | 10 ++++++---
net/xfrm/xfrm_user.c | 43 +++++++++++++++--------------------
4 files changed, 44 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 fdb41e8bb626..1541ca0cb13c 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1081,25 +1081,18 @@ 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);
+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,
struct sk_buff *skb,
unsigned int family, int reverse)
@@ -1110,13 +1103,9 @@ 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 __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)
@@ -1168,13 +1157,12 @@ 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_OUT))
- 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);
+ if (!net->xfrm.policy_count[XFRM_POLICY_OUT] &&
+ net->xfrm.policy_default[XFRM_POLICY_OUT] == XFRM_USERPOLICY_ACCEPT)
+ return true;
+
+ return (skb_dst(skb)->flags & DST_NOXFRM) ||
+ __xfrm_route_forward(skb, family);
}
static inline int xfrm4_route_forward(struct sk_buff *skb)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 04d1ce9b510f..01fe1e9cff86 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3158,7 +3158,7 @@ struct dst_entry *xfrm_lookup_with_ifid(struct net *net,
nopol:
if (!(dst_orig->dev->flags & IFF_LOOPBACK) &&
- !xfrm_default_allow(net, dir)) {
+ net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) {
err = -EPERM;
goto error;
}
@@ -3569,7 +3569,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
}
if (!pol) {
- if (!xfrm_default_allow(net, dir)) {
+ if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) {
XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
return 0;
}
@@ -3629,7 +3629,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
}
xfrm_nr = ti;
- if (!xfrm_default_allow(net, dir) && !xfrm_nr) {
+ if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK &&
+ !xfrm_nr) {
XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOSTATES);
goto reject;
}
@@ -4118,6 +4119,9 @@ static int __net_init xfrm_net_init(struct net *net)
spin_lock_init(&net->xfrm.xfrm_policy_lock);
seqcount_spinlock_init(&net->xfrm.xfrm_policy_hash_generation, &net->xfrm.xfrm_policy_lock);
mutex_init(&net->xfrm.xfrm_cfg_mutex);
+ net->xfrm.policy_default[XFRM_POLICY_IN] = XFRM_USERPOLICY_ACCEPT;
+ net->xfrm.policy_default[XFRM_POLICY_FWD] = XFRM_USERPOLICY_ACCEPT;
+ net->xfrm.policy_default[XFRM_POLICY_OUT] = XFRM_USERPOLICY_ACCEPT;
rv = xfrm_statistics_init(net);
if (rv < 0)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8cd6c8129004..2f6b64cf0975 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2009,12 +2009,9 @@ static int xfrm_notify_userpolicy(struct net *net)
}
up = nlmsg_data(nlh);
- up->in = net->xfrm.policy_default & XFRM_POL_DEFAULT_IN ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- up->fwd = net->xfrm.policy_default & XFRM_POL_DEFAULT_FWD ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- up->out = net->xfrm.policy_default & XFRM_POL_DEFAULT_OUT ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
+ up->in = net->xfrm.policy_default[XFRM_POLICY_IN];
+ up->fwd = net->xfrm.policy_default[XFRM_POLICY_FWD];
+ up->out = net->xfrm.policy_default[XFRM_POLICY_OUT];
nlmsg_end(skb, nlh);
@@ -2025,26 +2022,26 @@ static int xfrm_notify_userpolicy(struct net *net)
return err;
}
+static bool xfrm_userpolicy_is_valid(__u8 policy)
+{
+ return policy == XFRM_USERPOLICY_BLOCK ||
+ policy == XFRM_USERPOLICY_ACCEPT;
+}
+
static int xfrm_set_default(struct sk_buff *skb, struct nlmsghdr *nlh,
struct nlattr **attrs)
{
struct net *net = sock_net(skb->sk);
struct xfrm_userpolicy_default *up = nlmsg_data(nlh);
- if (up->in == XFRM_USERPOLICY_BLOCK)
- net->xfrm.policy_default |= XFRM_POL_DEFAULT_IN;
- else if (up->in == XFRM_USERPOLICY_ACCEPT)
- net->xfrm.policy_default &= ~XFRM_POL_DEFAULT_IN;
+ if (xfrm_userpolicy_is_valid(up->in))
+ net->xfrm.policy_default[XFRM_POLICY_IN] = up->in;
- if (up->fwd == XFRM_USERPOLICY_BLOCK)
- net->xfrm.policy_default |= XFRM_POL_DEFAULT_FWD;
- else if (up->fwd == XFRM_USERPOLICY_ACCEPT)
- net->xfrm.policy_default &= ~XFRM_POL_DEFAULT_FWD;
+ if (xfrm_userpolicy_is_valid(up->fwd))
+ net->xfrm.policy_default[XFRM_POLICY_FWD] = up->fwd;
- if (up->out == XFRM_USERPOLICY_BLOCK)
- net->xfrm.policy_default |= XFRM_POL_DEFAULT_OUT;
- else if (up->out == XFRM_USERPOLICY_ACCEPT)
- net->xfrm.policy_default &= ~XFRM_POL_DEFAULT_OUT;
+ if (xfrm_userpolicy_is_valid(up->out))
+ net->xfrm.policy_default[XFRM_POLICY_OUT] = up->out;
rt_genid_bump_all(net);
@@ -2074,13 +2071,9 @@ static int xfrm_get_default(struct sk_buff *skb, struct nlmsghdr *nlh,
}
r_up = nlmsg_data(r_nlh);
-
- r_up->in = net->xfrm.policy_default & XFRM_POL_DEFAULT_IN ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- r_up->fwd = net->xfrm.policy_default & XFRM_POL_DEFAULT_FWD ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
- r_up->out = net->xfrm.policy_default & XFRM_POL_DEFAULT_OUT ?
- XFRM_USERPOLICY_BLOCK : XFRM_USERPOLICY_ACCEPT;
+ r_up->in = net->xfrm.policy_default[XFRM_POLICY_IN];
+ r_up->fwd = net->xfrm.policy_default[XFRM_POLICY_FWD];
+ r_up->out = net->xfrm.policy_default[XFRM_POLICY_OUT];
nlmsg_end(r_skb, r_nlh);
return nlmsg_unicast(net->xfrm.nlsk, r_skb, portid);
--
2.33.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH ipsec-next v2] xfrm: rework default policy structure
2022-03-14 10:38 ` [PATCH ipsec-next v2] " Nicolas Dichtel
@ 2022-03-15 8:32 ` Antony Antony
2022-03-18 6:27 ` Steffen Klassert
1 sibling, 0 replies; 9+ messages in thread
From: Antony Antony @ 2022-03-15 8:32 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: steffen.klassert, herbert, antony.antony, leon, davem, kuba,
netdev
On Mon, Mar 14, 2022 at 11:38:22 +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>
Reviewed-by: Antony Antony <antony.antony@secunet.com>
I also ran few quick tests and it looks good.
thanks Nicolas.
-antony
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH ipsec-next v2] xfrm: rework default policy structure
2022-03-14 10:38 ` [PATCH ipsec-next v2] " Nicolas Dichtel
2022-03-15 8:32 ` Antony Antony
@ 2022-03-18 6:27 ` Steffen Klassert
1 sibling, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2022-03-18 6:27 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: herbert, antony.antony, leon, davem, kuba, netdev
On Mon, Mar 14, 2022 at 11:38:22AM +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>
Applied, thanks a lot Nicolas!
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-03-18 6:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-18 14:29 [PATCH net-next] xfrm: rework default policy structure Nicolas Dichtel
2021-11-18 19:09 ` Leon Romanovsky
2021-11-19 8:06 ` Nicolas Dichtel
2021-11-19 15:41 ` Leon Romanovsky
2021-11-19 17:31 ` Nicolas Dichtel
2021-11-21 14:07 ` Leon Romanovsky
2022-03-14 10:38 ` [PATCH ipsec-next v2] " Nicolas Dichtel
2022-03-15 8:32 ` Antony Antony
2022-03-18 6:27 ` Steffen Klassert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).