[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e6e7cb33-f359-43bd-959e-039e82c6915a@I-love.SAKURA.ne.jp>
Date: Tue, 23 Jul 2024 07:20:34 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jan Kara <jack@...e.cz>
Cc: Christian Brauner <brauner@...nel.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
LKML <linux-kernel@...r.kernel.org>,
linux-serial <linux-serial@...r.kernel.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] tty: tty_io: fix race between tty_fops and
hung_up_tty_fops
On 2024/07/23 1:24, Greg Kroah-Hartman wrote:
> On Mon, Jul 22, 2024 at 06:10:41PM +0200, Jan Kara wrote:
>> On Mon 22-07-24 16:41:22, Christian Brauner wrote:
>>> On Fri, Jul 19, 2024 at 10:37:47PM GMT, Tetsuo Handa wrote:
>>>> syzbot is reporting data race between __tty_hangup() and __fput(), and
>>>> Dmitry Vyukov mentioned that this race has possibility of NULL pointer
>>>> dereference, for tty_fops implements e.g. splice_read callback whereas
>>>> hung_up_tty_fops does not.
>>>>
>>>> CPU0 CPU1
>>>> ---- ----
>>>> do_splice_read() {
>>>> __tty_hangup() {
>>>> // f_op->splice_read was copy_splice_read
>>>> if (unlikely(!in->f_op->splice_read))
>>>> return warn_unsupported(in, "read");
>>>> filp->f_op = &hung_up_tty_fops;
>>>> // f_op->splice_read is now NULL
>>>> return in->f_op->splice_read(in, ppos, pipe, len, flags);
>>>> }
>>>> }
>>>>
>>>> Fix possibility of NULL pointer dereference by implementing missing
>>>> callbacks, and suppress KCSAN messages by adding __data_racy qualifier
>>>> to "struct file"->f_op .
>>>
>>> This f_op replacing without synchronization seems really iffy imho.
>>
>> Yeah, when I saw this I was also going "ouch". I was just waiting whether a
>> tty maintainer will comment ;)
>
> I really didn't want to :)
>
>> Anyway this replacement of ops in file /
>> inode has proven problematic in almost every single case where it was used
>> leading to subtle issues.
>
> Yeah, let's not do this.
https://lkml.kernel.org/r/18a58415-4aa9-4cba-97d2-b70384407313@I-love.SAKURA.ne.jp was a patch
that does not replace f_op, and
https://lkml.kernel.org/r/CAHk-=wgSOa_g+bxjNi+HQpC=6sHK2yKeoW-xOhb0-FVGMTDWjg@mail.gmail.com
was a comment from Linus.
>
> Let me dig after -rc1 is out and see if there's a better way to handle
> this contrived race condition...
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists