linux-kernel - Re: 答复: Re: 答复: Re: [PATCH] tty: max3100: Fix oops while 'cat/proc/tty/driver/ttyMAX'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <3fa25b44-d5e4-9b40-6a7d-e2ea4a35dca7@suse.cz>
Date:   Sun, 16 Sep 2018 10:09:51 +0200
From:   Jiri Slaby <jslaby@...e.cz>
To:     chen.lin5@....com.cn
Cc:     gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-serial@...r.kernel.org
Subject: Re: 答复: Re: 答复: Re: [PATCH] tty: max3100: Fix oops while 'cat/proc/tty/driver/ttyMAX'

On 09/15/2018, 04:14 AM, chen.lin5@....com.cn wrote:
> yes, creation and destroy of the workqueue is not locked, I think
> maybe there is some
> 
> remainder work to do in destroy-wq, so I cannot sure if there is
> any usage about lock destroy-wq.
> 
> 
> What you worried of the races is about this  ?  
> 
>    --> when max3100_shutdown,  destroy_workqueue is doing, s->workqueue
> is not NULL, at this moment, get_mctrl is executed, destroying wq is
> queued again.
> 
> bu this cannot happen, becasue s->force_end_work = 1
> before destroy_workqueue  , so max3100_dowork do nothing.

Oh, so this relies on flush_workqueue or destroy_workqueue to be a
barrier (so that the assignment to end_work is not reordered), correct?

> static void max3100_shutdown(struct uart_port *port)
> 
> {
> 
> ...
> 
> s->force_end_work = 1;
> 
> 
> if (s->poll_time > 0)
> 
> del_timer_sync(&s->timer);
> 
> 
> if (s->workqueue) {
> 
> flush_workqueue(s->workqueue);
> 
> destroy_workqueue(s->workqueue);
> 
> s->workqueue = NULL;
> 
> }
> 
> 
> 
> static void max3100_dowork(struct max3100_port *s)
> 
> {
> 
> if (!s->force_end_work && !freezing(current) && !s->suspending)
> 
> queue_work(s->workqueue, &s->work);
> 
> }

Also, on the first open:
        s->force_end_work = 0;
        s->parity = 0;
        s->rts = 0;

        sprintf(b, "max3100-%d", s->minor);
        s->workqueue = create_freezable_workqueue(b);
        if (!s->workqueue) {
                dev_warn(&s->spi->dev, "cannot create workqueue\n");
                return -EBUSY;
        }

Here, s->force_end_work is 0, s->workqueue is non-NULL, but s->work is
garbage until:

        INIT_WORK(&s->work, max3100_work);


INIT_WORK should be in max3100_probe as far as I can see (or at least
before create_freezable_workqueue), right? But those variable
assignments also rely on some implicit barrier which is not there.

thanks,
-- 
js
suse labs