[PATCH] nvme: validate cntlid's only for nvme >= 1.1.0

[PATCH] nvme: validate cntlid's only for nvme >= 1.1.0

[Maximilian Heyne]

Controller ID's (cntlid) for NVMe devices were introduced in version
1.1.0 of the specification. Controllers that follow the older 1.0.0 spec
don't set this field so it doesn't make sense to validate it. On the
contrary, when using SR-IOV this check breaks VFs as they are all part
of the same NVMe subsystem.

Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
Cc: <stable@vger.kernel.org> # 5.4+
---
 drivers/nvme/host/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 28f4388c1337..c4a991acc949 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2773,7 +2773,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 		put_device(&subsys->dev);
 		subsys = found;
 
-		if (!nvme_validate_cntlid(subsys, ctrl, id)) {
+		if (ctrl->vs >= NVME_VS(1, 1, 0) &&
+		    !nvme_validate_cntlid(subsys, ctrl, id)) {
 			ret = -EINVAL;
 			goto out_put_subsystem;
 		}
@@ -2883,7 +2884,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 			goto out_free;
 	}
 
-	if (!(ctrl->ops->flags & NVME_F_FABRICS))
+	if (!(ctrl->ops->flags & NVME_F_FABRICS) && ctrl->vs >= NVME_VS(1, 1, 0))
 		ctrl->cntlid = le16_to_cpu(id->cntlid);
 
 	if (!ctrl->identified) {
-- 
2.16.6




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] nvme: validate cntlid's only for nvme >= 1.1.0

[Christoph Hellwig]

On Tue, Jun 30, 2020 at 12:29:23PM +0000, Maximilian Heyne wrote:
> Controller ID's (cntlid) for NVMe devices were introduced in version
> 1.1.0 of the specification. Controllers that follow the older 1.0.0 spec
> don't set this field so it doesn't make sense to validate it. On the
> contrary, when using SR-IOV this check breaks VFs as they are all part
> of the same NVMe subsystem.
> 
> Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
> Cc: <stable@vger.kernel.org> # 5.4+

The first hunk looks ok, the second doesn't make sense as fabrics
was only added with NVMe 1.2.2.  I can fix it up when applying if you
are ok with that.

But you guys really shouldn't be doing SR-IOV with 1.0 controllers
independent of this..

Re: [PATCH] nvme: validate cntlid's only for nvme >= 1.1.0

[Christoph Hellwig]

On Tue, Jun 30, 2020 at 03:33:58PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 30, 2020 at 12:29:23PM +0000, Maximilian Heyne wrote:
> > Controller ID's (cntlid) for NVMe devices were introduced in version
> > 1.1.0 of the specification. Controllers that follow the older 1.0.0 spec
> > don't set this field so it doesn't make sense to validate it. On the
> > contrary, when using SR-IOV this check breaks VFs as they are all part
> > of the same NVMe subsystem.
> > 
> > Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
> > Cc: <stable@vger.kernel.org> # 5.4+
> 
> The first hunk looks ok, the second doesn't make sense as fabrics
> was only added with NVMe 1.2.2.  I can fix it up when applying if you
> are ok with that.
> 
> But you guys really shouldn't be doing SR-IOV with 1.0 controllers
> independent of this..

And actually - 1.0 did not have the concept of a subsystem.  So having
a duplicate serial number for a 1.0 controller actually is a pretty
nasty bug.  Can you point me to this broken controller?  Do you think
the OEM could fix it up to report a proper version number and controller
ID?

Re: [PATCH] nvme: validate cntlid's only for nvme >= 1.1.0

[Maximilian Heyne]

On 6/30/20 3:36 PM, Christoph Hellwig wrote:
> On Tue, Jun 30, 2020 at 03:33:58PM +0200, Christoph Hellwig wrote:
>> On Tue, Jun 30, 2020 at 12:29:23PM +0000, Maximilian Heyne wrote:
>>> Controller ID's (cntlid) for NVMe devices were introduced in version
>>> 1.1.0 of the specification. Controllers that follow the older 1.0.0 spec
>>> don't set this field so it doesn't make sense to validate it. On the
>>> contrary, when using SR-IOV this check breaks VFs as they are all part
>>> of the same NVMe subsystem.
>>>
>>> Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
>>> Cc: <stable@vger.kernel.org> # 5.4+
>>
>> The first hunk looks ok, the second doesn't make sense as fabrics
>> was only added with NVMe 1.2.2.  I can fix it up when applying if you
>> are ok with that.

I'd be totally ok with that.

>>
>> But you guys really shouldn't be doing SR-IOV with 1.0 controllers
>> independent of this..

So far it worked...

> 
> And actually - 1.0 did not have the concept of a subsystem.  So having
> a duplicate serial number for a 1.0 controller actually is a pretty
> nasty bug.  Can you point me to this broken controller?  Do you think
> the OEM could fix it up to report a proper version number and controller
> ID?
> 

I meant that the VF NVMe controllers will all land in the same subsystem from
the kernel's point of view, because, as you said, there was no idea of different
subsystems in the 1.0 spec.
It's an older in-house controller. Seems to set the same serial number for all
VF's. Should the firmware set unique serials for the VF's instead?

Thanks
Max



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] nvme: validate cntlid's only for nvme >= 1.1.0

[Keith Busch]

On Tue, Jun 30, 2020 at 04:01:45PM +0200, Maximilian Heyne wrote:
> On 6/30/20 3:36 PM, Christoph Hellwig wrote:
> > And actually - 1.0 did not have the concept of a subsystem.  So having
> > a duplicate serial number for a 1.0 controller actually is a pretty
> > nasty bug.  Can you point me to this broken controller?  Do you think
> > the OEM could fix it up to report a proper version number and controller
> > ID?
> > 
> 
> I meant that the VF NVMe controllers will all land in the same subsystem from
> the kernel's point of view, because, as you said, there was no idea of different
> subsystems in the 1.0 spec.

Each controller should have landed in its own subsystem in this case
rather than the same subsystem.

> It's an older in-house controller. Seems to set the same serial number for all
> VF's. Should the firmware set unique serials for the VF's instead?

Yes, the driver shouldn't be finding duplicate serial numbers.