nvmet_check_data_len() wrong behavior for nvme admin set features cmd

[Engel, Amit]

By the way, why not always call nvmet_check_data_len_lte() ?

Don’t we agree that the host is allowed to send sgl describing a buffer that is larger than the payload for any nvme io/admin cmd ?
(and not only for dsm / set_features cmd)

Thanks,
Amit

-----Original Message-----
From: Engel, Amit 
Sent: Tuesday, 3 January 2023 15:20
To: Sagi Grimberg <sagi at grimberg.me>
Cc: linux-nvme at lists.infradead.org; Grupi, Elad <Elad.Grupi at dell.com>; Zinger, Eldad <Eldad.Zinger at dell.com>
Subject: RE: nvmet_check_data_len() wrong behavior for nvme admin set features cmd

Ok I see, yes, this is exactly what we need.
I will add it also to set features

Thanks
Amit

-----Original Message-----
From: Sagi Grimberg <sagi at grimberg.me> 
Sent: Tuesday, 3 January 2023 15:06
To: Engel, Amit <Amit.Engel at Dell.com>
Cc: linux-nvme at lists.infradead.org; Grupi, Elad <Elad.Grupi at dell.com>; Zinger, Eldad <Eldad.Zinger at dell.com>
Subject: Re: nvmet_check_data_len() wrong behavior for nvme admin set features cmd


[EXTERNAL EMAIL] 


> Hi Sagi et al.
> 
> We see a wrong behaviour in nvmet/core.c  nvmet_check_data_len() function for nvme admin set_features command:
> Current nvmet_check_data_len() code is checking if data_len != req->transfer_len.
> 
> There are some feature ids which the transfer_len that is being sent by the host is larger than the payload itself.
> For example:
> fid 0xE, the data_len that is sent by the host is 8.
> fid 0xC, the data_len that is sent by the host is 256.
> 
> In this case, the error that is returned to the host is ‘NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR’, which is not accurate.
>  From our understanding, the host is allowed to send sgl describing a beffer that is larger than the payload.
> 
> We thought to change the condition in nvmet_check_data_len() to check if ‘data_len > req->transfer_len’.
> Only in this case, the expected status code is Indeed 
> NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR

We already have a handle for that in nvmet_check_data_len_lte() that is used by dsm handlers, it should be added to the specific places where it is appropriate.