[PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Hangbin Liu]

I planned to add the new XFRM state offload functions after Jianbo's
patchset [1], but it seems that may take some time. Therefore, I am
posting these two patches to net-next now, as our users are waiting for
this functionality. If Jianbo's patch is applied first, I can update these
patches accordingly.

[1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com

Hangbin Liu (2):
  bonding: Add ESN support to IPSec HW offload
  bonding: support xfrm state update

 drivers/net/bonding/bond_main.c | 76 +++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

-- 
2.45.0

[PATCH net-next 1/2] bonding: Add ESN support to IPSec HW offload

[Hangbin Liu]

Currently, users can see that bonding supports IPSec HW offload via ethtool.
However, this functionality does not work with NICs like Mellanox cards when
ESN (Extended Sequence Numbers) is enabled, as ESN functions are not yet
supported. This patch adds ESN support to the bonding IPSec device offload,
ensuring proper functionality with NICs that support ESN.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 38 +++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f9633a6f8571..4e3d7979fe01 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -629,10 +629,48 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
 	return err;
 }
 
+/**
+ * bond_advance_esn_state - ESN support for IPSec HW offload
+ * @xs: pointer to transformer state struct
+ **/
+static void bond_advance_esn_state(struct xfrm_state *xs)
+{
+	struct net_device *bond_dev = xs->xso.dev;
+	struct bond_ipsec *ipsec;
+	struct bonding *bond;
+	struct slave *slave;
+
+	if (!bond_dev)
+		return;
+
+	rcu_read_lock();
+	bond = netdev_priv(bond_dev);
+	slave = rcu_dereference(bond->curr_active_slave);
+
+	if (!slave)
+		goto out;
+
+	if (!xs->xso.real_dev)
+		goto out;
+
+	WARN_ON(xs->xso.real_dev != slave->dev);
+
+	if (!slave->dev->xfrmdev_ops ||
+	    !slave->dev->xfrmdev_ops->xdo_dev_state_advance_esn) {
+		slave_warn(bond_dev, slave->dev, "%s: no slave xdo_dev_state_advance_esn\n", __func__);
+		goto out;
+	}
+
+	slave->dev->xfrmdev_ops->xdo_dev_state_advance_esn(xs);
+out:
+	rcu_read_unlock();
+}
+
 static const struct xfrmdev_ops bond_xfrmdev_ops = {
 	.xdo_dev_state_add = bond_ipsec_add_sa,
 	.xdo_dev_state_delete = bond_ipsec_del_sa,
 	.xdo_dev_offload_ok = bond_ipsec_offload_ok,
+	.xdo_dev_state_advance_esn = bond_advance_esn_state,
 };
 #endif /* CONFIG_XFRM_OFFLOAD */
 
-- 
2.45.0

Re: [PATCH net-next 1/2] bonding: Add ESN support to IPSec HW offload

[Simon Horman]

On Fri, Aug 16, 2024 at 11:55:17AM +0800, Hangbin Liu wrote:
> Currently, users can see that bonding supports IPSec HW offload via ethtool.
> However, this functionality does not work with NICs like Mellanox cards when
> ESN (Extended Sequence Numbers) is enabled, as ESN functions are not yet
> supported. This patch adds ESN support to the bonding IPSec device offload,
> ensuring proper functionality with NICs that support ESN.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 38 +++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index f9633a6f8571..4e3d7979fe01 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -629,10 +629,48 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>  	return err;
>  }
>  
> +/**
> + * bond_advance_esn_state - ESN support for IPSec HW offload
> + * @xs: pointer to transformer state struct
> + **/
> +static void bond_advance_esn_state(struct xfrm_state *xs)
> +{
> +	struct net_device *bond_dev = xs->xso.dev;
> +	struct bond_ipsec *ipsec;

Hi Hangbin,

ipsec is unused in this function and should be removed.
Likewise in patch 2/2.

> +	struct bonding *bond;
> +	struct slave *slave;
> +
> +	if (!bond_dev)
> +		return;
> +
> +	rcu_read_lock();
> +	bond = netdev_priv(bond_dev);
> +	slave = rcu_dereference(bond->curr_active_slave);
> +
> +	if (!slave)
> +		goto out;
> +
> +	if (!xs->xso.real_dev)
> +		goto out;
> +
> +	WARN_ON(xs->xso.real_dev != slave->dev);
> +
> +	if (!slave->dev->xfrmdev_ops ||
> +	    !slave->dev->xfrmdev_ops->xdo_dev_state_advance_esn) {
> +		slave_warn(bond_dev, slave->dev, "%s: no slave xdo_dev_state_advance_esn\n", __func__);
> +		goto out;
> +	}
> +
> +	slave->dev->xfrmdev_ops->xdo_dev_state_advance_esn(xs);
> +out:
> +	rcu_read_unlock();
> +}

