Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers

From: Jens Axboe <axboe@kernel.dk>
To: Jann Horn <jannh@google.com>
Cc: linux-aio@kvack.org, linux-block@vger.kernel.org,
	Linux API <linux-api@vger.kernel.org>,
	hch@lst.de, jmoyer@redhat.com, Avi Kivity <avi@scylladb.com>
Subject: Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
Date: Tue, 29 Jan 2019 16:06:37 -0700	[thread overview]
Message-ID: <87b25ce2-a5bc-3e81-ddba-5c932c71b40f@kernel.dk> (raw)
In-Reply-To: <CAG48ez3x5x+P=8L1p-bHvcUnypeT1TizkXq2+Q0bCP0GdE-bog@mail.gmail.com>

On 1/29/19 4:03 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/29/19 3:44 PM, Jann Horn wrote:
>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> If we have fixed user buffers, we can map them into the kernel when we
>>>> setup the io_context. That avoids the need to do get_user_pages() for
>>>> each and every IO.
>>>>
>>>> To utilize this feature, the application must call io_uring_register()
>>>> after having setup an io_uring context, passing in
>>>> IORING_REGISTER_BUFFERS as the opcode. The argument must be a pointer
>>>> to an iovec array, and the nr_args should contain how many iovecs the
>>>> application wishes to map.
>>>>
>>>> If successful, these buffers are now mapped into the kernel, eligible
>>>> for IO. To use these fixed buffers, the application must use the
>>>> IORING_OP_READ_FIXED and IORING_OP_WRITE_FIXED opcodes, and then
>>>> set sqe->index to the desired buffer index. sqe->addr..sqe->addr+seq->len
>>>> must point to somewhere inside the indexed buffer.
>>>>
>>>> The application may register buffers throughout the lifetime of the
>>>> io_uring context. It can call io_uring_register() with
>>>> IORING_UNREGISTER_BUFFERS as the opcode to unregister the current set of
>>>> buffers, and then register a new set. The application need not
>>>> unregister buffers explicitly before shutting down the io_uring context.
> [...]
>>>> +       imu = &ctx->user_bufs[index];
>>>> +       buf_addr = READ_ONCE(sqe->addr);
>>>> +       if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
>>>
>>> This can wrap around if `buf_addr` or `len` is very big, right? Then
>>> you e.g. get past the first check because `buf_addr` is sufficiently
>>> big, and get past the second check because `buf_addr + len` wraps
>>> around and becomes small.
>>
>> Good point. I wonder if we have a verification helper for something like
>> this?
> 
> check_add_overflow() exists, I guess that might help a bit. I don't
> think I've seen a more specific helper for this situation.

Hmm, not super appropriate. How about something ala:

if (buf_addr + len < buf_addr)
    ... overflow ...

?

-- 
Jens Axboe