| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZfCfA3ygjSRDnMme@Antony2201.local>
Date: Tue, 12 Mar 2024 19:29:23 +0100
From: Antony Antony <antony@...nome.org>
To: Leon Romanovsky <leon@...nel.org>
Cc: Antony Antony <antony.antony@...unet.com>, netdev@...r.kernel.org,
devel@...ux-ipsec.org, Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH ipsec-next v2] xfrm: Add Direction to the SA in or out
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@...unet.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
Powered by blists - more mailing lists