...

-- 
pw-bot: cr

Re: [PATCH net-next 1/2] bonding: Add ESN support to IPSec HW offload

[kernel test robot]

Hi Hangbin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Hangbin-Liu/bonding-Add-ESN-support-to-IPSec-HW-offload/20240816-122016
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240816035518.203704-2-liuhangbin%40gmail.com
patch subject: [PATCH net-next 1/2] bonding: Add ESN support to IPSec HW offload
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20240817/202408171959.Qj3TkV2v-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240817/202408171959.Qj3TkV2v-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408171959.Qj3TkV2v-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/bonding/bond_main.c: In function 'bond_advance_esn_state':
>> drivers/net/bonding/bond_main.c:639:28: warning: unused variable 'ipsec' [-Wunused-variable]
     639 |         struct bond_ipsec *ipsec;
         |                            ^~~~~


vim +/ipsec +639 drivers/net/bonding/bond_main.c

   631	
   632	/**
   633	 * bond_advance_esn_state - ESN support for IPSec HW offload
   634	 * @xs: pointer to transformer state struct
   635	 **/
   636	static void bond_advance_esn_state(struct xfrm_state *xs)
   637	{
   638		struct net_device *bond_dev = xs->xso.dev;
 > 639		struct bond_ipsec *ipsec;
   640		struct bonding *bond;
   641		struct slave *slave;
   642	
   643		if (!bond_dev)
   644			return;
   645	
   646		rcu_read_lock();
   647		bond = netdev_priv(bond_dev);
   648		slave = rcu_dereference(bond->curr_active_slave);
   649	
   650		if (!slave)
   651			goto out;
   652	
   653		if (!xs->xso.real_dev)
   654			goto out;
   655	
   656		WARN_ON(xs->xso.real_dev != slave->dev);
   657	
   658		if (!slave->dev->xfrmdev_ops ||
   659		    !slave->dev->xfrmdev_ops->xdo_dev_state_advance_esn) {
   660			slave_warn(bond_dev, slave->dev, "%s: no slave xdo_dev_state_advance_esn\n", __func__);
   661			goto out;
   662		}
   663	
   664		slave->dev->xfrmdev_ops->xdo_dev_state_advance_esn(xs);
   665	out:
   666		rcu_read_unlock();
   667	}
   668	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net-next 2/2] bonding: support xfrm state update

[Hangbin Liu]

The patch add xfrm statistics update for bonding IPsec offload.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 38 +++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4e3d7979fe01..9a7caf677c71 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -666,11 +666,49 @@ static void bond_advance_esn_state(struct xfrm_state *xs)
 	rcu_read_unlock();
 }
 
+/**
+ * bond_xfrm_update_stats - Update xfrm state
+ * @xs: pointer to transformer state struct
+ **/
+static void bond_xfrm_update_stats(struct xfrm_state *xs)
+{
+	struct net_device *bond_dev = xs->xso.dev;
+	struct bond_ipsec *ipsec;
+	struct bonding *bond;
+	struct slave *slave;
+
+	if (!bond_dev)
+		return;
+
+	rcu_read_lock();
+	bond = netdev_priv(bond_dev);
+	slave = rcu_dereference(bond->curr_active_slave);
+
+	if (!slave)
+		goto out;
+
+	if (!xs->xso.real_dev)
+		goto out;
+
+	WARN_ON(xs->xso.real_dev != slave->dev);
+
+	if (!slave->dev->xfrmdev_ops ||
+	    !slave->dev->xfrmdev_ops->xdo_dev_state_update_stats) {
+		slave_warn(bond_dev, slave->dev, "%s: no slave xdo_dev_state_update_stats\n", __func__);
+		goto out;
+	}
+
+	slave->dev->xfrmdev_ops->xdo_dev_state_update_stats(xs);
+out:
+	rcu_read_unlock();
+}
+
 static const struct xfrmdev_ops bond_xfrmdev_ops = {
 	.xdo_dev_state_add = bond_ipsec_add_sa,
 	.xdo_dev_state_delete = bond_ipsec_del_sa,
 	.xdo_dev_offload_ok = bond_ipsec_offload_ok,
 	.xdo_dev_state_advance_esn = bond_advance_esn_state,
+	.xdo_dev_state_update_stats = bond_xfrm_update_stats,
 };
 #endif /* CONFIG_XFRM_OFFLOAD */
 
-- 
2.45.0

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Hangbin Liu]

On Fri, Aug 16, 2024 at 11:55:16AM +0800, Hangbin Liu wrote:
> I planned to add the new XFRM state offload functions after Jianbo's
> patchset [1], but it seems that may take some time. Therefore, I am
> posting these two patches to net-next now, as our users are waiting for
> this functionality. If Jianbo's patch is applied first, I can update these
> patches accordingly.
> 
> [1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com

Forgot to say, The xdo_dev_state_free will be added by Jianbo's patchset.
I will add the bonding xfrm policy offload in future.

Thanks
Hangbin

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Nikolay Aleksandrov]

