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

[Prabhakar Kushwaha <prabhakar.pkin@gmail.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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme