netdev - Re: [PATCH] [PATH xfrm offload] xfrm: bonding: Add xfrm packet offload for active-backup mode

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <aGN_q_aYSlHf_QRD@fedora>
Date: Tue, 1 Jul 2025 06:26:51 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Erwan Dufour <erwan.dufour@...hings.com>
Cc: Erwan Dufour <mrarmonius@...il.com>, netdev@...r.kernel.org,
	steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
	davem@...emloft.net, jv@...sburgh.net, saeedm@...dia.com,
	tariqt@...dia.com, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH] [PATH xfrm offload] xfrm: bonding: Add xfrm packet
 offload for active-backup mode

Hi Erwan,
On Mon, Jun 30, 2025 at 03:50:46PM +0200, Erwan Dufour wrote:
> Hi Liu,
> Thank's you for your feedback,
> You can find the new patch at the end if this email.
> 
> Please fix the code alignment. And all others in the code.
> 
> Sorry, I'm not really sure to understand. I use indentation in TAB mode
> which has a size of 4.
> I'm not sure to find alignment problem ?  Maybe I don't know the rules for
> this repository.

Tabs are 8 characters.

More coding styles, please check https://www.kernel.org/doc/html/latest/process/coding-style.html

> 
> In xfrm_add_policy() err out, it calls xfrm_dev_policy_delete() first and
> > then xfrm_dev_policy_free(). So why we free ipsec->list in
> > bond_ipsec_del_sp()
> > but no bond_ipsec_free_sp()?
> 
> Good question. I've taken inspiration from version 6.15 for these two
> functions.
> I've just seen that there's been a commit and now the ipsec is only
> released in the free function.
> On my review patch, only the function bond_ipsec_free_sp release ipsec, not
> bond_ipsec_delete_sp.
> The function bond_ipsec_free_sp is always called after the
> bond_ipsec_delete_sp function.
> This is why we now only release in bond_free_sp().

OK, I see your new patch freed the list in bond_free_sp() now.

> 
> 
> BTW, if (ipsec->xp == xp), should we delete the whole ipsec_list? Is it
> > possible ipsec->xs still exist?
> 
> In our case, the struct bond_ipsec *ipsec can have only one xs or one xp
> but not both.
> In functions bond_add_sp or bond_add_sa, we create the struct bond_ipsec
> and put the value.
> This is the only place we create a bond_ipsec, and we never update it
> either. We only read or delete it.

Hmm, I'm not very familiar with IPsec. I thought we can config xfrm state and
policy on the interface at same time. Need others review this part.

Thanks
Hangbin