LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Shoaib Rao <rao.shoaib@oracle.com>
Cc: Dmitry Vyukov <dvyukov@google.com>,
	syzbot <syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com>,
	andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
	christian.brauner@ubuntu.com, cong.wang@bytedance.com,
	daniel@iogearbox.net, davem@davemloft.net, edumazet@google.com,
	jamorris@linux.microsoft.com, john.fastabend@gmail.com,
	kafai@fb.com, kpsingh@kernel.org, kuba@kernel.org,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	netdev@vger.kernel.org, shuah@kernel.org, songliubraving@fb.com,
	syzkaller-bugs@googlegroups.com, yhs@fb.com
Subject: Re: [syzbot] BUG: sleeping function called from invalid context in _copy_to_iter
Date: Mon, 9 Aug 2021 21:41:19 +0000	[thread overview]
Message-ID: <YRGg/yTXTAL/1whP@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <c1ec22f6-ed3b-fe70-2c7e-38a534f01d2b@oracle.com>

On Mon, Aug 09, 2021 at 01:37:08PM -0700, Shoaib Rao wrote:

> > +#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> > +               mutex_lock(&u->iolock);
> > +               unix_state_lock(sk);
> > +
> > +               err = unix_stream_recv_urg(state);
> > +
> > +               unix_state_unlock(sk);
> > +               mutex_unlock(&u->iolock);
> > +#endif
> > 
> > is 100% broken, since you *are* attempting to copy data to userland between
> > spin_lock(&unix_sk(s)->lock) and spin_unlock(&unix_sk(s)->lock).
> 
> Yes, but why are we calling it unix_state_lock() why not
> unix_state_spinlock() ?

We'd never bothered with such naming conventions; keep in mind that
locking rules can and do change from time to time, and encoding the
nature of locking primitive into the name would result in tons of
noise.

> I have tons of experience doing kernel coding and you can never ever cover
> everything, that is why I wanted to root cause the issue instead of just
> turning off the check.
> 
> Imagine you or Eric make a mistake and break the kernel, how would you guys
> feel if I were to write a similar email?

Moderately embarrassed, at a guess, but what would that have to do with
somebody pointing the bug out?  Bonehead mistakes happen, they are embarrassing
no matter who catches them - trust me, it's no less unpleasant when you end
up being one who finds your own bug months after it went into the tree.  Been
there, done that...

Since you asked, as far as my reactions normally go:
	* I made a mistake that ended up screwing people over => can be
hideously embarrassing, no matter what.  No cause for that in your case,
AFAICS - it hadn't even gone into mainline yet.
	* I made a dumb mistake that got caught (again, doesn't matter
by whom) => unpleasant; shit happens (does it ever), but that's not
a tragedy.  Ought to look for the ways to catch the same kind of mistakes
and see if I have stepped into the same problem anywhere else - often
enough the blind spots strike more than once.  If the method of catching
the same kind of crap ends up being something like 'grep for <pattern>,
manually check the instances to weed out the false positive'... might
be worth running over the tree; often enough the blind spots are shared.
Would be partially applicable in your case ("if using an unfamiliar locking
helper, check what it does"), but not easily greppable.
	* I kept looking at bug report, missing the relevant indicators
despite the increasingly direct references to those by other people =>
mildly embarrassing (possibly more than mildly, if that persists for long).
Ought to get some coffee, wake up properly (if applicable, that is) and make
notes for myself re what to watch out for.  Partially applicable here;
I'm no telepath, but at a guess you missed the list of locks in the report
_and_ missed repeated references to some spinlock being involved.
Since the call chain had not (AFAICS) been missed, the question
"which spinlock do they keep blathering about?" wouldn't have been hard.
Might be useful to make note of, for the next time you have to deal with
such reports.
	* Somebody starts asking whether I bloody understand something
trivial => figure out what does that have to do with the situation at
hand, reply with the description of what I'd missed (again, quite possibly
the answer will be "enough coffee") and move on to figuring out how to
fix the damn bug.  Not exactly applicable here - the closest I can see
is Eric's question regarding the difference between mutex and spinlock.
In similar situation I'd go with something along the lines of "Sorry,
hadn't spotted the spinlock in question"; your reply had been a bit
more combative than that, but that's a matter of taste.  None of my
postings would fit into that class, AFAICS...
	* Somebody explains (in painful details) what's wrong with the
code => more or less the same as above, only with less temptation (for
me) to get defensive.  Reactions vary - some folks find it more offensive
than the previous one, but essentially it's the same thing.

	The above describes my reactions, in case it's not obvious -
I'm not saying that everyone should react the same way, but you've
asked how would I (or Eric) react in such-and-such case.  And I can't
speak for Eric, obviously...

  reply	other threads:[~2021-08-09 21:41 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-08 23:38 syzbot
2021-08-09 17:32 ` Shoaib Rao
2021-08-09 18:06   ` Dmitry Vyukov
2021-08-09 19:16     ` Shoaib Rao
2021-08-09 19:21       ` Dmitry Vyukov
2021-08-09 19:40         ` Shoaib Rao
2021-08-09 20:02           ` Eric Dumazet
2021-08-09 20:09             ` Eric Dumazet
2021-08-09 20:31               ` Shoaib Rao
2021-08-10  9:19                 ` Eric Dumazet
2021-08-10 17:50                   ` Shoaib Rao
2021-08-10 18:02                     ` Eric Dumazet
2021-08-10 18:29                       ` Shoaib Rao
2021-08-09 20:04           ` Al Viro
2021-08-09 20:16             ` Al Viro
2021-08-09 20:30               ` Shoaib Rao
2021-08-09 20:37               ` Shoaib Rao
2021-08-09 21:41                 ` Al Viro [this message]
2021-08-09 22:38                   ` Shoaib Rao
2021-08-09 19:57       ` Al Viro
2021-08-09 20:18         ` Shoaib Rao

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=YRGg/yTXTAL/1whP@zeniv-ca.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=christian.brauner@ubuntu.com \
    --cc=cong.wang@bytedance.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dvyukov@google.com \
    --cc=edumazet@google.com \
    --cc=jamorris@linux.microsoft.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=rao.shoaib@oracle.com \
    --cc=shuah@kernel.org \
    --cc=songliubraving@fb.com \
    --cc=syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=yhs@fb.com \
    --subject='Re: [syzbot] BUG: sleeping function called from invalid context in _copy_to_iter' \
    /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

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).