Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8904e414-6b3e-8fa3-67e1-709d51965372@I-love.SAKURA.ne.jp>
Date:   Sun, 23 Apr 2023 23:17:57 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [syzbot] [kernel?] KCSAN: data-race in __fput / __tty_hangup (4)
Content-Language: en-US
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Dmitry Vyukov <dvyukov@google.com>,
        syzbot <syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com
References: <00000000000013aaac05f9d44e7a@google.com>
 <CACT4Y+bj7jCOjGV5jCUPzN5zjgdHxRn9vkwQEBuPWVzMbMCnXw@mail.gmail.com>
 <6bec279c-07b3-d6f1-0860-4d6b136a2025@I-love.SAKURA.ne.jp>
 <2023042347-vaguely-monsieur-63ed@gregkh>
From:   Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
In-Reply-To: <2023042347-vaguely-monsieur-63ed@gregkh>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.