netdev - Re: [PATCH net V3 3/3] bonding: change ipsec

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZrV_b9IfYXwSm3pm@Laptop-X1>
Date: Fri, 9 Aug 2024 10:31:11 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Jianbo Liu <jianbol@...dia.com>
Cc: Tariq Toukan <tariqt@...dia.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	Leon Romanovsky <leonro@...dia.com>,
	"andy@...yhouse.net" <andy@...yhouse.net>,
	Gal Pressman <gal@...dia.com>,
	"jv@...sburgh.net" <jv@...sburgh.net>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	Saeed Mahameed <saeedm@...dia.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net V3 3/3] bonding: change ipsec_lock from spin lock to
 mutex

On Thu, Aug 08, 2024 at 10:05:26AM +0000, Jianbo Liu wrote:
> On Thu, 2024-08-08 at 17:34 +0800, Hangbin Liu wrote:
> > On Mon, Aug 05, 2024 at 08:03:57AM +0300, Tariq Toukan wrote:
> > >  drivers/net/bonding/bond_main.c | 75 +++++++++++++++++------------
> > > ----
> > >  include/net/bonding.h           |  2 +-
> > >  2 files changed, 40 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/drivers/net/bonding/bond_main.c
> > > b/drivers/net/bonding/bond_main.c
> > > index e550b1c08fdb..56764f1c39b8 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -481,35 +476,43 @@ static void bond_ipsec_add_sa_all(struct
> > > bonding *bond)
> > >         struct bond_ipsec *ipsec;
> > >         struct slave *slave;
> > >  
> > > -       rcu_read_lock();
> > > -       slave = rcu_dereference(bond->curr_active_slave);
> > > -       if (!slave)
> > > -               goto out;
> > > +       slave = rtnl_dereference(bond->curr_active_slave);
> > > +       real_dev = slave ? slave->dev : NULL;
> > > +       if (!real_dev)
> > > +               return;
> > >  
> > > -       real_dev = slave->dev;
> > > +       mutex_lock(&bond->ipsec_lock);
> > >         if (!real_dev->xfrmdev_ops ||
> > >             !real_dev->xfrmdev_ops->xdo_dev_state_add ||
> > >             netif_is_bond_master(real_dev)) {
> > > -               spin_lock_bh(&bond->ipsec_lock);
> > >                 if (!list_empty(&bond->ipsec_list))
> > >                         slave_warn(bond_dev, real_dev,
> > >                                    "%s: no slave
> > > xdo_dev_state_add\n",
> > >                                    __func__);
> > > -               spin_unlock_bh(&bond->ipsec_lock);
> > >                 goto out;
> > >         }
> > >  
> > > -       spin_lock_bh(&bond->ipsec_lock);
> > >         list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> > > +               struct net_device *dev = ipsec->xs->xso.real_dev;
> > > +
> > > +               /* If new state is added before ipsec_lock acquired
> > > */
> > > +               if (dev) {
> > > +                       if (dev == real_dev)
> > > +                               continue;
> > Hi Jianbo,
> > 
> > Why we skip the deleting here if dev == real_dev? What if the state
> 
> Here the bond active slave is updated. If dev == real_dev, the state
> (should be newly added) is offloaded to new active, so no need to
> delete and add back again.  
> 
> > is added again on the same slave? From the previous logic it looks we
> 
> Why is it added to the same slave? It's not the active one.

OK, I got what you mean now. Thanks for the explaination.

Reviewed-by: Hangbin Liu <liuhangbin@...il.com>