From: Kal Conley <kal.conley@xxxxxxxxxxx>
Date: Tue, 7 Mar 2023 19:58:51 +0100
>> The RCT declaration style is messed up in the whole block. Please move
>> lines around, there's nothing wrong in that.
>
> I think I figured out what this is. Is this preference documented
> somewhere? I will fix it.
It's when you sort the declarations by the line length. I.e.
short var a;
longest var b;
medium var c;
=>
longest var b;
medium var c;
short var a;
I think it's documented somewhere in the kernel. You can try grepping by
"Reverse Christmas Tree".
>
>>
>>> int err;
>>>
>>> if (chunk_size < XDP_UMEM_MIN_CHUNK_SIZE || chunk_size > PAGE_SIZE) {
>>> @@ -188,8 +189,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
>>> if (npgs > U32_MAX)
>>> return -EINVAL;
>>>
>>> - chunks = (unsigned int)div_u64_rem(size, chunk_size, &chunks_rem);
>>> - if (chunks == 0)
>>> + chunks = div_u64_rem(size, chunk_size, &chunks_rem);
>>> + if (chunks == 0 || chunks > U32_MAX)
>>
>> You can change the first cond to `!chunks` while at it, it's more
>> preferred than `== 0`.
>
> If you want, I can change it. I generally like to keep unrelated
> changes to a minimum.
You modify the line either way, so I don't see any reasons to keep the
code as-is. It's clear that replacing `== 0` to `!chunks` won't change
the logic anyhow.
>
>>
>>> return -EINVAL;
>>
>> Do you have any particular bugs that the current code leads to? Or it's
>> just something that might hypothetically happen?
>
> If the UMEM is large enough, the code is broke. Maybe it can be
> exploited somehow? It should be checked for exactly the same reasons
> as `npgs` right above it.
>
>>
>>>
>>> if (!unaligned_chunks && chunks_rem)
>>> @@ -201,7 +202,7 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
>>> umem->size = size;
>>> umem->headroom = headroom;
>>> umem->chunk_size = chunk_size;
>>> - umem->chunks = chunks;
>>> + umem->chunks = (u32)chunks;
>>
>> You already checked @chunks fits into 32 bits, so the cast can be
>> omitted here, it's redundant.
>
> I made it consistent with the line right below it. It seems like the
> cast may improve readability since it makes it known the truncation is
> on purpose. I don't see how that is redundant with the safety check.
> Should I change both lines?
I'd prefer to change both lines. You already check both @npgs and
@chunks for being <= %U32_MAX and anyone can see it from the code, so
the casts don't make anything more readable.
>
>>
>>> umem->npgs = (u32)npgs;
>>> umem->pgs = NULL;
>>> umem->user = NULL;
>>
>> Thanks,
>> Olek
>
> Kal
Thanks,
Olek
