[PATCH v3 ipsec-next 0/3] xfrm: offload api fixes

[PATCH v3 ipsec-next 0/3] xfrm: offload api fixes

[Shannon Nelson]

These are a couple of little fixes to the xfrm_offload API to make
life just a little easier for the poor driver developer.

Changes from v2:
 - fix up another kbuild robot complaint when CONFIG_XFRM_OFFLOAD is off
 - split out checks into a common function for register and feature check

Changes from v1:
 - removed netdev_err() notes  (Steffen)
 - fixed build when CONFIG_XFRM_OFFLOAD is off (kbuild robot)
 - split into multiple patches (me)


Shannon Nelson (3):
  xfrm: check for xdo_dev_state_free
  xfrm: check for xdo_dev_ops add and delete
  xfrm: wrap xfrmdev_ops with offload config

 include/linux/netdevice.h |  2 +-
 include/net/xfrm.h        |  3 ++-
 net/xfrm/xfrm_device.c    | 32 ++++++++++++++++++--------------
 3 files changed, 21 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH v3 ipsec-next 1/3] xfrm: check for xdo_dev_state_free

[Shannon Nelson]

The current XFRM code assumes that we've implemented the
xdo_dev_state_free() callback, even if it is meaningless to the driver.
This patch adds a check for it before calling, as done in other APIs,
to prevent a NULL function pointer kernel crash.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 include/net/xfrm.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index e015e16..dfabd04 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1891,7 +1891,8 @@ static inline void xfrm_dev_state_free(struct xfrm_state *x)
 	 struct net_device *dev = xso->dev;
 
 	if (dev && dev->xfrmdev_ops) {
-		dev->xfrmdev_ops->xdo_dev_state_free(x);
+		if (dev->xfrmdev_ops->xdo_dev_state_free)
+			dev->xfrmdev_ops->xdo_dev_state_free(x);
 		xso->dev = NULL;
 		dev_put(dev);
 	}
-- 
2.7.4

[PATCH v3 ipsec-next 2/3] xfrm: check for xdo_dev_ops add and delete

[Shannon Nelson]

This adds a check for the required add and delete functions up front
at registration time to be sure both are defined.

Since both the features check and the registration check are looking
at the same things, break out the check for both to call.

Lastly, for some reason the feature check was setting xfrmdev_ops to
NULL if the NETIF_F_HW_ESP bit was missing, which would probably
surprise the driver later if the driver turned its NETIF_F_HW_ESP bit
back on.  We shouldn't be messing with the driver's callback list, so
we stop doing that with this patch.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 net/xfrm/xfrm_device.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 30e5746..d271de5 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -142,17 +142,31 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
 EXPORT_SYMBOL_GPL(xfrm_dev_offload_ok);
 #endif
 
-static int xfrm_dev_register(struct net_device *dev)
+static int xfrm_api_check(struct net_device *dev)
 {
-	if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
-		return NOTIFY_BAD;
+#ifdef CONFIG_XFRM_OFFLOAD
 	if ((dev->features & NETIF_F_HW_ESP_TX_CSUM) &&
 	    !(dev->features & NETIF_F_HW_ESP))
 		return NOTIFY_BAD;
 
+	if ((dev->features & NETIF_F_HW_ESP) &&
+	    (!(dev->xfrmdev_ops &&
+	       dev->xfrmdev_ops->xdo_dev_state_add &&
+	       dev->xfrmdev_ops->xdo_dev_state_delete)))
+		return NOTIFY_BAD;
+#else
+	if (dev->features & (NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM))
+		return NOTIFY_BAD;
+#endif
+
 	return NOTIFY_DONE;
 }
 
