[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Hannes Reinecke]

The controller ID is supposed to be unique for each subsystem;
however, if it _isn't_ we will crash when calling device_add_disk()
for the namespaces.
While this surely is an error on the subsystem side we really shouldn't
crash. So simply use the 'instance' number when generating the disk
name to avoid this issue.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index f0716f6ce41f..2551264ef2b5 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -31,7 +31,7 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 		sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
 	} else if (ns->head->disk) {
 		sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
-				ctrl->cntlid, ns->head->instance);
+				ctrl->instance, ns->head->instance);
 		*flags = GENHD_FL_HIDDEN;
 	} else {
 		sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance,
-- 
2.16.4

[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Christoph Hellwig]

On Fri, Mar 29, 2019@10:57:54AM +0100, Hannes Reinecke wrote:
> The controller ID is supposed to be unique for each subsystem;
> however, if it _isn't_ we will crash when calling device_add_disk()
> for the namespaces.
> While this surely is an error on the subsystem side we really shouldn't
> crash. So simply use the 'instance' number when generating the disk
> name to avoid this issue.

I agree that we should not crash in that, but I'd rather print an
error message and reject using that controller with the duplicate
controller id than hacking around the issue.

[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Hannes Reinecke]

On 3/29/19 11:02 AM, Christoph Hellwig wrote:
> On Fri, Mar 29, 2019@10:57:54AM +0100, Hannes Reinecke wrote:
>> The controller ID is supposed to be unique for each subsystem;
>> however, if it _isn't_ we will crash when calling device_add_disk()
>> for the namespaces.
>> While this surely is an error on the subsystem side we really shouldn't
>> crash. So simply use the 'instance' number when generating the disk
>> name to avoid this issue.
> 
> I agree that we should not crash in that, but I'd rather print an
> error message and reject using that controller with the duplicate
> controller id than hacking around the issue.
> 
As it so happens, I have a patch for that, too, but thought this one an 
easier solution :-)

Will be sending that patch, then.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Keith Busch]

On Fri, Mar 29, 2019@03:23:18AM -0700, Hannes Reinecke wrote:
> On 3/29/19 11:02 AM, Christoph Hellwig wrote:
> > On Fri, Mar 29, 2019@10:57:54AM +0100, Hannes Reinecke wrote:
> >> The controller ID is supposed to be unique for each subsystem;
> >> however, if it _isn't_ we will crash when calling device_add_disk()
> >> for the namespaces.
> >> While this surely is an error on the subsystem side we really shouldn't
> >> crash. So simply use the 'instance' number when generating the disk
> >> name to avoid this issue.
> > 
> > I agree that we should not crash in that, but I'd rather print an
> > error message and reject using that controller with the duplicate
> > controller id than hacking around the issue.
> > 
> As it so happens, I have a patch for that, too, but thought this one an 
> easier solution :-)
> 
> Will be sending that patch, then.

We might want to use this patch anyway. It is possible some process
reference count is preventing the final release when we want to remove
a controller, and a subsequent discover, even if the cntlid is unqiue,
would create a duplicate disk name.

[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Hannes Reinecke]

On 4/1/19 7:17 AM, Keith Busch wrote:
> On Fri, Mar 29, 2019@03:23:18AM -0700, Hannes Reinecke wrote:
>> On 3/29/19 11:02 AM, Christoph Hellwig wrote:
>>> On Fri, Mar 29, 2019@10:57:54AM +0100, Hannes Reinecke wrote:
>>>> The controller ID is supposed to be unique for each subsystem;
>>>> however, if it _isn't_ we will crash when calling device_add_disk()
>>>> for the namespaces.
>>>> While this surely is an error on the subsystem side we really shouldn't
>>>> crash. So simply use the 'instance' number when generating the disk
>>>> name to avoid this issue.
>>>
>>> I agree that we should not crash in that, but I'd rather print an
>>> error message and reject using that controller with the duplicate
>>> controller id than hacking around the issue.
>>>
>> As it so happens, I have a patch for that, too, but thought this one an
>> easier solution :-)
>>
>> Will be sending that patch, then.
> 
> We might want to use this patch anyway. It is possible some process
> reference count is preventing the final release when we want to remove
> a controller, and a subsequent discover, even if the cntlid is unqiue,
> would create a duplicate disk name.
> 
I don't mind which patch is taken.
But crashing on misbehaving targets is never a good idea.

Do I need to send the other patch?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Keith Busch]

On Mon, Apr 01, 2019@04:50:13AM -0700, Hannes Reinecke wrote:
> On 4/1/19 7:17 AM, Keith Busch wrote:
> > We might want to use this patch anyway. It is possible some process
> > reference count is preventing the final release when we want to remove
> > a controller, and a subsequent discover, even if the cntlid is unqiue,
> > would create a duplicate disk name.
> > 
> I don't mind which patch is taken.
> But crashing on misbehaving targets is never a good idea.

I 100% agree. :)
 
> Do I need to send the other patch?

Can we do both? I like the warning when an invalid subsystem is detected,
but your original patch fixes the unique gendisk name, so I'd vote
to keep that good code, but justify it with a slightly different
changelog. Something along the following:

  A process holding an open reference to a removed disk prevents it
  from completing deletion, so its name continues to exist. A subsequent
  gendisk creation may have the same cntlid which risks collision when
  using that for the name. Use the unique ctrl->instance instead.

[PATCH] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Christoph Hellwig]

On Tue, Apr 02, 2019@02:55:40AM -0600, Keith Busch wrote:
> On Mon, Apr 01, 2019@04:50:13AM -0700, Hannes Reinecke wrote:
> > On 4/1/19 7:17 AM, Keith Busch wrote:
> > > We might want to use this patch anyway. It is possible some process
> > > reference count is preventing the final release when we want to remove
> > > a controller, and a subsequent discover, even if the cntlid is unqiue,
> > > would create a duplicate disk name.
> > > 
> > I don't mind which patch is taken.
> > But crashing on misbehaving targets is never a good idea.
> 
> I 100% agree. :)
>  
> > Do I need to send the other patch?
> 
> Can we do both? I like the warning when an invalid subsystem is detected,
> but your original patch fixes the unique gendisk name, so I'd vote
> to keep that good code, but justify it with a slightly different
> changelog. Something along the following:

Yes, let's do both.  Hannes, please resend a series with this patch
with the updated changelog and the duplicate cntlid check one.