netdev - Re: [PATCH net-next?] net: fungible: fix multiple build problems

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]
Date:   Fri, 4 Mar 2022 16:39:49 -0800
From:   Dimitris Michailidis <d.michailidis@...gible.com>
To:     Randy Dunlap <rdunlap@...radead.org>
Cc:     netdev@...r.kernel.org, patches@...ts.linux.dev,
        Dimitris Michailidis <dmichail@...gible.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next?] net: fungible: fix multiple build problems

On Fri, Mar 4, 2022 at 1:15 PM Randy Dunlap <rdunlap@...radead.org> wrote:
>
> It is currently possible to have CONFIG_FUN_ETH=y and CONFIG_TLS=m.

This combination is allowed if TLS_DEVICE=n, which I suspect you have in your
config. I don't think you can get the combination otherwise. In that
case the driver
doesn't have TLS support and doesn't need anything from the TLS module.
The compile problems you see I think come from the next item.

> This causes build errors. Therefore FUN_ETH should
>         depend on TLS || TLS=n
>
> TLS_DEVICE is a bool symbol, so there is no need to test it as
>         depends on TLS && TLS_DEVICE || TLS_DEVICE=n
>
> And due to rules of precedence, the above means
>         depends on (TLS && TLS_DEVICE) || TLS_DEVICE=n
> but that's probably not what was meant here. More likely it
> should have been
>         depends on TLS && (TLS_DEVICE || TLS_DEVICE=n)
>
> That no longer matters.
>
> Also, gcc 7.5 does not handle the "C language" vs. #ifdef preprocessor
> usage of IS_ENABLED() very well -- it is causing compile errors.

I believe this is the source of the errors you see but it's not the compiler's
fault or something specific to 7.5.0. The errors are because when
IS_ENABLED() is false some of the symbols in the "if" are undefined and the
compiler checks them regardless.

> $ gcc --version
> gcc (SUSE Linux) 7.5.0
>
> And then funeth uses sbitmap, so it should select SBITMAP in order
> to prevent build errors.

Indeed. I think the "select SBITMAP" should be added to "config FUN_CORE"
though as that is the module using sbitmap.

> Fixes these build errors:
>
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c: In function ‘write_pkt_desc’:
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:13: error: implicit declaration of function ‘tls_driver_ctx’ [-Werror=implicit-function-declaration]
>    tls_ctx = tls_driver_ctx(skb->sk, TLS_OFFLOAD_CTX_DIR_TX);
>              ^~~~~~~~~~~~~~
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:37: error: ‘TLS_OFFLOAD_CTX_DIR_TX’ undeclared (first use in this function); did you mean ‘BPF_OFFLOAD_MAP_FREE’?
>    tls_ctx = tls_driver_ctx(skb->sk, TLS_OFFLOAD_CTX_DIR_TX);
>                                      ^~~~~~~~~~~~~~~~~~~~~~
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:37: note: each undeclared identifier is reported only once for each function it appears in
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:245:23: error: dereferencing pointer to incomplete type ‘struct fun_ktls_tx_ctx’
>    tls->tlsid = tls_ctx->tlsid;
>                        ^~
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c: In function ‘fun_start_xmit’:
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:310:6: error: implicit declaration of function ‘tls_is_sk_tx_device_offloaded’ [-Werror=implicit-function-declaration]
>       tls_is_sk_tx_device_offloaded(skb->sk)) {
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:311:9: error: implicit declaration of function ‘fun_tls_tx’; did you mean ‘fun_xdp_tx’? [-Werror=implicit-function-declaration]
>    skb = fun_tls_tx(skb, q, &tls_len);
>          ^~~~~~~~~~
> ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:311:7: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
>    skb = fun_tls_tx(skb, q, &tls_len);
>        ^
>
> and
>
> ERROR: modpost: "__sbitmap_queue_get" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined!
> ERROR: modpost: "sbitmap_finish_wait" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined!
> ERROR: modpost: "sbitmap_queue_clear" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined!
> ERROR: modpost: "sbitmap_prepare_to_wait" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined!
> ERROR: modpost: "sbitmap_queue_init_node" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined!
> ERROR: modpost: "sbitmap_queue_wake_all" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined!
>
> #Fixes: not-merged-yet ("X")
> Signed-off-by: Randy Dunlap <rdunlap@...radead.org>
> Cc: Dimitris Michailidis <dmichail@...gible.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Jakub Kicinski <kuba@...nel.org>
> ---
>  drivers/net/ethernet/fungible/funeth/Kconfig     |    3 ++-
>  drivers/net/ethernet/fungible/funeth/funeth_tx.c |    9 ++++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> --- mmotm-2022-0303-2124.orig/drivers/net/ethernet/fungible/funeth/Kconfig
> +++ mmotm-2022-0303-2124/drivers/net/ethernet/fungible/funeth/Kconfig
> @@ -6,9 +6,10 @@
>  config FUN_ETH
>         tristate "Fungible Ethernet device driver"
>         depends on PCI && PCI_MSI
> -       depends on TLS && TLS_DEVICE || TLS_DEVICE=n
> +       depends on TLS || TLS=n
>         select NET_DEVLINK
>         select FUN_CORE
> +       select SBITMAP
>         help
>           This driver supports the Ethernet functionality of Fungible adapters.
>           It works with both physical and virtual functions.
> --- mmotm-2022-0303-2124.orig/drivers/net/ethernet/fungible/funeth/funeth_tx.c
> +++ mmotm-2022-0303-2124/drivers/net/ethernet/fungible/funeth/funeth_tx.c
> @@ -234,7 +234,8 @@ static unsigned int write_pkt_desc(struc
>                         fun_dataop_gl_init(gle, 0, 0, lens[i], addrs[i]);
>         }
>
> -       if (IS_ENABLED(CONFIG_TLS_DEVICE) && unlikely(tls_len)) {
> +#if IS_ENABLED(CONFIG_TLS_DEVICE)
> +       if (unlikely(tls_len)) {
>                 struct fun_eth_tls *tls = (struct fun_eth_tls *)gle;
>                 struct fun_ktls_tx_ctx *tls_ctx;
>
> @@ -250,6 +251,7 @@ static unsigned int write_pkt_desc(struc
>                 q->stats.tx_tls_pkts += 1 + extra_pkts;
>                 u64_stats_update_end(&q->syncp);
>         }
> +#endif
>
>         u64_stats_update_begin(&q->syncp);
>         q->stats.tx_bytes += skb->len + extra_bytes;
> @@ -306,12 +308,13 @@ netdev_tx_t fun_start_xmit(struct sk_buf
>         unsigned int tls_len = 0;
>         unsigned int ndesc;
>
> -       if (IS_ENABLED(CONFIG_TLS_DEVICE) && skb->sk &&
> -           tls_is_sk_tx_device_offloaded(skb->sk)) {
> +#if IS_ENABLED(CONFIG_TLS_DEVICE)
> +       if (skb->sk && tls_is_sk_tx_device_offloaded(skb->sk)) {
>                 skb = fun_tls_tx(skb, q, &tls_len);
>                 if (unlikely(!skb))
>                         goto dropped;
>         }
> +#endif
>
>         ndesc = write_pkt_desc(skb, q, tls_len);
>         if (unlikely(!ndesc)) {