| 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
| ||
|
Date: Sun, 3 Dec 2017 22:16:26 +0000
From: Yossi Kuperman <yossiku@...lanox.com>
To: Shannon Nelson <shannon.nelson@...cle.com>,
Aviv Heller <avivh@...lanox.com>,
Steffen Klassert <steffen.klassert@...unet.com>
CC: Herbert Xu <herbert@...dor.apana.org.au>,
Boris Pismenny <borisp@...lanox.com>,
Yevgeny Kliteynik <kliteyn@...lanox.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op
> -----Original Message-----
> From: Shannon Nelson [mailto:shannon.nelson@...cle.com]
> Sent: Sunday, December 3, 2017 12:11 AM
> To: Aviv Heller <avivh@...lanox.com>; Steffen Klassert
> <steffen.klassert@...unet.com>
> Cc: Herbert Xu <herbert@...dor.apana.org.au>; Boris Pismenny
> <borisp@...lanox.com>; Yossi Kuperman <yossiku@...lanox.com>;
> Yevgeny Kliteynik <kliteyn@...lanox.com>; netdev@...r.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, avivh@...lanox.com wrote:
> >> From: Aviv Heller <avivh@...lanox.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)?
Thanks for your comments.
> sln
>
> >
> >>
> >> Signed-off-by: Aviv Heller <avivh@...lanox.com>
> >> Signed-off-by: Yossi Kuperman <yossiku@...lanox.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;
> >>
Powered by blists - more mailing lists