netdev - RE: [PATCH rdma-rc] RDMA/mlx5: Fix devlink deadlock on net namespace deletion

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]

Date:   Mon, 19 Oct 2020 19:26:36 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     Leon Romanovsky <leon@...nel.org>,
        Doug Ledford <dledford@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Michael Guralnik <michaelgur@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...dia.com>
Subject: RE: [PATCH rdma-rc] RDMA/mlx5: Fix devlink deadlock on net namespace
 deletion



> From: Jason Gunthorpe <jgg@...dia.com>
> Sent: Tuesday, October 20, 2020 12:31 AM
> 
> On Mon, Oct 19, 2020 at 01:23:23PM +0000, Parav Pandit wrote:
> > > > -	err = register_netdevice_notifier(&dev->port[port_num].roce.nb);
> > > > +	err = register_netdevice_notifier_net(mlx5_core_net(dev->mdev),
> > > > +					      &dev->port[port_num].roce.nb);
> > >
> > > This looks racy, what lock needs to be held to keep *mlx5_core_net()
> stable?
> >
> > mlx5_core_net() cannot be accessed outside of mlx5 driver's load, unload,
> reload path.
> >
> > When this is getting executed, devlink cannot be executing reload.
> > This is guarded by devlink_reload_enable/disable calls done by mlx5 core.
> 
> A comment that devlink_reload_enable/disable() must be held would be
> helpful
> 
Yes. will add.

> > >
> > > >  	if (err) {
> > > >  		dev->port[port_num].roce.nb.notifier_call = NULL;
> > > >  		return err;
> > > > @@ -3335,7 +3336,8 @@ static int mlx5_add_netdev_notifier(struct
> > > >mlx5_ib_dev *dev, u8 port_num)  static void
> > > >mlx5_remove_netdev_notifier(struct mlx5_ib_dev *dev, u8 port_num)
> {
> > > >  	if (dev->port[port_num].roce.nb.notifier_call) {
> > > > -		unregister_netdevice_notifier(&dev-
> > > >port[port_num].roce.nb);
> > > > +		unregister_netdevice_notifier_net(mlx5_core_net(dev-
> > > >mdev),
> > > > +						  &dev-
> > > >port[port_num].roce.nb);
> > >
> > > This seems dangerous too, what if the mlx5_core_net changed before
> > > we get here?
> >
> > When I inspected driver, code, I am not aware of any code flow where
> > this can change before reaching here, because registration and
> > unregistration is done only in driver load, unload and reload path.
> > Reload can happen only after devlink_reload_enable() is done.
> 
> But we enable reload right after init_one
> 
> > > What are the rules for when devlink_net() changes?
> > >
> > devlink_net() changes only after unload() callback is completed in driver.
> 
> You mean mlx5_devlink_reload_down ?
> 
Right.
> That seems OK then
Ok. will work with Leon to add the comment.
> 
> Jason