netdev - Re: [RFC net-next] netmem: replace __netmem_clear_lsb() with netmem_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izPv8zmPaxzCSPAnybiCc0KrqjEZA+x5wpFOE8u=_nM1WA@mail.gmail.com>
Date: Mon, 28 Jul 2025 10:44:31 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Byungchul Park <byungchul@...com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, 
	hawk@...nel.org, toke@...hat.com, asml.silence@...il.com
Subject: Re: [RFC net-next] netmem: replace __netmem_clear_lsb() with netmem_to_nmdesc()

On Sun, Jul 27, 2025 at 9:21 PM Byungchul Park <byungchul@...com> wrote:
>
> Now that we have struct netmem_desc, it'd better access the pp fields
> via struct netmem_desc rather than struct net_iov.
>
> Introduce netmem_to_nmdesc() for safely converting netmem_ref to
> netmem_desc regardless of the type underneath e.i. netmem_desc, net_iov.
>
> While at it, remove __netmem_clear_lsb() and make netmem_to_nmdesc()
> used instead.
>
> Signed-off-by: Byungchul Park <byungchul@...com>

Thank you for working on paying this tech debt!

> ---
>  include/net/netmem.h   | 33 ++++++++++++++++-----------------
>  net/core/netmem_priv.h | 16 ++++++++--------
>  2 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index f7dacc9e75fd..33ae444a9745 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -265,24 +265,23 @@ static inline struct netmem_desc *__netmem_to_nmdesc(netmem_ref netmem)
>         return (__force struct netmem_desc *)netmem;
>  }
>
> -/* __netmem_clear_lsb - convert netmem_ref to struct net_iov * for access to
> - * common fields.
> - * @netmem: netmem reference to extract as net_iov.
> +/* netmem_to_nmdesc - convert netmem_ref to struct netmem_desc * for
> + * access to common fields.
> + * @netmem: netmem reference to get netmem_desc.
>   *
> - * All the sub types of netmem_ref (page, net_iov) have the same pp, pp_magic,
> - * dma_addr, and pp_ref_count fields at the same offsets. Thus, we can access
> - * these fields without a type check to make sure that the underlying mem is
> - * net_iov or page.
> + * All the sub types of netmem_ref (netmem_desc, net_iov) have the same
> + * pp, pp_magic, dma_addr, and pp_ref_count fields via netmem_desc.
>   *
> - * The resulting value of this function can only be used to access the fields
> - * that are NET_IOV_ASSERT_OFFSET'd. Accessing any other fields will result in
> - * undefined behavior.
> - *

I think instead of removing this warning, we want to add an
NET_IOV_ASSERT_OFFSET that asserts that net_iov->netmem_desc and
page->netmem_desc are in the same offset, and then add a note here
that this works because we assert that the netmem_desc offset in both
net_iov and page are the same.

> - * Return: the netmem_ref cast to net_iov* regardless of its underlying type.
> + * Return: the pointer to struct netmem_desc * regardless of its
> + * underlying type.
>   */
> -static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> +static inline struct netmem_desc *netmem_to_nmdesc(netmem_ref netmem)
>  {
> -       return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> +       if (netmem_is_net_iov(netmem))
> +               return &((struct net_iov *)((__force unsigned long)netmem &
> +                                           ~NET_IOV))->desc;
> +
> +       return __netmem_to_nmdesc(netmem);

The if statement generates overhead. I'd rather avoid it. We can
implement netmem_to_nmdesc like this, no?

netmem_to_nmdesc(netmem_ref netmem)
{
  return (struct netmem_desc)((__force unsigned long)netmem & ~NET_IOV);
}

Because netmem_desc is the first element in both net_iov and page for
the moment. (yes I know that will change eventually, but we don't have
to incur overhead of an extra if statement until netmem_desc is
removed from page, right?)


-- 
Thanks,
Mina