-----Original message-----
> From: Steffen Klassert
> Sent: Thursday, October 26 2017, 9:16 am
> To: Aviv Heller
> Cc: netdev-ow...@vger.kernel.org; av...@mellanox.com; Herbert Xu; Boris
> Pismenny; Yossi Kuperman; Yevgeny Kliteynik; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to
> occur after insertion
>
> On Wed, Oct 25, 2017 at 01:09:44PM +0000, Aviv Heller wrote:
> > -----Original message-----
> > > From: Steffen Klassert
> > > Sent: Wednesday, October 25 2017, 10:22 am
> > > To: av...@mellanox.com
> > > Cc: Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik;
> > > netdev@vger.kernel.org
> > > Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to
> > > occur after insertion
> > >
> > > On Tue, Oct 24, 2017 at 06:10:30PM +0300, av...@mellanox.com wrote:
> > > > From: Aviv Heller <av...@mellanox.com>
> > > >
> > > > Adding the state to the offload device prior to replay init in
> > > > xfrm_state_construct() will result in NULL dereference if a matching
> > > > ESP packet is received in between.
> > > >
> > > > Adding it after insertion also has the benefit of the driver not having
> > > > to check whether a state with the same match criteria already exists,
> > > > but forces us to use an atomic type for the offload_handle, to make
> > > > certain a stack-read/driver-write race won't result in reading corrupt
> > > > data.
> > >
> > > No, this will add multiple atomic operations to the packet path,
> > > even in the non offloaded case.
> > >
> > > I think the problem is that we set XFRM_STATE_VALID to early.
> > > This was not a problem before we had offloading because
> > > it was not possible to lookup this state before we inserted
> > > it into the SADB. Now that the driver holds a subset of states
> > > too, we need to make sure the state is fully initialized
> > > before we mark it as valid.
> > >
> > > The patch below should do it, in combination with your patch 1/3.
> > >
> > > Could you please test this?
> > >
> > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > > index b997f13..96eb263 100644
> > > --- a/net/xfrm/xfrm_user.c
> > > +++ b/net/xfrm/xfrm_user.c
> > > @@ -587,10 +587,6 @@ static struct xfrm_state
> > > *xfrm_state_construct(struct net *net,
> > > if (attrs[XFRMA_OUTPUT_MARK])
> > > x->props.output_mark = nla_get_u32(attrs[XFRMA_OUTPUT_MARK]);
> > >
> > > - err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
> > > - if (err)
> > > - goto error;
> > > -
> > > if (attrs[XFRMA_SEC_CTX]) {
> > > err = security_xfrm_state_alloc(x,
> > > nla_data(attrs[XFRMA_SEC_CTX]));
> > > @@ -620,6 +616,10 @@ static struct xfrm_state
> > > *xfrm_state_construct(struct net *net,
> > > /* override default values from above */
> > > xfrm_update_ae_params(x, attrs, 0);
> > >
> > > + err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
> > > + if (err)
> > > + goto error;
> > > +
> > > return x;
> > >
> > > error:
> > >
> >
> > Hi Steffen,
> >
> > This patch does not work, due to:
> > if (!x->type_offload)
> > return -EINVAL;
> >
> > test in xfrm_dev_state_add().
>
> There is certainly a way arround that :)
> The easiest I can think of would be to propagate XFRM_STATE_VALID
> only after the state is inserted into the SADBs. I.e. move the
> setting of XFRM_STATE_VALID out of __xfrm_init_state() and let the
> callers do it.
This does seem like the easiest solution, if we don't move state addition to
occur after insertion.
I'm waiting for our regression results (probably on Monday) for the patch
below, and would appreciate your comments:
From 4da5c12abb59113428668afffc42eb51c48fa894 Mon Sep 17 00:00:00 2001
From: Aviv Heller <av...@mellanox.com>
Date: Thu, 26 Oct 2017 16:30:41 +0300
Subject: [PATCH] Temp
Signed-off-by: Aviv Heller <av...@mellanox.com>
---
net/ipv4/ipcomp.c | 1 +
net/ipv6/ipcomp6.c | 1 +
net/key/af_key.c | 1 +
net/xfrm/xfrm_state.c | 6 +++---
net/xfrm/xfrm_user.c | 16 +++++++++-------
5 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/net/ipv4/ipcomp.c b/net/ipv4/ipcomp.c
index d97f4f2..0016162 100644
--- a/net/ipv4/ipcomp.c
+++ b/net/ipv4/ipcomp.c
@@ -80,6 +80,7 @@ static struct xfrm_state *ipcomp_tunnel_create(struct
xfrm_state *x)
if (xfrm_init_state(t))
goto error;
+ t->km.state = XFRM_STATE_VALID;
atomic_set(&t->tunnel_users, 1);
out:
return t;
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 54d165b..95b67f3 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -107,6 +107,7 @@ static struct xfrm_state *ipcomp6_tunnel_create(struct
xfrm_state *x)
if (xfrm_init_state(t))
goto error;
+ t->km.state = XFRM_STATE_VALID;
atomic_set(&t->tunnel_users, 1);
out:
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 10d7133..d1d43f7 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1276,6 +1276,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct
net *net,
goto out;
x->km.seq = hdr->sadb_msg_seq;
+ x->km.state = XFRM_STATE_VALID;
return x;
out:
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index a41e2ef..38b90fd 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1401,10 +1401,12 @@ static struct xfrm_state *xfrm_state_clone(struct
xfrm_state *orig,
x->km.seq = orig->km.seq;
x->replay = orig->replay;
x->preplay = orig->preplay;
+ x->km.state = XFRM_STATE_VALID;
return x;
- error:
+error:
+ x->km.state = XFRM_STATE_DEAD;
xfrm_state_put(x);
out:
return NULL;
@@ -2256,8 +2258,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool
init_replay, bool offload)
goto error;
}
- x->km.state = XFRM_STATE_VALID;
-
error:
return err;
}
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index ffe8d5e..df304c2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -595,13 +595,6 @@ static struct xfrm_state *xfrm_state_construct(struct net
*net,
goto error;
}
- if (attrs[XFRMA_OFFLOAD_DEV]) {
- err = xfrm_dev_state_add(net, x,
- nla_data(attrs[XFRMA_OFFLOAD_DEV]));
- if (err)
- goto error;
- }
-
if ((err = xfrm_alloc_replay_state_esn(&x->replay_esn, &x->preplay_esn,
attrs[XFRMA_REPLAY_ESN_VAL])))
goto error;
@@ -617,6 +610,15 @@ static struct xfrm_state *xfrm_state_construct(struct net
*net,
/* override default values from above */
xfrm_update_ae_params(x, attrs, 0);
+ x->km.state = XFRM_STATE_VALID;
+
+ if (attrs[XFRMA_OFFLOAD_DEV]) {
+ err = xfrm_dev_state_add(net, x,
+ nla_data(attrs[XFRMA_OFFLOAD_DEV]));
+ if (err)
+ goto error;
+ }
+
return x;
error:
--
1.8.3.1
