[PATCH] nvme: reject the ns when the block size is smaller than a sector

[PATCH] nvme: reject the ns when the block size is smaller than a sector

[Li Feng]

The nvme spec(1.4a, figure 248) says:
"A value smaller than 9 (i.e., 512 bytes) is not supported."

Signed-off-by: Li Feng <fengli@smartx.com>
---
 drivers/nvme/host/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f320273fc672..1f02e6e49a05 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2161,6 +2161,12 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
 
 	blk_mq_freeze_queue(ns->disk->queue);
 	ns->lba_shift = id->lbaf[lbaf].ds;
+	if (ns->lba_shift < 9) {
+		pr_warn("%s: bad lba_shift(%d)\n", ns->disk->disk_name, ns->lba_shift);
+		ret = -1;
+		goto out_unfreeze;
+	}
+
 	nvme_set_queue_limits(ns->ctrl, ns->queue);
 
 	if (ns->head->ids.csi == NVME_CSI_ZNS) {
-- 
2.29.2

Re: [PATCH] nvme: reject the ns when the block size is smaller than a sector

[Johannes Thumshirn]

On 13/01/2021 17:07, Li Feng wrote:
> The nvme spec(1.4a, figure 248) says:
> "A value smaller than 9 (i.e., 512 bytes) is not supported."
> 
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
>  drivers/nvme/host/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f320273fc672..1f02e6e49a05 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2161,6 +2161,12 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>  
>  	blk_mq_freeze_queue(ns->disk->queue);
>  	ns->lba_shift = id->lbaf[lbaf].ds;
> +	if (ns->lba_shift < 9) {
> +		pr_warn("%s: bad lba_shift(%d)\n", ns->disk->disk_name, ns->lba_shift);
> +		ret = -1;
> +		goto out_unfreeze;
> +	}
> +
>  	nvme_set_queue_limits(ns->ctrl, ns->queue);
>  
>  	if (ns->head->ids.csi == NVME_CSI_ZNS) {
> 


But this only catches a physical block size < 512 for NVMe, not any other block device.

Please fix it for the general case in blk_queue_physical_block_size().

Thanks,
	Johannes

Re: [PATCH] nvme: reject the ns when the block size is smaller than a sector

[Sagi Grimberg]

>> The nvme spec(1.4a, figure 248) says:
>> "A value smaller than 9 (i.e., 512 bytes) is not supported."
>>
>> Signed-off-by: Li Feng <fengli@smartx.com>
>> ---
>>   drivers/nvme/host/core.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index f320273fc672..1f02e6e49a05 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -2161,6 +2161,12 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>>   
>>   	blk_mq_freeze_queue(ns->disk->queue);
>>   	ns->lba_shift = id->lbaf[lbaf].ds;
>> +	if (ns->lba_shift < 9) {
>> +		pr_warn("%s: bad lba_shift(%d)\n", ns->disk->disk_name, ns->lba_shift);
>> +		ret = -1;

Meaningful errno would be useful. Also better to use dev_warn

>> +		goto out_unfreeze;
>> +	}
>> +
>>   	nvme_set_queue_limits(ns->ctrl, ns->queue);
>>   
>>   	if (ns->head->ids.csi == NVME_CSI_ZNS) {
>>
> 
> 
> But this only catches a physical block size < 512 for NVMe, not any other block device.
> 
> Please fix it for the general case in blk_queue_physical_block_size().

We actually call that later and would probably be better to check here..

Re: [PATCH] nvme: reject the ns when the block size is smaller than a sector

[Feng Li]

Sagi Grimberg <sagi@grimberg.me> 于2021年1月14日周四 上午6:13写道：
>
>
> >> The nvme spec(1.4a, figure 248) says:
> >> "A value smaller than 9 (i.e., 512 bytes) is not supported."
> >>
> >> Signed-off-by: Li Feng <fengli@smartx.com>
> >> ---
> >>   drivers/nvme/host/core.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >> index f320273fc672..1f02e6e49a05 100644
> >> --- a/drivers/nvme/host/core.c
> >> +++ b/drivers/nvme/host/core.c
> >> @@ -2161,6 +2161,12 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> >>
> >>      blk_mq_freeze_queue(ns->disk->queue);
> >>      ns->lba_shift = id->lbaf[lbaf].ds;
> >> +    if (ns->lba_shift < 9) {
> >> +            pr_warn("%s: bad lba_shift(%d)\n", ns->disk->disk_name, ns->lba_shift);
> >> +            ret = -1;
>
> Meaningful errno would be useful. Also better to use dev_warn
>
> >> +            goto out_unfreeze;
> >> +    }
> >> +
> >>      nvme_set_queue_limits(ns->ctrl, ns->queue);
> >>
> >>      if (ns->head->ids.csi == NVME_CSI_ZNS) {
> >>
> >
> >
> > But this only catches a physical block size < 512 for NVMe, not any other block device.
> >
> > Please fix it for the general case in blk_queue_physical_block_size().
>
> We actually call that later and would probably be better to check here..
Thanks for your comments.

The prototype is:
void blk_queue_logical_block_size(struct request_queue *, unsigned int);

So change it to:
bool blk_queue_logical_block_size(struct request_queue *, unsigned int);

And check all callers of this function, right?

Re: [PATCH] nvme: reject the ns when the block size is smaller than a sector

[Christoph Hellwig]

On Wed, Jan 13, 2021 at 02:12:59PM -0800, Sagi Grimberg wrote:
>> But this only catches a physical block size < 512 for NVMe, not any other block device.
>>
>> Please fix it for the general case in blk_queue_physical_block_size().
>
> We actually call that later and would probably be better to check here..

We had a series for that a short while ago that got lost.

Re: [PATCH] nvme: reject the ns when the block size is smaller than a sector

[Sagi Grimberg]

>>> But this only catches a physical block size < 512 for NVMe, not any other block device.
>>>
>>> Please fix it for the general case in blk_queue_physical_block_size().
>>
>> We actually call that later and would probably be better to check here..
> 
> We had a series for that a short while ago that got lost.

What was it called?

Re: [PATCH] nvme: reject the ns when the block size is smaller than a sector

[Li Feng]

Christoph Hellwig <hch@lst.de> 于2021年1月15日周五 上午1:43写道：
>
> On Wed, Jan 13, 2021 at 02:12:59PM -0800, Sagi Grimberg wrote:
> >> But this only catches a physical block size < 512 for NVMe, not any other block device.
> >>
> >> Please fix it for the general case in blk_queue_physical_block_size().
> >
> > We actually call that later and would probably be better to check here..
>
> We had a series for that a short while ago that got lost.
Where is the series? Or could you advise on how to fix this issue is acceptable?
This issue will generate an oops, check it here:
https://lkml.org/lkml/2021/1/12/1064

Thanks,
Feng Li