Re: [PATCHSET 0/5] User mapped provided buffer rings

[Jens Axboe <axboe@kernel.dk>]

On 3/16/23 1:08?PM, John David Anglin wrote:
> On 2023-03-15 5:18 p.m., Jens Axboe wrote:
>> On 3/15/23 2:38?PM, Jens Axboe wrote:
>>> On 3/15/23 2:07?PM, Helge Deller wrote:
>>>> On 3/15/23 21:03, Helge Deller wrote:
>>>>> Hi Jens,
>>>>>
>>>>> Thanks for doing those fixes!
>>>>>
>>>>> On 3/14/23 18:16, Jens Axboe wrote:
>>>>>> One issue that became apparent when running io_uring code on parisc is
>>>>>> that for data shared between the application and the kernel, we must
>>>>>> ensure that it's placed correctly to avoid aliasing issues that render
>>>>>> it useless.
>>>>>>
>>>>>> The first patch in this series is from Helge, and ensures that the
>>>>>> SQ/CQ rings are mapped appropriately. This makes io_uring actually work
>>>>>> there.
>>>>>>
>>>>>> Patches 2..4 are prep patches for patch 5, which adds a variant of
>>>>>> ring mapped provided buffers that have the kernel allocate the memory
>>>>>> for them and the application mmap() it. This brings these mapped
>>>>>> buffers in line with how the SQ/CQ rings are managed too.
>>>>>>
>>>>>> I'm not fully sure if this ONLY impacts archs that set SHM_COLOUR,
>>>>>> of which there is only parisc, or if SHMLBA setting archs (of which
>>>>>> there are others) are impact to any degree as well...
>>>>> It would be interesting to find out. I'd assume that other arches,
>>>>> e.g. sparc, might have similiar issues.
>>>>> Have you tested your patches on other arches as well?
>>>> By the way, I've now tested this series on current git head on an
>>>> older parisc box (with PA8700 / PCX-W2 CPU).
>>>>
>>>> Results of liburing testsuite:
>>>> Tests timed out (1): <send-zerocopy.t> - (may not be a failure)
>>>> Tests failed (5): <buf-ring.t> <file-verify.t> <poll-race-mshot.t> <ringbuf-read.t> <send_recvmsg.t>
>> If you update your liburing git copy, switch to the ring-buf-alloc branch,
>> then all of the above should work:
> With master liburing branch, test/poll-race-mshot.t still crashes my rp3440:
> Running test poll-race-mshot.t Bad cqe res -233
> Bad cqe res -233
> Bad cqe res -233
> 
> There is a total lockup with no messages of any kind.
> 
> I think the io_uring code needs to reject user supplied ring buffers that are not equivalently mapped
> to the corresponding kernel pages.  Don't know if it would be possible to reallocate kernel pages so they
> are equivalently mapped.

We can do that, you'd just want to add that check in io_pin_pbuf_ring()
when the pages have been mapped AND we're on an arch that has those
kinds of requirements. Maybe something like the below, totally
untested...

I am puzzled where the crash is coming from, though. It should just hit
the -ENOBUFS case as it can't find a buffer, and that'd terminate that
request. Which does seem to be what is happening above, that is really
no different than an attempt to read/receive from a buffer group that
has no buffers available. So a bit puzzling on what makes your kernel
crash after that has happened, as we do have generic test cases that
exercise that explicitly.


diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index cd1d9dddf58e..73f290aca7f1 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -491,6 +491,15 @@ static int io_pin_pbuf_ring(struct io_uring_buf_reg *reg,
 		return PTR_ERR(pages);
 
 	br = page_address(pages[0]);
+#ifdef SHM_COLOUR
+	if ((reg->ring_addr & (unsigned long) br) & SHM_COLOUR) {
+		int i;
+
+		for (i = 0; i < nr_pages; i++)
+			unpin_user_page(pages[i]);
+		return -EINVAL;
+	}
+#endif
 	bl->buf_pages = pages;
 	bl->buf_nr_pages = nr_pages;
 	bl->buf_ring = br;

-- 
Jens Axboe