Re: [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4)

[Tetsuo Handa]

On 2023/04/23 23:03, Greg Kroah-Hartman wrote:
>> Next step is to convert from
>>
>>   if (!f_op->$callbackname) {
>>     return error;
>>   }
>>   return f_op->$callbackname($args);
>>
>> to
>>
>>   fn = READ_ONCE(f_op->$callbackname);
>>   if (!fn) {
>>     return error;
>>   }
>>   return fn($args);
>>
>> pattern.
> 
> Why?  What does this solve differently than the first one?  What can
> change the fops pointer between the check and call path?  If something
> can change it, then do NOT make that type of check in the first place
> (or put a proper lock in place.)

__tty_hangup() is changing filp->f_op like

	spin_lock(&tty->files_lock);
	/* This breaks for file handles being sent over AF_UNIX sockets ? */
	list_for_each_entry(priv, &tty->tty_files, list) {
		filp = priv->file;
		if (filp->f_op->write_iter == redirected_tty_write)
			cons_filp = filp;
		if (filp->f_op->write_iter != tty_write)
			continue;
		closecount++;
		__tty_fasync(-1, filp, 0);	/* can't block */
		filp->f_op = &hung_up_tty_fops;
	}
	spin_unlock(&tty->files_lock);

but filp->f_op readers are (of course) not protected by tty->files_lock.
Like Dmitry mentioned

  hung_up_tty_fops does not have splice_read, while other fops have.

, patterns like

	if (unlikely(!in->f_op->splice_read))
	    return warn_unsupported(in, "read");
	return in->f_op->splice_read(in, ppos, pipe, len, flags);

is not safe (if compiler generates a code that re-reads the pointer).

Using READ_ONCE() is for preventing the compiler to re-read, and using data_race()
is for teaching KCSAN that race while happens during READ_ONCE()/WRITE_ONCE()
is acceptable.

>> First step (which Dmitry mentioned) is to avoid potential NULL pointer dereferences
>> caused by
>>
>>   if (!f_op->$callbackname) {
>>     return error;
>>   }
>>   return f_op->$callbackname($args);
>>
>> pattern, for the next step will touch too many locations to change all at once whereas
>> the first step could be handled by implementing dummy function for all missing $callbackname.
> 
> I do not understand, what "callbackname" is the problem here?  Just
> splice_read?  Something else?

I haven't checked.

>                                And where would it need to be modified
> and why can't we do it in one place only (i.e. install a default handler
> instead.)

Yes, adding a dummy splice_read callback handler to hung_up_tty_fops is
what I call the first step.