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

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

[Prabhakar Kushwaha]

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.

Signed-off-by: Omkar Kulkarni <okulkarni@marvell.com>
Signed-off-by: Shai Malin <smalin@marvell.com>
Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
---
 drivers/nvme/host/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index c7bd37103cf4..f9b527e71c13 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2533,7 +2533,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 	}
 
 	if (opts->mask & NVMF_OPT_HOST_IFACE) {
-		ctrl->ndev = dev_get_by_name(&init_net, opts->host_iface);
+		ctrl->ndev = __dev_get_by_name(&init_net, opts->host_iface);
 		if (!ctrl->ndev) {
 			pr_err("invalid interface passed: %s\n",
 			       opts->host_iface);
-- 
2.30.1


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

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

[Christoph Hellwig]

On Fri, Jun 18, 2021 at 04:39:56PM +0300, Prabhakar Kushwaha 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?

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

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

[Sagi Grimberg]

>> 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.

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

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

[Prabhakar Kushwaha]

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

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

[Belanger, Martin]

> 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

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

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

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

[Christoph Hellwig]

I've applied the slightly updated version below to make it a little
more clear what is going on:

---
From 9de7d173c10b6be09fe9d5b7010ef182465897a1 Mon Sep 17 00:00:00 2001
From: Prabhakar Kushwaha <pkushwaha@marvell.com>
Date: Fri, 18 Jun 2021 16:39:56 +0300
Subject: nvme-tcp: use __dev_get_by_name instead dev_get_by_name for
 OPT_HOST_IFACE

dev_get_by_name() finds network device by name but it also increases the
reference count.

If a 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.

To fix this, use __dev_get_by_name() which finds network device by
name without increasing any reference counter.

Fixes: 3ede8f72a9a2 ("nvme-tcp: allow selecting the network interface for connections")
Signed-off-by: Omkar Kulkarni <okulkarni@marvell.com>
Signed-off-by: Shai Malin <smalin@marvell.com>
Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
[hch: remove the ->ndev member entirely]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/tcp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index c7bd37103cf4..2b46c6e0226f 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -123,7 +123,6 @@ struct nvme_tcp_ctrl {
 	struct blk_mq_tag_set	admin_tag_set;
 	struct sockaddr_storage addr;
 	struct sockaddr_storage src_addr;
-	struct net_device	*ndev;
 	struct nvme_ctrl	ctrl;
 
 	struct work_struct	err_work;
@@ -2533,8 +2532,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 	}
 
 	if (opts->mask & NVMF_OPT_HOST_IFACE) {
-		ctrl->ndev = dev_get_by_name(&init_net, opts->host_iface);
-		if (!ctrl->ndev) {
+		if (!__dev_get_by_name(&init_net, opts->host_iface)) {
 			pr_err("invalid interface passed: %s\n",
 			       opts->host_iface);
 			ret = -ENODEV;
-- 
2.30.2


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

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

[Prabhakar Kushwaha]

Hi Christoph,

On Thu, Jun 24, 2021 at 12:05 PM Christoph Hellwig <hch@lst.de> wrote:
>
> I've applied the slightly updated version below to make it a little
> more clear what is going on:
>
> ---
> From 9de7d173c10b6be09fe9d5b7010ef182465897a1 Mon Sep 17 00:00:00 2001
> From: Prabhakar Kushwaha <pkushwaha@marvell.com>
> Date: Fri, 18 Jun 2021 16:39:56 +0300
> Subject: nvme-tcp: use __dev_get_by_name instead dev_get_by_name for
>  OPT_HOST_IFACE
>
> dev_get_by_name() finds network device by name but it also increases the
> reference count.
>
> If a 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.
>
> To fix this, use __dev_get_by_name() which finds network device by
> name without increasing any reference counter.
>
> Fixes: 3ede8f72a9a2 ("nvme-tcp: allow selecting the network interface for connections")
> Signed-off-by: Omkar Kulkarni <okulkarni@marvell.com>
> Signed-off-by: Shai Malin <smalin@marvell.com>
> Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> [hch: remove the ->ndev member entirely]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

I am not finding this patch on updated "branch nvme-5.14" having the
latest commit 0755d3be2d9b("nvme-tcp: can't set sk_user_data without
write_lock").
Am I missing something?  Please note, earlier it was there with commit
ff5af4bfb6af.

Issue can still be found on the nvme-5.14 branch (top of the tree).
Are you planning to fix this problem using any other approach?

--pk

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

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

[Christoph Hellwig]

On Sun, Jul 11, 2021 at 06:47:37PM +0530, Prabhakar Kushwaha wrote:
> I am not finding this patch on updated "branch nvme-5.14" having the
> latest commit 0755d3be2d9b("nvme-tcp: can't set sk_user_data without
> write_lock").
> Am I missing something?  Please note, earlier it was there with commit
> ff5af4bfb6af.
> 
> Issue can still be found on the nvme-5.14 branch (top of the tree).
> Are you planning to fix this problem using any other approach?

Sorry, it looks like it got lost in a rebase.  I'll pick it up for
nvme-5.14 again.

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