On 23 May 10:58, Mina Almasry wrote:
On Thu, May 22, 2025 at 4:54 PM Saeed Mahameed <saeed@xxxxxxxxxx> wrote:
>> static inline void
>> mlx5e_copy_skb_header(struct mlx5e_rq *rq, struct sk_buff *skb,
>> - struct page *page, dma_addr_t addr,
>> + netmem_ref netmem, dma_addr_t addr,
>> int offset_from, int dma_offset, u32 headlen)
>> {
>> - const void *from = page_address(page) + offset_from;
>> + const void *from = netmem_address(netmem) + offset_from;
>
>I think this needs a check that netmem_address != NULL and safe error
>handling in case it is? If the netmem is unreadable, netmem_address
>will return NULL, and because you add offset_from to it, you can't
>NULL check from as well.
>
Nope, this code path is not for GRO_HW, it is always safe to assume this is
not iov_netmem.
OK, thanks for checking. It may be worth it to add
DEBUG_NET_WARN_ON_ONCE(netmem_address(netmem)); in these places where
Too ugly and will only be caught in DEBUG env with netmem_iov enabled on a
somehow broken driver, so if you already doing that I am sure
you won't mind a crash :) in your debug env..
Also I don't expect any of mlx5 developers to confuse between header data
split paths and other paths.. but maybe a comment somewhere should cover
this gap.
you're assuming the netmem is readable and has a valid address. It
would be a very subtle bug later on if someone moves the code or
something and suddenly you have unreadable netmem being funnelled
through these code paths. But up to you.
Cosmin, let's add comments on the shampo skb functions and the relevant
lines of code, maybe it will help preventing future mistakes.
--
Thanks,
Mina
