09.05.2024 09:41, Jiri Slaby wrote:
On 08. 05. 24, 11:30, kovalev@xxxxxxxxxxxx wrote:
From: Vasiliy Kovalev <kovalev@xxxxxxxxxxxx>
A possible scenario in which a deadlock may occur is as follows:
flush_to_ldisc() {
mutex_lock(&buf->lock);
tty_port_default_receive_buf() {
tty_ldisc_receive_buf() {
n_tty_receive_buf2() {
n_tty_receive_buf_common() {
n_tty_receive_char_special() {
isig() {
tty_driver_flush_buffer() {
pty_flush_buffer() {
tty_buffer_flush() {
mutex_lock(&buf->lock); (DEADLOCK)
flush_to_ldisc() and tty_buffer_flush() functions they use the same mutex
(&buf->lock), but not necessarily the same struct tty_bufhead object.
>
"not necessarily" -- so does it mean that it actually can happen (and we
should fix it) or not at all (and we should annotate the mutex)?
During debugging, when running the reproducer multiple times, I failed
to catch a situation where these mutexes have the same address in memory
in the above call scenario, so I'm not sure that such a situation is
possible. But earlier, a thread is triggered that accesses the same
structure (and mutex), so LOCKDEP tools throw a warning:
thread 0:
flush_to_ldisc() {
mutex_lock(&buf->lock) // Address mutex == 0xA
n_tty_receive_buf_common();
mutex_unlock(&buf->lock) // Address mutex == 0xA
}
thread 1:
flush_to_ldisc() {
mutex_lock(&buf->lock) // Address mutex == 0xB
n_tty_receive_buf_common() {
isig() {
tty_driver_flush_buffer() {
pty_flush_buffer() {
tty_buffer_flush() {
mutex_lock(&buf->lock) // Address mutex == 0xA ->
throw Warning
// successful continuation
...
}
However, you should probably use a separate mutex for the
tty_buffer_flush() function to exclude such a situation.
...
Cc: stable@xxxxxxxxxxxxxxx
What commit does this fix?
I will assume that the commit of introducing mutexes in these functions:
e9975fdec013 ("tty: Ensure single-threaded flip buffer consumer with mutex")
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -226,7 +226,7 @@ void tty_buffer_flush(struct tty_struct *tty,
struct tty_ldisc *ld)
atomic_inc(&buf->priority);
- mutex_lock(&buf->lock);
+ mutex_lock(&buf->flush_mtx);
Hmm, how does this protect against concurrent buf pickup. We free it
here and the racing thread can start using it, or?
Yes, assuming that such a scenario is possible..
Otherwise, if such a scenario is not possible and the patch is
inappropriate, then you need to mark this mutex in some way to tell
lockdep tools to ignore this place..
/* paired w/ release in __tty_buffer_request_room; ensures there
are
* no pending memory accesses to the freed buffer
*/
thanks,
--
Regards,
Vasiliy Kovalev
