On Thu, Aug 09, 2012 at 01:16:20PM +0200, Stanislaw Gruszka wrote: > On Wed, Aug 08, 2012 at 04:27:25PM +0100, Alan Cox wrote: > > On Wed, 08 Aug 2012 16:28:47 +0200 > > Stanislav Kozina <skozina@xxxxxxxxxx> wrote: > > > > > Fix possible panic caused by unlocked access to tty->read_cnt in > > > while-loop condition in n_tty_read(). > > > > Should this also be removing the BUG_ON check you noted in the other > > email was not valid now ? > > You talk about > http://marc.info/?l=linux-serial&m=134318985920881&w=2 > > Is possible that we can call n_tty_read() after n_tty_close() ? How oterwise > tty->read_buf could become NULL? > > If I understand correctly Stanislav's patch solve below race condtion: > > CPU0 CPU1 > n_tty_read: reset_buffer_flags: > > while (nr && tty->read_cnt) { > > spin_lock_irqsave(&tty->read_lock, flags); > tty->read_head = tty->read_tail = tty->read_cnt = 0; > spin_lock_irqsave(&tty->read_lock, flags); > > spin_lock_irqsave(&tty->read_lock, flags); > > tty->read_cnt--; > > spin_lock_irqsave(&tty->read_lock, flags); > > /* Now tty->read_cnt is negative */ > > } > > what itself could have varsious nasty consequences, i.e. ininite > loop. Is also possible that negative tty->read_cnt would result in > tty->read_buf == NULL ? If so, I'm not quite understand that. I looked a bit more at this. Excluding memory corruption which could zero tty struct, the only possibility to nullify tty->read_buf is call to n_tty_close(). So NULL pointer dereference on n_tty_read, in "while (nr && tty->read_cnt)" loop can only be caused by calling n_tty_close(), while performing n_tty_read(). Stanislav patch solve that problem because we do not touch tty->read_buf any longer once tty->read_cnt become 0, and because n_tty_close() clear tty->read_cnt (by calling n_tty_flush_buffer() -> reset_buffer_flags()). However looks like main problem persist, we should never do n_tty_read() after/during n_tty_close() and before n_tty_open(). That must be an issue in upper layer i.e. tty_io and tty_ldisc, which should give guarantee about ->close(), ->read(), ->open() ordering. I'm going to look at that more closely. Stanislaw -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
