Re: [PATCH 05/18] Add io_uring IO interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/29/19 8:56 AM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 4:46 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>> On 1/28/19 7:21 PM, Jann Horn wrote:
>>> Please create a local copy of the request before parsing it to keep
>>> the data from changing under you. Additionally, it might make sense to
>>> annotate every pointer to shared memory with a comment, or something
>>> like that, to ensure that anyone looking at the code can immediately
>>> see for which pointers special caution is required on access.
>>
>> I took a look at the viability of NOT having to local copy the data, and
>> I don't think it's too bad. Local copy has a noticeable impact on the
>> performance, hence I'd really (REALLY) like to avoid it.
>>
>> Here's something on top of the current git branch. I think I even went a
>> bit too far in some areas, but it should hopefully catch the cases where
>> we might end up double evaluating the parts of the sqe that we depend
>> on. For most of the sqe reading we don't really care too much. For
>> instance, the sqe->user_data. If the app changes this field, then it
>> just gets whatever passed back in cqe->user_data. That's not a kernel
>> issue.
>>
>> For cases like addr/len etc validation, it should be sound. I'll double
>> check this in the morning as well, and obviously would need to be folded
>> in along the way.
>>
>> I'd appreciate your opinion on this part, if you see any major issues
>> with it, or if I missed something.
> 
> The io_sqe_needs_user() checks still look racy. If that helper sees a
> IORING_OP_READ_FIXED, but then __io_submit_sqe() sees a
> IORING_OP_READV - especially if this happens in io_sq_wq_submit_work()
> -, I think you could potentially end up in places like
> io_import_iovec() without having done the set_fs(USER_DS) and
> use_mm(), causing the access to potentially occur with KERNEL_DS and a
> lazy mm.

Indeed, for that case I think we should just copy the sqe. It's in the
async offload context anyway, so a copy won't really change anything
in terms of performance. And since the gap is so large between the two
problematic spots, it'd be trickier to fix.

-- 
Jens Axboe




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux