| Message ID | 20230829001201.11892-1-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: > From: Kautuk Consul <kconsul@linux.vnet.ibm.com> > > The read and write methods return successfully even if the virtio device > is closed (virtiodev is 0) and it is not able to send or receive any > characters. > > Make the read and write methods return 0 to indicate they did not > succeed in this case. > > This also fixes an invalid stack access in the read method. > > Fixes: 8174acd ("virtio-serial: Close device completely") > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v2: > - Rework commit message slightly > --- > board-qemu/slof/virtio-serial.fs | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs > index 82868e2..41e2e04 100644 > --- a/board-qemu/slof/virtio-serial.fs > +++ b/board-qemu/slof/virtio-serial.fs > @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop > ; > > : write ( addr len -- actual ) > - virtiodev 0= IF nip EXIT THEN > + virtiodev 0= IF 2drop 0 EXIT THEN > tuck > 0 ?DO > dup c@ virtiodev SWAP virtio-serial-putchar > @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop > > : read ( addr len -- actual ) > 0= IF drop 0 EXIT THEN > - virtiodev 0= IF nip EXIT THEN > + virtiodev 0= IF drop 0 EXIT THEN > virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN > virtiodev virtio-serial-getchar swap c! 1 > ; Reviewed-by: Thomas Huth <thuth@redhat.com>
On 29/08/2023 10:12, Jordan Niethe wrote: > From: Kautuk Consul <kconsul@linux.vnet.ibm.com> > > The read and write methods return successfully even if the virtio device > is closed (virtiodev is 0) and it is not able to send or receive any > characters. > > Make the read and write methods return 0 to indicate they did not > succeed in this case. > > This also fixes an invalid stack access in the read method. > > Fixes: 8174acd ("virtio-serial: Close device completely") > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v2: > - Rework commit message slightly > --- > board-qemu/slof/virtio-serial.fs | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs > index 82868e2..41e2e04 100644 > --- a/board-qemu/slof/virtio-serial.fs > +++ b/board-qemu/slof/virtio-serial.fs > @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop > ; > > : write ( addr len -- actual ) > - virtiodev 0= IF nip EXIT THEN > + virtiodev 0= IF 2drop 0 EXIT THEN > tuck > 0 ?DO > dup c@ virtiodev SWAP virtio-serial-putchar > @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop > > : read ( addr len -- actual ) > 0= IF drop 0 EXIT THEN > - virtiodev 0= IF nip EXIT THEN > + virtiodev 0= IF drop 0 EXIT THEN This is going to leave @addr on stack, no? The write gets it right though. > virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN > virtiodev virtio-serial-getchar swap c! 1 > ;
On Mon, Sep 4, 2023 at 3:08 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 29/08/2023 10:12, Jordan Niethe wrote: > > From: Kautuk Consul <kconsul@linux.vnet.ibm.com> > > > > The read and write methods return successfully even if the virtio device > > is closed (virtiodev is 0) and it is not able to send or receive any > > characters. > > > > Make the read and write methods return 0 to indicate they did not > > succeed in this case. > > > > This also fixes an invalid stack access in the read method. > > > > Fixes: 8174acd ("virtio-serial: Close device completely") > > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > v2: > > - Rework commit message slightly > > --- > > board-qemu/slof/virtio-serial.fs | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs > > index 82868e2..41e2e04 100644 > > --- a/board-qemu/slof/virtio-serial.fs > > +++ b/board-qemu/slof/virtio-serial.fs > > @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop > > ; > > > > : write ( addr len -- actual ) > > - virtiodev 0= IF nip EXIT THEN > > + virtiodev 0= IF 2drop 0 EXIT THEN > > tuck > > 0 ?DO > > dup c@ virtiodev SWAP virtio-serial-putchar > > @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop > > > > : read ( addr len -- actual ) > > 0= IF drop 0 EXIT THEN > > - virtiodev 0= IF nip EXIT THEN > > + virtiodev 0= IF drop 0 EXIT THEN > > > This is going to leave @addr on stack, no? The write gets it right though. The read method begins with 0= which consumes top of stack. The write method does not have that which is why they are different. > > > > virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN > > virtiodev virtio-serial-getchar swap c! 1 > > ; > > -- > Alexey > >
On 04/09/2023 15:19, Jordan Niethe wrote: > On Mon, Sep 4, 2023 at 3:08 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> >> >> On 29/08/2023 10:12, Jordan Niethe wrote: >>> From: Kautuk Consul <kconsul@linux.vnet.ibm.com> >>> >>> The read and write methods return successfully even if the virtio device >>> is closed (virtiodev is 0) and it is not able to send or receive any >>> characters. >>> >>> Make the read and write methods return 0 to indicate they did not >>> succeed in this case. >>> >>> This also fixes an invalid stack access in the read method. >>> >>> Fixes: 8174acd ("virtio-serial: Close device completely") >>> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> >>> --- >>> v2: >>> - Rework commit message slightly >>> --- >>> board-qemu/slof/virtio-serial.fs | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs >>> index 82868e2..41e2e04 100644 >>> --- a/board-qemu/slof/virtio-serial.fs >>> +++ b/board-qemu/slof/virtio-serial.fs >>> @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop >>> ; >>> >>> : write ( addr len -- actual ) >>> - virtiodev 0= IF nip EXIT THEN >>> + virtiodev 0= IF 2drop 0 EXIT THEN >>> tuck >>> 0 ?DO >>> dup c@ virtiodev SWAP virtio-serial-putchar >>> @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop >>> >>> : read ( addr len -- actual ) >>> 0= IF drop 0 EXIT THEN >>> - virtiodev 0= IF nip EXIT THEN >>> + virtiodev 0= IF drop 0 EXIT THEN >> >> >> This is going to leave @addr on stack, no? The write gets it right though. > > The read method begins with 0= which consumes top of stack. > The write method does not have that which is why they are different. ah, you're right. I dislike forth :) btw since now these do report failures (or, to be precise, 0 chars written/read which is not exactly an error), what effect does it have? For example, something is writing a buffer in a loop to a device and incrementing some counter by the value returned by "write" and now it returns 0 instead of passed "len", or there is nothing like this? > >> >> >>> virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN >>> virtiodev virtio-serial-getchar swap c! 1 >>> ; >> >> -- >> Alexey >> >>
On Thu, Sep 7, 2023 at 1:37 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 04/09/2023 15:19, Jordan Niethe wrote: > > On Mon, Sep 4, 2023 at 3:08 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > >> > >> > >> On 29/08/2023 10:12, Jordan Niethe wrote: > >>> From: Kautuk Consul <kconsul@linux.vnet.ibm.com> > >>> > >>> The read and write methods return successfully even if the virtio device > >>> is closed (virtiodev is 0) and it is not able to send or receive any > >>> characters. > >>> > >>> Make the read and write methods return 0 to indicate they did not > >>> succeed in this case. > >>> > >>> This also fixes an invalid stack access in the read method. > >>> > >>> Fixes: 8174acd ("virtio-serial: Close device completely") > >>> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com> > >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > >>> --- > >>> v2: > >>> - Rework commit message slightly > >>> --- > >>> board-qemu/slof/virtio-serial.fs | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs > >>> index 82868e2..41e2e04 100644 > >>> --- a/board-qemu/slof/virtio-serial.fs > >>> +++ b/board-qemu/slof/virtio-serial.fs > >>> @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop > >>> ; > >>> > >>> : write ( addr len -- actual ) > >>> - virtiodev 0= IF nip EXIT THEN > >>> + virtiodev 0= IF 2drop 0 EXIT THEN > >>> tuck > >>> 0 ?DO > >>> dup c@ virtiodev SWAP virtio-serial-putchar > >>> @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop > >>> > >>> : read ( addr len -- actual ) > >>> 0= IF drop 0 EXIT THEN > >>> - virtiodev 0= IF nip EXIT THEN > >>> + virtiodev 0= IF drop 0 EXIT THEN > >> > >> > >> This is going to leave @addr on stack, no? The write gets it right though. > > > > The read method begins with 0= which consumes top of stack. > > The write method does not have that which is why they are different. > > ah, you're right. I dislike forth :) > > btw since now these do report failures (or, to be precise, 0 chars > written/read which is not exactly an error), I think it can be described as reporting failure based on the ieee1275 descriptions of read and write: - "If actual is zero or negative, the read operation did not succeed" - "If actual is less than len, the write did not succeed" > what effect does it have? > For example, something is writing a buffer in a loop to a device and > incrementing some counter by the value returned by "write" and now it > returns 0 instead of passed "len", or there is nothing like this? I was worried about that too. The v1 of the series fixed the stack misaccess but still reported success. AFAICS, that only callers of the read and write methods are term-io-key and term-io-emit. term-io-emit ignores the return value of the write method. term-io-key will call read until it returns >= 0. So if "read" is called with the virtiodev == 0 it will loop forever. prom_init closes stdin prior to quiescing so it won't happen there, but it is conceivable of a client that calls quiesce and then tries to read from stdin and then gets trapped. We could add some circuit breaker to term-io-key? Or just keep the read and write methods reporting success? > > > > >> > >> > >>> virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN > >>> virtiodev virtio-serial-getchar swap c! 1 > >>> ; > >> > >> -- > >> Alexey > >> > >> > > -- > Alexey > >
On 08/09/2023 08.43, Jordan Niethe wrote: > On Thu, Sep 7, 2023 at 1:37 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> On 04/09/2023 15:19, Jordan Niethe wrote: >>> On Mon, Sep 4, 2023 at 3:08 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: ... >>>>> diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs >>>>> index 82868e2..41e2e04 100644 >>>>> --- a/board-qemu/slof/virtio-serial.fs >>>>> +++ b/board-qemu/slof/virtio-serial.fs >>>>> @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop >>>>> ; >>>>> >>>>> : write ( addr len -- actual ) >>>>> - virtiodev 0= IF nip EXIT THEN >>>>> + virtiodev 0= IF 2drop 0 EXIT THEN >>>>> tuck >>>>> 0 ?DO >>>>> dup c@ virtiodev SWAP virtio-serial-putchar >>>>> @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop >>>>> >>>>> : read ( addr len -- actual ) >>>>> 0= IF drop 0 EXIT THEN >>>>> - virtiodev 0= IF nip EXIT THEN >>>>> + virtiodev 0= IF drop 0 EXIT THEN >>>> >>>> >>>> This is going to leave @addr on stack, no? The write gets it right though. >>> >>> The read method begins with 0= which consumes top of stack. >>> The write method does not have that which is why they are different. >> >> ah, you're right. I dislike forth :) >> >> btw since now these do report failures (or, to be precise, 0 chars >> written/read which is not exactly an error), > > I think it can be described as reporting failure based on the ieee1275 > descriptions of > read and write: > > - "If actual is zero or negative, the read operation did not succeed" > - "If actual is less than len, the write did not succeed" I agree. It's not like in libc where they have a slightly different meaning of the return values. It's fine for Open Firmware as it is done in this patch, as far as I can tell. >> what effect does it have? >> For example, something is writing a buffer in a loop to a device and >> incrementing some counter by the value returned by "write" and now it >> returns 0 instead of passed "len", or there is nothing like this? > > I was worried about that too. The v1 of the series fixed the stack > misaccess but still reported success. > > AFAICS, that only callers of the read and write methods are > term-io-key and term-io-emit. > term-io-emit ignores the return value of the write method. > term-io-key will call read until it returns >= 0. > > So if "read" is called with the virtiodev == 0 it will loop forever. > > prom_init closes stdin prior to quiescing so it won't happen there, > but it is conceivable of a client that calls quiesce and then tries to > read from stdin and then gets trapped. > > We could add some circuit breaker to term-io-key? > Or just keep the read and write methods reporting success? I think we should be fine. "term-io-key" is supposed to block until there is a real key pressed. If a caller wants to avoid blocking, they have to use the "term-io-key?" (or rather "key?") Forth word first to check whether a key is available or not, and that should give the proper return code (0 for no key available). Thus IMHO these two patches should be fine for inclusion now. Or, Alexey, what do you think? Thomas PS: A colleague of mine just ran into the same issue and confirmed that the patches are fixes the problem in his case, too: https://issues.redhat.com/browse/RHEL-3709
On 15/09/2023 18:34, Thomas Huth wrote: > On 08/09/2023 08.43, Jordan Niethe wrote: >> On Thu, Sep 7, 2023 at 1:37 PM Alexey Kardashevskiy <aik@ozlabs.ru> >> wrote: >>> On 04/09/2023 15:19, Jordan Niethe wrote: >>>> On Mon, Sep 4, 2023 at 3:08 PM Alexey Kardashevskiy <aik@ozlabs.ru> >>>> wrote: > ... >>>>>> diff --git a/board-qemu/slof/virtio-serial.fs >>>>>> b/board-qemu/slof/virtio-serial.fs >>>>>> index 82868e2..41e2e04 100644 >>>>>> --- a/board-qemu/slof/virtio-serial.fs >>>>>> +++ b/board-qemu/slof/virtio-serial.fs >>>>>> @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop >>>>>> ; >>>>>> >>>>>> : write ( addr len -- actual ) >>>>>> - virtiodev 0= IF nip EXIT THEN >>>>>> + virtiodev 0= IF 2drop 0 EXIT THEN >>>>>> tuck >>>>>> 0 ?DO >>>>>> dup c@ virtiodev SWAP virtio-serial-putchar >>>>>> @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop >>>>>> >>>>>> : read ( addr len -- actual ) >>>>>> 0= IF drop 0 EXIT THEN >>>>>> - virtiodev 0= IF nip EXIT THEN >>>>>> + virtiodev 0= IF drop 0 EXIT THEN >>>>> >>>>> >>>>> This is going to leave @addr on stack, no? The write gets it right >>>>> though. >>>> >>>> The read method begins with 0= which consumes top of stack. >>>> The write method does not have that which is why they are different. >>> >>> ah, you're right. I dislike forth :) >>> >>> btw since now these do report failures (or, to be precise, 0 chars >>> written/read which is not exactly an error), >> >> I think it can be described as reporting failure based on the ieee1275 >> descriptions of >> read and write: >> >> - "If actual is zero or negative, the read operation did not succeed" >> - "If actual is less than len, the write did not succeed" > > I agree. It's not like in libc where they have a slightly different > meaning of the return values. It's fine for Open Firmware as it is done > in this patch, as far as I can tell. > >>> what effect does it have? >>> For example, something is writing a buffer in a loop to a device and >>> incrementing some counter by the value returned by "write" and now it >>> returns 0 instead of passed "len", or there is nothing like this? >> >> I was worried about that too. The v1 of the series fixed the stack >> misaccess but still reported success. >> >> AFAICS, that only callers of the read and write methods are >> term-io-key and term-io-emit. >> term-io-emit ignores the return value of the write method. >> term-io-key will call read until it returns >= 0. >> >> So if "read" is called with the virtiodev == 0 it will loop forever. >> >> prom_init closes stdin prior to quiescing so it won't happen there, >> but it is conceivable of a client that calls quiesce and then tries to >> read from stdin and then gets trapped. >> >> We could add some circuit breaker to term-io-key? >> Or just keep the read and write methods reporting success? > > I think we should be fine. "term-io-key" is supposed to block until > there is a real key pressed. If a caller wants to avoid blocking, they > have to use the "term-io-key?" (or rather "key?") Forth word first to > check whether a key is available or not, and that should give the proper > return code (0 for no key available). > > Thus IMHO these two patches should be fine for inclusion now. Or, > Alexey, what do you think? I just pushed it out, thanks everyone! and sorry for the delay, had to fix my smtp relay which I have not used for a while :) > > Thomas > > > PS: A colleague of mine just ran into the same issue and confirmed that > the patches are fixes the problem in his case, too: > https://issues.redhat.com/browse/RHEL-3709 >
On Mon, Sep 18, 2023 at 8:18 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 15/09/2023 18:34, Thomas Huth wrote: > > On 08/09/2023 08.43, Jordan Niethe wrote: > >> On Thu, Sep 7, 2023 at 1:37 PM Alexey Kardashevskiy <aik@ozlabs.ru> > >> wrote: > >>> On 04/09/2023 15:19, Jordan Niethe wrote: > >>>> On Mon, Sep 4, 2023 at 3:08 PM Alexey Kardashevskiy <aik@ozlabs.ru> > >>>> wrote: > > ... > >>>>>> diff --git a/board-qemu/slof/virtio-serial.fs > >>>>>> b/board-qemu/slof/virtio-serial.fs > >>>>>> index 82868e2..41e2e04 100644 > >>>>>> --- a/board-qemu/slof/virtio-serial.fs > >>>>>> +++ b/board-qemu/slof/virtio-serial.fs > >>>>>> @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop > >>>>>> ; > >>>>>> > >>>>>> : write ( addr len -- actual ) > >>>>>> - virtiodev 0= IF nip EXIT THEN > >>>>>> + virtiodev 0= IF 2drop 0 EXIT THEN > >>>>>> tuck > >>>>>> 0 ?DO > >>>>>> dup c@ virtiodev SWAP virtio-serial-putchar > >>>>>> @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop > >>>>>> > >>>>>> : read ( addr len -- actual ) > >>>>>> 0= IF drop 0 EXIT THEN > >>>>>> - virtiodev 0= IF nip EXIT THEN > >>>>>> + virtiodev 0= IF drop 0 EXIT THEN > >>>>> > >>>>> > >>>>> This is going to leave @addr on stack, no? The write gets it right > >>>>> though. > >>>> > >>>> The read method begins with 0= which consumes top of stack. > >>>> The write method does not have that which is why they are different. > >>> > >>> ah, you're right. I dislike forth :) > >>> > >>> btw since now these do report failures (or, to be precise, 0 chars > >>> written/read which is not exactly an error), > >> > >> I think it can be described as reporting failure based on the ieee1275 > >> descriptions of > >> read and write: > >> > >> - "If actual is zero or negative, the read operation did not succeed" > >> - "If actual is less than len, the write did not succeed" > > > > I agree. It's not like in libc where they have a slightly different > > meaning of the return values. It's fine for Open Firmware as it is done > > in this patch, as far as I can tell. > > > >>> what effect does it have? > >>> For example, something is writing a buffer in a loop to a device and > >>> incrementing some counter by the value returned by "write" and now it > >>> returns 0 instead of passed "len", or there is nothing like this? > >> > >> I was worried about that too. The v1 of the series fixed the stack > >> misaccess but still reported success. > >> > >> AFAICS, that only callers of the read and write methods are > >> term-io-key and term-io-emit. > >> term-io-emit ignores the return value of the write method. > >> term-io-key will call read until it returns >= 0. > >> > >> So if "read" is called with the virtiodev == 0 it will loop forever. > >> > >> prom_init closes stdin prior to quiescing so it won't happen there, > >> but it is conceivable of a client that calls quiesce and then tries to > >> read from stdin and then gets trapped. > >> > >> We could add some circuit breaker to term-io-key? > >> Or just keep the read and write methods reporting success? > > > > I think we should be fine. "term-io-key" is supposed to block until > > there is a real key pressed. If a caller wants to avoid blocking, they > > have to use the "term-io-key?" (or rather "key?") Forth word first to > > check whether a key is available or not, and that should give the proper > > return code (0 for no key available). > > > > Thus IMHO these two patches should be fine for inclusion now. Or, > > Alexey, what do you think? > > > I just pushed it out, thanks everyone! and sorry for the delay, had to > fix my smtp relay which I have not used for a while :) Thanks Alexey. > > > > > Thomas > > > > > > PS: A colleague of mine just ran into the same issue and confirmed that > > the patches are fixes the problem in his case, too: > > https://issues.redhat.com/browse/RHEL-3709 > > > > -- > Alexey > >
diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs index 82868e2..41e2e04 100644 --- a/board-qemu/slof/virtio-serial.fs +++ b/board-qemu/slof/virtio-serial.fs @@ -67,7 +67,7 @@ virtiodev virtio-serial-init drop ; : write ( addr len -- actual ) - virtiodev 0= IF nip EXIT THEN + virtiodev 0= IF 2drop 0 EXIT THEN tuck 0 ?DO dup c@ virtiodev SWAP virtio-serial-putchar @@ -78,7 +78,7 @@ virtiodev virtio-serial-init drop : read ( addr len -- actual ) 0= IF drop 0 EXIT THEN - virtiodev 0= IF nip EXIT THEN + virtiodev 0= IF drop 0 EXIT THEN virtiodev virtio-serial-haschar 0= IF 0 swap c! -2 EXIT THEN virtiodev virtio-serial-getchar swap c! 1 ;