On Fri, Jul 6, 2018 at 7:24 PM, Tycho Andersen [off-list ref] wrote:

Looking in uart_port_startup(), it seems that circ->buf (state->xmit.buf)
protected by the "per-port mutex", which based on uart_port_check() is
state->port.mutex. Indeed, the lock acquired in uart_put_char() is
uport->lock, i.e. not the same lock.

Anyway, since the lock is not acquired, if uart_shutdown() is called, the
last chunk of that function may release state->xmit.buf before its assigned
to null, and cause the race above.

To fix it, let's lock uport->lock when allocating/deallocating
state->xmit.buf in addition to the per-port mutex.

Thanks for fixing this!

Reviewed-by: Andy Shevchenko <redacted>

Some nitpicks though.

+       unsigned long page, flags = 0;

I would rather put on separate lines and btw assignment is not needed.
It all goes through macros.

-       if (!state->xmit.buf) {
-               /* This is protected by the per port mutex */
-               page = get_zeroed_page(GFP_KERNEL);
-               if (!page)
-                       return -ENOMEM;
+       page = get_zeroed_page(GFP_KERNEL);
+       if (!page)
+               return -ENOMEM;
+       if (!state->xmit.buf) {
                state->xmit.buf = (unsigned char *) page;
                uart_circ_clear(&state->xmit);
+       } else {
+               free_page(page);
        }

I see original code, but since you are adding else, does it make sense
to switch to positive condition?

+       unsigned long flags = 0;

Ditto about assignment.

-- 
With Best Regards,
Andy Shevchenko