RE: [PATCH] nvme-tcp: Use __dev_get_by_name instead dev_get_by_name for OPT_HOST_IFACE

["Belanger, Martin" <Martin.Belanger@dell.com>]

> On Sat, Jun 19, 2021 at 12:44 AM Sagi Grimberg <sagi@grimberg.me> wrote:
> >
> >
> > >> dev_get_by_name() finds network device by name but it also
> > >> increases reference count.
> > >> Increasing the ref count,
> > >> If nvme-tcp queue is present and the network device driver is
> > >> removed before nvme_tcp, we will face the following continuous log:
> > >>    "kernel:unregister_netdevice: waiting for <eth> to become
> > >>    free. Usage count = 2"
> > >> And rmmod further halts. Similar case arises during reboot/shutdown
> > >> with nvme-tcp queue present and both never completes.
> > >>
> > >> As a fix we will use __dev_get_by_name() which find network device
> > >> by name without increasing any reference counter.
> > >
> > > And when you remove it without the refcount we'll now have a stale
> > > kernel pointer?
> >
> > The netdev is not actually needed, its to make sure that it exist as
> > reference in the host_iface afaict (the netdev is never referenced
> > afterwards), and I'm pretty sure there was a matching put somewhere in
> > the former versions of this patch.
> 
> Patch 3ede8f72a9a2 (“nvme-tcp: allow selecting the network interface for
> connections") exposed to this problem in the following cases:
> A)  create nvme-tcp queue, disconnect queue and remove network driver
> B)  create nvme-tcp queue and remove network driver
> 
> “A” can be solved by putting dev_put() before kfree(ctrl):
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index
> c7bd37103cf4..93639b5b795f 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2155,6 +2155,8 @@ static void nvme_tcp_free_ctrl(struct nvme_ctrl
> *nctrl)
>         nvmf_free_options(nctrl->opts);
> free_ctrl:
>         kfree(ctrl->queues);
> +       if (ctrl->ndev)
> +               dev_put(ctrl->ndev);
>         kfree(ctrl);
> }
> 
> @@ -2586,6 +2588,8 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct
> device *dev,
> out_kfree_queues:
>         kfree(ctrl->queues);
> out_free_ctrl:
> +       if (ctrl->ndev)
> +               dev_put(ctrl->ndev);
>         kfree(ctrl);
>         return ERR_PTR(ret);
> }
> 
> But even using dev_put() will not solve “B” in case of the removal of the
> network driver before nvme-tcp.
> 
> This was our initial motivation for using  __dev_get_by_name().
> It will solve both “A” and “B” and it will behave the same with or without the
> usage of the OPT_HOST_IFACE option.
> i.e. If we remove the network driver while controllers are connected it will end
> with nvme timeouts.
> 
> --pk

Sagi is right. The function dev_get_by_name() was used to verify that the interface name exists. We don't need to increase the ref count here and using __dev_get_by_name() is the right thing to do.
Martin
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme