linux-kernel - Re: [PATCH v3] tty: tty_io: remove hung_up_tty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wgSOa_g+bxjNi+HQpC=6sHK2yKeoW-xOhb0-FVGMTDWjg@mail.gmail.com>
Date: Sat, 27 Apr 2024 12:02:32 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Dmitry Vyukov <dvyukov@...gle.com>, 
	syzbot <syzbot+b7c3ba8cdc2f6cf83c21@...kaller.appspotmail.com>, 
	linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com, 
	Nathan Chancellor <nathan@...nel.org>, Arnd Bergmann <arnd@...nel.org>, Al Viro <viro@...iv.linux.org.uk>, 
	Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH v3] tty: tty_io: remove hung_up_tty_fops

On Fri, 26 Apr 2024 at 23:21, Tetsuo Handa
<penguin-kernel@...ove.sakura.ne.jp> wrote:
>
> syzbot is reporting data race between __tty_hangup() and __fput(), for
> filp->f_op readers are not holding tty->files_lock.

Hmm. I looked round, and we actually have another case of this:
snd_card_disconnect() also does

                        mfile->file->f_op = &snd_shutdown_f_ops;

and I don't think tty->files_lock (or, in the sound case,
&card->files_lock) is at all relevant, since the users of f_ops don't
use it or care.

That said, I really think we'd be better off just keeping the current
model, and have the "you get one or the other". For the two cases that
do this, do that f_op replacement with a WRITE_ONCE(), and just make
the rule be that you have to have all the same ops in both the
original and the shutdown version.

I do *not* think it's at all better to replace (in two different
places) the racy f_op thing with another racy 'hungup' flag.

The sound case is actually a bit more involved, since it tries to deal
with module counts. That looks potentially bogus. It does

                        fops_get(mfile->file->f_op);

after it has installed the snd_shutdown_f_ops, but in snd_open() it
has done the proper

        replace_fops(file, new_fops);

which actually drops the module count for the old one. So the sound
case seems to possibly leak a module ref on disconnect. That's a
separate issue, though.

                      Linus

                    Linus