On 16/08/2024 06:55, Hangbin Liu wrote:
> I planned to add the new XFRM state offload functions after Jianbo's
> patchset [1], but it seems that may take some time. Therefore, I am
> posting these two patches to net-next now, as our users are waiting for
> this functionality. If Jianbo's patch is applied first, I can update these
> patches accordingly.
> 
> [1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com
> 
> Hangbin Liu (2):
>   bonding: Add ESN support to IPSec HW offload
>   bonding: support xfrm state update
> 
>  drivers/net/bonding/bond_main.c | 76 +++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 

the set looks good to me, one minor cosmetic nit is that the two
functions look very much alike only difference is the actual call
can you maybe factor out the boilerplate?

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Hangbin Liu]

On Fri, Aug 16, 2024 at 09:00:17AM +0300, Nikolay Aleksandrov wrote:
> the set looks good to me, one minor cosmetic nit is that the two
> functions look very much alike only difference is the actual call
> can you maybe factor out the boilerplate?

Thanks, I will update the patch to use a common function for the checking.

Hangbin

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Nikolay Aleksandrov]

On 16/08/2024 06:55, Hangbin Liu wrote:
> I planned to add the new XFRM state offload functions after Jianbo's
> patchset [1], but it seems that may take some time. Therefore, I am
> posting these two patches to net-next now, as our users are waiting for
> this functionality. If Jianbo's patch is applied first, I can update these
> patches accordingly.
> 
> [1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com
> 
> Hangbin Liu (2):
>   bonding: Add ESN support to IPSec HW offload
>   bonding: support xfrm state update
> 
>  drivers/net/bonding/bond_main.c | 76 +++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 

(not related to this set, but to bond xfrm)
By the way looking at bond's xfrm code, what prevents bond_ipsec_offload_ok()
from dereferencing a null ptr?
I mean it does:
        curr_active = rcu_dereference(bond->curr_active_slave);
        real_dev = curr_active->dev;

If this is running only under RCU as the code suggests then
curr_active_slave can change to NULL in parallel. Should there be a
check for curr_active before deref or am I missing something?

Cheers,
 Nik

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Hangbin Liu]

On Fri, Aug 16, 2024 at 09:06:12AM +0300, Nikolay Aleksandrov wrote:
> On 16/08/2024 06:55, Hangbin Liu wrote:
> > I planned to add the new XFRM state offload functions after Jianbo's
> > patchset [1], but it seems that may take some time. Therefore, I am
> > posting these two patches to net-next now, as our users are waiting for
> > this functionality. If Jianbo's patch is applied first, I can update these
> > patches accordingly.
> > 
> > [1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com
> > 
> > Hangbin Liu (2):
> >   bonding: Add ESN support to IPSec HW offload
> >   bonding: support xfrm state update
> > 
> >  drivers/net/bonding/bond_main.c | 76 +++++++++++++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> > 
> 
> (not related to this set, but to bond xfrm)
> By the way looking at bond's xfrm code, what prevents bond_ipsec_offload_ok()
> from dereferencing a null ptr?
> I mean it does:
>         curr_active = rcu_dereference(bond->curr_active_slave);
>         real_dev = curr_active->dev;
> 
> If this is running only under RCU as the code suggests then
> curr_active_slave can change to NULL in parallel. Should there be a
> check for curr_active before deref or am I missing something?

Yes, we can do like
real_dev = curr_active ? curr_active->dev : NULL;

Thanks
Hangbin

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Nikolay Aleksandrov]

On 16/08/2024 11:32, Hangbin Liu wrote:
> On Fri, Aug 16, 2024 at 09:06:12AM +0300, Nikolay Aleksandrov wrote:
>> On 16/08/2024 06:55, Hangbin Liu wrote:
>>> I planned to add the new XFRM state offload functions after Jianbo's
>>> patchset [1], but it seems that may take some time. Therefore, I am
>>> posting these two patches to net-next now, as our users are waiting for
>>> this functionality. If Jianbo's patch is applied first, I can update these
>>> patches accordingly.
>>>
>>> [1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com
>>>
>>> Hangbin Liu (2):
>>>   bonding: Add ESN support to IPSec HW offload
>>>   bonding: support xfrm state update
>>>
>>>  drivers/net/bonding/bond_main.c | 76 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 76 insertions(+)
>>>
>>
>> (not related to this set, but to bond xfrm)
>> By the way looking at bond's xfrm code, what prevents bond_ipsec_offload_ok()
>> from dereferencing a null ptr?
>> I mean it does:
>>         curr_active = rcu_dereference(bond->curr_active_slave);
>>         real_dev = curr_active->dev;
>>
>> If this is running only under RCU as the code suggests then
>> curr_active_slave can change to NULL in parallel. Should there be a
>> check for curr_active before deref or am I missing something?
> 
> Yes, we can do like
> real_dev = curr_active ? curr_active->dev : NULL;
> 
> Thanks
> Hangbin

Right, let me try and trigger it and I'll send a patch. :)

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Nikolay Aleksandrov]

