On Sun, May 16, 2010 at 16:07, Jan Kiszka <jan.kis...@web.de> wrote:
> Jan Kiszka wrote:
>> Paolo Giarrusso wrote:
>>> On Mon, May 10, 2010 at 19:43, Jan Kiszka <jan.kis...@web.de> wrote:
>>>> Lukas Czerner wrote:
>> Right, there was more broken. Besides the locking fix, this should do
>> the trick by avoiding the recursive free_winch call:
>> diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
>> index d85fcb9..fa66eb4 100644
>> --- a/arch/um/drivers/line.c
>> +++ b/arch/um/drivers/line.c
>> @@ -731,8 +731,10 @@ static void free_winch(struct winch *winch, int
>> free_irq_ok)
>>
>> if (winch->pid != -1)
>> os_kill_process(winch->pid, 1);
>> - if (winch->fd != -1)
>> + if (winch->fd != -1) {
>> os_close_file(winch->fd);
>> + winch->fd = -1;
>> + }
>> if (winch->stack != 0)
>> free_stack(winch->stack, 0);
>> if (free_irq_ok)
>>
>> (note: untested)
>
> Well, thought about it again, this is the result:
Given a look, it makes sense. But still, I think that moving the
free_irq call to earlier might make the result more stable.
Moreover, do you really need to avoid the handler calling free_irq?
That should be safe, as long as the IRQ is shutdown by cleanup
routines as the first thing, before the call to
list_del(&winch->list).
> uml: Fix winch cleanup races
>
> Calling free_winch from within the IRQ handler is broken for many
> reasons: It can leave the IRQ line registered and it races with
> parallel cleanup by unregister_winch and winch_cleanup.
>
> Resolve this via a per-winch spinlock that protects pid and fd of the
> winch structure.
>
> Signed-off-by: Jan Kiszka <jan.kis...@web.de>
> ---
> arch/um/drivers/line.c | 50 ++++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
> index 7f8f649..4b9d177 100644
> --- a/arch/um/drivers/line.c
> +++ b/arch/um/drivers/line.c
> @@ -723,20 +723,30 @@ struct winch {
> int pid;
> struct tty_struct *tty;
> unsigned long stack;
> + spinlock_t lock;
> };
>
> -static void free_winch(struct winch *winch, int free_irq_ok)
> +static void free_winch(struct winch *winch)
> {
> - list_del(&winch->list);
> + spin_lock_irq(&winch->lock);
>
> - if (winch->pid != -1)
> + if (winch->pid != -1) {
> os_kill_process(winch->pid, 1);
> - if (winch->fd != -1)
> + winch->pid = -1;
> + }
> + if (winch->fd != -1) {
> os_close_file(winch->fd);
> + winch->fd = -1;
> + }
> +
> + spin_unlock_irq(&winch->lock);
> +
> if (winch->stack != 0)
> free_stack(winch->stack, 0);
> - if (free_irq_ok)
> - um_free_irq(WINCH_IRQ, winch);
> +
> + um_free_irq(WINCH_IRQ, winch);
> +
> + list_del(&winch->list);
> kfree(winch);
> }
>
> @@ -748,6 +758,8 @@ static irqreturn_t winch_interrupt(int irq, void *data)
> int err;
> char c;
>
> + spin_lock(&winch->lock);
> +
> if (winch->fd != -1) {
> err = generic_read(winch->fd, &c, NULL);
> if (err < 0) {
> @@ -756,10 +768,17 @@ static irqreturn_t winch_interrupt(int irq, void *data)
> "read failed, errno = %d\n", -err);
> printk(KERN_ERR "fd %d is losing SIGWINCH "
> "support\n", winch->tty_fd);
> - free_winch(winch, 0);
> - return IRQ_HANDLED;
> +
> + if (winch->pid != -1) {
> + os_kill_process(winch->pid, 1);
> + winch->pid = -1;
> + }
> + os_close_file(winch->fd);
> + winch->fd = -1;
> +
> + goto unlock_out;
> }
> - goto out;
> + goto reactivate_out;
> }
> }
> tty = winch->tty;
> @@ -771,9 +790,14 @@ static irqreturn_t winch_interrupt(int irq, void *data)
> kill_pgrp(tty->pgrp, SIGWINCH, 1);
> }
> }
> - out:
> +
> +reactivate_out:
> if (winch->fd != -1)
> reactivate_fd(winch->fd, WINCH_IRQ);
> +
> +unlock_out:
> + spin_unlock(&winch->lock);
> +
> return IRQ_HANDLED;
> }
>
> @@ -795,6 +819,8 @@ void register_winch_irq(int fd, int tty_fd, int pid,
> struct tty_struct *tty,
> .tty = tty,
> .stack = stack });
>
> + spin_lock_init(&winch->lock);
> +
> if (um_request_irq(WINCH_IRQ, fd, IRQ_READ, winch_interrupt,
> IRQF_DISABLED | IRQF_SHARED | IRQF_SAMPLE_RANDOM,
> "winch", winch) < 0) {
> @@ -828,7 +854,7 @@ static void unregister_winch(struct tty_struct *tty)
> list_for_each(ele, &winch_handlers) {
> winch = list_entry(ele, struct winch, list);
> if (winch->tty == tty) {
> - free_winch(winch, 1);
> + free_winch(winch);
> break;
> }
> }
> @@ -844,7 +870,7 @@ static void winch_cleanup(void)
>
> list_for_each_safe(ele, next, &winch_handlers) {
> winch = list_entry(ele, struct winch, list);
> - free_winch(winch, 1);
> + free_winch(winch);
> }
>
> spin_unlock(&winch_handler_lock);
>
>
>
--
Paolo Giarrusso
------------------------------------------------------------------------------
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
