netdev - Re: [PATCH net 4/4] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <e6ca706217ae4d5b89098ee700df12e42e879821.camel@nvidia.com>
Date: Wed, 31 Jul 2024 06:29:51 +0000
From: Jianbo Liu <jianbol@...dia.com>
To: Tariq Toukan <tariqt@...dia.com>, "jv@...sburgh.net" <jv@...sburgh.net>,
	"andy@...yhouse.net" <andy@...yhouse.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>
CC: Leon Romanovsky <leonro@...dia.com>, Gal Pressman <gal@...dia.com>, Saeed
 Mahameed <saeedm@...dia.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net 4/4] bonding: change ipsec_lock from spin lock to
 mutex

On Tue, 2024-07-30 at 13:28 +0200, Paolo Abeni wrote:
> 
> 
> On 7/29/24 14:44, Tariq Toukan wrote:
> > From: Jianbo Liu <jianbol@...dia.com>
> > 
> > In the cited commit, bond->ipsec_lock is added to protect
> > ipsec_list,
> > hence xdo_dev_state_add and xdo_dev_state_delete are called inside
> > this lock. As ipsec_lock is spin lock and such xfrmdev ops may
> > sleep,
> 
> minor nit: missing 'a' here ^^

OK, thanks.

> 
> > diff --git a/drivers/net/bonding/bond_main.c
> > b/drivers/net/bonding/bond_main.c
> > index 763d807be311..bced29813478 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -439,38 +439,33 @@ static int bond_ipsec_add_sa(struct
> > xfrm_state *xs,
> >         rcu_read_lock();
> >         bond = netdev_priv(bond_dev);
> >         slave = rcu_dereference(bond->curr_active_slave);
> 
> Is even this caller always under RTNL lock? if so it would be better 
> replace  rcu_dereference() with rtnl_dereference() and drop the rcu 
> lock, so it's clear that real_dev can't go away here.
> 
> Similar question for bond_ipsec_delete_sa, below.
> 

No, I don't think they are called under RTNL lock.

> > -       if (!slave) {
> > -               rcu_read_unlock();
> > +       real_dev = slave ? slave->dev : NULL;
> > +       rcu_read_unlock();
> > +       if (!real_dev)
> >                 return -ENODEV;
> > -       }
> >   
> > -       real_dev = slave->dev;
> >         if (!real_dev->xfrmdev_ops ||
> >             !real_dev->xfrmdev_ops->xdo_dev_state_add ||
> >             netif_is_bond_master(real_dev)) {
> >                 NL_SET_ERR_MSG_MOD(extack, "Slave does not support
> > ipsec offload");
> > -               rcu_read_unlock();
> >                 return -EINVAL;
> >         }
> >   
> >         ipsec = kmalloc(sizeof(*ipsec), GFP_ATOMIC);
> 
> I guess at this point you can use GFP_KERNEL here.
> 

Good point, thanks.

> [...]
> > @@ -482,34 +477,44 @@ static void bond_ipsec_add_sa_all(struct
> > bonding *bond)
> >         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;
> > +       rcu_read_unlock();
> 
> You can drop the rcu read lock/unlock here.

Yes, I will drop rcu read lock/unlock for these 4 functions.

> 
> [...]
> > @@ -569,14 +574,13 @@ static void bond_ipsec_del_sa_all(struct
> > bonding *bond)
> >         struct slave *slave;
> >   
> >         rcu_read_lock();
> > -       slave = rcu_dereference(bond->curr_active_slave);
> > -       if (!slave) {
> > -               rcu_read_unlock();
> > +       slave = rtnl_dereference(bond->curr_active_slave);
> > +       real_dev = slave ? slave->dev : NULL;
> > +       rcu_read_unlock();
> 
> Same here.
> 
> Thanks,
> 
> Paolo
>