diff mbox series

[v2,15/18] hw/block/nvme: reject invalid nsid values in active namespace id list

Message ID 20200703063420.2241014-16-its@irrelevant.dk (mailing list archive)
State New, archived
Headers show
Series hw/block/nvme: bump to v1.3 | expand

Commit Message

Klaus Jensen July 3, 2020, 6:34 a.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
Active Namespace ID list.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Philippe Mathieu-Daudé July 3, 2020, 8:20 a.m. UTC | #1 
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
> Active Namespace ID list.

Can we have a definition instead of this 0xfffffffe magic value please?

> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 65c2fa3ac1f4..0dac7a41ddae 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>  
>      trace_pci_nvme_identify_nslist(min_nsid);
>  
> +    if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
> +        return NVME_INVALID_NSID | NVME_DNR;
> +    }
> +
>      list = g_malloc0(data_len);
>      for (i = 0; i < n->num_namespaces; i++) {
>          if (i < min_nsid) {
>
Klaus Jensen July 3, 2020, 8:37 a.m. UTC | #2 
On Jul  3 10:20, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
> > Active Namespace ID list.
> 
> Can we have a definition instead of this 0xfffffffe magic value please?
> 

Hmm, not really actually. It's not a magic value, its just because the
logic in Active Namespace ID list would require that it should report
any namespaces with ids *higher* than the one specified, so since
0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
needs to be as well.

What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
The original condition just reads well if you are sitting with the spec
on the side.

> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 65c2fa3ac1f4..0dac7a41ddae 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> >  
> >      trace_pci_nvme_identify_nslist(min_nsid);
> >  
> > +    if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
> > +        return NVME_INVALID_NSID | NVME_DNR;
> > +    }
> > +
> >      list = g_malloc0(data_len);
> >      for (i = 0; i < n->num_namespaces; i++) {
> >          if (i < min_nsid) {
> > 
>
Philippe Mathieu-Daudé July 3, 2020, 9:14 a.m. UTC | #3 
On 7/3/20 10:37 AM, Klaus Jensen wrote:
> On Jul  3 10:20, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
>>> Active Namespace ID list.
>>
>> Can we have a definition instead of this 0xfffffffe magic value please?
>>
> 
> Hmm, not really actually. It's not a magic value, its just because the
> logic in Active Namespace ID list would require that it should report
> any namespaces with ids *higher* than the one specified, so since
> 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
> needs to be as well.

OK.

> 
> What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
> The original condition just reads well if you are sitting with the spec
> on the side.

IMO this is clearer:

  if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
      return NVME_INVALID_NSID | NVME_DNR;
  }

Whichever form you prefer you can amend to the respin patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> ---
>>>  hw/block/nvme.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index 65c2fa3ac1f4..0dac7a41ddae 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>>>  
>>>      trace_pci_nvme_identify_nslist(min_nsid);
>>>  
>>> +    if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
>>> +        return NVME_INVALID_NSID | NVME_DNR;
>>> +    }
>>> +
>>>      list = g_malloc0(data_len);
>>>      for (i = 0; i < n->num_namespaces; i++) {
>>>          if (i < min_nsid) {
>>>
>>
>
Klaus Jensen July 3, 2020, 9:22 a.m. UTC | #4 
On Jul  3 11:14, Philippe Mathieu-Daudé wrote:
> On 7/3/20 10:37 AM, Klaus Jensen wrote:
> > On Jul  3 10:20, Philippe Mathieu-Daudé wrote:
> >> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> >>> From: Klaus Jensen <k.jensen@samsung.com>
> >>>
> >>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
> >>> Active Namespace ID list.
> >>
> >> Can we have a definition instead of this 0xfffffffe magic value please?
> >>
> > 
> > Hmm, not really actually. It's not a magic value, its just because the
> > logic in Active Namespace ID list would require that it should report
> > any namespaces with ids *higher* than the one specified, so since
> > 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
> > needs to be as well.
> 
> OK.
> 
> > 
> > What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
> > The original condition just reads well if you are sitting with the spec
> > on the side.
> 
> IMO this is clearer:
> 
>   if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
>       return NVME_INVALID_NSID | NVME_DNR;
>   }
> 

But since min_nsid is uint32_t that would not be wise ;)

I'll go with the - 1 and add a comment!

> Whichever form you prefer you can amend to the respin patch:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> > 
> >>>
> >>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >>> ---
> >>>  hw/block/nvme.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >>> index 65c2fa3ac1f4..0dac7a41ddae 100644
> >>> --- a/hw/block/nvme.c
> >>> +++ b/hw/block/nvme.c
> >>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> >>>  
> >>>      trace_pci_nvme_identify_nslist(min_nsid);
> >>>  
> >>> +    if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
> >>> +        return NVME_INVALID_NSID | NVME_DNR;
> >>> +    }
> >>> +
> >>>      list = g_malloc0(data_len);
> >>>      for (i = 0; i < n->num_namespaces; i++) {
> >>>          if (i < min_nsid) {
> >>>
> >>
> > 
>
Philippe Mathieu-Daudé July 3, 2020, 10:59 a.m. UTC | #5 
On 7/3/20 11:22 AM, Klaus Jensen wrote:
> On Jul  3 11:14, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 10:37 AM, Klaus Jensen wrote:
>>> On Jul  3 10:20, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
>>>>> Active Namespace ID list.
>>>>
>>>> Can we have a definition instead of this 0xfffffffe magic value please?
>>>>
>>>
>>> Hmm, not really actually. It's not a magic value, its just because the
>>> logic in Active Namespace ID list would require that it should report
>>> any namespaces with ids *higher* than the one specified, so since
>>> 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
>>> needs to be as well.
>>
>> OK.
>>
>>>
>>> What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
>>> The original condition just reads well if you are sitting with the spec
>>> on the side.
>>
>> IMO this is clearer:
>>
>>   if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
>>       return NVME_INVALID_NSID | NVME_DNR;
>>   }
>>
> 
> But since min_nsid is uint32_t that would not be wise ;)

Hmm indeed.

> 
> I'll go with the - 1 and add a comment!

Good, thanks.

> 
>> Whichever form you prefer you can amend to the respin patch:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>>
>>>>>
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> ---
>>>>>  hw/block/nvme.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>> index 65c2fa3ac1f4..0dac7a41ddae 100644
>>>>> --- a/hw/block/nvme.c
>>>>> +++ b/hw/block/nvme.c
>>>>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>>>>>  
>>>>>      trace_pci_nvme_identify_nslist(min_nsid);
>>>>>  
>>>>> +    if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
>>>>> +        return NVME_INVALID_NSID | NVME_DNR;
>>>>> +    }
>>>>> +
>>>>>      list = g_malloc0(data_len);
>>>>>      for (i = 0; i < n->num_namespaces; i++) {
>>>>>          if (i < min_nsid) {
>>>>>
>>>>
>>>
>>
>
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 65c2fa3ac1f4..0dac7a41ddae 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -956,6 +956,10 @@  static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
 
     trace_pci_nvme_identify_nslist(min_nsid);
 
+    if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
+        return NVME_INVALID_NSID | NVME_DNR;
+    }
+
     list = g_malloc0(data_len);
     for (i = 0; i < n->num_namespaces; i++) {
         if (i < min_nsid) {