Re: madvise/fadvise 32-bit length

[Stefan <source@s.muenzel.net>]

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.