From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: syzbot <syzbot+3d2c27c2b7dc2a94814d@syzkaller.appspotmail.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jirislaby@kernel.org>,
linux-kernel@vger.kernel.org, snovitoll@gmail.com,
syzkaller-bugs@googlegroups.com
Subject: Re: WARNING in iov_iter_revert (2)
Date: Sat, 20 Feb 2021 19:29:57 +0000 [thread overview]
Message-ID: <YDFjNRv+DNL/Xh8W@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <YDFJKR5uG1N+g9TL@zeniv-ca.linux.org.uk>
On Sat, Feb 20, 2021 at 05:38:49PM +0000, Al Viro wrote:
> On Sat, Feb 20, 2021 at 08:56:40AM -0800, Linus Torvalds wrote:
> > Al,
> > This is the "FIXME! Have Al check this!" case in do_tty_write(). You were
> > in on that whole discussion, but we never did get to that issue...
> >
> > There are some subtle rules about doing the iov_iter_revert(), but what's
> > the best way to do this properly? Instead of doing a copy_from_iter() and
> > then reverting the part that didn't fit in the buffer, doing a
> > non-advancing copy and then advancing the amount that did fit, or what?
> >
> > I still don't have power, so this is all me on mobile with html email
> > (sorry), and limited ability to really look closer.
> >
> > "Help me, Albi-wan Viro, you're my only hope"
>
> Will check... BTW, when you get around to doing pulls, could you pick
> the replacement (in followup) instead of the first pull request for
> work.namei? Jens has caught a braino in the last commit there...
It turned out to be really amusing. What happens is write(fd, NULL, 0)
on /dev/ttyprintk, with N_GSM0710 for ldisc (== "pass the data as
is to tty->op->write()". And that's the first write since opening
that sucker, so we end up with
/* write_buf/write_cnt is protected by the atomic_write_lock mutex */
if (tty->write_cnt < chunk) {
unsigned char *buf_chunk;
if (chunk < 1024)
chunk = 1024;
buf_chunk = kmalloc(chunk, GFP_KERNEL);
if (!buf_chunk) {
ret = -ENOMEM;
goto out;
}
kfree(tty->write_buf);
tty->write_cnt = chunk;
tty->write_buf = buf_chunk;
}
doing nothing - ->write_cnt is still 0 and ->write_buf - NULL. Then
we copy 0 bytes from source to ->write_buf(), which reports that 0
bytes had been copied, TYVM. Then we call
ret = write(tty, file, tty->write_buf, size);
i.e.
ret = gsm_write(tty, file, NULL, 0);
which calls
tpk_write(tty, NULL, 0)
which does
tpk_printk(NULL, 0);
and _that_ has a very special semantics:
int i = tpk_curr;
if (buf == NULL) {
tpk_flush();
return i;
}
i.e. it *can* return a positive number that gets propagated all way
back to do_tty_write(). And then you notice that it has reports
successful write of amount other than what you'd passed and tries
to pull back. By amount passed - amount written. With iov_iter_revert()
saying that some tosser has asked it to revert by something close to
~(size_t)0.
IOW, it's not iov_iter_revert() being weird or do_tty_write() misuing it -
it's tpk_write() playing silly buggers. Note that old tree would've
gone through seriously weird contortions on the same call:
// chunk and count are 0, ->write_buf is NULL
for (;;) {
size_t size = count;
if (size > chunk)
size = chunk;
ret = -EFAULT;
if (copy_from_user(tty->write_buf, buf, size))
break;
ret = write(tty, file, tty->write_buf, size);
if (ret <= 0)
break;
written += ret;
buf += ret;
count -= ret;
if (!count)
break;
ret = -ERESTARTSYS;
if (signal_pending(current))
break;
cond_resched();
}
and we get written = ret = small positive, count = - that amount,
buf = NULL + that mount. On the next iteration size = 0 (since
chunk is still 0), with same no-op copy_from_user() of 0 bytes,
then gsm_write(tty, file, NULL, 0) and since tpk_flush() zeroes
tpk_curr we finally get 0 out of tpk_printk/tpk_write/gsm_write
and bugger off on if (ret <= 0). Then we have the value in written
returned.
So yeah, this return value *was* returned to userland. Except that
if we had done any writes before that, we'd find ->write_buf
non-NULL and the magical semantics of write(fd, NULL, 0) would
*not* have triggered - we would've gotten zero.
Do we want to preserve that weirdness of /dev/ttyprintk writes?
That's orthogonal to the iov_iter uses in there.
next prev parent reply other threads:[~2021-02-20 19:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-16 16:18 WARNING in iov_iter_revert (2) syzbot
2021-02-20 14:25 ` syzbot
[not found] ` <CAHk-=wiEBTD884i-U9DU7aDdRxXuz66Q1r-rKTiJUzZoYFgp+g@mail.gmail.com>
2021-02-20 17:38 ` Al Viro
2021-02-20 17:40 ` [git pull] work.namei stuff (v2) Al Viro
2021-02-21 17:43 ` Linus Torvalds
2021-02-21 18:39 ` pr-tracker-bot
2021-02-20 19:29 ` Al Viro [this message]
2021-02-20 19:40 ` WARNING in iov_iter_revert (2) Al Viro
2021-02-21 0:45 ` Linus Torvalds
2021-02-21 8:37 ` Sabyrzhan Tasbolatov
2021-02-21 17:20 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YDFjNRv+DNL/Xh8W@zeniv-ca.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=snovitoll@gmail.com \
--cc=syzbot+3d2c27c2b7dc2a94814d@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).