[PATCH v1 0/2] Fix boot failure caused by loop abortion

[PATCH v1 0/2] Fix boot failure caused by loop abortion

[Yuan Yu]

A goto following an error check may cause boot failure, because the
function that returns the status is supposed to be called for all the
possible Puns in the SCSI channel.

This patch removes the check and the goto so that the while loop will
continue to run until all devices are scanned.

The changes can be seen at:
https://github.com/yyu/edk2/tree/scsi_bus_fix_v1

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Sivaparvathi chellaiah <sivaparvathic@ami.com>

Yuan Yu (2):
  MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c
  MdeModulePkg: Clean up unused Status.

 MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

-- 
2.39.0.314.g84b9a713c41-goog

[PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c

[Yuan Yu]

A while loop in SCSIBusDriverBindingStart() is supposed to scan all the
possible Puns in the SCSI channel by calling ScsiScanCreateDevice() for
each of them. Therefore, we should not abort the loop even when one of
the Puns is disconnected.

The following is one of the scenarios.

SCSIBusDriverBindingStart()
> ScsiScanCreateDevice()
  > DiscoverScsiDevice()
    > ScsiInquiryCommand()
      > ...
        > ParseResponse()

When virtio-scsi returns VIRTIO_SCSI_S_BAD_TARGET, ParseResponse() in
VirtioScsi.c will return EFI_TIMEOUT:

    case VIRTIO_SCSI_S_BAD_TARGET:
      //
      // This is non-intuitive but explicitly required by the
      // EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() specification for
      // disconnected (but otherwise valid) target / LUN addresses.
      //
      Packet->HostAdapterStatus =
        EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT_COMMAND;
      return EFI_TIMEOUT;

This will eventually cause DiscoverScsiDevice() to return FALSE, which
will cause ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES:

  if (!DiscoverScsiDevice (ScsiIoDevice)) {
    Status = EFI_OUT_OF_RESOURCES;
    goto ErrorExit;
  }

In sum, "disconnected (but otherwise valid) target / LUN addresses" can
result in EFI_OUT_OF_RESOURCES and when this happens the while loop in
SCSIBusDriverBindingStart() should continue, not abort.

Without this fix, the loop can terminate prematurely with good devices
not having a chance to be discovered. If this good device is the boot
device, boot will fail.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Sivaparvathi chellaiah <sivaparvathic@ami.com>

Signed-off-by: Yuan Yu <yuanyu@google.com>
---
 MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
index fbe14c772496..2ed816da4abe 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
+++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
@@ -533,9 +533,6 @@ SCSIBusDriverBindingStart (
     // then create handle and install scsi i/o protocol.
     //
     Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun, ScsiBusDev);
-    if (Status == EFI_OUT_OF_RESOURCES) {
-      goto ErrorExit;
-    }
   }
 
   return EFI_SUCCESS;
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c

[Wu, Hao A]

Thanks for the patch, inline comments below:


> -----Original Message-----
> From: Yuan Yu <yuanyu@google.com>
> Sent: Wednesday, January 18, 2023 5:14 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; C, sivaparvathi <sivaparvathic@ami.com>
> Subject: [PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c
> 
> A while loop in SCSIBusDriverBindingStart() is supposed to scan all the
> possible Puns in the SCSI channel by calling ScsiScanCreateDevice() for
> each of them. Therefore, we should not abort the loop even when one of
> the Puns is disconnected.
> 
> The following is one of the scenarios.
> 
> SCSIBusDriverBindingStart()
> > ScsiScanCreateDevice()
>   > DiscoverScsiDevice()
>     > ScsiInquiryCommand()
>       > ...
>         > ParseResponse()
> 
> When virtio-scsi returns VIRTIO_SCSI_S_BAD_TARGET, ParseResponse() in
> VirtioScsi.c will return EFI_TIMEOUT:
> 
>     case VIRTIO_SCSI_S_BAD_TARGET:
>       //
>       // This is non-intuitive but explicitly required by the
>       // EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() specification for
>       // disconnected (but otherwise valid) target / LUN addresses.
>       //
>       Packet->HostAdapterStatus =
>         EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT_COMMAND;
>       return EFI_TIMEOUT;
> 
> This will eventually cause DiscoverScsiDevice() to return FALSE, which
> will cause ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES:
> 
>   if (!DiscoverScsiDevice (ScsiIoDevice)) {
>     Status = EFI_OUT_OF_RESOURCES;
>     goto ErrorExit;
>   }


I think when DiscoverScsiDevice() returns FALSE, it (the current code implementation) is improper
to simply return EFI_OUT_OF_RESOURCES.

My take is that, under the scope of the SCSI driver, EFI_OUT_OF_RESOURCES should be used as return
status when memory allocation fails (due to insufficient free memory).

But when DiscoverScsiDevice() returns FALSE, there are cases where the cause is SCSI command
execution failure like you mentioned above.

So my recommendation is to refine the interface for DiscoverScsiDevice() to return accurate status
of the execution of the function, like
* EFI_SUCCESS for device successfully discovered;
* EFI_OUT_OF_RESOURCES for memory allocation failure;
* EFI_NOT_FOUND for cases when:
    a) SCSI command execution failure or
    b) Return data of the command indicates no device

Maybe the refined interface can look like:
EFI_STATUS
DiscoverScsiDevice (
  IN OUT  SCSI_IO_DEV  *ScsiIoDevice
  )

The caller of DiscoverScsiDevice() will return proper status rather than simply returning
EFI_OUT_OF_RESOURCES when device discovery fails.


The reason for the below logic in SCSIBusDriverBindingStart():

    Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun, ScsiBusDev);
    if (Status == EFI_OUT_OF_RESOURCES) {
      goto ErrorExit;
    }

is that it expects ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES as an indication of lack
of memory resource in the system. Under such situation, subsequent memory allocations are very likely
to fail as well, so there is little value in continuing the loop for further device discovery.


Best Regards,
Hao Wu


> 
> In sum, "disconnected (but otherwise valid) target / LUN addresses" can
> result in EFI_OUT_OF_RESOURCES and when this happens the while loop in
> SCSIBusDriverBindingStart() should continue, not abort.
> 
> Without this fix, the loop can terminate prematurely with good devices
> not having a chance to be discovered. If this good device is the boot
> device, boot will fail.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Sivaparvathi chellaiah <sivaparvathic@ami.com>
> 
> Signed-off-by: Yuan Yu <yuanyu@google.com>
> ---
>  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> index fbe14c772496..2ed816da4abe 100644
> --- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> +++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> @@ -533,9 +533,6 @@ SCSIBusDriverBindingStart (
>      // then create handle and install scsi i/o protocol.
>      //
>      Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun,
> ScsiBusDev);
> -    if (Status == EFI_OUT_OF_RESOURCES) {
> -      goto ErrorExit;
> -    }
>    }
> 
>    return EFI_SUCCESS;
> --
> 2.39.0.314.g84b9a713c41-goog

Re: [PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c

[Yuan Yu]

[-- Attachment #1: Type: text/plain, Size: 5164 bytes --]

Hi Hao Wu,

Thank you very much for the review and the detailed advice, which is
much appreciated!

I have made the changes accordingly and sent patch v2. Please take another
look.

Thank you!

Yuan


On Wed, Jan 18, 2023 at 10:33 PM Wu, Hao A <hao.a.wu@intel.com> wrote:

> Thanks for the patch, inline comments below:
>
>
> > -----Original Message-----
> > From: Yuan Yu <yuanyu@google.com>
> > Sent: Wednesday, January 18, 2023 5:14 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; C, sivaparvathi <sivaparvathic@ami.com>
> > Subject: [PATCH v1 1/2] MdeModulePkg: Fix bug in ScsiBusDxe/ScsiBus.c
> >
> > A while loop in SCSIBusDriverBindingStart() is supposed to scan all the
> > possible Puns in the SCSI channel by calling ScsiScanCreateDevice() for
> > each of them. Therefore, we should not abort the loop even when one of
> > the Puns is disconnected.
> >
> > The following is one of the scenarios.
> >
> > SCSIBusDriverBindingStart()
> > > ScsiScanCreateDevice()
> >   > DiscoverScsiDevice()
> >     > ScsiInquiryCommand()
> >       > ...
> >         > ParseResponse()
> >
> > When virtio-scsi returns VIRTIO_SCSI_S_BAD_TARGET, ParseResponse() in
> > VirtioScsi.c will return EFI_TIMEOUT:
> >
> >     case VIRTIO_SCSI_S_BAD_TARGET:
> >       //
> >       // This is non-intuitive but explicitly required by the
> >       // EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() specification for
> >       // disconnected (but otherwise valid) target / LUN addresses.
> >       //
> >       Packet->HostAdapterStatus =
> >         EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT_COMMAND;
> >       return EFI_TIMEOUT;
> >
> > This will eventually cause DiscoverScsiDevice() to return FALSE, which
> > will cause ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES:
> >
> >   if (!DiscoverScsiDevice (ScsiIoDevice)) {
> >     Status = EFI_OUT_OF_RESOURCES;
> >     goto ErrorExit;
> >   }
>
>
> I think when DiscoverScsiDevice() returns FALSE, it (the current code
> implementation) is improper
> to simply return EFI_OUT_OF_RESOURCES.
>
> My take is that, under the scope of the SCSI driver, EFI_OUT_OF_RESOURCES
> should be used as return
> status when memory allocation fails (due to insufficient free memory).
>
> But when DiscoverScsiDevice() returns FALSE, there are cases where the
> cause is SCSI command
> execution failure like you mentioned above.
>
> So my recommendation is to refine the interface for DiscoverScsiDevice()
> to return accurate status
> of the execution of the function, like
> * EFI_SUCCESS for device successfully discovered;
> * EFI_OUT_OF_RESOURCES for memory allocation failure;
> * EFI_NOT_FOUND for cases when:
>     a) SCSI command execution failure or
>     b) Return data of the command indicates no device
>
> Maybe the refined interface can look like:
> EFI_STATUS
> DiscoverScsiDevice (
>   IN OUT  SCSI_IO_DEV  *ScsiIoDevice
>   )
>
> The caller of DiscoverScsiDevice() will return proper status rather than
> simply returning
> EFI_OUT_OF_RESOURCES when device discovery fails.
>
>
> The reason for the below logic in SCSIBusDriverBindingStart():
>
>     Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun,
> ScsiBusDev);
>     if (Status == EFI_OUT_OF_RESOURCES) {
>       goto ErrorExit;
>     }
>
> is that it expects ScsiScanCreateDevice() to return EFI_OUT_OF_RESOURCES
> as an indication of lack
> of memory resource in the system. Under such situation, subsequent memory
> allocations are very likely
> to fail as well, so there is little value in continuing the loop for
> further device discovery.
>
>
> Best Regards,
> Hao Wu
>
>
> >
> > In sum, "disconnected (but otherwise valid) target / LUN addresses" can
> > result in EFI_OUT_OF_RESOURCES and when this happens the while loop in
> > SCSIBusDriverBindingStart() should continue, not abort.
> >
> > Without this fix, the loop can terminate prematurely with good devices
> > not having a chance to be discovered. If this good device is the boot
> > device, boot will fail.
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Sivaparvathi chellaiah <sivaparvathic@ami.com>
> >
> > Signed-off-by: Yuan Yu <yuanyu@google.com>
> > ---
> >  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > index fbe14c772496..2ed816da4abe 100644
> > --- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > +++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
> > @@ -533,9 +533,6 @@ SCSIBusDriverBindingStart (
> >      // then create handle and install scsi i/o protocol.
> >      //
> >      Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun,
> > ScsiBusDev);
> > -    if (Status == EFI_OUT_OF_RESOURCES) {
> > -      goto ErrorExit;
> > -    }
> >    }
> >
> >    return EFI_SUCCESS;
> > --
> > 2.39.0.314.g84b9a713c41-goog
>
>

[-- Attachment #2: Type: text/html, Size: 7185 bytes --]

[PATCH v1 2/2] MdeModulePkg: Clean up unused Status.

[Yuan Yu]

Logically, the while loop that contains ScsiScanCreateDevice() should
continue regardless what the returned Status is, because the purpose of
the while loop is to scan all possible Puns in the SCSI channel.

Without this fix, some static analyzer may complain about the unused
return value.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Sivaparvathi chellaiah <sivaparvathic@ami.com>

Signed-off-by: Yuan Yu <yuanyu@google.com>
---
 MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
index 2ed816da4abe..7f133253a256 100644
--- a/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
+++ b/MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c
@@ -532,7 +532,7 @@ SCSIBusDriverBindingStart (
     // Scan for the scsi device, if it attaches to the scsi bus,
     // then create handle and install scsi i/o protocol.
     //
-    Status = ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun, ScsiBusDev);
+    ScsiScanCreateDevice (This, Controller, &ScsiTargetId, Lun, ScsiBusDev);
   }
 
   return EFI_SUCCESS;
-- 
2.39.0.314.g84b9a713c41-goog