Messages in this thread | | | Date | Sun, 28 Apr 2024 19:19:52 +0900 | Subject | Re: [PATCH v3] tty: tty_io: remove hung_up_tty_fops | From | Tetsuo Handa <> |
| |
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
| |