From: Vasiliy Kovalev <kovalev@altlinux.org>
To: Jiri Slaby <jirislaby@kernel.org>,
kovalev@altlinux.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Cc: lvc-project@linuxtesting.org, dutyrok@altlinux.org,
oficerovas@altlinux.org, stable@vger.kernel.org
Subject: Re: [PATCH] tty: Fix possible deadlock in tty_buffer_flush
Date: Thu, 9 May 2024 13:32:43 +0300 [thread overview]
Message-ID: <face96b8-6e69-0f2d-1cb0-a20acd906338@basealt.ru> (raw)
In-Reply-To: <e167d14c-76d3-46b4-aca5-b6003f9cbfc1@kernel.org>
09.05.2024 09:41, Jiri Slaby wrote:
> On 08. 05. 24, 11:30, kovalev@altlinux.org wrote:
>> From: Vasiliy Kovalev <kovalev@altlinux.org>
>>
>> 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@vger.kernel.org
>
> 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
prev parent reply other threads:[~2024-05-09 10:32 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 9:30 [PATCH] tty: Fix possible deadlock in tty_buffer_flush kovalev
2024-05-09 6:41 ` Jiri Slaby
2024-05-09 10:32 ` Vasiliy Kovalev [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=face96b8-6e69-0f2d-1cb0-a20acd906338@basealt.ru \
--to=kovalev@altlinux.org \
--cc=dutyrok@altlinux.org \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=lvc-project@linuxtesting.org \
--cc=oficerovas@altlinux.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).