Messages in this thread | | | From | David Howells <> | Subject | Re: kernel BUG at fs/pipe.c:LINE! | Date | Thu, 05 Dec 2019 13:06:30 +0000 |
| |
Eric Biggers <ebiggers@kernel.org> wrote:
> static __poll_t > pipe_poll(struct file *filp, poll_table *wait) > { > __poll_t mask; > struct pipe_inode_info *pipe = filp->private_data; > unsigned int head = READ_ONCE(pipe->head); > unsigned int tail = READ_ONCE(pipe->tail); > > poll_wait(filp, &pipe->wait, wait); > > BUG_ON(pipe_occupancy(head, tail) > pipe->ring_size); > > It's not holding the pipe mutex, right? So 'head', 'tail' and 'ring_size' can > all be changed concurrently, and they aren't read atomically with respect to > each other. > > How do you propose to implement poll() correctly with the new head + tail > approach? Just take the mutex?
Firstly, the BUG_ON() check probably isn't necessary here - the same issue with occupancy being seen to be greater than the queue depth existed previously (there was no locking around the read of pipe->nrbufs and pipe->buffers). I added a sanity check.
Secondly, it should be possible to make it such that just the spinlock suffices. The following few patches make the main pipe read/write routines use the spinlock so as not to be interfered with by notification insertion.
I didn't roll the spinlock out to splice and suchlike since I prohibit splicing to a notifications pipe because of the iov_iter_revert() fun.
David
| |