On Mon, Jul 22, 2024 at 06:10:41PM +0200, Jan Kara wrote:
> On Mon 22-07-24 16:41:22, Christian Brauner wrote:
> > On Fri, Jul 19, 2024 at 10:37:47PM GMT, Tetsuo Handa wrote:
> > > syzbot is reporting data race between __tty_hangup() and __fput(), and
> > > Dmitry Vyukov mentioned that this race has possibility of NULL pointer
> > > dereference, for tty_fops implements e.g. splice_read callback whereas
> > > hung_up_tty_fops does not.
> > >
> > > CPU0 CPU1
> > > ---- ----
> > > do_splice_read() {
> > > __tty_hangup() {
> > > // f_op->splice_read was copy_splice_read
> > > if (unlikely(!in->f_op->splice_read))
> > > return warn_unsupported(in, "read");
> > > filp->f_op = &hung_up_tty_fops;
> > > // f_op->splice_read is now NULL
> > > return in->f_op->splice_read(in, ppos, pipe, len, flags);
> > > }
> > > }
> > >
> > > Fix possibility of NULL pointer dereference by implementing missing
> > > callbacks, and suppress KCSAN messages by adding __data_racy qualifier
> > > to "struct file"->f_op .
> >
> > This f_op replacing without synchronization seems really iffy imho.
>
> Yeah, when I saw this I was also going "ouch". I was just waiting whether a
> tty maintainer will comment ;)
I really didn't want to :)
> Anyway this replacement of ops in file /
> inode has proven problematic in almost every single case where it was used
> leading to subtle issues.
Yeah, let's not do this.
Let me dig after -rc1 is out and see if there's a better way to handle
this contrived race condition...
thanks,
greg k-h
