On Fri, Dec 07, 2018 at 11:43:50AM +0530, Selvin Xavier wrote:
> On Fri, Dec 7, 2018 at 12:27 AM Jason Gunthorpe <jgg@xxxxxxxxxxxx> wrote:
> >
> > On Thu, Dec 06, 2018 at 10:49:44AM -0800, Selvin Xavier wrote:
> > > From: Somnath Kotur <somnath.kotur@xxxxxxxxxxxx>
> > >
> > > - Add more flags for better granularity in rdev init/uninit path.
> > > - bnxt_re_ib_reg() handles two main functionalities - initializing
> > > the device and registering with the IB stack. Split it into 2
> > > functions i.e. bnxt_re_dev_init() and bnxt_re_ib_init() to account
> > > for the same thereby improve modularity. Do the same for
> > > bnxt_re_ib_unreg()i.e. split into two functions - bnxt_re_dev_uninit()
> > > and bnxt_re_ib_uninit().
> > > - Since bnxt_re_dev_init() part of bnxt_re_ib_reg()
> > > takes rtnl_lock inside for the entire function, so it is better to
> > > invoke it from bnxt_netdev_event() itself where the rtnl_lock is held
> > > already for NETDEV_EVENT.
> >
> > Drivers cannot hold rtnl_lock while calling ib_unregister_device() (or
> > really any lock, unless done carefully). Looks like bnxt does this
> > already and prehaps this patch makes it worse?
> >
> Not really. You are right that bnxt is already calling
> ib_unregister_device from rtnl_lock.
> Current code has three instances of calling ib_unregister_device from
> rtnl_lock context,
> We are removing two of them in this patch.
>
> Un-int routine is split into two. bnxt_re_ib_uninit and bnxt_re_remove_device .
> bnxt_re_ib_uninit calls ib_unregister_device and
> bnxt_re_remove_device destroys the resources from HW.
>
> In this patch, we call bnxt_re_ib_uninit outside rtnl_lock and
> bnxt_re_remove_device with rtnl_lock
> (except in bnxt_re_shutdown which i will explain later).
>
> In case of mod_exit, bnxt_re_ib_uninit is invoked first outside the rtnl_lock.
> After this, rtnl_lock is acquired and bnxt_re_remove_device is invoked.
Yuk.
I'm looking at this driver and I see the same basic racy mess I see in
rxe regarding liftime and locking. Look at the patch I just sent to
Steve and consider using its new functions in this driver too instead
of the bnxt_re_dev_list, and other related buggyness.
Obviously the bnxt_re_shutdown problem has to be fixed too..
> >> @@ -1555,9 +1581,14 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
> >> */
> >> if (atomic_read(&rdev->sched_count) > 0)
> >> goto exit;
> >> - bnxt_re_ib_unreg(rdev);
> >> - bnxt_re_remove_one(rdev);
> >> - bnxt_re_dev_unreg(rdev);
> >> + /* Schedule work for unregistering from IB stack */
> >> + if (test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags)) {
> >> + sch_work = true;
> >> + break;
> >> + }
>
> if the IB device is registered, we schedule a work to invoke
> ib_unregister_device and return notifier.
> So bnxt_re_ib_uninit is invoked from the bnxt_re_task.
BNXT_RE_FLAG_IBDEV_REGISTERED is also all racy and wrong.
> Since the netdev reference is not released by bnxt_re driver (ref is
> released in bnxt_re_remove_device),
> the network stack will invoke all notifiers again till all the
> references are released.
> During the second or later invocation of the netdev notifier, we
> proceed with the bnxt_re_remove_device.
That is also racy. unregister_netdevice_notifier() could have
happened between the first and second call, so now it leaks.
If ib_unregister happens the driver must do the remove_device stuff
immediately.
So, I think you need to use that stuff I sent to Steve. This patch
might be an improvement if the above is fixed, maybe, it isn't
clear. (and Steve, now that I see the same mess in bnxt, I'm more than
convinced that is the right approach to your problem too)
However, I'd rather see this as part of a series that comprehensively
fixes all of this.
Jason
