Re: [syzbot] BUG: sleeping function called from invalid context in _copy_to_iter

[Shoaib Rao]

On 8/9/21 1:16 PM, Al Viro wrote:
> On Mon, Aug 09, 2021 at 08:04:40PM +0000, Al Viro wrote:
> > On Mon, Aug 09, 2021 at 12:40:03PM -0700, Shoaib Rao wrote:
> >
> > > Page faults occur all the time, the page may not even be in the cache or the
> > > mapping is not there (mmap), so I would not consider this a bug. The code
> > > should complain about all other calls as they are also copying  to user
> > > pages. I must not be following some semantics for the code to be triggered
> > > but I can not figure that out. What is the recommended interface to do user
> > > copy from kernel?
> > 	What are you talking about?  Yes, page faults happen.  No, they
> > must not be triggered in contexts when you cannot afford going to sleep.
> > In particular, you can't do that while holding a spinlock.
> >
> > 	There are things that can't be done under a spinlock.  If your
> > commit is attempting that, it's simply broken.
> ... in particular, this
>
> +#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() ?

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?

Shoaib

> You can't do blocking operations under a spinlock.  And copyout is inherently
> a blocking operation - it can require any kind of IO to complete.  If you
> have the destination (very much valid - no bad addresses there) in the middle
> of a page mmapped from a file and currently not paged in, you *must* read
> the current contents of the page, at least into the parts of page that
> are not going to be overwritten by your copyout.  No way around that.  And
> that can involve any kind of delays and any amount of disk/network/whatnot
> traffic.
>
> You fundamentally can not do that kind of thing without giving the CPU up.
> And under a spinlock you are not allowed to do that.
>
> In the current form that commit is obviously broken.
I am