Date: Sun, 28 Apr 2024 19:19:52 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Linus Torvalds <torvalds@...ux-foundation.org>
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 2024/04/28 4:02, Linus Torvalds wrote:
> 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;

OK. That one needs to be fixed as well.

> 
> 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.

More precisely, the users of f_op can't access it. For example,
do_splice_read() cannot understand that "in" argument refers to a tty
device and therefore will not know about tty->files_lock.

> 
> 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.

If we keep the current model, WRITE_ONCE() is not sufficient.

My understanding is that KCSAN's report like
https://elixir.bootlin.com/linux/v6.9-rc5/source/Documentation/dev-tools/kcsan.rst#L56
will remain unless we wrap all f_op readers using data_race() macro. That is,
we will need to define a wrapper like

static inline struct file_operations *f_op(struct file *file)
{
	/*
	 * Ignore race in order to silence KCSAN, for __tty_hangup() or
	 * snd_card_disconnect() might update f_op while file is in use.
	 */
	return data_race(file->f_op);
}

and do for example

-	if (unlikely(!in->f_op->splice_read))
+	if (unlikely(!f_op(in)->splice_read))

for https://elixir.bootlin.com/linux/v6.9-rc5/source/fs/splice.c#L977 and

-	return in->f_op->splice_read(in, ppos, pipe, len, flags);
+	return f_op(in)->splice_read(in, ppos, pipe, len, flags);

for https://elixir.bootlin.com/linux/v6.9-rc5/source/fs/splice.c#L985 .

Are VFS people happy with such change? I guess that VFS people assume that
file->f_op does not get updated while file is in use. Also, such data_race()
usage does not match one of situations listed in
https://elixir.bootlin.com/linux/v6.9-rc5/source/tools/memory-model/Documentation/access-marking.txt#L58 .

> 
> 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.

This approach allows VFS people to assume that file->f_op does not
get updated while file is in use.

> 
> 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);

replace_fops() is intended to be used *ONLY* from ->open() instances.

> 
> 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

Powered by blists - more mailing lists