* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2023-02-28 17:11 [PATCH] scsi: megasas: Internal cdbs have 16-byte length Guenter Roeck
@ 2023-02-28 22:00 ` Philippe Mathieu-Daudé
2023-03-01 12:42 ` Michael Tokarev
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-28 22:00 UTC (permalink / raw)
To: Guenter Roeck, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel, Hannes Reinecke
+Hannes
On 28/2/23 18:11, Guenter Roeck wrote:
> Host drivers do not necessarily set cdb_len in megasas io commands.
> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
> scsi_req_new()"), this results in failures to boot Linux from affected
> SCSI drives because cdb_len is set to 0 by the host driver.
> Set the cdb length to its actual size to solve the problem.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> hw/scsi/megasas.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 9cbbb16121..d624866bb6 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
> uint8_t cdb[16];
> int len;
> struct SCSIDevice *sdev = NULL;
> - int target_id, lun_id, cdb_len;
> + int target_id, lun_id;
>
> lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
> lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
> @@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>
> target_id = cmd->frame->header.target_id;
> lun_id = cmd->frame->header.lun_id;
> - cdb_len = cmd->frame->header.cdb_len;
>
> if (target_id < MFI_MAX_LD && lun_id == 0) {
> sdev = scsi_device_find(&s->bus, 0, target_id, lun_id);
> @@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
> return MFI_STAT_DEVICE_NOT_FOUND;
> }
>
> - if (cdb_len > 16) {
> - trace_megasas_scsi_invalid_cdb_len(
> - mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
> - megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
> - cmd->frame->header.scsi_status = CHECK_CONDITION;
> - s->event_count++;
> - return MFI_STAT_SCSI_DONE_WITH_ERROR;
> - }
> -
> cmd->iov_size = lba_count * sdev->blocksize;
> if (megasas_map_sgl(s, cmd, &cmd->frame->io.sgl)) {
> megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
> @@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>
> megasas_encode_lba(cdb, lba_start, lba_count, is_write);
> cmd->req = scsi_req_new(sdev, cmd->index,
> - lun_id, cdb, cdb_len, cmd);
> + lun_id, cdb, sizeof(cdb), cmd);
I haven't checked the spec, but this logic makes sense to me, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> if (!cmd->req) {
> trace_megasas_scsi_req_alloc_failed(
> mfi_frame_desc(frame_cmd), target_id, lun_id);
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2023-02-28 17:11 [PATCH] scsi: megasas: Internal cdbs have 16-byte length Guenter Roeck
2023-02-28 22:00 ` Philippe Mathieu-Daudé
@ 2023-03-01 12:42 ` Michael Tokarev
2024-02-17 9:06 ` Michael Tokarev
2023-03-03 9:02 ` Fiona Ebner
2024-11-21 19:41 ` Paolo Bonzini
3 siblings, 1 reply; 10+ messages in thread
From: Michael Tokarev @ 2023-03-01 12:42 UTC (permalink / raw)
To: Guenter Roeck, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel, Guenter Roeck,
qemu-stable
28.02.2023 20:11, Guenter Roeck wrote:
> Host drivers do not necessarily set cdb_len in megasas io commands.
> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
> scsi_req_new()"), this results in failures to boot Linux from affected
> SCSI drives because cdb_len is set to 0 by the host driver.
> Set the cdb length to its actual size to solve the problem.
This smells like a -stable material (CC qemu-stable@).
/mjt
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2023-03-01 12:42 ` Michael Tokarev
@ 2024-02-17 9:06 ` Michael Tokarev
2024-02-17 15:36 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Michael Tokarev @ 2024-02-17 9:06 UTC (permalink / raw)
To: Guenter Roeck, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel, qemu-stable
> 28.02.2023 20:11, Guenter Roeck wrote:
>> Host drivers do not necessarily set cdb_len in megasas io commands.
>> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
>> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
>> scsi_req_new()"), this results in failures to boot Linux from affected
>> SCSI drives because cdb_len is set to 0 by the host driver.
>> Set the cdb length to its actual size to solve the problem.
Has this been lost/forgotten?
/mjt
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2024-02-17 9:06 ` Michael Tokarev
@ 2024-02-17 15:36 ` Guenter Roeck
2024-11-21 18:53 ` Fabiano Rosas
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2024-02-17 15:36 UTC (permalink / raw)
To: Michael Tokarev, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel, qemu-stable
On 2/17/24 01:06, Michael Tokarev wrote:
>> 28.02.2023 20:11, Guenter Roeck wrote:
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
>>> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
>>> scsi_req_new()"), this results in failures to boot Linux from affected
>>> SCSI drives because cdb_len is set to 0 by the host driver.
>>> Set the cdb length to its actual size to solve the problem.
>
> Has this been lost/forgotten?
>
Not sure. My understanding was that I could not prove that this is how
real hardware handles the situation, thus it wasn't applied. I carry it
locally in my builds of qemu, so it is not a problem for me. Note that
I didn't check if the problem has since been fixed differently. Maybe
that is the case and the problem no longer exists in the upstream version
of qemu.
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2024-02-17 15:36 ` Guenter Roeck
@ 2024-11-21 18:53 ` Fabiano Rosas
0 siblings, 0 replies; 10+ messages in thread
From: Fabiano Rosas @ 2024-11-21 18:53 UTC (permalink / raw)
To: Guenter Roeck, Michael Tokarev, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel, qemu-stable
Guenter Roeck <linux@roeck-us.net> writes:
> On 2/17/24 01:06, Michael Tokarev wrote:
>>> 28.02.2023 20:11, Guenter Roeck wrote:
>>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>>> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
>>>> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
>>>> scsi_req_new()"), this results in failures to boot Linux from affected
>>>> SCSI drives because cdb_len is set to 0 by the host driver.
>>>> Set the cdb length to its actual size to solve the problem.
>>
>> Has this been lost/forgotten?
>>
>
> Not sure. My understanding was that I could not prove that this is how
> real hardware handles the situation, thus it wasn't applied. I carry it
> locally in my builds of qemu, so it is not a problem for me. Note that
> I didn't check if the problem has since been fixed differently. Maybe
> that is the case and the problem no longer exists in the upstream version
> of qemu.
The issue still happens upstream and I have just written the exact same
fix for it, so I'm adding my:
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2023-02-28 17:11 [PATCH] scsi: megasas: Internal cdbs have 16-byte length Guenter Roeck
2023-02-28 22:00 ` Philippe Mathieu-Daudé
2023-03-01 12:42 ` Michael Tokarev
@ 2023-03-03 9:02 ` Fiona Ebner
2023-03-03 15:10 ` Guenter Roeck
2024-11-21 19:41 ` Paolo Bonzini
3 siblings, 1 reply; 10+ messages in thread
From: Fiona Ebner @ 2023-03-03 9:02 UTC (permalink / raw)
To: Guenter Roeck, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel
Am 28.02.23 um 18:11 schrieb Guenter Roeck:
> Host drivers do not necessarily set cdb_len in megasas io commands.
> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
> scsi_req_new()"), this results in failures to boot Linux from affected
> SCSI drives because cdb_len is set to 0 by the host driver.
> Set the cdb length to its actual size to solve the problem.
>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
But I do have a question:
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> hw/scsi/megasas.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 9cbbb16121..d624866bb6 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
> uint8_t cdb[16];
> int len;
> struct SCSIDevice *sdev = NULL;
> - int target_id, lun_id, cdb_len;
> + int target_id, lun_id;
>
> lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
> lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
> @@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>
> target_id = cmd->frame->header.target_id;
> lun_id = cmd->frame->header.lun_id;
> - cdb_len = cmd->frame->header.cdb_len;
>
> if (target_id < MFI_MAX_LD && lun_id == 0) {
> sdev = scsi_device_find(&s->bus, 0, target_id, lun_id);
> @@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
> return MFI_STAT_DEVICE_NOT_FOUND;
> }
>
> - if (cdb_len > 16) {
> - trace_megasas_scsi_invalid_cdb_len(
> - mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
> - megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
> - cmd->frame->header.scsi_status = CHECK_CONDITION;
> - s->event_count++;
> - return MFI_STAT_SCSI_DONE_WITH_ERROR;
> - }
Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
consequence of
> Host drivers do not necessarily set cdb_len in megasas io commands.
that this can be uninitialized memory and we need to assume it was not
explicitly set?
Best Regards,
Fiona
> -
> cmd->iov_size = lba_count * sdev->blocksize;
> if (megasas_map_sgl(s, cmd, &cmd->frame->io.sgl)) {
> megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
> @@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>
> megasas_encode_lba(cdb, lba_start, lba_count, is_write);
> cmd->req = scsi_req_new(sdev, cmd->index,
> - lun_id, cdb, cdb_len, cmd);
> + lun_id, cdb, sizeof(cdb), cmd);
> if (!cmd->req) {
> trace_megasas_scsi_req_alloc_failed(
> mfi_frame_desc(frame_cmd), target_id, lun_id);
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2023-03-03 9:02 ` Fiona Ebner
@ 2023-03-03 15:10 ` Guenter Roeck
2023-03-06 7:36 ` Fiona Ebner
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2023-03-03 15:10 UTC (permalink / raw)
To: Fiona Ebner, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel
On 3/3/23 01:02, Fiona Ebner wrote:
> Am 28.02.23 um 18:11 schrieb Guenter Roeck:
>> Host drivers do not necessarily set cdb_len in megasas io commands.
>> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
>> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
>> scsi_req_new()"), this results in failures to boot Linux from affected
>> SCSI drives because cdb_len is set to 0 by the host driver.
>> Set the cdb length to its actual size to solve the problem.
>>
>
> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
>
> But I do have a question:
>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> hw/scsi/megasas.c | 14 ++------------
>> 1 file changed, 2 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 9cbbb16121..d624866bb6 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>> uint8_t cdb[16];
>> int len;
>> struct SCSIDevice *sdev = NULL;
>> - int target_id, lun_id, cdb_len;
>> + int target_id, lun_id;
>>
>> lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
>> lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
>> @@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>>
>> target_id = cmd->frame->header.target_id;
>> lun_id = cmd->frame->header.lun_id;
>> - cdb_len = cmd->frame->header.cdb_len;
>>
>> if (target_id < MFI_MAX_LD && lun_id == 0) {
>> sdev = scsi_device_find(&s->bus, 0, target_id, lun_id);
>> @@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>> return MFI_STAT_DEVICE_NOT_FOUND;
>> }
>>
>> - if (cdb_len > 16) {
>> - trace_megasas_scsi_invalid_cdb_len(
>> - mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
>> - megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
>> - cmd->frame->header.scsi_status = CHECK_CONDITION;
>> - s->event_count++;
>> - return MFI_STAT_SCSI_DONE_WITH_ERROR;
>> - }
>
> Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
> consequence of
>
>> Host drivers do not necessarily set cdb_len in megasas io commands.
>
> that this can be uninitialized memory and we need to assume it was not
> explicitly set?
>
I doubt that real hardware uses or checks the field for the affected commands
because that would be pointless, but it is really up to you to decide how
you want to handle it.
Guenter
> Best Regards,
> Fiona
>
>> -
>> cmd->iov_size = lba_count * sdev->blocksize;
>> if (megasas_map_sgl(s, cmd, &cmd->frame->io.sgl)) {
>> megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
>> @@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd *cmd, int frame_cmd)
>>
>> megasas_encode_lba(cdb, lba_start, lba_count, is_write);
>> cmd->req = scsi_req_new(sdev, cmd->index,
>> - lun_id, cdb, cdb_len, cmd);
>> + lun_id, cdb, sizeof(cdb), cmd);
>> if (!cmd->req) {
>> trace_megasas_scsi_req_alloc_failed(
>> mfi_frame_desc(frame_cmd), target_id, lun_id);
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2023-03-03 15:10 ` Guenter Roeck
@ 2023-03-06 7:36 ` Fiona Ebner
0 siblings, 0 replies; 10+ messages in thread
From: Fiona Ebner @ 2023-03-06 7:36 UTC (permalink / raw)
To: Guenter Roeck, Hannes Reinecke
Cc: Paolo Bonzini, Fam Zheng, qemu-block, qemu-devel
Am 03.03.23 um 16:10 schrieb Guenter Roeck:
> On 3/3/23 01:02, Fiona Ebner wrote:
>> Am 28.02.23 um 18:11 schrieb Guenter Roeck:
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>> With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
>>> exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
>>> scsi_req_new()"), this results in failures to boot Linux from affected
>>> SCSI drives because cdb_len is set to 0 by the host driver.
>>> Set the cdb length to its actual size to solve the problem.
>>>
>>
>> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
>>
>> But I do have a question:
>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> hw/scsi/megasas.c | 14 ++------------
>>> 1 file changed, 2 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>>> index 9cbbb16121..d624866bb6 100644
>>> --- a/hw/scsi/megasas.c
>>> +++ b/hw/scsi/megasas.c
>>> @@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> uint8_t cdb[16];
>>> int len;
>>> struct SCSIDevice *sdev = NULL;
>>> - int target_id, lun_id, cdb_len;
>>> + int target_id, lun_id;
>>> lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
>>> lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
>>> @@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> target_id = cmd->frame->header.target_id;
>>> lun_id = cmd->frame->header.lun_id;
>>> - cdb_len = cmd->frame->header.cdb_len;
>>> if (target_id < MFI_MAX_LD && lun_id == 0) {
>>> sdev = scsi_device_find(&s->bus, 0, target_id, lun_id);
>>> @@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> return MFI_STAT_DEVICE_NOT_FOUND;
>>> }
>>> - if (cdb_len > 16) {
>>> - trace_megasas_scsi_invalid_cdb_len(
>>> - mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
>>> - megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
>>> - cmd->frame->header.scsi_status = CHECK_CONDITION;
>>> - s->event_count++;
>>> - return MFI_STAT_SCSI_DONE_WITH_ERROR;
>>> - }
>>
>> Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
>> consequence of
>>
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>
>> that this can be uninitialized memory and we need to assume it was not
>> explicitly set?
>>
>
> I doubt that real hardware uses or checks the field for the affected
> commands
> because that would be pointless, but it is really up to you to decide how
> you want to handle it.
>
> Guenter
Okay, thank you for the explanation!
>
>> Best Regards,
>> Fiona
>>
>>> -
>>> cmd->iov_size = lba_count * sdev->blocksize;
>>> if (megasas_map_sgl(s, cmd, &cmd->frame->io.sgl)) {
>>> megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
>>> @@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s,
>>> MegasasCmd *cmd, int frame_cmd)
>>> megasas_encode_lba(cdb, lba_start, lba_count, is_write);
>>> cmd->req = scsi_req_new(sdev, cmd->index,
>>> - lun_id, cdb, cdb_len, cmd);
>>> + lun_id, cdb, sizeof(cdb), cmd);
>>> if (!cmd->req) {
>>> trace_megasas_scsi_req_alloc_failed(
>>> mfi_frame_desc(frame_cmd), target_id, lun_id);
>>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
2023-02-28 17:11 [PATCH] scsi: megasas: Internal cdbs have 16-byte length Guenter Roeck
` (2 preceding siblings ...)
2023-03-03 9:02 ` Fiona Ebner
@ 2024-11-21 19:41 ` Paolo Bonzini
3 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2024-11-21 19:41 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hannes Reinecke, Fam Zheng, qemu-block, qemu-devel
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread