Re: [PATCH 3/3] Rework PdoStartDevice()

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 14/12/2023 15:32, Owen Smith wrote:
> Moves MIB table parsing, detection of aliased device and copying of network
> settings to a seperate function, to avoid any failures to the START_DEVICE
typo... I'll fix

> IRP if network alias detection and settings copying code fails. The only failure
> of note is STATUS_PNP_REBOOT_REQUIRED, indicating the VM requires a reboot to
> replace emulated devices.
> Windows Update will reject driver updates if sufficient install failures or
> failures within 2 days of install are detected (> 5% of installs). Completing
> the START_DEVICE IRP with any failure other than STATUS_PNP_REBOOT_REQUIRED will
> cause an installation failure (Code 10) and lead to driver updates being removed
> from Windows Update.
Ouch. That's nasty.

> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> ---
>   src/xenvif/pdo.c | 146 +++++++++++++++++++++++++----------------------
>   1 file changed, 77 insertions(+), 69 deletions(-)
>
> diff --git a/src/xenvif/pdo.c b/src/xenvif/pdo.c
> index f23b047..0cef2d5 100644
> --- a/src/xenvif/pdo.c
> +++ b/src/xenvif/pdo.c
> @@ -95,8 +95,6 @@ struct _XENVIF_PDO {
>   
>       PXENVIF_VIF_CONTEXT         VifContext;
>       XENVIF_VIF_INTERFACE        VifInterface;
> -
> -    BOOLEAN                     HasAlias;
>   };
>   
>   static FORCEINLINE PVOID
> @@ -1272,64 +1270,34 @@ PdoUnplugRequested(
>       return State;
>   }
>   
> -static DECLSPEC_NOINLINE NTSTATUS
> -PdoStartDevice(
> +static FORCEINLINE NTSTATUS
> +PdoParseMibTable(
>       IN  PXENVIF_PDO     Pdo,
> -    IN  PIRP            Irp
> +    IN  HANDLE          SoftwareKey
>       )
>   {
>       NTSTATUS            (*__GetIfTable2)(PMIB_IF_TABLE2 *);
>       VOID                (*__FreeMibTable)(PVOID);
>       PMIB_IF_TABLE2      Table;
>       ULONG               Index;
> -    PIO_STACK_LOCATION  StackLocation;
> -    HANDLE              SoftwareKey;
> -    HANDLE              HardwareKey;
>       GUID                Guid;
>       NTSTATUS            status;
>   
> -    status = STATUS_UNSUCCESSFUL;
> -    if (Pdo->HasAlias)
> -        goto fail1;
> -
> -    if (DriverSafeMode())
> -        goto fail2;
> -
> -    status = RegistryOpenSoftwareKey(__PdoGetDeviceObject(Pdo),
> -                                     KEY_READ,
> -                                     &SoftwareKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail3;
> -
> -    status = RegistryOpenHardwareKey(__PdoGetDeviceObject(Pdo),
> -                                     KEY_ALL_ACCESS,
> -                                     &HardwareKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail4;
> -
> -    (VOID) PdoSetFriendlyName(Pdo,
> -                              SoftwareKey,
> -                              HardwareKey);
> -
> -    status = __PdoSetCurrentAddress(Pdo, SoftwareKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail5;
> -
>       status = LinkGetRoutineAddress("netio.sys",
>                                      "GetIfTable2",
>                                      (PVOID *)&__GetIfTable2);
>       if (!NT_SUCCESS(status))
> -        goto fail6;
> +        goto fail1;
>   
>       status = LinkGetRoutineAddress("netio.sys",
>                                      "FreeMibTable",
>                                      (PVOID *)&__FreeMibTable);
>       if (!NT_SUCCESS(status))
> -        goto fail7;
> +        goto fail2;
>   
>       status = __GetIfTable2(&Table);
>       if (!NT_SUCCESS(status))
> -        goto fail8;
> +        goto fail3;
>   
>       //
>       // Look for a network interface with the same permanent address
> @@ -1364,16 +1332,8 @@ PdoStartDevice(
>                               &Row->InterfaceGuid,
>                               &Row->InterfaceLuid);
>   
> -        Pdo->HasAlias = TRUE;
> -        break;
> -    }
> -
> -    if (Pdo->HasAlias || !PdoUnplugRequested(Pdo)) {
> -        PdoUnplugRequest(Pdo, TRUE);
> -        DriverRequestReboot();
> -
>           status = STATUS_PNP_REBOOT_REQUIRED;
> -        goto fail9;
> +        goto fail4;
>       }
>   
>       //
> @@ -1416,11 +1376,76 @@ PdoStartDevice(
>                                  &Luid);
>       }
>   
> +    __FreeMibTable(Table);
> +
> +    return STATUS_SUCCESS;
> +
> +fail4:
> +    Error("fail4\n");
> +
> +    __FreeMibTable(Table);
> +
> +fail3:
> +    Error("fail3\n");
> +
> +fail2:
> +    Error("fail2\n");
> +
> +fail1:
> +    Error("fail1 (%08x)\n", status);
> +
> +    return status;
> +}
> +
> +static DECLSPEC_NOINLINE NTSTATUS
> +PdoStartDevice(
> +    IN  PXENVIF_PDO     Pdo,
> +    IN  PIRP            Irp
> +    )
> +{
> +    PIO_STACK_LOCATION  StackLocation;
> +    HANDLE              SoftwareKey;
> +    HANDLE              HardwareKey;
> +    NTSTATUS            status;
> +
> +    status = STATUS_UNSUCCESSFUL;
> +    if (DriverSafeMode())
> +        goto fail1;
> +
> +    status = RegistryOpenSoftwareKey(__PdoGetDeviceObject(Pdo),
> +                                     KEY_READ,
> +                                     &SoftwareKey);
> +    if (!NT_SUCCESS(status))
> +        goto fail2;
> +
> +    status = RegistryOpenHardwareKey(__PdoGetDeviceObject(Pdo),
> +                                     KEY_ALL_ACCESS,
> +                                     &HardwareKey);
> +    if (!NT_SUCCESS(status))
> +        goto fail3;
> +
> +    (VOID) PdoSetFriendlyName(Pdo,
> +                              SoftwareKey,
> +                              HardwareKey);
> +
> +    status = __PdoSetCurrentAddress(Pdo, SoftwareKey);
> +    if (!NT_SUCCESS(status))
> +        goto fail4;
> +
> +    status = PdoParseMibTable(Pdo, SoftwareKey);
> +    if (status == STATUS_PNP_REBOOT_REQUIRED || !PdoUnplugRequested(Pdo)) {
So, if the parsing fails for some other reason then the settings restore 
will not happen... but meh. May as well make PdoParseMibTable() return a 
boolean; true if a reboot is require, false otherwise. It'll work as-is 
though so...
Acked-by: Paul Durrant <paul@xxxxxxx>

> +        PdoUnplugRequest(Pdo, TRUE);
> +        DriverRequestReboot();
> +
> +        status = STATUS_PNP_REBOOT_REQUIRED;
> +        goto fail5;
> +    }
> +