| Message ID | 20230829001201.11892-2-jniethe5@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] virtio-serial: Make read and write methods report failure | expand |
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@linux.vnet.ibm.com> > Co-developed-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > Signed-off-by: Jordan Niethe <jniethe5@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@redhat.com>
On 29/8/23 5:07 pm, Thomas Huth wrote: > 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@linux.vnet.ibm.com> >> Co-developed-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> >> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> >> Signed-off-by: Jordan Niethe <jniethe5@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@redhat.com> Thanks for reviewing. >
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 )
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@linux.vnet.ibm.com> Co-developed-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v2: - Reword init comment --- board-qemu/slof/virtio-serial.fs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)