On Fri, Jul 6, 2018 at 7:24 PM, Tycho Andersen <[email protected]> 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 <[email protected]> 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
