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

On Sat, Jun 19, 2021 at 12:44 AM Sagi Grimberg [off-list ref] wrote:

quoted

quoted

quoted

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