Re: madvise/fadvise 32-bit length

[Jens Axboe <axboe@kernel.dk>]

On 6/1/24 9:51 AM, Stefan wrote:
> On 1/6/2024 17:35, Jens Axboe wrote:
>> On 6/1/24 9:22 AM, Stefan wrote:
>>> On 1/6/2024 17:05, Jens Axboe wrote:
>>>> On 6/1/24 8:19 AM, Jens Axboe wrote:
>>>>> On 6/1/24 3:43 AM, Stefan wrote:
>>>>>> io_uring uses the __u32 len field in order to pass the length to
>>>>>> madvise and fadvise, but these calls use an off_t, which is 64bit on
>>>>>> 64bit platforms.
>>>>>>
>>>>>> When using liburing, the length is silently truncated to 32bits (so
>>>>>> 8GB length would become zero, which has a different meaning of "until
>>>>>> the end of the file" for fadvise).
>>>>>>
>>>>>> If my understanding is correct, we could fix this by introducing new
>>>>>> operations MADVISE64 and FADVISE64, which use the addr3 field instead
>>>>>> of the length field for length.
>>>>>
>>>>> We probably just want to introduce a flag and ensure that older stable
>>>>> kernels check it, and then use a 64-bit field for it when the flag is
>>>>> set.
>>>>
>>>> I think this should do it on the kernel side, as we already check these
>>>> fields and return -EINVAL as needed. Should also be trivial to backport.
>>>> Totally untested... Might want a FEAT flag for this, or something where
>>>> it's detectable, to make the liburing change straight forward.
>>>>
>>>>
>>>> diff --git a/io_uring/advise.c b/io_uring/advise.c
>>>> index 7085804c513c..cb7b881665e5 100644
>>>> --- a/io_uring/advise.c
>>>> +++ b/io_uring/advise.c
>>>> @@ -17,14 +17,14 @@
>>>>    struct io_fadvise {
>>>>        struct file            *file;
>>>>        u64                offset;
>>>> -    u32                len;
>>>> +    u64                len;
>>>>        u32                advice;
>>>>    };
>>>>      struct io_madvise {
>>>>        struct file            *file;
>>>>        u64                addr;
>>>> -    u32                len;
>>>> +    u64                len;
>>>>        u32                advice;
>>>>    };
>>>>    @@ -33,11 +33,13 @@ int io_madvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>>    #if defined(CONFIG_ADVISE_SYSCALLS) && defined(CONFIG_MMU)
>>>>        struct io_madvise *ma = io_kiocb_to_cmd(req, struct io_madvise);
>>>>    -    if (sqe->buf_index || sqe->off || sqe->splice_fd_in)
>>>> +    if (sqe->buf_index || sqe->splice_fd_in)
>>>>            return -EINVAL;
>>>>          ma->addr = READ_ONCE(sqe->addr);
>>>> -    ma->len = READ_ONCE(sqe->len);
>>>> +    ma->len = READ_ONCE(sqe->off);
>>>> +    if (!ma->len)
>>>> +        ma->len = READ_ONCE(sqe->len);
>>>>        ma->advice = READ_ONCE(sqe->fadvise_advice);
>>>>        req->flags |= REQ_F_FORCE_ASYNC;
>>>>        return 0;
>>>> @@ -78,11 +80,13 @@ int io_fadvise_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>>    {
>>>>        struct io_fadvise *fa = io_kiocb_to_cmd(req, struct io_fadvise);
>>>>    -    if (sqe->buf_index || sqe->addr || sqe->splice_fd_in)
>>>> +    if (sqe->buf_index || sqe->splice_fd_in)
>>>>            return -EINVAL;
>>>>          fa->offset = READ_ONCE(sqe->off);
>>>> -    fa->len = READ_ONCE(sqe->len);
>>>> +    fa->len = READ_ONCE(sqe->addr);
>>>> +    if (!fa->len)
>>>> +        fa->len = READ_ONCE(sqe->len);
>>>>        fa->advice = READ_ONCE(sqe->fadvise_advice);
>>>>        if (io_fadvise_force_async(fa))
>>>>            req->flags |= REQ_F_FORCE_ASYNC;
>>>>
>>>
>>>
>>> If we want to have the length in the same field in both *ADVISE
>>> operations, we can put a flag in splice_fd_in/optlen.
>>
>> I don't think that part matters that much.
>>
>>> Maybe the explicit flag is a bit clearer for users of the API
>>> compared to the implicit flag when setting sqe->len to zero?
>>
>> We could go either way. The unused fields returning -EINVAL if set right
>> now can serve as the flag field - if you have it set, then that is your
>> length. If not, then the old style is the length. That's the approach I
>> took, rather than add an explicit flag to it. Existing users that would
>> set the 64-bit length fields would get -EINVAL already. And since the
>> normal flags field is already used for advice flags, I'd prefer just
>> using the existing 64-bit zero fields for it rather than add a flag in
>> an odd location. Would also make for an easier backport to stable.
>>
>> But don't feel that strongly about that part.
>>
>> Attached kernel patch with FEAT added, and liburing patch with 64
>> versions added.
>>
> 
> Sounds good!
> Do we want to do anything about the current (32-bit) functions in
> liburing? They silently truncate the user's values, so either marking
> them deprecated or changing the type of length in the arguments to a
> __u32 could help.

I like changing it to an __u32, and then we'll add a note to the man
page for them as well (with references to the 64-bit variants).

I still need to write a test and actually test the patches, but I'll get
to that Monday. If you want to write a test case that checks the 64-bit
range, then please do!

-- 
Jens Axboe