On 16/08/2024 11:37, Nikolay Aleksandrov wrote:
> On 16/08/2024 11:32, Hangbin Liu wrote:
>> On Fri, Aug 16, 2024 at 09:06:12AM +0300, Nikolay Aleksandrov wrote:
>>> On 16/08/2024 06:55, Hangbin Liu wrote:
>>>> I planned to add the new XFRM state offload functions after Jianbo's
>>>> patchset [1], but it seems that may take some time. Therefore, I am
>>>> posting these two patches to net-next now, as our users are waiting for
>>>> this functionality. If Jianbo's patch is applied first, I can update these
>>>> patches accordingly.
>>>>
>>>> [1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com
>>>>
>>>> Hangbin Liu (2):
>>>>   bonding: Add ESN support to IPSec HW offload
>>>>   bonding: support xfrm state update
>>>>
>>>>  drivers/net/bonding/bond_main.c | 76 +++++++++++++++++++++++++++++++++
>>>>  1 file changed, 76 insertions(+)
>>>>
>>>
>>> (not related to this set, but to bond xfrm)
>>> By the way looking at bond's xfrm code, what prevents bond_ipsec_offload_ok()
>>> from dereferencing a null ptr?
>>> I mean it does:
>>>         curr_active = rcu_dereference(bond->curr_active_slave);
>>>         real_dev = curr_active->dev;
>>>
>>> If this is running only under RCU as the code suggests then
>>> curr_active_slave can change to NULL in parallel. Should there be a
>>> check for curr_active before deref or am I missing something?
>>
>> Yes, we can do like
>> real_dev = curr_active ? curr_active->dev : NULL;
>>
>> Thanks
>> Hangbin
> 
> Right, let me try and trigger it and I'll send a patch. :)
> 

Just fyi I was able to trigger this null deref easily and one more
thing - there is a second null deref after this one because real_dev is
used directly :(

I'll send a patch to fix both in a bit.

Re: [PATCH net-next 0/2] Bonding: support new xfrm state offload functions

[Nikolay Aleksandrov]

On 16/08/2024 12:04, Nikolay Aleksandrov wrote:
> On 16/08/2024 11:37, Nikolay Aleksandrov wrote:
>> On 16/08/2024 11:32, Hangbin Liu wrote:
>>> On Fri, Aug 16, 2024 at 09:06:12AM +0300, Nikolay Aleksandrov wrote:
>>>> On 16/08/2024 06:55, Hangbin Liu wrote:
>>>>> I planned to add the new XFRM state offload functions after Jianbo's
>>>>> patchset [1], but it seems that may take some time. Therefore, I am
>>>>> posting these two patches to net-next now, as our users are waiting for
>>>>> this functionality. If Jianbo's patch is applied first, I can update these
>>>>> patches accordingly.
>>>>>
>>>>> [1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com
>>>>>
>>>>> Hangbin Liu (2):
>>>>>   bonding: Add ESN support to IPSec HW offload
>>>>>   bonding: support xfrm state update
>>>>>
>>>>>  drivers/net/bonding/bond_main.c | 76 +++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 76 insertions(+)
>>>>>
>>>>
>>>> (not related to this set, but to bond xfrm)
>>>> By the way looking at bond's xfrm code, what prevents bond_ipsec_offload_ok()
>>>> from dereferencing a null ptr?
>>>> I mean it does:
>>>>         curr_active = rcu_dereference(bond->curr_active_slave);
>>>>         real_dev = curr_active->dev;
>>>>
>>>> If this is running only under RCU as the code suggests then
>>>> curr_active_slave can change to NULL in parallel. Should there be a
>>>> check for curr_active before deref or am I missing something?
>>>
>>> Yes, we can do like
>>> real_dev = curr_active ? curr_active->dev : NULL;
>>>
>>> Thanks
>>> Hangbin
>>
>> Right, let me try and trigger it and I'll send a patch. :)
>>
> 
> Just fyi I was able to trigger this null deref easily and one more
> thing - there is a second null deref after this one because real_dev is
> used directly :(
> 
> I'll send a patch to fix both in a bit.
> 
> 

TBH I don't know how bond xfrm code is running at all. There are *many*
bugs when I took a closer look. I'll try to prepare a patch-set to address
them all. This code is seriously broken...