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

[Jens Axboe <axboe@kernel.dk>]

On 3/16/23 1:46 PM, Jens Axboe wrote:
> 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) {

& (SHM_COLOUR - 1)) {

of course...

-- 
Jens Axboe