On 1/3/22 7:17 AM, Jann Horn wrote:
> On Fri, Dec 24, 2021 at 3:35 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>> io_uring should be protecting the current file position with the
>> file position lock, ->f_pos_lock. Grab and track the lock state when
>> the request is being issued, and make the unlock part of request
>> cleaning.
>>
>> Fixes: ba04291eb66e ("io_uring: allow use of offset == -1 to mean file position")
>> Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>>
>> ---
>>
>> Main thing I don't like here:
>>
>> - We're holding the f_pos_lock across the kernel/user boundary, as
>> it's held for the duration of the IO. Alternatively we could
>> keep it local to io_read() and io_write() and lose REQ_F_CUR_POS_LOCK,
>> but will messy up those functions more and add more items to the
>> fast path (which current position read/write definitely is not).
>>
>> Suggestions welcome...
>
> Oh, that's not pretty... is it guaranteed that the
Right, hence why it's an RFC :-)
> __f_unlock_pos(req->file) will happen in the same task as the
> io_file_pos_lock(req, false), and have you tried running this with
It might unlock from a thread off that task, depends on how the execution
happens. And as it stands, it'll also potentially exit the kernel with
the lock held until it completes.
> lockdep and mutex debugging enabled? Could a task deadlock if it tried
> to do a read() on a file while io_uring is already holding the
> position lock?
lockdep will complain about the leaving the kernel with it held aspect
for sure.
I think the better solution here is, as I suggested in the patch, to
keep it local to io_read() and io_write() rather than try and track it.
Which is a bit annoying in terms of adding mostly useless code to the
fast path, but... Don't think there's a better way.
--
Jens Axboe
