* [PATCH ipsec-next] xfrm: Add Direction to the SA in or out
@ 2024-03-03 21:08 Antony Antony
2024-03-03 22:30 ` Eyal Birger
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Antony Antony @ 2024-03-03 21:08 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu; +Cc: netdev, devel
This patch introduces the 'dir' attribute, 'in' or 'out', to the
xfrm_state, SA, enhancing usability by delineating the scope of values
based on direction. An input SA will now exclusively encompass values
pertinent to input, effectively segregating them from output-related
values. This change aims to streamline the configuration process and
improve the overall clarity of SA attributes.
Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
include/net/xfrm.h | 1 +
include/uapi/linux/xfrm.h | 8 ++++++++
net/xfrm/xfrm_compat.c | 6 ++++--
net/xfrm/xfrm_device.c | 5 +++++
net/xfrm/xfrm_state.c | 1 +
net/xfrm/xfrm_user.c | 43 +++++++++++++++++++++++++++++++++++----
6 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 1d107241b901..91348a03469c 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -289,6 +289,7 @@ struct xfrm_state {
/* Private data of this transformer, format is opaque,
* interpreted by xfrm_type methods. */
void *data;
+ enum xfrm_sa_dir dir;
};
static inline struct net *xs_net(struct xfrm_state *x)
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 6a77328be114..2f1d67239301 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -141,6 +141,13 @@ enum {
XFRM_POLICY_MAX = 3
};
+enum xfrm_sa_dir {
+ XFRM_SA_DIR_UNSET = 0,
+ XFRM_SA_DIR_IN = 1,
+ XFRM_SA_DIR_OUT = 2,
+ XFRM_SA_DIR_MAX = 3,
+};
+
enum {
XFRM_SHARE_ANY, /* No limitations */
XFRM_SHARE_SESSION, /* For this session only */
@@ -315,6 +322,7 @@ enum xfrm_attr_type_t {
XFRMA_SET_MARK_MASK, /* __u32 */
XFRMA_IF_ID, /* __u32 */
XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
+ XFRMA_SA_DIR, /* __u8 */
__XFRMA_MAX
#define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
index 655fe4ff8621..de0e1508f870 100644
--- a/net/xfrm/xfrm_compat.c
+++ b/net/xfrm/xfrm_compat.c
@@ -129,6 +129,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
[XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
[XFRMA_IF_ID] = { .type = NLA_U32 },
[XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
+ [XFRMA_SA_DIR] = { .type = NLA_U8 },
};
static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
@@ -277,9 +278,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
case XFRMA_SET_MARK_MASK:
case XFRMA_IF_ID:
case XFRMA_MTIMER_THRESH:
+ case XFRMA_SA_DIR:
return xfrm_nla_cpy(dst, src, nla_len(src));
default:
- BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
+ BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
pr_warn_once("unsupported nla_type %d\n", src->nla_type);
return -EOPNOTSUPP;
}
@@ -434,7 +436,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
int err;
if (type > XFRMA_MAX) {
- BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
+ BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
NL_SET_ERR_MSG(extack, "Bad attribute");
return -EOPNOTSUPP;
}
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 3784534c9185..11339d7d7140 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
return -EINVAL;
}
+ if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
+ NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
+ return -EINVAL;
+ }
+
is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
/* We don't yet support UDP encapsulation and TFC padding. */
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index bda5327bf34d..0d6f5a49002f 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
x->lastused = orig->lastused;
x->new_mapping = 0;
x->new_mapping_sport = 0;
+ x->dir = orig->dir;
return x;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index ad01997c3aa9..fe4576e96dd4 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -360,6 +360,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
}
}
+ if (attrs[XFRMA_SA_DIR]) {
+ u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
+
+ if (!sa_dir || sa_dir >= XFRM_SA_DIR_MAX) {
+ NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range");
+ err = -EINVAL;
+ goto out;
+ }
+ }
+
out:
return err;
}
@@ -627,6 +637,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH];
+ struct nlattr *dir = attrs[XFRMA_SA_DIR];
if (re && x->replay_esn && x->preplay_esn) {
struct xfrm_replay_state_esn *replay_esn;
@@ -661,6 +672,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
if (mt)
x->mapping_maxage = nla_get_u32(mt);
+
+ if (dir)
+ x->dir = nla_get_u8(dir);
}
static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
@@ -1182,8 +1196,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
if (ret)
goto out;
}
- if (x->mapping_maxage)
+ if (x->mapping_maxage) {
ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage);
+ if (ret)
+ goto out;
+ }
+ if (x->dir)
+ ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
out:
return ret;
}
@@ -2399,7 +2418,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x)
+ nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur))
+ nla_total_size(sizeof(struct xfrm_mark))
+ nla_total_size(4) /* XFRM_AE_RTHR */
- + nla_total_size(4); /* XFRM_AE_ETHR */
+ + nla_total_size(4) /* XFRM_AE_ETHR */
+ + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */
}
static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
@@ -2453,6 +2473,11 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
goto out_cancel;
err = xfrm_if_id_put(skb, x->if_id);
+ if (err)
+ goto out_cancel;
+ if (x->dir)
+ err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
+
if (err)
goto out_cancel;
@@ -3046,6 +3071,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
[XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
[XFRMA_IF_ID] = { .type = NLA_U32 },
[XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
+ [XFRMA_SA_DIR] = { .type = NLA_U8 },
};
EXPORT_SYMBOL_GPL(xfrma_policy);
@@ -3186,8 +3212,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb)
static inline unsigned int xfrm_expire_msgsize(void)
{
- return NLMSG_ALIGN(sizeof(struct xfrm_user_expire))
- + nla_total_size(sizeof(struct xfrm_mark));
+ return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) +
+ nla_total_size(sizeof(struct xfrm_mark)) +
+ nla_total_size(sizeof_field(struct xfrm_state, dir));
}
static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
@@ -3214,6 +3241,11 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct
if (err)
return err;
+ if (x->dir)
+ err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
+ if (err)
+ return err;
+
nlmsg_end(skb, nlh);
return 0;
}
@@ -3321,6 +3353,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x)
if (x->mapping_maxage)
l += nla_total_size(sizeof(x->mapping_maxage));
+ if (x->dir)
+ l += nla_total_size(sizeof(x->dir));
+
return l;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH ipsec-next] xfrm: Add Direction to the SA in or out
2024-03-03 21:08 [PATCH ipsec-next] xfrm: Add Direction to the SA in or out Antony Antony
@ 2024-03-03 22:30 ` Eyal Birger
2024-03-06 18:38 ` [DKIM] Re: [devel-ipsec] " Antony Antony
2024-03-04 7:42 ` Leon Romanovsky
2024-03-10 20:03 ` [PATCH ipsec-next v2] " Antony Antony
2 siblings, 1 reply; 10+ messages in thread
From: Eyal Birger @ 2024-03-03 22:30 UTC (permalink / raw)
To: antony.antony; +Cc: Steffen Klassert, Herbert Xu, netdev, devel
Hi Antony,
On Sun, Mar 3, 2024 at 1:09 PM Antony Antony <antony.antony@secunet.com> wrote:
>
> This patch introduces the 'dir' attribute, 'in' or 'out', to the
> xfrm_state, SA, enhancing usability by delineating the scope of values
> based on direction. An input SA will now exclusively encompass values
> pertinent to input, effectively segregating them from output-related
> values. This change aims to streamline the configuration process and
> improve the overall clarity of SA attributes.
Nice! Minor comments below.
>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
> include/net/xfrm.h | 1 +
> include/uapi/linux/xfrm.h | 8 ++++++++
> net/xfrm/xfrm_compat.c | 6 ++++--
> net/xfrm/xfrm_device.c | 5 +++++
> net/xfrm/xfrm_state.c | 1 +
> net/xfrm/xfrm_user.c | 43 +++++++++++++++++++++++++++++++++++----
> 6 files changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 1d107241b901..91348a03469c 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -289,6 +289,7 @@ struct xfrm_state {
> /* Private data of this transformer, format is opaque,
> * interpreted by xfrm_type methods. */
> void *data;
> + enum xfrm_sa_dir dir;
> };
>
> static inline struct net *xs_net(struct xfrm_state *x)
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index 6a77328be114..2f1d67239301 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -141,6 +141,13 @@ enum {
> XFRM_POLICY_MAX = 3
> };
>
> +enum xfrm_sa_dir {
> + XFRM_SA_DIR_UNSET = 0,
> + XFRM_SA_DIR_IN = 1,
> + XFRM_SA_DIR_OUT = 2,
> + XFRM_SA_DIR_MAX = 3,
nit: comma is redundant after a "MAX" no?
> +};
> +
> enum {
> XFRM_SHARE_ANY, /* No limitations */
> XFRM_SHARE_SESSION, /* For this session only */
> @@ -315,6 +322,7 @@ enum xfrm_attr_type_t {
> XFRMA_SET_MARK_MASK, /* __u32 */
> XFRMA_IF_ID, /* __u32 */
> XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
> + XFRMA_SA_DIR, /* __u8 */
> __XFRMA_MAX
>
> #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
> diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> index 655fe4ff8621..de0e1508f870 100644
> --- a/net/xfrm/xfrm_compat.c
> +++ b/net/xfrm/xfrm_compat.c
> @@ -129,6 +129,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> [XFRMA_IF_ID] = { .type = NLA_U32 },
> [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> + [XFRMA_SA_DIR] = { .type = NLA_U8 },
> };
>
> static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
> @@ -277,9 +278,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
> case XFRMA_SET_MARK_MASK:
> case XFRMA_IF_ID:
> case XFRMA_MTIMER_THRESH:
> + case XFRMA_SA_DIR:
> return xfrm_nla_cpy(dst, src, nla_len(src));
> default:
> - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> pr_warn_once("unsupported nla_type %d\n", src->nla_type);
> return -EOPNOTSUPP;
> }
> @@ -434,7 +436,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
> int err;
>
> if (type > XFRMA_MAX) {
> - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> NL_SET_ERR_MSG(extack, "Bad attribute");
> return -EOPNOTSUPP;
> }
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 3784534c9185..11339d7d7140 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
> return -EINVAL;
> }
>
> + if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
> + NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
> + return -EINVAL;
> + }
> +
> is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
>
> /* We don't yet support UDP encapsulation and TFC padding. */
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index bda5327bf34d..0d6f5a49002f 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> x->lastused = orig->lastused;
> x->new_mapping = 0;
> x->new_mapping_sport = 0;
> + x->dir = orig->dir;
>
> return x;
>
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index ad01997c3aa9..fe4576e96dd4 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -360,6 +360,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
> }
> }
>
> + if (attrs[XFRMA_SA_DIR]) {
> + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
> +
> + if (!sa_dir || sa_dir >= XFRM_SA_DIR_MAX) {
> + NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range");
> + err = -EINVAL;
> + goto out;
> + }
> + }
> +
> out:
> return err;
> }
> @@ -627,6 +637,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
> struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
> struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
> struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH];
> + struct nlattr *dir = attrs[XFRMA_SA_DIR];
>
> if (re && x->replay_esn && x->preplay_esn) {
> struct xfrm_replay_state_esn *replay_esn;
> @@ -661,6 +672,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
>
> if (mt)
> x->mapping_maxage = nla_get_u32(mt);
> +
> + if (dir)
> + x->dir = nla_get_u8(dir);
It's not clear to me why this belongs in xfrm_update_ae_params().
IOW, why isn't this done in xfrm_state_construct(), like if_id?
> }
>
> static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
> @@ -1182,8 +1196,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
> if (ret)
> goto out;
> }
> - if (x->mapping_maxage)
> + if (x->mapping_maxage) {
> ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage);
> + if (ret)
> + goto out;
> + }
> + if (x->dir)
> + ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> out:
> return ret;
> }
> @@ -2399,7 +2418,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x)
> + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur))
> + nla_total_size(sizeof(struct xfrm_mark))
> + nla_total_size(4) /* XFRM_AE_RTHR */
> - + nla_total_size(4); /* XFRM_AE_ETHR */
> + + nla_total_size(4) /* XFRM_AE_ETHR */
> + + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */
> }
>
> static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
> @@ -2453,6 +2473,11 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
> goto out_cancel;
>
> err = xfrm_if_id_put(skb, x->if_id);
> + if (err)
> + goto out_cancel;
> + if (x->dir)
> + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> +
> if (err)
> goto out_cancel;
>
> @@ -3046,6 +3071,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
> [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> [XFRMA_IF_ID] = { .type = NLA_U32 },
> [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> + [XFRMA_SA_DIR] = { .type = NLA_U8 },
Maybe add a ".strict_start_type"?
>
> };
> EXPORT_SYMBOL_GPL(xfrma_policy);
>
> @@ -3186,8 +3212,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb)
>
> static inline unsigned int xfrm_expire_msgsize(void)
> {
> - return NLMSG_ALIGN(sizeof(struct xfrm_user_expire))
> - + nla_total_size(sizeof(struct xfrm_mark));
> + return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) +
> + nla_total_size(sizeof(struct xfrm_mark)) +
> + nla_total_size(sizeof_field(struct xfrm_state, dir));
> }
>
> static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
> @@ -3214,6 +3241,11 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct
> if (err)
> return err;
>
> + if (x->dir)
> + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> + if (err)
> + return err;
> +
> nlmsg_end(skb, nlh);
> return 0;
> }
> @@ -3321,6 +3353,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x)
> if (x->mapping_maxage)
> l += nla_total_size(sizeof(x->mapping_maxage));
>
> + if (x->dir)
> + l += nla_total_size(sizeof(x->dir));
> +
> return l;
> }
>
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ipsec-next] xfrm: Add Direction to the SA in or out
2024-03-03 21:08 [PATCH ipsec-next] xfrm: Add Direction to the SA in or out Antony Antony
2024-03-03 22:30 ` Eyal Birger
@ 2024-03-04 7:42 ` Leon Romanovsky
2024-03-06 19:28 ` [DKIM] Re: [devel-ipsec] " Antony Antony
2024-03-10 20:03 ` [PATCH ipsec-next v2] " Antony Antony
2 siblings, 1 reply; 10+ messages in thread
From: Leon Romanovsky @ 2024-03-04 7:42 UTC (permalink / raw)
To: Antony Antony; +Cc: Steffen Klassert, Herbert Xu, netdev, devel
On Sun, Mar 03, 2024 at 10:08:41PM +0100, Antony Antony wrote:
> This patch introduces the 'dir' attribute, 'in' or 'out', to the
> xfrm_state, SA, enhancing usability by delineating the scope of values
> based on direction. An input SA will now exclusively encompass values
> pertinent to input, effectively segregating them from output-related
> values. This change aims to streamline the configuration process and
> improve the overall clarity of SA attributes.
>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
> include/net/xfrm.h | 1 +
> include/uapi/linux/xfrm.h | 8 ++++++++
> net/xfrm/xfrm_compat.c | 6 ++++--
> net/xfrm/xfrm_device.c | 5 +++++
> net/xfrm/xfrm_state.c | 1 +
> net/xfrm/xfrm_user.c | 43 +++++++++++++++++++++++++++++++++++----
> 6 files changed, 58 insertions(+), 6 deletions(-)
<...>
> +enum xfrm_sa_dir {
> + XFRM_SA_DIR_UNSET = 0,
> + XFRM_SA_DIR_IN = 1,
> + XFRM_SA_DIR_OUT = 2,
> + XFRM_SA_DIR_MAX = 3,
> +};
<...>
> + if (attrs[XFRMA_SA_DIR]) {
> + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
> +
> + if (!sa_dir || sa_dir >= XFRM_SA_DIR_MAX) {
Netlink approach is to rely on attrs[XFRMA_SA_DIR] as a marker for XFRM_SA_DIR_UNSET.
If attrs[XFRMA_SA_DIR] == NULL, then the direction is XFRM_SA_DIR_UNSET.
Also, why do you need XFRM_SA_DIR_MAX in UAPI?
Thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [DKIM] Re: [devel-ipsec] [PATCH ipsec-next] xfrm: Add Direction to the SA in or out
2024-03-03 22:30 ` Eyal Birger
@ 2024-03-06 18:38 ` Antony Antony
2024-03-06 18:57 ` Eyal Birger
0 siblings, 1 reply; 10+ messages in thread
From: Antony Antony @ 2024-03-06 18:38 UTC (permalink / raw)
To: Eyal Birger; +Cc: antony.antony, netdev, devel, Herbert Xu
Hi Eyal,
On Sun, Mar 03, 2024 at 02:30:46PM -0800, Eyal Birger via Devel wrote:
..
> On Sun, Mar 3, 2024 at 1:09 PM Antony Antony <antony.antony@secunet.com>
> wrote:
> >
> > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > xfrm_state, SA, enhancing usability by delineating the scope of values
> > based on direction. An input SA will now exclusively encompass values
> > pertinent to input, effectively segregating them from output-related
> > values. This change aims to streamline the configuration process and
> > improve the overall clarity of SA attributes.
>
> Nice! Minor comments below.
Thanks for your feedback. See my response below. I don't understand how
would I add ".strict_start_type".
> >
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > ---
> > include/net/xfrm.h | 1 +
> > include/uapi/linux/xfrm.h | 8 ++++++++
> > net/xfrm/xfrm_compat.c | 6 ++++--
> > net/xfrm/xfrm_device.c | 5 +++++
> > net/xfrm/xfrm_state.c | 1 +
> > net/xfrm/xfrm_user.c | 43 +++++++++++++++++++++++++++++++++++----
> > 6 files changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > index 1d107241b901..91348a03469c 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -289,6 +289,7 @@ struct xfrm_state {
> > /* Private data of this transformer, format is opaque,
> > * interpreted by xfrm_type methods. */
> > void *data;
> > + enum xfrm_sa_dir dir;
> > };
> >
> > static inline struct net *xs_net(struct xfrm_state *x)
> > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > index 6a77328be114..2f1d67239301 100644
> > --- a/include/uapi/linux/xfrm.h
> > +++ b/include/uapi/linux/xfrm.h
> > @@ -141,6 +141,13 @@ enum {
> > XFRM_POLICY_MAX = 3
> > };
> >
> > +enum xfrm_sa_dir {
> > + XFRM_SA_DIR_UNSET = 0,
> > + XFRM_SA_DIR_IN = 1,
> > + XFRM_SA_DIR_OUT = 2,
> > + XFRM_SA_DIR_MAX = 3,
>
> nit: comma is redundant after a "MAX" no?
I removed it. Actually XFRM_SA_DIR_MAX also as Leon pointed out it is not
necessary.
>
> > +};
> > +
> > enum {
> > XFRM_SHARE_ANY, /* No limitations */
> > XFRM_SHARE_SESSION, /* For this session only */
> > @@ -315,6 +322,7 @@ enum xfrm_attr_type_t {
> > XFRMA_SET_MARK_MASK, /* __u32 */
> > XFRMA_IF_ID, /* __u32 */
> > XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
> > + XFRMA_SA_DIR, /* __u8 */
> > __XFRMA_MAX
> >
> > #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
> > diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> > index 655fe4ff8621..de0e1508f870 100644
> > --- a/net/xfrm/xfrm_compat.c
> > +++ b/net/xfrm/xfrm_compat.c
> > @@ -129,6 +129,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> > [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> > [XFRMA_IF_ID] = { .type = NLA_U32 },
> > [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> > + [XFRMA_SA_DIR] = { .type = NLA_U8 },
> > };
> >
> > static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
> > @@ -277,9 +278,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
> > case XFRMA_SET_MARK_MASK:
> > case XFRMA_IF_ID:
> > case XFRMA_MTIMER_THRESH:
> > + case XFRMA_SA_DIR:
> > return xfrm_nla_cpy(dst, src, nla_len(src));
> > default:
> > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > pr_warn_once("unsupported nla_type %d\n", src->nla_type);
> > return -EOPNOTSUPP;
> > }
> > @@ -434,7 +436,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
> > int err;
> >
> > if (type > XFRMA_MAX) {
> > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > NL_SET_ERR_MSG(extack, "Bad attribute");
> > return -EOPNOTSUPP;
> > }
> > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > index 3784534c9185..11339d7d7140 100644
> > --- a/net/xfrm/xfrm_device.c
> > +++ b/net/xfrm/xfrm_device.c
> > @@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
> > return -EINVAL;
> > }
> >
> > + if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
> > + NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
> > + return -EINVAL;
> > + }
> > +
> > is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
> >
> > /* We don't yet support UDP encapsulation and TFC padding. */
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index bda5327bf34d..0d6f5a49002f 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> > x->lastused = orig->lastused;
> > x->new_mapping = 0;
> > x->new_mapping_sport = 0;
> > + x->dir = orig->dir;
> >
> > return x;
> >
> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index ad01997c3aa9..fe4576e96dd4 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -360,6 +360,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
> > }
> > }
> >
> > + if (attrs[XFRMA_SA_DIR]) {
> > + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
> > +
> > + if (!sa_dir || sa_dir >= XFRM_SA_DIR_MAX) {
> > + NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range");
> > + err = -EINVAL;
> > + goto out;
> > + }
> > + }
> > +
> > out:
> > return err;
> > }
> > @@ -627,6 +637,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
> > struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
> > struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
> > struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH];
> > + struct nlattr *dir = attrs[XFRMA_SA_DIR];
> >
> > if (re && x->replay_esn && x->preplay_esn) {
> > struct xfrm_replay_state_esn *replay_esn;
> > @@ -661,6 +672,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
> >
> > if (mt)
> > x->mapping_maxage = nla_get_u32(mt);
> > +
> > + if (dir)
> > + x->dir = nla_get_u8(dir);
>
> It's not clear to me why this belongs in xfrm_update_ae_params().
> IOW, why isn't this done in xfrm_state_construct(), like if_id?
I am in favor adding direction to all NL messages which include xfrm_state
for consitancy. Also when offload is enabled and xuo flags falg
XFRM_OFFLOAD_INBOUND is set. I am hopping in the long term offload can also
x->dir instead of this xuo->flags.
> > }
> >
> > static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
> > @@ -1182,8 +1196,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
> > if (ret)
> > goto out;
> > }
> > - if (x->mapping_maxage)
> > + if (x->mapping_maxage) {
> > ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage);
> > + if (ret)
> > + goto out;
> > + }
> > + if (x->dir)
> > + ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> > out:
> > return ret;
> > }
> > @@ -2399,7 +2418,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x)
> > + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur))
> > + nla_total_size(sizeof(struct xfrm_mark))
> > + nla_total_size(4) /* XFRM_AE_RTHR */
> > - + nla_total_size(4); /* XFRM_AE_ETHR */
> > + + nla_total_size(4) /* XFRM_AE_ETHR */
> > + + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */
> > }
> >
> > static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
> > @@ -2453,6 +2473,11 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
> > goto out_cancel;
> >
> > err = xfrm_if_id_put(skb, x->if_id);
> > + if (err)
> > + goto out_cancel;
> > + if (x->dir)
> > + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> > +
> > if (err)
> > goto out_cancel;
> >
> > @@ -3046,6 +3071,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
> > [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> > [XFRMA_IF_ID] = { .type = NLA_U32 },
> > [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> > + [XFRMA_SA_DIR] = { .type = NLA_U8 },
>
> Maybe add a ".strict_start_type"?
Isn't this for the first element? I don't understand your suggestion.
Are you advising to add to [XFRMA_SA_DIR]? Would you like explain a bit
more?
>
> >
> > };
> > EXPORT_SYMBOL_GPL(xfrma_policy);
> >
> > @@ -3186,8 +3212,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb)
> >
> > static inline unsigned int xfrm_expire_msgsize(void)
> > {
> > - return NLMSG_ALIGN(sizeof(struct xfrm_user_expire))
> > - + nla_total_size(sizeof(struct xfrm_mark));
> > + return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) +
> > + nla_total_size(sizeof(struct xfrm_mark)) +
> > + nla_total_size(sizeof_field(struct xfrm_state, dir));
> > }
> >
> > static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
> > @@ -3214,6 +3241,11 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct
> > if (err)
> > return err;
> >
> > + if (x->dir)
> > + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> > + if (err)
> > + return err;
> > +
> > nlmsg_end(skb, nlh);
> > return 0;
> > }
> > @@ -3321,6 +3353,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x)
> > if (x->mapping_maxage)
> > l += nla_total_size(sizeof(x->mapping_maxage));
> >
> > + if (x->dir)
> > + l += nla_total_size(sizeof(x->dir));
> > +
> > return l;
> > }
> >
> > --
> > 2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [DKIM] Re: [devel-ipsec] [PATCH ipsec-next] xfrm: Add Direction to the SA in or out
2024-03-06 18:38 ` [DKIM] Re: [devel-ipsec] " Antony Antony
@ 2024-03-06 18:57 ` Eyal Birger
2024-03-10 20:05 ` Antony Antony
0 siblings, 1 reply; 10+ messages in thread
From: Eyal Birger @ 2024-03-06 18:57 UTC (permalink / raw)
To: Antony Antony; +Cc: antony.antony, netdev, devel, Herbert Xu
Hi Antony,
On Wed, Mar 6, 2024 at 10:38 AM Antony Antony <antony@phenome.org> wrote:
>
> Hi Eyal,
>
> On Sun, Mar 03, 2024 at 02:30:46PM -0800, Eyal Birger via Devel wrote:
> ..
> > On Sun, Mar 3, 2024 at 1:09 PM Antony Antony <antony.antony@secunet.com>
> > wrote:
> > >
> > > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > > xfrm_state, SA, enhancing usability by delineating the scope of values
> > > based on direction. An input SA will now exclusively encompass values
> > > pertinent to input, effectively segregating them from output-related
> > > values. This change aims to streamline the configuration process and
> > > improve the overall clarity of SA attributes.
> >
> > Nice! Minor comments below.
>
> Thanks for your feedback. See my response below. I don't understand how
> would I add ".strict_start_type".
>
> > >
> > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > ---
> > > include/net/xfrm.h | 1 +
> > > include/uapi/linux/xfrm.h | 8 ++++++++
> > > net/xfrm/xfrm_compat.c | 6 ++++--
> > > net/xfrm/xfrm_device.c | 5 +++++
> > > net/xfrm/xfrm_state.c | 1 +
> > > net/xfrm/xfrm_user.c | 43 +++++++++++++++++++++++++++++++++++----
> > > 6 files changed, 58 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > > index 1d107241b901..91348a03469c 100644
> > > --- a/include/net/xfrm.h
> > > +++ b/include/net/xfrm.h
> > > @@ -289,6 +289,7 @@ struct xfrm_state {
> > > /* Private data of this transformer, format is opaque,
> > > * interpreted by xfrm_type methods. */
> > > void *data;
> > > + enum xfrm_sa_dir dir;
> > > };
> > >
> > > static inline struct net *xs_net(struct xfrm_state *x)
> > > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > > index 6a77328be114..2f1d67239301 100644
> > > --- a/include/uapi/linux/xfrm.h
> > > +++ b/include/uapi/linux/xfrm.h
> > > @@ -141,6 +141,13 @@ enum {
> > > XFRM_POLICY_MAX = 3
> > > };
> > >
> > > +enum xfrm_sa_dir {
> > > + XFRM_SA_DIR_UNSET = 0,
> > > + XFRM_SA_DIR_IN = 1,
> > > + XFRM_SA_DIR_OUT = 2,
> > > + XFRM_SA_DIR_MAX = 3,
> >
> > nit: comma is redundant after a "MAX" no?
>
> I removed it. Actually XFRM_SA_DIR_MAX also as Leon pointed out it is not
> necessary.
Great.
>
> >
> > > +};
> > > +
> > > enum {
> > > XFRM_SHARE_ANY, /* No limitations */
> > > XFRM_SHARE_SESSION, /* For this session only */
> > > @@ -315,6 +322,7 @@ enum xfrm_attr_type_t {
> > > XFRMA_SET_MARK_MASK, /* __u32 */
> > > XFRMA_IF_ID, /* __u32 */
> > > XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
> > > + XFRMA_SA_DIR, /* __u8 */
> > > __XFRMA_MAX
> > >
> > > #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
> > > diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> > > index 655fe4ff8621..de0e1508f870 100644
> > > --- a/net/xfrm/xfrm_compat.c
> > > +++ b/net/xfrm/xfrm_compat.c
> > > @@ -129,6 +129,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> > > [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> > > [XFRMA_IF_ID] = { .type = NLA_U32 },
> > > [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> > > + [XFRMA_SA_DIR] = { .type = NLA_U8 },
> > > };
> > >
> > > static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
> > > @@ -277,9 +278,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
> > > case XFRMA_SET_MARK_MASK:
> > > case XFRMA_IF_ID:
> > > case XFRMA_MTIMER_THRESH:
> > > + case XFRMA_SA_DIR:
> > > return xfrm_nla_cpy(dst, src, nla_len(src));
> > > default:
> > > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > > pr_warn_once("unsupported nla_type %d\n", src->nla_type);
> > > return -EOPNOTSUPP;
> > > }
> > > @@ -434,7 +436,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
> > > int err;
> > >
> > > if (type > XFRMA_MAX) {
> > > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > > NL_SET_ERR_MSG(extack, "Bad attribute");
> > > return -EOPNOTSUPP;
> > > }
> > > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > > index 3784534c9185..11339d7d7140 100644
> > > --- a/net/xfrm/xfrm_device.c
> > > +++ b/net/xfrm/xfrm_device.c
> > > @@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
> > > return -EINVAL;
> > > }
> > >
> > > + if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
> > > + NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
> > > + return -EINVAL;
> > > + }
> > > +
> > > is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
> > >
> > > /* We don't yet support UDP encapsulation and TFC padding. */
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > index bda5327bf34d..0d6f5a49002f 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > > @@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> > > x->lastused = orig->lastused;
> > > x->new_mapping = 0;
> > > x->new_mapping_sport = 0;
> > > + x->dir = orig->dir;
> > >
> > > return x;
> > >
> > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > > index ad01997c3aa9..fe4576e96dd4 100644
> > > --- a/net/xfrm/xfrm_user.c
> > > +++ b/net/xfrm/xfrm_user.c
> > > @@ -360,6 +360,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
> > > }
> > > }
> > >
> > > + if (attrs[XFRMA_SA_DIR]) {
> > > + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
> > > +
> > > + if (!sa_dir || sa_dir >= XFRM_SA_DIR_MAX) {
> > > + NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range");
> > > + err = -EINVAL;
> > > + goto out;
> > > + }
> > > + }
> > > +
> > > out:
> > > return err;
> > > }
> > > @@ -627,6 +637,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
> > > struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
> > > struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
> > > struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH];
> > > + struct nlattr *dir = attrs[XFRMA_SA_DIR];
> > >
> > > if (re && x->replay_esn && x->preplay_esn) {
> > > struct xfrm_replay_state_esn *replay_esn;
> > > @@ -661,6 +672,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
> > >
> > > if (mt)
> > > x->mapping_maxage = nla_get_u32(mt);
> > > +
> > > + if (dir)
> > > + x->dir = nla_get_u8(dir);
> >
> > It's not clear to me why this belongs in xfrm_update_ae_params().
> > IOW, why isn't this done in xfrm_state_construct(), like if_id?
>
> I am in favor adding direction to all NL messages which include xfrm_state
> for consitancy. Also when offload is enabled and xuo flags falg
> XFRM_OFFLOAD_INBOUND is set. I am hopping in the long term offload can also
> x->dir instead of this xuo->flags.
I see. Thanks for the explanation.
>
>
> > > }
> > >
> > > static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
> > > @@ -1182,8 +1196,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
> > > if (ret)
> > > goto out;
> > > }
> > > - if (x->mapping_maxage)
> > > + if (x->mapping_maxage) {
> > > ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage);
> > > + if (ret)
> > > + goto out;
> > > + }
> > > + if (x->dir)
> > > + ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> > > out:
> > > return ret;
> > > }
> > > @@ -2399,7 +2418,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x)
> > > + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur))
> > > + nla_total_size(sizeof(struct xfrm_mark))
> > > + nla_total_size(4) /* XFRM_AE_RTHR */
> > > - + nla_total_size(4); /* XFRM_AE_ETHR */
> > > + + nla_total_size(4) /* XFRM_AE_ETHR */
> > > + + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */
> > > }
> > >
> > > static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
> > > @@ -2453,6 +2473,11 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
> > > goto out_cancel;
> > >
> > > err = xfrm_if_id_put(skb, x->if_id);
> > > + if (err)
> > > + goto out_cancel;
> > > + if (x->dir)
> > > + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> > > +
> > > if (err)
> > > goto out_cancel;
> > >
> > > @@ -3046,6 +3071,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
> > > [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> > > [XFRMA_IF_ID] = { .type = NLA_U32 },
> > > [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> > > + [XFRMA_SA_DIR] = { .type = NLA_U8 },
> >
> > Maybe add a ".strict_start_type"?
>
> Isn't this for the first element? I don't understand your suggestion.
> Are you advising to add to [XFRMA_SA_DIR]? Would you like explain a bit
> more?
See comment on strict_start_type in include/net/netlink.h
Basically for this you'd add:
[XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR },
To the start of the xfrma_policy array.
Eyal.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [DKIM] Re: [devel-ipsec] [PATCH ipsec-next] xfrm: Add Direction to the SA in or out
2024-03-04 7:42 ` Leon Romanovsky
@ 2024-03-06 19:28 ` Antony Antony
0 siblings, 0 replies; 10+ messages in thread
From: Antony Antony @ 2024-03-06 19:28 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Antony Antony, netdev, devel, Herbert Xu
On Mon, Mar 04, 2024 at 09:42:39AM +0200, Leon Romanovsky via Devel wrote:
> On Sun, Mar 03, 2024 at 10:08:41PM +0100, Antony Antony wrote:
> > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > xfrm_state, SA, enhancing usability by delineating the scope of values
> > based on direction. An input SA will now exclusively encompass values
> > pertinent to input, effectively segregating them from output-related
> > values. This change aims to streamline the configuration process and
> > improve the overall clarity of SA attributes.
> >
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > ---
> > include/net/xfrm.h | 1 +
> > include/uapi/linux/xfrm.h | 8 ++++++++
> > net/xfrm/xfrm_compat.c | 6 ++++--
> > net/xfrm/xfrm_device.c | 5 +++++
> > net/xfrm/xfrm_state.c | 1 +
> > net/xfrm/xfrm_user.c | 43 +++++++++++++++++++++++++++++++++++----
> > 6 files changed, 58 insertions(+), 6 deletions(-)
>
> <...>
>
> > +enum xfrm_sa_dir {
> > + XFRM_SA_DIR_UNSET = 0,
> > + XFRM_SA_DIR_IN = 1,
> > + XFRM_SA_DIR_OUT = 2,
> > + XFRM_SA_DIR_MAX = 3,
> > +};
>
> <...>
>
> > + if (attrs[XFRMA_SA_DIR]) {
> > + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
> > +
> > + if (!sa_dir || sa_dir >= XFRM_SA_DIR_MAX) {
>
> Netlink approach is to rely on attrs[XFRMA_SA_DIR] as a marker for XFRM_SA_DIR_UNSET.
> If attrs[XFRMA_SA_DIR] == NULL, then the direction is XFRM_SA_DIR_UNSET.
yes I aggree.
the above 'if' changed to
if (sa_dir != XFRM_SA_DIR_IN && sa_dir != XFRM_SA_DIR_OUT) {
> Also, why do you need XFRM_SA_DIR_MAX in UAPI?
Agree, I removed it.
thanks,
-antony
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH ipsec-next v2] xfrm: Add Direction to the SA in or out
2024-03-03 21:08 [PATCH ipsec-next] xfrm: Add Direction to the SA in or out Antony Antony
2024-03-03 22:30 ` Eyal Birger
2024-03-04 7:42 ` Leon Romanovsky
@ 2024-03-10 20:03 ` Antony Antony
2024-03-11 7:25 ` Leon Romanovsky
2 siblings, 1 reply; 10+ messages in thread
From: Antony Antony @ 2024-03-10 20:03 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu; +Cc: netdev, devel
This patch introduces the 'dir' attribute, 'in' or 'out', to the
xfrm_state, SA, enhancing usability by delineating the scope of values
based on direction. An input SA will now exclusively encompass values
pertinent to input, effectively segregating them from output-related
values. This change aims to streamline the configuration process and
improve the overall clarity of SA attributes.
v1->v2:
- use .strict_start_type in struct nla_policy xfrma_policy
- delete redundant XFRM_SA_DIR_MAX enum
Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
include/net/xfrm.h | 1 +
include/uapi/linux/xfrm.h | 7 +++++++
net/xfrm/xfrm_compat.c | 7 +++++--
net/xfrm/xfrm_device.c | 5 +++++
net/xfrm/xfrm_state.c | 1 +
net/xfrm/xfrm_user.c | 44 +++++++++++++++++++++++++++++++++++----
6 files changed, 59 insertions(+), 6 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 1d107241b901..91348a03469c 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -289,6 +289,7 @@ struct xfrm_state {
/* Private data of this transformer, format is opaque,
* interpreted by xfrm_type methods. */
void *data;
+ enum xfrm_sa_dir dir;
};
static inline struct net *xs_net(struct xfrm_state *x)
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 6a77328be114..46a9770c720c 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -141,6 +141,12 @@ enum {
XFRM_POLICY_MAX = 3
};
+enum xfrm_sa_dir {
+ XFRM_SA_DIR_UNSET = 0,
+ XFRM_SA_DIR_IN = 1,
+ XFRM_SA_DIR_OUT = 2
+};
+
enum {
XFRM_SHARE_ANY, /* No limitations */
XFRM_SHARE_SESSION, /* For this session only */
@@ -315,6 +321,7 @@ enum xfrm_attr_type_t {
XFRMA_SET_MARK_MASK, /* __u32 */
XFRMA_IF_ID, /* __u32 */
XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
+ XFRMA_SA_DIR, /* __u8 */
__XFRMA_MAX
#define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
index 655fe4ff8621..007dee03b1bc 100644
--- a/net/xfrm/xfrm_compat.c
+++ b/net/xfrm/xfrm_compat.c
@@ -98,6 +98,7 @@ static const int compat_msg_min[XFRM_NR_MSGTYPES] = {
};
static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
+ [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR },
[XFRMA_SA] = { .len = XMSGSIZE(compat_xfrm_usersa_info)},
[XFRMA_POLICY] = { .len = XMSGSIZE(compat_xfrm_userpolicy_info)},
[XFRMA_LASTUSED] = { .type = NLA_U64},
@@ -129,6 +130,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
[XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
[XFRMA_IF_ID] = { .type = NLA_U32 },
[XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
+ [XFRMA_SA_DIR] = { .type = NLA_U8}
};
static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
@@ -277,9 +279,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
case XFRMA_SET_MARK_MASK:
case XFRMA_IF_ID:
case XFRMA_MTIMER_THRESH:
+ case XFRMA_SA_DIR:
return xfrm_nla_cpy(dst, src, nla_len(src));
default:
- BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
+ BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
pr_warn_once("unsupported nla_type %d\n", src->nla_type);
return -EOPNOTSUPP;
}
@@ -434,7 +437,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
int err;
if (type > XFRMA_MAX) {
- BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
+ BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
NL_SET_ERR_MSG(extack, "Bad attribute");
return -EOPNOTSUPP;
}
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 3784534c9185..11339d7d7140 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
return -EINVAL;
}
+ if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
+ NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
+ return -EINVAL;
+ }
+
is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
/* We don't yet support UDP encapsulation and TFC padding. */
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index bda5327bf34d..0d6f5a49002f 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
x->lastused = orig->lastused;
x->new_mapping = 0;
x->new_mapping_sport = 0;
+ x->dir = orig->dir;
return x;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index ad01997c3aa9..ae9d2edb4769 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -360,6 +360,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
}
}
+ if (attrs[XFRMA_SA_DIR]) {
+ u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
+
+ if (sa_dir != XFRM_SA_DIR_IN && sa_dir != XFRM_SA_DIR_OUT) {
+ NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range");
+ err = -EINVAL;
+ goto out;
+ }
+ }
+
out:
return err;
}
@@ -627,6 +637,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH];
+ struct nlattr *dir = attrs[XFRMA_SA_DIR];
if (re && x->replay_esn && x->preplay_esn) {
struct xfrm_replay_state_esn *replay_esn;
@@ -661,6 +672,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
if (mt)
x->mapping_maxage = nla_get_u32(mt);
+
+ if (dir)
+ x->dir = nla_get_u8(dir);
}
static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
@@ -1182,8 +1196,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
if (ret)
goto out;
}
- if (x->mapping_maxage)
+ if (x->mapping_maxage) {
ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage);
+ if (ret)
+ goto out;
+ }
+ if (x->dir)
+ ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
out:
return ret;
}
@@ -2399,7 +2418,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x)
+ nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur))
+ nla_total_size(sizeof(struct xfrm_mark))
+ nla_total_size(4) /* XFRM_AE_RTHR */
- + nla_total_size(4); /* XFRM_AE_ETHR */
+ + nla_total_size(4) /* XFRM_AE_ETHR */
+ + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */
}
static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
@@ -2453,6 +2473,11 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
goto out_cancel;
err = xfrm_if_id_put(skb, x->if_id);
+ if (err)
+ goto out_cancel;
+ if (x->dir)
+ err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
+
if (err)
goto out_cancel;
@@ -3015,6 +3040,7 @@ EXPORT_SYMBOL_GPL(xfrm_msg_min);
#undef XMSGSIZE
const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
+ [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR },
[XFRMA_SA] = { .len = sizeof(struct xfrm_usersa_info)},
[XFRMA_POLICY] = { .len = sizeof(struct xfrm_userpolicy_info)},
[XFRMA_LASTUSED] = { .type = NLA_U64},
@@ -3046,6 +3072,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
[XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
[XFRMA_IF_ID] = { .type = NLA_U32 },
[XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
+ [XFRMA_SA_DIR] = { .type = NLA_U8 }
};
EXPORT_SYMBOL_GPL(xfrma_policy);
@@ -3186,8 +3213,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb)
static inline unsigned int xfrm_expire_msgsize(void)
{
- return NLMSG_ALIGN(sizeof(struct xfrm_user_expire))
- + nla_total_size(sizeof(struct xfrm_mark));
+ return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) +
+ nla_total_size(sizeof(struct xfrm_mark)) +
+ nla_total_size(sizeof_field(struct xfrm_state, dir));
}
static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
@@ -3214,6 +3242,11 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct
if (err)
return err;
+ if (x->dir)
+ err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
+ if (err)
+ return err;
+
nlmsg_end(skb, nlh);
return 0;
}
@@ -3321,6 +3354,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x)
if (x->mapping_maxage)
l += nla_total_size(sizeof(x->mapping_maxage));
+ if (x->dir)
+ l += nla_total_size(sizeof(x->dir));
+
return l;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [DKIM] Re: [devel-ipsec] [PATCH ipsec-next] xfrm: Add Direction to the SA in or out
2024-03-06 18:57 ` Eyal Birger
@ 2024-03-10 20:05 ` Antony Antony
0 siblings, 0 replies; 10+ messages in thread
From: Antony Antony @ 2024-03-10 20:05 UTC (permalink / raw)
To: Eyal Birger; +Cc: Antony Antony, antony.antony, netdev, devel, Herbert Xu
Hi Eyal,
On Wed, Mar 06, 2024 at 10:57:57AM -0800, Eyal Birger wrote:
> Hi Antony,
>
> On Wed, Mar 6, 2024 at 10:38 AM Antony Antony <antony@phenome.org> wrote:
> >
> > Hi Eyal,
> >
> > On Sun, Mar 03, 2024 at 02:30:46PM -0800, Eyal Birger via Devel wrote:
> > ..
> > > On Sun, Mar 3, 2024 at 1:09 PM Antony Antony <antony.antony@secunet.com>
> > > wrote:
> > > >
> > > > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > > > xfrm_state, SA, enhancing usability by delineating the scope of values
> > > > based on direction. An input SA will now exclusively encompass values
> > > > pertinent to input, effectively segregating them from output-related
> > > > values. This change aims to streamline the configuration process and
> > > > improve the overall clarity of SA attributes.
> > >
> > > Nice! Minor comments below.
> >
> > Thanks for your feedback. See my response below. I don't understand how
> > would I add ".strict_start_type".
> >
> > > >
> > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > ---
> > > > include/net/xfrm.h | 1 +
> > > > include/uapi/linux/xfrm.h | 8 ++++++++
> > > > net/xfrm/xfrm_compat.c | 6 ++++--
> > > > net/xfrm/xfrm_device.c | 5 +++++
> > > > net/xfrm/xfrm_state.c | 1 +
> > > > net/xfrm/xfrm_user.c | 43 +++++++++++++++++++++++++++++++++++----
> > > > 6 files changed, 58 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > > > index 1d107241b901..91348a03469c 100644
> > > > --- a/include/net/xfrm.h
> > > > +++ b/include/net/xfrm.h
> > > > @@ -289,6 +289,7 @@ struct xfrm_state {
> > > > /* Private data of this transformer, format is opaque,
> > > > * interpreted by xfrm_type methods. */
> > > > void *data;
> > > > + enum xfrm_sa_dir dir;
> > > > };
> > > >
> > > > static inline struct net *xs_net(struct xfrm_state *x)
> > > > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > > > index 6a77328be114..2f1d67239301 100644
> > > > --- a/include/uapi/linux/xfrm.h
> > > > +++ b/include/uapi/linux/xfrm.h
> > > > @@ -141,6 +141,13 @@ enum {
> > > > XFRM_POLICY_MAX = 3
> > > > };
> > > >
> > > > +enum xfrm_sa_dir {
> > > > + XFRM_SA_DIR_UNSET = 0,
> > > > + XFRM_SA_DIR_IN = 1,
> > > > + XFRM_SA_DIR_OUT = 2,
> > > > + XFRM_SA_DIR_MAX = 3,
> > >
> > > nit: comma is redundant after a "MAX" no?
> >
> > I removed it. Actually XFRM_SA_DIR_MAX also as Leon pointed out it is not
> > necessary.
>
> Great.
>
> >
> > >
> > > > +};
> > > > +
> > > > enum {
> > > > XFRM_SHARE_ANY, /* No limitations */
> > > > XFRM_SHARE_SESSION, /* For this session only */
> > > > @@ -315,6 +322,7 @@ enum xfrm_attr_type_t {
> > > > XFRMA_SET_MARK_MASK, /* __u32 */
> > > > XFRMA_IF_ID, /* __u32 */
> > > > XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
> > > > + XFRMA_SA_DIR, /* __u8 */
> > > > __XFRMA_MAX
> > > >
> > > > #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
> > > > diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> > > > index 655fe4ff8621..de0e1508f870 100644
> > > > --- a/net/xfrm/xfrm_compat.c
> > > > +++ b/net/xfrm/xfrm_compat.c
> > > > @@ -129,6 +129,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> > > > [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> > > > [XFRMA_IF_ID] = { .type = NLA_U32 },
> > > > [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> > > > + [XFRMA_SA_DIR] = { .type = NLA_U8 },
> > > > };
> > > >
> > > > static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
> > > > @@ -277,9 +278,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
> > > > case XFRMA_SET_MARK_MASK:
> > > > case XFRMA_IF_ID:
> > > > case XFRMA_MTIMER_THRESH:
> > > > + case XFRMA_SA_DIR:
> > > > return xfrm_nla_cpy(dst, src, nla_len(src));
> > > > default:
> > > > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > > > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > > > pr_warn_once("unsupported nla_type %d\n", src->nla_type);
> > > > return -EOPNOTSUPP;
> > > > }
> > > > @@ -434,7 +436,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
> > > > int err;
> > > >
> > > > if (type > XFRMA_MAX) {
> > > > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > > > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > > > NL_SET_ERR_MSG(extack, "Bad attribute");
> > > > return -EOPNOTSUPP;
> > > > }
> > > > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > > > index 3784534c9185..11339d7d7140 100644
> > > > --- a/net/xfrm/xfrm_device.c
> > > > +++ b/net/xfrm/xfrm_device.c
> > > > @@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
> > > > + NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
> > > >
> > > > /* We don't yet support UDP encapsulation and TFC padding. */
> > > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > > index bda5327bf34d..0d6f5a49002f 100644
> > > > --- a/net/xfrm/xfrm_state.c
> > > > +++ b/net/xfrm/xfrm_state.c
> > > > @@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> > > > x->lastused = orig->lastused;
> > > > x->new_mapping = 0;
> > > > x->new_mapping_sport = 0;
> > > > + x->dir = orig->dir;
> > > >
> > > > return x;
> > > >
> > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > > > index ad01997c3aa9..fe4576e96dd4 100644
> > > > --- a/net/xfrm/xfrm_user.c
> > > > +++ b/net/xfrm/xfrm_user.c
> > > > @@ -360,6 +360,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
> > > > }
> > > > }
> > > >
> > > > + if (attrs[XFRMA_SA_DIR]) {
> > > > + u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
> > > > +
> > > > + if (!sa_dir || sa_dir >= XFRM_SA_DIR_MAX) {
> > > > + NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range");
> > > > + err = -EINVAL;
> > > > + goto out;
> > > > + }
> > > > + }
> > > > +
> > > > out:
> > > > return err;
> > > > }
> > > > @@ -627,6 +637,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
> > > > struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
> > > > struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
> > > > struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH];
> > > > + struct nlattr *dir = attrs[XFRMA_SA_DIR];
> > > >
> > > > if (re && x->replay_esn && x->preplay_esn) {
> > > > struct xfrm_replay_state_esn *replay_esn;
> > > > @@ -661,6 +672,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
> > > >
> > > > if (mt)
> > > > x->mapping_maxage = nla_get_u32(mt);
> > > > +
> > > > + if (dir)
> > > > + x->dir = nla_get_u8(dir);
> > >
> > > It's not clear to me why this belongs in xfrm_update_ae_params().
> > > IOW, why isn't this done in xfrm_state_construct(), like if_id?
> >
> > I am in favor adding direction to all NL messages which include xfrm_state
> > for consitancy. Also when offload is enabled and xuo flags falg
> > XFRM_OFFLOAD_INBOUND is set. I am hopping in the long term offload can also
> > x->dir instead of this xuo->flags.
>
> I see. Thanks for the explanation.
>
> >
> >
> > > > }
> > > >
> > > > static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
> > > > @@ -1182,8 +1196,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
> > > > if (ret)
> > > > goto out;
> > > > }
> > > > - if (x->mapping_maxage)
> > > > + if (x->mapping_maxage) {
> > > > ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage);
> > > > + if (ret)
> > > > + goto out;
> > > > + }
> > > > + if (x->dir)
> > > > + ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> > > > out:
> > > > return ret;
> > > > }
> > > > @@ -2399,7 +2418,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x)
> > > > + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur))
> > > > + nla_total_size(sizeof(struct xfrm_mark))
> > > > + nla_total_size(4) /* XFRM_AE_RTHR */
> > > > - + nla_total_size(4); /* XFRM_AE_ETHR */
> > > > + + nla_total_size(4) /* XFRM_AE_ETHR */
> > > > + + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */
> > > > }
> > > >
> > > > static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
> > > > @@ -2453,6 +2473,11 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
> > > > goto out_cancel;
> > > >
> > > > err = xfrm_if_id_put(skb, x->if_id);
> > > > + if (err)
> > > > + goto out_cancel;
> > > > + if (x->dir)
> > > > + err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
> > > > +
> > > > if (err)
> > > > goto out_cancel;
> > > >
> > > > @@ -3046,6 +3071,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
> > > > [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> > > > [XFRMA_IF_ID] = { .type = NLA_U32 },
> > > > [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> > > > + [XFRMA_SA_DIR] = { .type = NLA_U8 },
> > >
> > > Maybe add a ".strict_start_type"?
> >
> > Isn't this for the first element? I don't understand your suggestion.
> > Are you advising to add to [XFRMA_SA_DIR]? Would you like explain a bit
> > more?
>
> See comment on strict_start_type in include/net/netlink.h
So this and future values added to xfrma_policy are strictly valideated!
>
> Basically for this you'd add:
>
> [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR },
I assumet we need the same for compat_policy too?
Thanks Eyal. I will send v2 version soon.
-antony
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ipsec-next v2] xfrm: Add Direction to the SA in or out
2024-03-10 20:03 ` [PATCH ipsec-next v2] " Antony Antony
@ 2024-03-11 7:25 ` Leon Romanovsky
2024-03-12 18:29 ` Antony Antony
0 siblings, 1 reply; 10+ messages in thread
From: Leon Romanovsky @ 2024-03-11 7:25 UTC (permalink / raw)
To: Antony Antony; +Cc: Steffen Klassert, Herbert Xu, netdev, devel
On Sun, Mar 10, 2024 at 09:03:47PM +0100, Antony Antony wrote:
> This patch introduces the 'dir' attribute, 'in' or 'out', to the
> xfrm_state, SA, enhancing usability by delineating the scope of values
> based on direction. An input SA will now exclusively encompass values
> pertinent to input, effectively segregating them from output-related
> values. This change aims to streamline the configuration process and
> improve the overall clarity of SA attributes.
>
> v1->v2:
Please try to avoid replying new patch versions as reply-to previous
versions. It breaks the threading and makes it hard to track the patches.
> - use .strict_start_type in struct nla_policy xfrma_policy
> - delete redundant XFRM_SA_DIR_MAX enum
Please put changelog after ---, it will allow us to make sure that
commit message will be clean once the patch is applied.
>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
> include/net/xfrm.h | 1 +
> include/uapi/linux/xfrm.h | 7 +++++++
> net/xfrm/xfrm_compat.c | 7 +++++--
> net/xfrm/xfrm_device.c | 5 +++++
> net/xfrm/xfrm_state.c | 1 +
> net/xfrm/xfrm_user.c | 44 +++++++++++++++++++++++++++++++++++----
> 6 files changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 1d107241b901..91348a03469c 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -289,6 +289,7 @@ struct xfrm_state {
> /* Private data of this transformer, format is opaque,
> * interpreted by xfrm_type methods. */
> void *data;
> + enum xfrm_sa_dir dir;
> };
>
> static inline struct net *xs_net(struct xfrm_state *x)
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index 6a77328be114..46a9770c720c 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -141,6 +141,12 @@ enum {
> XFRM_POLICY_MAX = 3
> };
>
> +enum xfrm_sa_dir {
> + XFRM_SA_DIR_UNSET = 0,
It doesn't belong to UAPI. Kernel and user space use netlink attribute to understand
if direction is set.
> + XFRM_SA_DIR_IN = 1,
> + XFRM_SA_DIR_OUT = 2
> +};
> +
> enum {
> XFRM_SHARE_ANY, /* No limitations */
> XFRM_SHARE_SESSION, /* For this session only */
> @@ -315,6 +321,7 @@ enum xfrm_attr_type_t {
> XFRMA_SET_MARK_MASK, /* __u32 */
> XFRMA_IF_ID, /* __u32 */
> XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
> + XFRMA_SA_DIR, /* __u8 */
> __XFRMA_MAX
>
> #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
> diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> index 655fe4ff8621..007dee03b1bc 100644
> --- a/net/xfrm/xfrm_compat.c
> +++ b/net/xfrm/xfrm_compat.c
> @@ -98,6 +98,7 @@ static const int compat_msg_min[XFRM_NR_MSGTYPES] = {
> };
>
> static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR },
> [XFRMA_SA] = { .len = XMSGSIZE(compat_xfrm_usersa_info)},
> [XFRMA_POLICY] = { .len = XMSGSIZE(compat_xfrm_userpolicy_info)},
> [XFRMA_LASTUSED] = { .type = NLA_U64},
> @@ -129,6 +130,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> [XFRMA_IF_ID] = { .type = NLA_U32 },
> [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> + [XFRMA_SA_DIR] = { .type = NLA_U8}
> };
>
> static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
> @@ -277,9 +279,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
> case XFRMA_SET_MARK_MASK:
> case XFRMA_IF_ID:
> case XFRMA_MTIMER_THRESH:
> + case XFRMA_SA_DIR:
> return xfrm_nla_cpy(dst, src, nla_len(src));
> default:
> - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> pr_warn_once("unsupported nla_type %d\n", src->nla_type);
> return -EOPNOTSUPP;
> }
> @@ -434,7 +437,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
> int err;
>
> if (type > XFRMA_MAX) {
> - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> NL_SET_ERR_MSG(extack, "Bad attribute");
> return -EOPNOTSUPP;
> }
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 3784534c9185..11339d7d7140 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
> return -EINVAL;
> }
>
> + if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
It will break backward compatibility. In old iproute2 XFRM_OFFLOAD_INBOUND is set, but x->dir is not set yet.
> + NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
> + return -EINVAL;
> + }
> +
> is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
Thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH ipsec-next v2] xfrm: Add Direction to the SA in or out
2024-03-11 7:25 ` Leon Romanovsky
@ 2024-03-12 18:29 ` Antony Antony
0 siblings, 0 replies; 10+ messages in thread
From: Antony Antony @ 2024-03-12 18:29 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Antony Antony, netdev, devel, Herbert Xu
Hi Leon,
On Mon, Mar 11, 2024 at 09:25:45AM +0200, Leon Romanovsky via Devel wrote:
> On Sun, Mar 10, 2024 at 09:03:47PM +0100, Antony Antony wrote:
> > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > xfrm_state, SA, enhancing usability by delineating the scope of values
> > based on direction. An input SA will now exclusively encompass values
> > pertinent to input, effectively segregating them from output-related
> > values. This change aims to streamline the configuration process and
> > improve the overall clarity of SA attributes.
> >
> > v1->v2:
>
> Please try to avoid replying new patch versions as reply-to previous
> versions. It breaks the threading and makes it hard to track the patches.
>
> > - use .strict_start_type in struct nla_policy xfrma_policy
> > - delete redundant XFRM_SA_DIR_MAX enum
>
> Please put changelog after ---, it will allow us to make sure that
> commit message will be clean once the patch is applied.
>
> >
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > ---
> > include/net/xfrm.h | 1 +
> > include/uapi/linux/xfrm.h | 7 +++++++
> > net/xfrm/xfrm_compat.c | 7 +++++--
> > net/xfrm/xfrm_device.c | 5 +++++
> > net/xfrm/xfrm_state.c | 1 +
> > net/xfrm/xfrm_user.c | 44 +++++++++++++++++++++++++++++++++++----
> > 6 files changed, 59 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > index 1d107241b901..91348a03469c 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -289,6 +289,7 @@ struct xfrm_state {
> > /* Private data of this transformer, format is opaque,
> > * interpreted by xfrm_type methods. */
> > void *data;
> > + enum xfrm_sa_dir dir;
> > };
> >
> > static inline struct net *xs_net(struct xfrm_state *x)
> > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > index 6a77328be114..46a9770c720c 100644
> > --- a/include/uapi/linux/xfrm.h
> > +++ b/include/uapi/linux/xfrm.h
> > @@ -141,6 +141,12 @@ enum {
> > XFRM_POLICY_MAX = 3
> > };
> >
> > +enum xfrm_sa_dir {
> > + XFRM_SA_DIR_UNSET = 0,
>
> It doesn't belong to UAPI. Kernel and user space use netlink attribute to understand
> if direction is set.
I found it is useful to check "x->dir != XFRM_SA_DIR_UNSET for e.g. in
iproute2:) We could check that in different ways. I removed it as you
prefer.
>
> > + XFRM_SA_DIR_IN = 1,
> > + XFRM_SA_DIR_OUT = 2
> > +};
> > +
> > enum {
> > XFRM_SHARE_ANY, /* No limitations */
> > XFRM_SHARE_SESSION, /* For this session only */
> > @@ -315,6 +321,7 @@ enum xfrm_attr_type_t {
> > XFRMA_SET_MARK_MASK, /* __u32 */
> > XFRMA_IF_ID, /* __u32 */
> > XFRMA_MTIMER_THRESH, /* __u32 in seconds for input SA */
> > + XFRMA_SA_DIR, /* __u8 */
> > __XFRMA_MAX
> >
> > #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK /* Compatibility */
> > diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> > index 655fe4ff8621..007dee03b1bc 100644
> > --- a/net/xfrm/xfrm_compat.c
> > +++ b/net/xfrm/xfrm_compat.c
> > @@ -98,6 +98,7 @@ static const int compat_msg_min[XFRM_NR_MSGTYPES] = {
> > };
> >
> > static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> > + [XFRMA_UNSPEC] = { .strict_start_type = XFRMA_SA_DIR },
> > [XFRMA_SA] = { .len = XMSGSIZE(compat_xfrm_usersa_info)},
> > [XFRMA_POLICY] = { .len = XMSGSIZE(compat_xfrm_userpolicy_info)},
> > [XFRMA_LASTUSED] = { .type = NLA_U64},
> > @@ -129,6 +130,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
> > [XFRMA_SET_MARK_MASK] = { .type = NLA_U32 },
> > [XFRMA_IF_ID] = { .type = NLA_U32 },
> > [XFRMA_MTIMER_THRESH] = { .type = NLA_U32 },
> > + [XFRMA_SA_DIR] = { .type = NLA_U8}
> > };
> >
> > static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
> > @@ -277,9 +279,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
> > case XFRMA_SET_MARK_MASK:
> > case XFRMA_IF_ID:
> > case XFRMA_MTIMER_THRESH:
> > + case XFRMA_SA_DIR:
> > return xfrm_nla_cpy(dst, src, nla_len(src));
> > default:
> > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > pr_warn_once("unsupported nla_type %d\n", src->nla_type);
> > return -EOPNOTSUPP;
> > }
> > @@ -434,7 +437,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
> > int err;
> >
> > if (type > XFRMA_MAX) {
> > - BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
> > + BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
> > NL_SET_ERR_MSG(extack, "Bad attribute");
> > return -EOPNOTSUPP;
> > }
> > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > index 3784534c9185..11339d7d7140 100644
> > --- a/net/xfrm/xfrm_device.c
> > +++ b/net/xfrm/xfrm_device.c
> > @@ -253,6 +253,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
> > return -EINVAL;
> > }
> >
> > + if (xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir != XFRM_SA_DIR_IN) {
>
> It will break backward compatibility. In old iproute2 XFRM_OFFLOAD_INBOUND is set, but x->dir is not set yet.
I meant to check "x->dir == XFRM_SA_DIR_OUT"
thanks,
-antony
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-12 18:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-03 21:08 [PATCH ipsec-next] xfrm: Add Direction to the SA in or out Antony Antony
2024-03-03 22:30 ` Eyal Birger
2024-03-06 18:38 ` [DKIM] Re: [devel-ipsec] " Antony Antony
2024-03-06 18:57 ` Eyal Birger
2024-03-10 20:05 ` Antony Antony
2024-03-04 7:42 ` Leon Romanovsky
2024-03-06 19:28 ` [DKIM] Re: [devel-ipsec] " Antony Antony
2024-03-10 20:03 ` [PATCH ipsec-next v2] " Antony Antony
2024-03-11 7:25 ` Leon Romanovsky
2024-03-12 18:29 ` Antony Antony
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).