[PATCH] nvme-core: warn and abort if shared ns with empty list

[irvin cote]

But in the case you are describing, wouldn't it fail at
nvme_subsys_check_duplicate_ids?

On Tue, 6 Jun 2023 at 19:55, Keith Busch <kbusch at kernel.org> wrote:
>
> On Tue, Jun 06, 2023 at 10:21:34AM +0200, Irvin Cote wrote:
> > In nvme_find_ns_head if we find a shared ns but it has an empty
> > list or its refcount is 0 we gloss over it and go to the next.
> > Ignoring when such a case happens means that when nvme_find_ns_head
> > returns NULL, we could create a new nshead with the same nsid. Hence
> > we could find ourselves with two shared nsheads having the same nsid.
> > Instead warn the user through dev_err and exit.
>
> Let's say you're holding a reference on the old "head" and then hot
> remove the drive. The kernel will tear down the device and its
> namespace, but the subsystem and empty head will remain due to that
> held reference.
>
> Now plug the drive back in. Today, the controller will attach to the
> previous subsystem, and make a new "head" for its namespace (because the
> old one is empty), and you can continue from there, though the new
> handle names may look a bit odd.
>
> This change won't make the re-added namespace visible to the user, which
> doesn't sound right.
>
> > Signed-off-by: Irvin Cote <irvincoteg at gmail.com>
> > ---
> >  drivers/nvme/host/core.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index c8a471482421..175f1516083f 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -3341,6 +3341,11 @@ static struct nvme_ns_head *nvme_find_ns_head(struct nvme_ctrl *ctrl,
> >                       continue;
> >               if (!list_empty(&h->list) && nvme_tryget_ns_head(h))
> >                       return h;
> > +             else {
> > +                     dev_err(ctrl->device,
> > +                             "Found shared namespace with empty list or 0 refcount\n");
>
> I don't think we're guaranteed this path is using a "shared" namespace.
>
> > +                     return ERR_PTR(-ENOENT);
> > +             }
> >       }
> >
> >       return NULL;