[PATCH net-next 1/3] xfrm: Fix xfrm_replay_overflow_offload_esn

[PATCH net-next 1/3] xfrm: Fix xfrm_replay_overflow_offload_esn

[yossefe]

From: Yossef Efraim <yossefe@mellanox.com>

In case of wrap around, replay_esn->oseq_hi is not updated
before it is tested for it's actual value, leading function
to fail with overflow indication and packets being dropped.

This patch updates replay_esn->oseq_hi in the right place.

Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for offloading")
Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
---
 net/xfrm/xfrm_replay.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
index 8b23c5b..0250181 100644
--- a/net/xfrm/xfrm_replay.c
+++ b/net/xfrm/xfrm_replay.c
@@ -666,7 +666,7 @@ static int xfrm_replay_overflow_offload_esn(struct xfrm_state *x, struct sk_buff
 		if (unlikely(oseq < replay_esn->oseq)) {
 			XFRM_SKB_CB(skb)->seq.output.hi = ++oseq_hi;
 			xo->seq.hi = oseq_hi;
-
+			replay_esn->oseq_hi = oseq_hi;
 			if (replay_esn->oseq_hi == 0) {
 				replay_esn->oseq--;
 				replay_esn->oseq_hi--;
@@ -678,7 +678,6 @@ static int xfrm_replay_overflow_offload_esn(struct xfrm_state *x, struct sk_buff
 		}
 
 		replay_esn->oseq = oseq;
-		replay_esn->oseq_hi = oseq_hi;
 
 		if (xfrm_aevent_is_on(net))
 			x->repl->notify(x, XFRM_REPLAY_UPDATE);
-- 
2.8.1

[PATCH net-next 2/3] xfrm: Fix xfrm_dev_state_add to fail for unsupported HW SA option

[yossefe]

From: Yossef Efraim <yossefe@mellanox.com>

xfrm_dev_state_add function returns success for unsupported HW SA options.
Resulting the calling function to create SW SA without corrlating HW SA.
Desipte IPSec device offloading option was chosen.
These not supported HW SA options are hard coded within xfrm_dev_state_add
function.
SW backward compatibility will break if we add any of these option as old
HW will fail with new SW.

This patch changes the behaviour to return -EINVAL in case unsupported
option is chosen.
Notifying user application regarding failure and not breaking backward
compatibility for newly added HW SA options.

Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
---
 net/xfrm/xfrm_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 30e5746..dc68d9c 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -67,7 +67,7 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 
 	/* We don't yet support UDP encapsulation, TFC padding and ESN. */
 	if (x->encap || x->tfcpad || (x->props.flags & XFRM_STATE_ESN))
-		return 0;
+		return -EINVAL;
 
 	dev = dev_get_by_index(net, xuo->ifindex);
 	if (!dev) {
-- 
2.8.1

[PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

[yossefe]

From: Yossef Efraim <yossefe@mellanox.com>

This patch adds ESN support to IPsec device offload.
Adding new xfrm device operation to synchronize device ESN.

Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
---
 include/linux/netdevice.h |  1 +
 include/net/xfrm.h        | 12 ++++++++++++
 net/xfrm/xfrm_device.c    |  4 ++--
 net/xfrm/xfrm_replay.c    |  2 ++
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7de7656..d4e9198 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -825,6 +825,7 @@ struct xfrmdev_ops {
 	void	(*xdo_dev_state_free) (struct xfrm_state *x);
 	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
 				       struct xfrm_state *x);
+	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
 };
 #endif
 
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index dc28a98..372bfcb 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1863,6 +1863,14 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		       struct xfrm_user_offload *xuo);
 bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x);
 
+static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
+{
+	struct xfrm_state_offload *xso = &x->xso;
+
+	if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn)
+		xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);
+}
+
 static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
 {
 	struct xfrm_state *x = dst->xfrm;
@@ -1920,6 +1928,10 @@ static inline bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x
 	return false;
 }
 
+static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
+{
+}
+
 static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
 {
 	return false;
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index dc68d9c..fc7e1e44 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -65,8 +65,8 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 	if (!x->type_offload)
 		return -EINVAL;
 
-	/* We don't yet support UDP encapsulation, TFC padding and ESN. */
-	if (x->encap || x->tfcpad || (x->props.flags & XFRM_STATE_ESN))
+	/* We don't yet support UDP encapsulation and TFC padding. */
+	if (x->encap || x->tfcpad)
 		return -EINVAL;
 
 	dev = dev_get_by_index(net, xuo->ifindex);
diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
index 0250181..1d38c6a 100644
--- a/net/xfrm/xfrm_replay.c
+++ b/net/xfrm/xfrm_replay.c
@@ -551,6 +551,8 @@ static void xfrm_replay_advance_esn(struct xfrm_state *x, __be32 net_seq)
 			bitnr = replay_esn->replay_window - (diff - pos);
 	}
 
+	xfrm_dev_state_advance_esn(x);
+
 	nr = bitnr >> 5;
 	bitnr = bitnr & 0x1F;
 	replay_esn->bmp[nr] |= (1U << bitnr);
-- 
2.8.1

Re: [PATCH net-next 1/3] xfrm: Fix xfrm_replay_overflow_offload_esn

[Steffen Klassert]

On Tue, Nov 28, 2017 at 11:49:28AM +0200, yossefe@mellanox.com wrote:
> From: Yossef Efraim <yossefe@mellanox.com>
> 
> In case of wrap around, replay_esn->oseq_hi is not updated
> before it is tested for it's actual value, leading function
> to fail with overflow indication and packets being dropped.
> 
> This patch updates replay_esn->oseq_hi in the right place.
> 
> Fixes: d7dbefc45cf5 ("xfrm: Add xfrm_replay_overflow functions for offloading")
> Signed-off-by: Yossef Efraim <yossefe@mellanox.com>

Applied to ipsec-next, thanks!

Re: [PATCH net-next 2/3] xfrm: Fix xfrm_dev_state_add to fail for unsupported HW SA option

[Steffen Klassert]

On Tue, Nov 28, 2017 at 11:49:29AM +0200, yossefe@mellanox.com wrote:
> From: Yossef Efraim <yossefe@mellanox.com>
> 
> xfrm_dev_state_add function returns success for unsupported HW SA options.
> Resulting the calling function to create SW SA without corrlating HW SA.
> Desipte IPSec device offloading option was chosen.
> These not supported HW SA options are hard coded within xfrm_dev_state_add
> function.
> SW backward compatibility will break if we add any of these option as old
> HW will fail with new SW.
> 
> This patch changes the behaviour to return -EINVAL in case unsupported
> option is chosen.
> Notifying user application regarding failure and not breaking backward
> compatibility for newly added HW SA options.
> 
> Signed-off-by: Yossef Efraim <yossefe@mellanox.com>

Also applied to ipsec-next, thanks a lot!

Re: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

[Steffen Klassert]

On Tue, Nov 28, 2017 at 11:49:30AM +0200, yossefe@mellanox.com wrote:
> From: Yossef Efraim <yossefe@mellanox.com>
> 
> This patch adds ESN support to IPsec device offload.
> Adding new xfrm device operation to synchronize device ESN.
> 
> Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
> ---
>  include/linux/netdevice.h |  1 +
>  include/net/xfrm.h        | 12 ++++++++++++
>  net/xfrm/xfrm_device.c    |  4 ++--
>  net/xfrm/xfrm_replay.c    |  2 ++
>  4 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7de7656..d4e9198 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -825,6 +825,7 @@ struct xfrmdev_ops {
>  	void	(*xdo_dev_state_free) (struct xfrm_state *x);
>  	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
>  				       struct xfrm_state *x);
> +	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);

We now have a documentation for the xfrm offloadin API in the
ipsec-next tree. Please document the new device operation
there and resubmit.

Thanks!

Re: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

[Shannon Nelson]

On 11/30/2017 10:23 PM, Steffen Klassert wrote:
> On Tue, Nov 28, 2017 at 11:49:30AM +0200, yossefe@mellanox.com wrote:
>> From: Yossef Efraim <yossefe@mellanox.com>
>>
>> This patch adds ESN support to IPsec device offload.
>> Adding new xfrm device operation to synchronize device ESN.
>>
>> Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
>> ---
>>   include/linux/netdevice.h |  1 +
>>   include/net/xfrm.h        | 12 ++++++++++++
>>   net/xfrm/xfrm_device.c    |  4 ++--
>>   net/xfrm/xfrm_replay.c    |  2 ++
>>   4 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 7de7656..d4e9198 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -825,6 +825,7 @@ struct xfrmdev_ops {
>>   	void	(*xdo_dev_state_free) (struct xfrm_state *x);
>>   	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
>>   				       struct xfrm_state *x);
>> +	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
> 
> We now have a documentation for the xfrm offloadin API in the
> ipsec-next tree. Please document the new device operation
> there and resubmit.

Please be sure to specify what the offloading driver is expected to do 
with this.

sln

> 
> Thanks!
> 

Re: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

[Shannon Nelson]

On 11/28/2017 1:49 AM, yossefe@mellanox.com wrote:
> From: Yossef Efraim <yossefe@mellanox.com>
> 
> This patch adds ESN support to IPsec device offload.
> Adding new xfrm device operation to synchronize device ESN.
> 
> Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
> ---
>   include/linux/netdevice.h |  1 +
>   include/net/xfrm.h        | 12 ++++++++++++
>   net/xfrm/xfrm_device.c    |  4 ++--
>   net/xfrm/xfrm_replay.c    |  2 ++
>   4 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7de7656..d4e9198 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -825,6 +825,7 @@ struct xfrmdev_ops {
>   	void	(*xdo_dev_state_free) (struct xfrm_state *x);
>   	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
>   				       struct xfrm_state *x);
> +	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
>   };
>   #endif
>   
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index dc28a98..372bfcb 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1863,6 +1863,14 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>   		       struct xfrm_user_offload *xuo);
>   bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x);
>   
> +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
> +{
> +	struct xfrm_state_offload *xso = &x->xso;
> +
> +	if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn)
> +		xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);

If there isn't an implementation for xdo_dev_state_advance_esn, should 
this even have been called?  See my comment below about checking the 
XFRM_STATE_ESN.

What is the driver expected to do with this?  I would guess that maybe 
the hardware doing the offload needs to know when the ESN is advanced so 
that it can change the ICV calculation accordingly, but that only is 
useful for hardware that knows about ESN.

> +}
> +
>   static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
>   {
>   	struct xfrm_state *x = dst->xfrm;
> @@ -1920,6 +1928,10 @@ static inline bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x
>   	return false;
>   }
>   
> +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
> +{
> +}
> +
>   static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
>   {
>   	return false;
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index dc68d9c..fc7e1e44 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -65,8 +65,8 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>   	if (!x->type_offload)
>   		return -EINVAL;
>   
> -	/* We don't yet support UDP encapsulation, TFC padding and ESN. */
> -	if (x->encap || x->tfcpad || (x->props.flags & XFRM_STATE_ESN))
> +	/* We don't yet support UDP encapsulation and TFC padding. */
> +	if (x->encap || x->tfcpad)
>   		return -EINVAL;

Maybe we should check to see that xdo_dev_state_advance_esn is 
implemented before allowing XFRM_STATE_ESN?

sln

>   
>   	dev = dev_get_by_index(net, xuo->ifindex);
> diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
> index 0250181..1d38c6a 100644
> --- a/net/xfrm/xfrm_replay.c
> +++ b/net/xfrm/xfrm_replay.c
> @@ -551,6 +551,8 @@ static void xfrm_replay_advance_esn(struct xfrm_state *x, __be32 net_seq)
>   			bitnr = replay_esn->replay_window - (diff - pos);
>   	}
>   
> +	xfrm_dev_state_advance_esn(x);
> +
>   	nr = bitnr >> 5;
>   	bitnr = bitnr & 0x1F;
>   	replay_esn->bmp[nr] |= (1U << bitnr);
> 

RE: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

[Yossef Efraim]

Ok 
Doc updated.
Sending this patch again with all the fixes.

Thanks!

> -----Original Message-----
> From: Steffen Klassert [mailto:steffen.klassert@secunet.com]
> Sent: Friday, December 01, 2017 8:23 AM
> To: Yossef Efraim <yossefe@mellanox.com>
> Cc: David S. Miller <davem@davemloft.net>; Herbert Xu
> <herbert@gondor.apana.org.au>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Boris Pismenny <borisp@mellanox.com>; Yevgeny
> Kliteynik <kliteyn@mellanox.com>; Yossi Kuperman <yossiku@mellanox.com>
> Subject: Re: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload
> 
> On Tue, Nov 28, 2017 at 11:49:30AM +0200, yossefe@mellanox.com wrote:
> > From: Yossef Efraim <yossefe@mellanox.com>
> >
> > This patch adds ESN support to IPsec device offload.
> > Adding new xfrm device operation to synchronize device ESN.
> >
> > Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
> > ---
> >  include/linux/netdevice.h |  1 +
> >  include/net/xfrm.h        | 12 ++++++++++++
> >  net/xfrm/xfrm_device.c    |  4 ++--
> >  net/xfrm/xfrm_replay.c    |  2 ++
> >  4 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 7de7656..d4e9198 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -825,6 +825,7 @@ struct xfrmdev_ops {
> >  	void	(*xdo_dev_state_free) (struct xfrm_state *x);
> >  	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
> >  				       struct xfrm_state *x);
> > +	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
> 
> We now have a documentation for the xfrm offloadin API in the ipsec-next tree.
> Please document the new device operation there and resubmit.
> 
> Thanks!

RE: [PATCH net-next 3/3] xfrm: Add ESN support for IPSec HW offload

[Yossef Efraim]

> On 11/28/2017 1:49 AM, yossefe@mellanox.com wrote:
> > From: Yossef Efraim <yossefe@mellanox.com>
> >
> > This patch adds ESN support to IPsec device offload.
> > Adding new xfrm device operation to synchronize device ESN.
> >
> > Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
> > ---
> >   include/linux/netdevice.h |  1 +
> >   include/net/xfrm.h        | 12 ++++++++++++
> >   net/xfrm/xfrm_device.c    |  4 ++--
> >   net/xfrm/xfrm_replay.c    |  2 ++
> >   4 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 7de7656..d4e9198 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -825,6 +825,7 @@ struct xfrmdev_ops {
> >   	void	(*xdo_dev_state_free) (struct xfrm_state *x);
> >   	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
> >   				       struct xfrm_state *x);
> > +	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
> >   };
> >   #endif
> >
> > diff --git a/include/net/xfrm.h b/include/net/xfrm.h index
> > dc28a98..372bfcb 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -1863,6 +1863,14 @@ int xfrm_dev_state_add(struct net *net, struct
> xfrm_state *x,
> >   		       struct xfrm_user_offload *xuo);
> >   bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x);
> >
> > +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x) {
> > +	struct xfrm_state_offload *xso = &x->xso;
> > +
> > +	if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn)
> > +		xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);
> 
> If there isn't an implementation for xdo_dev_state_advance_esn, should this
> even have been called?  See my comment below about checking the
> XFRM_STATE_ESN.
>

Thanks for the input , answered below
 
> What is the driver expected to do with this?  I would guess that maybe the
> hardware doing the offload needs to know when the ESN is advanced so that it
> can change the ICV calculation accordingly, but that only is useful for hardware
> that knows about ESN.
>

The driver checks packet seq number and sets ESN state machine accordingly.
Meaning it will increase ESN HW counter when needed.
We are submitting driver code now, so you review it
 
> > +}
> > +
> >   static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
> >   {
> >   	struct xfrm_state *x = dst->xfrm;
> > @@ -1920,6 +1928,10 @@ static inline bool xfrm_dev_offload_ok(struct
> sk_buff *skb, struct xfrm_state *x
> >   	return false;
> >   }
> >
> > +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x) {
> > +}
> > +
> >   static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
> >   {
> >   	return false;
> > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index
> > dc68d9c..fc7e1e44 100644
> > --- a/net/xfrm/xfrm_device.c
> > +++ b/net/xfrm/xfrm_device.c
> > @@ -65,8 +65,8 @@ int xfrm_dev_state_add(struct net *net, struct
> xfrm_state *x,
> >   	if (!x->type_offload)
> >   		return -EINVAL;
> >
> > -	/* We don't yet support UDP encapsulation, TFC padding and ESN. */
> > -	if (x->encap || x->tfcpad || (x->props.flags & XFRM_STATE_ESN))
> > +	/* We don't yet support UDP encapsulation and TFC padding. */
> > +	if (x->encap || x->tfcpad)
> >   		return -EINVAL;
> 
> Maybe we should check to see that xdo_dev_state_advance_esn is
> implemented before allowing XFRM_STATE_ESN?
> 
> sln
> 

We thought about adding some kind of capability queries (same goes for udp encap and tcf padding) while we wrote this feature.
This is possible but out of this patch set scope as it is general.
Steffen - your thoughts ?

> >
> >   	dev = dev_get_by_index(net, xuo->ifindex); diff --git
> > a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c index
> > 0250181..1d38c6a 100644
> > --- a/net/xfrm/xfrm_replay.c
> > +++ b/net/xfrm/xfrm_replay.c
> > @@ -551,6 +551,8 @@ static void xfrm_replay_advance_esn(struct
> xfrm_state *x, __be32 net_seq)
> >   			bitnr = replay_esn->replay_window - (diff - pos);
> >   	}
> >
> > +	xfrm_dev_state_advance_esn(x);
> > +
> >   	nr = bitnr >> 5;
> >   	bitnr = bitnr & 0x1F;
> >   	replay_esn->bmp[nr] |= (1U << bitnr);
> >