Re: tty: WARNING in n_hdlc_tty_read

[Alan Cox]

On Fri, 11 Dec 2015 14:44:00 +0100
Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:

> Hello,
> 
> I am hitting the following WARNING on commit
> aa53685549a2cfb5f175b0c4a20bc9aa1e5a1b85 (Dec 8):

What surprises me is that the warnings didn't trigger for whoever
tested the code.

It also looks to me like the claim in the commit that

" 1. neither BKL or tty mutex are required for correct operation"

is probably not entirely true (though close).

Consider the case where two reads occur in parallel. In that situation
nothing protects hdlc->rx_free_buf_list.count during the decision whether
to kfree or n_hdlc_buf_put.

The more specific problem you are hitting I think those is the attempt to
make use of set_current_state and add_wait_queue directly.

After we set the task to "uninterruptible sleep" we dequeue a frame which
is fine and correctly locked, but then call copy_to_user which can block.
No can do.


My first thought would be to rip out all the raw wait queue messing about
and replace the lot with something slightly more 21st century. Something
a bit like


	if (file->f_flags & O_NONBLOCK)
		rbuf = n_hdlc_get(...)
	else
		wait_event_interruptible(&tty->read_wait, (rbuf =
			n_hdlc_get(&n_hdlc->rx_buf_list)) != NULL);

	if (rbuf)
		...

	if (signal_pending(current))
		...

ought to get close - but that doesn't fit the list.count race.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/