+static int xfrm_dev_register(struct net_device *dev)
+{
+	return xfrm_api_check(dev);
+}
+
 static int xfrm_dev_unregister(struct net_device *dev)
 {
 	xfrm_policy_cache_flush();
@@ -161,16 +175,7 @@ static int xfrm_dev_unregister(struct net_device *dev)
 
 static int xfrm_dev_feat_change(struct net_device *dev)
 {
-	if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
-		return NOTIFY_BAD;
-	else if (!(dev->features & NETIF_F_HW_ESP))
-		dev->xfrmdev_ops = NULL;
-
-	if ((dev->features & NETIF_F_HW_ESP_TX_CSUM) &&
-	    !(dev->features & NETIF_F_HW_ESP))
-		return NOTIFY_BAD;
-
-	return NOTIFY_DONE;
+	return xfrm_api_check(dev);
 }
 
 static int xfrm_dev_down(struct net_device *dev)
-- 
2.7.4

[PATCH v3 ipsec-next 3/3] xfrm: wrap xfrmdev_ops with offload config

[Shannon Nelson]

There's no reason to define netdev->xfrmdev_ops if
the offload facility is not CONFIG'd in.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
---
 include/linux/netdevice.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2eaac7d..145d0de 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1697,7 +1697,7 @@ struct net_device {
 	const struct ndisc_ops *ndisc_ops;
 #endif
 
-#ifdef CONFIG_XFRM
+#ifdef CONFIG_XFRM_OFFLOAD
 	const struct xfrmdev_ops *xfrmdev_ops;
 #endif
 
-- 
2.7.4

Re: [PATCH v3 ipsec-next 3/3] xfrm: wrap xfrmdev_ops with offload config

[Marcelo Ricardo Leitner]

On Tue, Dec 19, 2017 at 03:35:49PM -0800, Shannon Nelson wrote:
> There's no reason to define netdev->xfrmdev_ops if
> the offload facility is not CONFIG'd in.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>

This one could use a Fixes tag perhaps:
Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")

as in theory the build was broken since then, as it added:
+#ifdef CONFIG_XFRM_OFFLOAD
+struct xfrmdev_ops {
...
+#ifdef CONFIG_XFRM
+       const struct xfrmdev_ops *xfrmdev_ops;

So the pointer would have an undefined type
  if CONFIG_XFRM && !CONFIG_XFRM_OFFLOAD
Though I couldn't reproduce this, not sure why.

But.. is it buildable with this patch? I mine failed:

obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
                      xfrm_input.o xfrm_output.o \
                      xfrm_sysctl.o xfrm_replay.o xfrm_device.o

so xfrm_device is always in if CONFIG_XFRM is there,
xfrm_dev_init(), via xfrm_dev_notifier -> xfrm_dev_event() ->
  xfrm_dev_register() and then:

static int xfrm_dev_register(struct net_device *dev)
{
        if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
                                                 ^^^^^^^^^^^^^^^^

We can't control CONFIG_XFRM_OFFLOAD directly, so unless you
unselected other offloadings such as INET_ESP_OFFLOAD, it is still on.

linux/net/xfrm/xfrm_device.c: In function ‘xfrm_dev_register’:
linux/net/xfrm/xfrm_device.c:147:48: error: ‘struct net_device’ has no member named ‘xfrmdev_ops’; did you mean ‘netdev_ops’?
  if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
                                                ^~~~~~~~~~~
                                                netdev_ops


> ---
>  include/linux/netdevice.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2eaac7d..145d0de 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1697,7 +1697,7 @@ struct net_device {
>  	const struct ndisc_ops *ndisc_ops;
>  #endif
>  
> -#ifdef CONFIG_XFRM
> +#ifdef CONFIG_XFRM_OFFLOAD
>  	const struct xfrmdev_ops *xfrmdev_ops;
>  #endif
>  
> -- 
> 2.7.4
> 

Re: [PATCH v3 ipsec-next 3/3] xfrm: wrap xfrmdev_ops with offload config

[Shannon Nelson]

On 12/20/2017 8:03 AM, Marcelo Ricardo Leitner wrote:
> On Tue, Dec 19, 2017 at 03:35:49PM -0800, Shannon Nelson wrote:
>> There's no reason to define netdev->xfrmdev_ops if
>> the offload facility is not CONFIG'd in.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> 
> This one could use a Fixes tag perhaps:
> Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> 
> as in theory the build was broken since then, as it added:
> +#ifdef CONFIG_XFRM_OFFLOAD
> +struct xfrmdev_ops {
> ...
> +#ifdef CONFIG_XFRM
> +       const struct xfrmdev_ops *xfrmdev_ops;
> 
> So the pointer would have an undefined type
>    if CONFIG_XFRM && !CONFIG_XFRM_OFFLOAD
> Though I couldn't reproduce this, not sure why.

Hmmm, I don't think this requires a "Fixes" tag, as the code all worked 
just fine, I'm just doing a little cleaning.

Patch 2/3 adds a more intense look at the data structure, so I needed to 
change it to the CONFIG_XFRM_OFFLOAD so as to not break the build. 
Since the xfrmdev_ops field is now never used unless we have 
CONFIG_XFRM_OFFLOAD, we can change the net_device definition to be just 
a bit smaller without it.

> 
> But.. is it buildable with this patch? I mine failed:
> 
> obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
>                        xfrm_input.o xfrm_output.o \
>                        xfrm_sysctl.o xfrm_replay.o xfrm_device.o
> 
> so xfrm_device is always in if CONFIG_XFRM is there,
> xfrm_dev_init(), via xfrm_dev_notifier -> xfrm_dev_event() ->
>    xfrm_dev_register() and then:
> 
> static int xfrm_dev_register(struct net_device *dev)
> {
>          if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)

This looks like you haven't applied version 3 of the 2nd patch "xfrm: 
check for xdo_dev_ops add and delete".  I missed this in the earlier 
version (not enough compile tests), but version 3 of patch 2/3  should 
address it.

sln

>                                                   ^^^^^^^^^^^^^^^^
> 
> We can't control CONFIG_XFRM_OFFLOAD directly, so unless you
> unselected other offloadings such as INET_ESP_OFFLOAD, it is still on.
> 
> linux/net/xfrm/xfrm_device.c: In function ‘xfrm_dev_register’:
> linux/net/xfrm/xfrm_device.c:147:48: error: ‘struct net_device’ has no member named ‘xfrmdev_ops’; did you mean ‘netdev_ops’?
>    if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
>                                                  ^~~~~~~~~~~
>                                                  netdev_ops
> 
> 
>> ---
>>   include/linux/netdevice.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 2eaac7d..145d0de 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1697,7 +1697,7 @@ struct net_device {
>>   	const struct ndisc_ops *ndisc_ops;
>>   #endif
>>   
>> -#ifdef CONFIG_XFRM
>> +#ifdef CONFIG_XFRM_OFFLOAD
>>   	const struct xfrmdev_ops *xfrmdev_ops;
>>   #endif
>>   
>> -- 
>> 2.7.4
>>

Re: [PATCH v3 ipsec-next 3/3] xfrm: wrap xfrmdev_ops with offload config

[Marcelo Ricardo Leitner]

On Wed, Dec 20, 2017 at 08:22:40AM -0800, Shannon Nelson wrote:
> On 12/20/2017 8:03 AM, Marcelo Ricardo Leitner wrote:
> > On Tue, Dec 19, 2017 at 03:35:49PM -0800, Shannon Nelson wrote:
> > > There's no reason to define netdev->xfrmdev_ops if
> > > the offload facility is not CONFIG'd in.
> > > 
> > > Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> > 
> > This one could use a Fixes tag perhaps:
> > Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> > 
> > as in theory the build was broken since then, as it added:
> > +#ifdef CONFIG_XFRM_OFFLOAD
> > +struct xfrmdev_ops {
> > ...
> > +#ifdef CONFIG_XFRM
> > +       const struct xfrmdev_ops *xfrmdev_ops;
> > 
> > So the pointer would have an undefined type
> >    if CONFIG_XFRM && !CONFIG_XFRM_OFFLOAD
> > Though I couldn't reproduce this, not sure why.
> 
> Hmmm, I don't think this requires a "Fixes" tag, as the code all worked just
> fine, I'm just doing a little cleaning.

I still don't get how it works, but okay.

> 
> Patch 2/3 adds a more intense look at the data structure, so I needed to
> change it to the CONFIG_XFRM_OFFLOAD so as to not break the build. Since the
> xfrmdev_ops field is now never used unless we have CONFIG_XFRM_OFFLOAD, we
> can change the net_device definition to be just a bit smaller without it.
> 
> > 
> > But.. is it buildable with this patch? I mine failed:
> > 
> > obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
> >                        xfrm_input.o xfrm_output.o \
> >                        xfrm_sysctl.o xfrm_replay.o xfrm_device.o
> > 
> > so xfrm_device is always in if CONFIG_XFRM is there,
> > xfrm_dev_init(), via xfrm_dev_notifier -> xfrm_dev_event() ->
> >    xfrm_dev_register() and then:
> > 
> > static int xfrm_dev_register(struct net_device *dev)
> > {
> >          if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
> 
> This looks like you haven't applied version 3 of the 2nd patch "xfrm: check
> for xdo_dev_ops add and delete".  I missed this in the earlier version (not
> enough compile tests), but version 3 of patch 2/3  should address it.

Right you are, missed it here.

Thanks,
Marcelo

Re: [PATCH v3 ipsec-next 0/3] xfrm: offload api fixes

[Steffen Klassert]

On Tue, Dec 19, 2017 at 03:35:46PM -0800, Shannon Nelson wrote:
> These are a couple of little fixes to the xfrm_offload API to make
> life just a little easier for the poor driver developer.
> 
> Changes from v2:
>  - fix up another kbuild robot complaint when CONFIG_XFRM_OFFLOAD is off
>  - split out checks into a common function for register and feature check
> 
> Changes from v1:
>  - removed netdev_err() notes  (Steffen)
>  - fixed build when CONFIG_XFRM_OFFLOAD is off (kbuild robot)
>  - split into multiple patches (me)
> 
> 
> Shannon Nelson (3):
>   xfrm: check for xdo_dev_state_free
>   xfrm: check for xdo_dev_ops add and delete
>   xfrm: wrap xfrmdev_ops with offload config

All applied to ipsec-next, thanks Shannon!