* [PATCH 2/3] um: tty: Fix handling of close in tty lines
2020-12-07 17:19 [PATCH 1/3] um: Monitor error events in IRQ controller anton.ivanov
@ 2020-12-07 17:19 ` anton.ivanov
2020-12-07 17:19 ` [PATCH 3/3] um: chan_xterm: Fix fd leak anton.ivanov
2020-12-07 17:35 ` [PATCH 1/3] um: Monitor error events in IRQ controller Johannes Berg
2 siblings, 0 replies; 6+ messages in thread
From: anton.ivanov @ 2020-12-07 17:19 UTC (permalink / raw)
To: linux-um; +Cc: richard, Anton Ivanov
From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Fix a logical error in tty reading. We get 0 and errno == EAGAIN
on the first attempt to read from a closed file descriptor.
Compared to that a true EAGAIN is EAGAIN and -1.
If we check errno for EAGAIN first, before checking the return
value we miss the fact that the descriptor is closed.
This bug is as old as the driver. It was not showing up with
the original POLL based IRQ controller, because it was
producing multiple events. Switching to EPOLL unmasked it.
Fixes: ff6a17989c08 ("Epoll based IRQ controller")
Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
arch/um/drivers/chan_user.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/um/drivers/chan_user.c b/arch/um/drivers/chan_user.c
index 4d80526a4236..d8845d4aac6a 100644
--- a/arch/um/drivers/chan_user.c
+++ b/arch/um/drivers/chan_user.c
@@ -26,10 +26,10 @@ int generic_read(int fd, char *c_out, void *unused)
n = read(fd, c_out, sizeof(*c_out));
if (n > 0)
return n;
- else if (errno == EAGAIN)
- return 0;
else if (n == 0)
return -EIO;
+ else if (errno == EAGAIN)
+ return 0;
return -errno;
}
--
2.20.1
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] um: chan_xterm: Fix fd leak
2020-12-07 17:19 [PATCH 1/3] um: Monitor error events in IRQ controller anton.ivanov
2020-12-07 17:19 ` [PATCH 2/3] um: tty: Fix handling of close in tty lines anton.ivanov
@ 2020-12-07 17:19 ` anton.ivanov
2020-12-07 17:35 ` [PATCH 1/3] um: Monitor error events in IRQ controller Johannes Berg
2 siblings, 0 replies; 6+ messages in thread
From: anton.ivanov @ 2020-12-07 17:19 UTC (permalink / raw)
To: linux-um; +Cc: richard, Anton Ivanov
From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
xterm serial channel was leaking a fd used in setting up the
port helper
This bug is prehistoric - it predates switching to git. The "fixes"
header here is really just to mark all the versions we would like this to
apply to which is "Anything from the Cretaceous period onwards".
No dinosaurs were harmed in fixing this bug.
Fixes: b40997b872cd ("um: drivers/xterm.c: fix a file descriptor leak")
Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
arch/um/drivers/xterm.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/um/drivers/xterm.c b/arch/um/drivers/xterm.c
index fc7f1e746703..87ca4a47cd66 100644
--- a/arch/um/drivers/xterm.c
+++ b/arch/um/drivers/xterm.c
@@ -18,6 +18,7 @@
struct xterm_chan {
int pid;
int helper_pid;
+ int chan_fd;
char *title;
int device;
int raw;
@@ -33,6 +34,7 @@ static void *xterm_init(char *str, int device, const struct chan_opts *opts)
return NULL;
*data = ((struct xterm_chan) { .pid = -1,
.helper_pid = -1,
+ .chan_fd = -1,
.device = device,
.title = opts->xterm_title,
.raw = opts->raw } );
@@ -149,6 +151,7 @@ static int xterm_open(int input, int output, int primary, void *d,
goto out_kill;
}
+ data->chan_fd = fd;
new = xterm_fd(fd, &data->helper_pid);
if (new < 0) {
err = new;
@@ -206,6 +209,8 @@ static void xterm_close(int fd, void *d)
os_kill_process(data->helper_pid, 0);
data->helper_pid = -1;
+ if (data->chan_fd != -1)
+ os_close_file(data->chan_fd);
os_close_file(fd);
}
--
2.20.1
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] um: Monitor error events in IRQ controller
2020-12-07 17:19 [PATCH 1/3] um: Monitor error events in IRQ controller anton.ivanov
2020-12-07 17:19 ` [PATCH 2/3] um: tty: Fix handling of close in tty lines anton.ivanov
2020-12-07 17:19 ` [PATCH 3/3] um: chan_xterm: Fix fd leak anton.ivanov
@ 2020-12-07 17:35 ` Johannes Berg
2020-12-07 18:11 ` Anton Ivanov
2 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2020-12-07 17:35 UTC (permalink / raw)
To: anton.ivanov, linux-um; +Cc: richard
On Mon, 2020-12-07 at 17:19 +0000, anton.ivanov@cambridgegreys.com
wrote:
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>
> Ensure that file closes, connection closes, etc are propagated
> as interrupts in the interrupt controller.
>
> Fixes: ff6a17989c08 ("Epoll based IRQ controller")
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> ---
> arch/um/os-Linux/irq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/um/os-Linux/irq.c b/arch/um/os-Linux/irq.c
> index d508310ee5e1..f1732c308c61 100644
> --- a/arch/um/os-Linux/irq.c
> +++ b/arch/um/os-Linux/irq.c
> @@ -48,7 +48,7 @@ int os_epoll_triggered(int index, int events)
> int os_event_mask(int irq_type)
> {
> if (irq_type == IRQ_READ)
> - return EPOLLIN | EPOLLPRI;
> + return EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLRDHUP;
> if (irq_type == IRQ_WRITE)
> return EPOLLOUT;
Why not monitor it also for write?
johannes
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] um: Monitor error events in IRQ controller
2020-12-07 17:35 ` [PATCH 1/3] um: Monitor error events in IRQ controller Johannes Berg
@ 2020-12-07 18:11 ` Anton Ivanov
2020-12-09 17:49 ` Anton Ivanov
0 siblings, 1 reply; 6+ messages in thread
From: Anton Ivanov @ 2020-12-07 18:11 UTC (permalink / raw)
To: Johannes Berg, linux-um; +Cc: richard
On 07/12/2020 17:35, Johannes Berg wrote:
> On Mon, 2020-12-07 at 17:19 +0000, anton.ivanov@cambridgegreys.com
> wrote:
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> Ensure that file closes, connection closes, etc are propagated
>> as interrupts in the interrupt controller.
>>
>> Fixes: ff6a17989c08 ("Epoll based IRQ controller")
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> ---
>> arch/um/os-Linux/irq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/um/os-Linux/irq.c b/arch/um/os-Linux/irq.c
>> index d508310ee5e1..f1732c308c61 100644
>> --- a/arch/um/os-Linux/irq.c
>> +++ b/arch/um/os-Linux/irq.c
>> @@ -48,7 +48,7 @@ int os_epoll_triggered(int index, int events)
>> int os_event_mask(int irq_type)
>> {
>> if (irq_type == IRQ_READ)
>> - return EPOLLIN | EPOLLPRI;
>> + return EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLRDHUP;
>> if (irq_type == IRQ_WRITE)
>> return EPOLLOUT;
>
> Why not monitor it also for write?
The write code in most drivers has no error checking and no close upon
error.
I will have to go through them and ensure they behave correctly first.
So - yes, it's a good idea and we will add it later, once I have
double-checked all drivers.
>
> johannes
>
>
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] um: Monitor error events in IRQ controller
2020-12-07 18:11 ` Anton Ivanov
@ 2020-12-09 17:49 ` Anton Ivanov
0 siblings, 0 replies; 6+ messages in thread
From: Anton Ivanov @ 2020-12-09 17:49 UTC (permalink / raw)
To: Johannes Berg, linux-um; +Cc: richard
On 07/12/2020 18:11, Anton Ivanov wrote:
> On 07/12/2020 17:35, Johannes Berg wrote:
>> On Mon, 2020-12-07 at 17:19 +0000, anton.ivanov@cambridgegreys.com
>> wrote:
>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>>
>>> Ensure that file closes, connection closes, etc are propagated
>>> as interrupts in the interrupt controller.
>>>
>>> Fixes: ff6a17989c08 ("Epoll based IRQ controller")
>>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>> ---
>>> arch/um/os-Linux/irq.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/um/os-Linux/irq.c b/arch/um/os-Linux/irq.c
>>> index d508310ee5e1..f1732c308c61 100644
>>> --- a/arch/um/os-Linux/irq.c
>>> +++ b/arch/um/os-Linux/irq.c
>>> @@ -48,7 +48,7 @@ int os_epoll_triggered(int index, int events)
>>> int os_event_mask(int irq_type)
>>> {
>>> if (irq_type == IRQ_READ)
>>> - return EPOLLIN | EPOLLPRI;
>>> + return EPOLLIN | EPOLLPRI | EPOLLERR | EPOLLHUP | EPOLLRDHUP;
>>> if (irq_type == IRQ_WRITE)
>>> return EPOLLOUT;
>>
>> Why not monitor it also for write?
>
> The write code in most drivers has no error checking and no close upon
> error.
>
> I will have to go through them and ensure they behave correctly first.
>
> So - yes, it's a good idea and we will add it later, once I have
> double-checked all drivers.
I have a couple of further clean-up patches to follow, but they need
both your series and my series to go in first.
They are mostly cosmetic - ensuring correct behavior on epoll errors and
suppressing messages for conditions which are not really errors, but
"business as usual".
A.
>
>>
>> johannes
>>
>>
>> _______________________________________________
>> linux-um mailing list
>> linux-um@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-um
>>
>
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 6+ messages in thread