On 12/3/2017 2:16 PM, Yossi Kuperman wrote:
-----Original Message-----
From: Shannon Nelson [mailto:shannon.nel...@oracle.com]
Sent: Sunday, December 3, 2017 12:11 AM
To: Aviv Heller <av...@mellanox.com>; Steffen Klassert
<steffen.klass...@secunet.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>; Boris Pismenny
<bor...@mellanox.com>; Yossi Kuperman <yoss...@mellanox.com>;
Yevgeny Kliteynik <klit...@mellanox.com>; netdev@vger.kernel.org
Subject: Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
On 12/1/2017 11:47 AM, Shannon Nelson wrote:
On 11/28/2017 9:55 AM, 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.
In order to inhibit driver offload logic from processing the state's
packets prior to the xfrm_state object being completely initialized
and added to the SADBs, a new activate() operation was added to
inform the driver the aforementioned conditions have been met.
Are there also conditions where you would want to temporarily
deactivate, or pause, the incoming driver offload, followed then by
another activate?
Nope.
sln
Instead of setting up a half-ready state that needs the activate() operation to
finish, can we instead just move the xfrm_dev_state_add() call to after the
xfrm_init_replay()? Especially since this really only makes sense for the
inbound, and makes no sense for the outbound path.
It might solve the problem this time, but still the state is not fully
initialized and it might break
in the future. Adding an SA to hardware can fail, and if we were to place
xfrm_dev_state_add()
after the SA is fully initialized, we will be forced to roll-back. Let alone
the possible things that could
go wrong once an SA is fully active while the hardware is oblivious. The core
issue here is that we
can't construct the SA and configure the hardware atomically, hence the two
steps. The first step is
to ensure the hardware can be configured properly, if not abort before the SA
is initialized, and the
second step is to just "mark" the state as active. Please note that the we
don't explicitly mark it, it
is active just by its existence in the hash table, which we already have and
maintain.
Out of curiosity, what's wrong with adding yet another callback (not on the
critical-path)?
The existing model for hardware offloads is a single call to the driver
to enable or disable - either the hardware does the offload or it
doesn't. The closest examples are probably mac filters and vlan tags,
as well as flow filters: in all three cases the stack hands the offload
request to the driver and expects the offload to start at that point.
There is no in-between step of "here's an offload, but don't do it yet".
In reference the comment elsewhere about being "symmetric with
xdo_dev_state_delete and xdo_dev_state_free", I'm not convinced that the
xdo_dev_state_free is necessary: if the driver was already asked to
delete the SA, why should it care about a free later?
The driver model should be kept simple. There's no reason that I can
see for the driver to need a two-step commit, either for the add or for
the delete. When the stack asks the driver to do an offload, it should
be ready for the offload to happen.
sln
Thanks for your comments.
sln
Signed-off-by: Aviv Heller <av...@mellanox.com>
Signed-off-by: Yossi Kuperman <yoss...@mellanox.com>
---
v1 -> v2:
- Separate to state addition and then activation, instead
of relocating dev state addition call.
---
include/linux/netdevice.h | 1 +
include/net/xfrm.h | 12 ++++++++++++
net/xfrm/xfrm_user.c | 5 +++++
3 files changed, 18 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2eaac7d..c6ca356 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -819,6 +819,7 @@ struct netdev_xdp {
#ifdef CONFIG_XFRM_OFFLOAD
struct xfrmdev_ops {
int (*xdo_dev_state_add) (struct xfrm_state *x);
+ void (*xdo_dev_state_activate) (struct xfrm_state *x);
void (*xdo_dev_state_delete) (struct xfrm_state *x);
void (*xdo_dev_state_free) (struct xfrm_state *x);
bool (*xdo_dev_offload_ok) (struct sk_buff *skb, diff --git
a/include/net/xfrm.h b/include/net/xfrm.h index e015e16..324374e
100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1877,6 +1877,14 @@ static inline bool xfrm_dst_offload_ok(struct
dst_entry *dst)
return false;
}
+static inline void xfrm_dev_state_activate(struct xfrm_state *x) {
+ struct xfrm_state_offload *xso = &x->xso;
+
+ if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_activate)
+ xso->dev->xfrmdev_ops->xdo_dev_state_activate(x);
+}
+
static inline void xfrm_dev_state_delete(struct xfrm_state *x)
{
struct xfrm_state_offload *xso = &x->xso; @@ -1907,6 +1915,10
@@ static inline int xfrm_dev_state_add(struct net *net, struct
xfrm_state *x, stru
return 0;
}
+static inline void xfrm_dev_state_activate(struct xfrm_state *x) { }
+
static inline void xfrm_dev_state_delete(struct xfrm_state *x)
{
}
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index
e44a0fe..d06f579 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -662,6 +662,11 @@ static int xfrm_add_sa(struct sk_buff *skb,
struct nlmsghdr *nlh,
goto out;
}
+ spin_lock_bh(&x->lock);
+ if (x->km.state == XFRM_STATE_VALID)
+ xfrm_dev_state_activate(x);
+ spin_unlock_bh(&x->lock);
+
c.seq = nlh->nlmsg_seq;
c.portid = nlh->nlmsg_pid;
c.event = nlh->nlmsg_type;
