[SLOF] [PATCH v2 2/2] virtio-serial: Do not close stdout on quiesce

[Thomas Huth]

On 29/08/2023 02.12, Jordan Niethe wrote:
> Commit 76fee95 ("slof: Only close stdout for virtio-serial devices")
> says that commit cf28264 ("virtio-serial: Rework shutdown sequence")
> fixed a hang. The problem was believed to be that it was necessary to
> close stdout to shutdown the underlying virtio device.
> 
> Commit cf28264 ("virtio-serial: Rework shutdown sequence") closed stdout
> on quiesce. This meant when prom_init() called write on stdout after
> quiesce, there is a use after free so this is unreliable, and can also
> hang (especially after reboots).
> 
> Quiescing is intended to put hardware into a safe state for the client
> to take over. It is incorrect for SLOF to close ihandles that the client
> could still be using, even after a quiesce.
> 
> Rather than closing the stdout device, all that needs to happen is to
> ensure virtio-serial-shutdown gets called. On quiesce, close the virtio
> device, but leave the stdout device itself open.
> 
> Commit 8174acd ("virtio-serial: Close device completely") handles reads
> and writes as no-ops if the underlying virtio device is closed so there
> is no problem with the client calling "write" on stdout after this, but
> no output will be displayed.
> 
> Fixes: cf28264 ("virtio-serial: Rework shutdown sequence")
> Debugged-by: Kautuk Consul <kconsul at linux.vnet.ibm.com>
> Co-developed-by: Kautuk Consul <kconsul at linux.vnet.ibm.com>
> Signed-off-by: Kautuk Consul <kconsul at linux.vnet.ibm.com>
> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> ---
> v2:
>     - Reword init comment
> ---
>   board-qemu/slof/virtio-serial.fs | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs
> index 41e2e04..de42cc7 100644
> --- a/board-qemu/slof/virtio-serial.fs
> +++ b/board-qemu/slof/virtio-serial.fs
> @@ -33,16 +33,14 @@ virtio-setup-vd VALUE virtiodev
>   : virtio-serial-term-key?  virtiodev virtio-serial-haschar ;
>   : virtio-serial-term-key   BEGIN virtio-serial-term-key? UNTIL virtiodev virtio-serial-getchar ;
>   
> -: virtio-serial-close-stdout s" stdout" get-chosen IF decode-int nip nip close-dev THEN ;
> -
>   \ Basic device initialization - which has only to be done once
>   : init  ( -- )
>   virtiodev virtio-serial-init drop
>       TRUE to initialized?
> -    \ Linux closes stdin at some point in prom_init(). This internally triggers a
> -    \ quiesce in SLOF. We must ensure stdout gets closed as well otherwise the
> -    \ device cannot be reset properly and the boot will hang.
> -    ['] virtio-serial-close-stdout add-quiesce-xt
> +    \ virtiodev must be shutdown at quiesce so the device is reset properly.
> +    \ The read and write methods can be called after quiesce so must handle
> +    \ virtiodev being closed.
> +    ['] shutdown add-quiesce-xt
>   ;
>   
>   0 VALUE open-count
> @@ -62,8 +60,8 @@ virtiodev virtio-serial-init drop
>       open-count 0> IF
>           open-count 1 - dup to open-count
>           0= IF shutdown THEN
> +        close
>       THEN
> -    close
>   ;
>   
>   : write ( addr len -- actual )

Reviewed-by: Thomas Huth <thuth at redhat.com>