From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Dmitry Vyukov <dvyukov@google.com>,
syzbot <syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com>,
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
Nathan Chancellor <nathan@kernel.org>,
Arnd Bergmann <arnd@kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>,
Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH v2] tty: tty_io: remove hung_up_tty_fops
Date: Tue, 30 May 2023 20:57:42 +0900 [thread overview]
Message-ID: <8edbd558-a05f-c775-4d0c-09367e688682@I-love.SAKURA.ne.jp> (raw)
In-Reply-To: <2023053005-alongside-unvisited-d9af@gregkh>
On 2023/05/30 19:44, Greg Kroah-Hartman wrote:
> On Sun, May 14, 2023 at 10:02:26AM +0900, Tetsuo Handa wrote:
>> If we care about only NULL pointer dereference, implementing missing
>> callbacks to hung_up_tty_fops is fine. But if we also care about KCSAN
>> reports, we will need to wrap all filp->f_op usages which are reachable
>> via tty_fops callbacks using data_race().
>
> I'm missing something here. Why would KCSAN report problems if we
> implement the needed callbacks in hung_up_tty_fops? And what reports
> would they be?
Unlike atomic operations such as atomic_read()/atomic_set(), normal read/write
operations are not atomic for KCSAN. KCSAN reports some value being changed
during a read/write.
In this report, KCSAN detected that __tty_hangup() changed the value of
filp->f_op from 0xffffffff84e91ed0 to 0xffffffff84e91dc0 at
filp->f_op = &hung_up_tty_fops;
line when __fput() was reading the value of filp->f_op at
if (file->f_op->release)
line.
Even if we implement the needed callbacks in hung_up_tty_fops,
KCSAN will continue reporting that the value of filp->f_op changes.
>
> And why would data_race() help here?
data_race() tells KCSAN not to report.
data_race() is used when the race KCSAN checks is harmless.
>> @@ -182,7 +182,7 @@ int tty_alloc_file(struct file *file)
>> {
>> struct tty_file_private *priv;
>>
>> - priv = kmalloc(sizeof(*priv), GFP_KERNEL);
>> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>
> Why is this zeroing out everything now? Just because you added one
> bool? Why not just set the bool properly instead?
Because I consider that this function is not performance critical where
avoid increasing code size by zeroing out everything is acceptable.
>> -static long hung_up_tty_compat_ioctl(struct file *file,
>> +static inline long hung_up_tty_compat_ioctl(struct file *file,
>> unsigned int cmd, unsigned long arg)
>> {
>> return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
>> }
>
> Marking these as inline, and then treating them as a function pointer,
> seems like a horrid way to work around a compiler warning. As they
> really are not inline functions anymore, but yet the compiler doesn't
> know that. Odds are once the compiler gets smarter, the warnings will
> return, so please, solve this properly.
Since this patch removes "struct file_operations hung_up_tty_fops"
which was the only source of treating as a function pointer,
these inlined functions are no longer treated as a function pointer.
>> @@ -619,7 +608,8 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
>> continue;
>> closecount++;
>> __tty_fasync(-1, filp, 0); /* can't block */
>> - filp->f_op = &hung_up_tty_fops;
>> + /* Accept race with tty_hung_up_p() test. */
>> + data_race(priv->hung = true);
>
> Why accept it? Say why it's not really an issue here.
Because whether tty_hung_up_p() sees true or false due to concurrent
access does not matter. The race KCSAN reported is harmless (unless
callbacks suddenly disappear).
>> @@ -743,7 +733,9 @@ void tty_vhangup_session(struct tty_struct *tty)
>> */
>> int tty_hung_up_p(struct file *filp)
>> {
>> - return (filp && filp->f_op == &hung_up_tty_fops);
>> + return filp && filp->f_op == &tty_fops &&
>> + /* Accept race with __tty_hangup(). */
>> + data_race(((struct tty_file_private *) filp->private_data)->hung);
>
> Same here.
Because whether __tty_hangup() already changed from false to true due to
concurrent access does not matter. The race KCSAN reported is harmless (unless
callbacks suddenly disappear).
>> @@ -911,6 +903,8 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
>> struct tty_struct *tty = file_tty(file);
>> struct tty_ldisc *ld;
>>
>> + if (tty_hung_up_p(file))
>> + return hung_up_tty_read(iocb, to);
>
> What happens if you hang up _right_ after this check? There's no
> locking here, right? Same everywhere else you have this pattern, you
> made the race window smaller, but it's still there from what I can see.
We cannot close the race window without introducing locking,
but we don't need to close the race window.
The race KCSAN found in this report is harmless, as long as callbacks
reachable via filp->f_op does not disappear.
This patch prevents filp->f_op from suddenly disappearing callbacks,
by not changing the value of filp->f_op.
>> @@ -255,6 +255,7 @@ struct tty_file_private {
>> struct tty_struct *tty;
>> struct file *file;
>> struct list_head list;
>> + bool hung;
>
> No hint as to what "hung" means here?
Whether __tty_hangup() was called or not.
next prev parent reply other threads:[~2023-05-30 11:59 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-21 8:18 [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4) syzbot
2023-04-21 8:21 ` Dmitry Vyukov
2023-04-21 15:12 ` Tetsuo Handa
2023-04-21 16:02 ` Tetsuo Handa
2023-04-23 23:34 ` Al Viro
2023-04-23 23:55 ` Tetsuo Handa
2023-04-24 0:44 ` Al Viro
2023-04-24 1:09 ` Tetsuo Handa
2023-04-25 14:47 ` Tetsuo Handa
2023-04-25 16:03 ` Al Viro
2023-04-25 22:09 ` Tetsuo Handa
2023-04-26 11:05 ` [PATCH] tty: tty_io: remove hung_up_tty_fops Tetsuo Handa
2023-04-28 16:27 ` Nathan Chancellor
2023-04-28 16:41 ` Tetsuo Handa
2023-04-28 17:11 ` Al Viro
2023-04-29 10:43 ` Tetsuo Handa
2023-04-28 17:31 ` Greg Kroah-Hartman
2023-04-29 15:21 ` Guenter Roeck
2023-05-01 18:42 ` Geert Uytterhoeven
2023-05-14 1:02 ` [PATCH v2] " Tetsuo Handa
2023-05-30 10:44 ` Greg Kroah-Hartman
2023-05-30 11:57 ` Tetsuo Handa [this message]
2023-05-30 12:51 ` Greg Kroah-Hartman
2024-04-27 6:20 ` [PATCH v3] " Tetsuo Handa
2024-04-27 19:02 ` Linus Torvalds
2024-04-28 10:19 ` Tetsuo Handa
2024-04-28 18:50 ` Linus Torvalds
2024-04-29 13:55 ` Marco Elver
2024-04-29 15:38 ` Linus Torvalds
2024-05-01 18:45 ` Paul E. McKenney
2024-05-01 18:56 ` Linus Torvalds
2024-05-01 19:02 ` Paul E. McKenney
2024-05-01 20:14 ` Marco Elver
2024-05-01 21:06 ` Linus Torvalds
2024-05-01 21:20 ` Linus Torvalds
2024-05-01 21:49 ` Paul E. McKenney
2024-05-01 22:32 ` Paul E. McKenney
2024-05-02 16:37 ` Boqun Feng
2024-05-03 23:59 ` Paul E. McKenney
2024-05-04 0:14 ` Linus Torvalds
2024-05-04 5:08 ` Paul E. McKenney
2024-05-04 17:50 ` Linus Torvalds
2024-05-04 18:18 ` Paul E. McKenney
2024-05-04 19:11 ` Linus Torvalds
2024-05-04 19:25 ` Linus Torvalds
2024-05-04 22:17 ` Paul E. McKenney
2024-05-04 22:04 ` Paul E. McKenney
2024-05-02 14:14 ` Marco Elver
2024-05-02 16:42 ` Tetsuo Handa
2024-05-02 17:20 ` Marco Elver
2024-05-02 17:29 ` Linus Torvalds
2024-05-02 18:14 ` Al Viro
2024-05-02 19:29 ` Marco Elver
2024-05-02 23:54 ` Tetsuo Handa
2024-05-03 1:12 ` Linus Torvalds
2023-04-23 13:28 ` [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4) Tetsuo Handa
2023-04-23 14:00 ` Greg Kroah-Hartman
2023-04-23 14:03 ` Greg Kroah-Hartman
2023-04-23 14:17 ` Tetsuo Handa
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=8edbd558-a05f-c775-4d0c-09367e688682@I-love.SAKURA.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=arnd@kernel.org \
--cc=dvyukov@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nathan@kernel.org \
--cc=syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=viro@zeniv.linux.org.uk \
/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).