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

[Hangbin Liu <liuhangbin@gmail.com>]

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