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

[Jordan Niethe]

On Mon, Aug 28, 2023 at 5:14 PM Thomas Huth <thuth at redhat.com> wrote:
>
> On 28/08/2023 03.37, 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>
> > ---
> >   board-qemu/slof/virtio-serial.fs | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs
> > index 41e2e04..54dacff 100644
> > --- a/board-qemu/slof/virtio-serial.fs
> > +++ b/board-qemu/slof/virtio-serial.fs
> > @@ -33,8 +33,6 @@ 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
> > @@ -42,7 +40,7 @@ virtiodev virtio-serial-init drop
> >       \ 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
>
> Please adjust the comment (since stdout now does not get closed anymore).

Sure, will send a v2.

>
>   Thanks,
>    Thomas
>
>
> >       \ device cannot be reset properly and the boot will hang.
> > -    ['] virtio-serial-close-stdout add-quiesce-xt
> > +    ['] 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 )
>