netdev - Re: [PATCH net-next] memory-provider: fix compilation issue without SYSFS

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: <CAHS8izOkpnLM_Uev79skrmdQjdOGwy_oYWV7xb3hNpSb=yYZ6g@mail.gmail.com>
Date: Thu, 12 Sep 2024 05:49:23 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: "Matthieu Baerts (NGI0)" <matttbe@...nel.org>
Cc: mptcp@...ts.linux.dev, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Kaiyuan Zhang <kaiyuanz@...gle.com>, Willem de Bruijn <willemb@...gle.com>, 
	Pavel Begunkov <asml.silence@...il.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] memory-provider: fix compilation issue without SYSFS

On Thu, Sep 12, 2024 at 3:25 AM Matthieu Baerts (NGI0)
<matttbe@...nel.org> wrote:
>
> When CONFIG_SYSFS is not set, the kernel fails to compile:
>
>      net/core/page_pool_user.c:368:45: error: implicit declaration of function 'get_netdev_rx_queue_index' [-Werror=implicit-function-declaration]
>       368 |                 if (pool->slow.queue_idx == get_netdev_rx_queue_index(rxq)) {
>           |                                             ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> When CONFIG_SYSFS is not set, get_netdev_rx_queue_index() is not defined
> as well. In this case, page_pool_check_memory_provider() cannot check
> the memory provider, and a success answer can be returned instead.
>

Thanks Matthieu, and sorry about that.

I have reproduced the build error and the fix resolves it. But...

> Fixes: 0f9214046893 ("memory-provider: dmabuf devmem memory provider")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>
> ---
>  net/core/page_pool_user.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/core/page_pool_user.c b/net/core/page_pool_user.c
> index 48335766c1bf..a98c0a76b33f 100644
> --- a/net/core/page_pool_user.c
> +++ b/net/core/page_pool_user.c
> @@ -353,6 +353,7 @@ void page_pool_unlist(struct page_pool *pool)
>  int page_pool_check_memory_provider(struct net_device *dev,
>                                     struct netdev_rx_queue *rxq)
>  {
> +#ifdef CONFIG_SYSFS
>         struct net_devmem_dmabuf_binding *binding = rxq->mp_params.mp_priv;
>         struct page_pool *pool;
>         struct hlist_node *n;
> @@ -372,6 +373,9 @@ int page_pool_check_memory_provider(struct net_device *dev,
>         }
>         mutex_unlock(&page_pools_lock);
>         return -ENODATA;
> +#else
> +       return 0;

...we can't assume success when we cannot check the memory provider.
The memory provider check is somewhat critical; we rely on it to
detect that the driver does not support memory providers or is not
doing the right thing, and report that to the user. I don't think we
can silently disable the check when the CONFIG_SYSFS is disabled.
Please return -ENODATA or some other error here.

If we disable devmem TCP for !CONFIG_SYSFS we should probably add
something to the docs saying this. I can do that in a follow up
change.

However, I'm looking at the definition of get_netdev_rx_queue_index()
and at first glance I don't see anything there that is actually
dependent on CONFIG_SYSFS. Can we do this instead? I have build-tested
it and it resolves the build issue as well:

```
diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
index ac34f5fb4f71..596836abf7bf 100644
--- a/include/net/netdev_rx_queue.h
+++ b/include/net/netdev_rx_queue.h
@@ -45,7 +45,6 @@ __netif_get_rx_queue(struct net_device *dev, unsigned int rxq)
        return dev->_rx + rxq;
 }

-#ifdef CONFIG_SYSFS
 static inline unsigned int
 get_netdev_rx_queue_index(struct netdev_rx_queue *queue)
 {
@@ -55,7 +54,6 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue)
        BUG_ON(index >= dev->num_rx_queues);
        return index;
 }
-#endif
 ```

Matthieu, I'm happy to follow up with v2 of this fix if you don't have time.

-- 
Thanks,
Mina