lkml.org 
[lkml]   [2024]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3] tty: tty_io: remove hung_up_tty_fops
From
On 2024/04/28 4:02, Linus Torvalds wrote:
> On Fri, 26 Apr 2024 at 23:21, Tetsuo Handa
> <penguin-kernel@i-love.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


\
 
 \ /
  Last update: 2024-04-28 12:20    [W:0.116 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site