[PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n

[PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n

[Tom Seewald]

After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
building the kernel with CHELSIO_T4=y and CHELSIO_TLS_DEVICE=n results
in the following error:

ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
`cxgb_select_queue':
cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'

This is caused by cxgb_select_queue() calling cxgb4_is_ktls_skb() without
checking if CHELSIO_TLS_DEVICE=y. Fix this by calling cxgb4_is_ktls_skb()
only when this config option is enabled.

Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
Signed-off-by: Tom Seewald <tseewald@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 7fd264a6d085..8e8783afd6df 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -1176,7 +1176,9 @@ static u16 cxgb_select_queue(struct net_device *dev, struct sk_buff *skb,
 		txq = netdev_pick_tx(dev, skb, sb_dev);
 		if (xfrm_offload(skb) || is_ptp_enabled(skb, dev) ||
 		    skb->encapsulation ||
-		    cxgb4_is_ktls_skb(skb) ||
+#if IS_ENABLED(CONFIG_CHELSIO_TLS_DEVICE)
+		cxgb4_is_ktls_skb(skb) ||
+#endif /* CHELSIO_TLS_DEVICE */
 		    (proto != IPPROTO_TCP && proto != IPPROTO_UDP))
 			txq = txq % pi->nqsets;
 
-- 
2.20.1

Re: [PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n

[Jakub Kicinski]

On Sun, 15 Nov 2020 20:31:40 -0600 Tom Seewald wrote:
> After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> building the kernel with CHELSIO_T4=y and CHELSIO_TLS_DEVICE=n results
> in the following error:
> 
> ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> `cxgb_select_queue':
> cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
> 
> This is caused by cxgb_select_queue() calling cxgb4_is_ktls_skb() without
> checking if CHELSIO_TLS_DEVICE=y. Fix this by calling cxgb4_is_ktls_skb()
> only when this config option is enabled.
> 
> Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> Signed-off-by: Tom Seewald <tseewald@gmail.com>
> ---
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 7fd264a6d085..8e8783afd6df 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -1176,7 +1176,9 @@ static u16 cxgb_select_queue(struct net_device *dev, struct sk_buff *skb,
>  		txq = netdev_pick_tx(dev, skb, sb_dev);
>  		if (xfrm_offload(skb) || is_ptp_enabled(skb, dev) ||
>  		    skb->encapsulation ||
> -		    cxgb4_is_ktls_skb(skb) ||
> +#if IS_ENABLED(CONFIG_CHELSIO_TLS_DEVICE)
> +		cxgb4_is_ktls_skb(skb) ||
> +#endif /* CHELSIO_TLS_DEVICE */
>  		    (proto != IPPROTO_TCP && proto != IPPROTO_UDP))
>  			txq = txq % pi->nqsets;
>  

The tls header already tries to solve this issue, it just does it
poorly. This is a better fix:

diff --git a/include/net/tls.h b/include/net/tls.h
index baf1e99d8193..2ff3f4f7954a 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -441,11 +441,11 @@ struct sk_buff *
 tls_validate_xmit_skb(struct sock *sk, struct net_device *dev,
                      struct sk_buff *skb);
 
 static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk)
 {
-#ifdef CONFIG_SOCK_VALIDATE_XMIT
+#ifdef CONFIG_TLS_DEVICE
        return sk_fullsock(sk) &&
               (smp_load_acquire(&sk->sk_validate_xmit_skb) ==
               &tls_validate_xmit_skb);
 #else
        return false;


Please test this and submit if it indeed solves the problem.

Thanks!

Re: [PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n

[Tom Seewald]

On Tue, Nov 17, 2020 at 4:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 15 Nov 2020 20:31:40 -0600 Tom Seewald wrote:
> > After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> > building the kernel with CHELSIO_T4=y and CHELSIO_TLS_DEVICE=n results
> > in the following error:
> >
> > ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> > `cxgb_select_queue':
> > cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
> >
> > This is caused by cxgb_select_queue() calling cxgb4_is_ktls_skb() without
> > checking if CHELSIO_TLS_DEVICE=y. Fix this by calling cxgb4_is_ktls_skb()
> > only when this config option is enabled.
> >
> > Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> > Signed-off-by: Tom Seewald <tseewald@gmail.com>
> > ---
> >  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > index 7fd264a6d085..8e8783afd6df 100644
> > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > @@ -1176,7 +1176,9 @@ static u16 cxgb_select_queue(struct net_device *dev, struct sk_buff *skb,
> >               txq = netdev_pick_tx(dev, skb, sb_dev);
> >               if (xfrm_offload(skb) || is_ptp_enabled(skb, dev) ||
> >                   skb->encapsulation ||
> > -                 cxgb4_is_ktls_skb(skb) ||
> > +#if IS_ENABLED(CONFIG_CHELSIO_TLS_DEVICE)
> > +             cxgb4_is_ktls_skb(skb) ||
> > +#endif /* CHELSIO_TLS_DEVICE */
> >                   (proto != IPPROTO_TCP && proto != IPPROTO_UDP))
> >                       txq = txq % pi->nqsets;
> >
>
> The tls header already tries to solve this issue, it just does it
> poorly. This is a better fix:
>
> diff --git a/include/net/tls.h b/include/net/tls.h
> index baf1e99d8193..2ff3f4f7954a 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -441,11 +441,11 @@ struct sk_buff *
>  tls_validate_xmit_skb(struct sock *sk, struct net_device *dev,
>                       struct sk_buff *skb);
>
>  static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk)
>  {
> -#ifdef CONFIG_SOCK_VALIDATE_XMIT
> +#ifdef CONFIG_TLS_DEVICE
>         return sk_fullsock(sk) &&
>                (smp_load_acquire(&sk->sk_validate_xmit_skb) ==
>                &tls_validate_xmit_skb);
>  #else
>         return false;
>
>
> Please test this and submit if it indeed solves the problem.
>
> Thanks!

Hi Jakub,

Thanks for the reply, unfortunately that patch does not resolve the
issue, I still get the same error as before. After looking into this a
bit further, the issue seems to be with CONFIG_TLS=m as everything
works when CONFIG_TLS=y.

I also see that there was a similar issue [1] reported by Intel's
kbuild test robot where the cxgb4 driver isn't able to see the TLS
symbols when CONFIG_TLS=m.

[1] https://lkml.org/lkml/2020/8/7/601

Re: [PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n

[Jakub Kicinski]

On Wed, 18 Nov 2020 23:40:40 -0600 Tom Seewald wrote:
> On Tue, Nov 17, 2020 at 4:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Sun, 15 Nov 2020 20:31:40 -0600 Tom Seewald wrote:  
> > > After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> > > building the kernel with CHELSIO_T4=y and CHELSIO_TLS_DEVICE=n results
> > > in the following error:
> > >
> > > ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> > > `cxgb_select_queue':
> > > cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
> > >
> > > This is caused by cxgb_select_queue() calling cxgb4_is_ktls_skb() without
> > > checking if CHELSIO_TLS_DEVICE=y. Fix this by calling cxgb4_is_ktls_skb()
> > > only when this config option is enabled.
> > >
> > > Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> > > Signed-off-by: Tom Seewald <tseewald@gmail.com>
> > > ---
> > >  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > > index 7fd264a6d085..8e8783afd6df 100644
> > > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > > @@ -1176,7 +1176,9 @@ static u16 cxgb_select_queue(struct net_device *dev, struct sk_buff *skb,
> > >               txq = netdev_pick_tx(dev, skb, sb_dev);
> > >               if (xfrm_offload(skb) || is_ptp_enabled(skb, dev) ||
> > >                   skb->encapsulation ||
> > > -                 cxgb4_is_ktls_skb(skb) ||
> > > +#if IS_ENABLED(CONFIG_CHELSIO_TLS_DEVICE)
> > > +             cxgb4_is_ktls_skb(skb) ||
> > > +#endif /* CHELSIO_TLS_DEVICE */
> > >                   (proto != IPPROTO_TCP && proto != IPPROTO_UDP))
> > >                       txq = txq % pi->nqsets;
> > >  
> >
> > The tls header already tries to solve this issue, it just does it
> > poorly. This is a better fix:
> >
> > diff --git a/include/net/tls.h b/include/net/tls.h
> > index baf1e99d8193..2ff3f4f7954a 100644
> > --- a/include/net/tls.h
> > +++ b/include/net/tls.h
> > @@ -441,11 +441,11 @@ struct sk_buff *
> >  tls_validate_xmit_skb(struct sock *sk, struct net_device *dev,
> >                       struct sk_buff *skb);
> >
> >  static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk)
> >  {
> > -#ifdef CONFIG_SOCK_VALIDATE_XMIT
> > +#ifdef CONFIG_TLS_DEVICE
> >         return sk_fullsock(sk) &&
> >                (smp_load_acquire(&sk->sk_validate_xmit_skb) ==
> >                &tls_validate_xmit_skb);
> >  #else
> >         return false;
> >
> >
> > Please test this and submit if it indeed solves the problem.
> >
> > Thanks!  
> 
> Hi Jakub,
> 
> Thanks for the reply, unfortunately that patch does not resolve the
> issue, I still get the same error as before. After looking into this a
> bit further, the issue seems to be with CONFIG_TLS=m as everything
> works when CONFIG_TLS=y.
> 
> I also see that there was a similar issue [1] reported by Intel's
> kbuild test robot where the cxgb4 driver isn't able to see the TLS
> symbols when CONFIG_TLS=m.

Interesting. Does your original patch solve the allyesconfig + TLS=m
problem?

Seems to me that CHELSIO_T4 should depend on (TLS || TLS=n), the
CONFIG_CHELSIO_TLS_DEVICE has the dependency but AFAICT nothing prevents
CONFIG_CHELSIO_TLS_DEVICE=m and CHELSIO_T4=y and cxgb4_main.c is under
the latter.

Re: [PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n

[Tom Seewald]

On Thu, Nov 19, 2020 at 11:37 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Nov 2020 23:40:40 -0600 Tom Seewald wrote:
> > On Tue, Nov 17, 2020 at 4:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Sun, 15 Nov 2020 20:31:40 -0600 Tom Seewald wrote:
> > > > After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> > > > building the kernel with CHELSIO_T4=y and CHELSIO_TLS_DEVICE=n results
> > > > in the following error:
> > > >
> > > > ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> > > > `cxgb_select_queue':
> > > > cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
> > > >
> > > > This is caused by cxgb_select_queue() calling cxgb4_is_ktls_skb() without
> > > > checking if CHELSIO_TLS_DEVICE=y. Fix this by calling cxgb4_is_ktls_skb()
> > > > only when this config option is enabled.
> > > >
> > > > Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> > > > Signed-off-by: Tom Seewald <tseewald@gmail.com>
> > > > ---
> > > >  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > > > index 7fd264a6d085..8e8783afd6df 100644
> > > > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > > > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> > > > @@ -1176,7 +1176,9 @@ static u16 cxgb_select_queue(struct net_device *dev, struct sk_buff *skb,
> > > >               txq = netdev_pick_tx(dev, skb, sb_dev);
> > > >               if (xfrm_offload(skb) || is_ptp_enabled(skb, dev) ||
> > > >                   skb->encapsulation ||
> > > > -                 cxgb4_is_ktls_skb(skb) ||
> > > > +#if IS_ENABLED(CONFIG_CHELSIO_TLS_DEVICE)
> > > > +             cxgb4_is_ktls_skb(skb) ||
> > > > +#endif /* CHELSIO_TLS_DEVICE */
> > > >                   (proto != IPPROTO_TCP && proto != IPPROTO_UDP))
> > > >                       txq = txq % pi->nqsets;
> > > >
> > >
> > > The tls header already tries to solve this issue, it just does it
> > > poorly. This is a better fix:
> > >
> > > diff --git a/include/net/tls.h b/include/net/tls.h
> > > index baf1e99d8193..2ff3f4f7954a 100644
> > > --- a/include/net/tls.h
> > > +++ b/include/net/tls.h
> > > @@ -441,11 +441,11 @@ struct sk_buff *
> > >  tls_validate_xmit_skb(struct sock *sk, struct net_device *dev,
> > >                       struct sk_buff *skb);
> > >
> > >  static inline bool tls_is_sk_tx_device_offloaded(struct sock *sk)
> > >  {
> > > -#ifdef CONFIG_SOCK_VALIDATE_XMIT
> > > +#ifdef CONFIG_TLS_DEVICE
> > >         return sk_fullsock(sk) &&
> > >                (smp_load_acquire(&sk->sk_validate_xmit_skb) ==
> > >                &tls_validate_xmit_skb);
> > >  #else
> > >         return false;
> > >
> > >
> > > Please test this and submit if it indeed solves the problem.
> > >
> > > Thanks!
> >
> > Hi Jakub,
> >
> > Thanks for the reply, unfortunately that patch does not resolve the
> > issue, I still get the same error as before. After looking into this a
> > bit further, the issue seems to be with CONFIG_TLS=m as everything
> > works when CONFIG_TLS=y.
> >
> > I also see that there was a similar issue [1] reported by Intel's
> > kbuild test robot where the cxgb4 driver isn't able to see the TLS
> > symbols when CONFIG_TLS=m.
>
> Interesting. Does your original patch solve the allyesconfig + TLS=m
> problem?

No it does not solve it, my original patch was incorrect and should
not be applied. It only masks the issue when using my specific kernel
config.

> Seems to me that CHELSIO_T4 should depend on (TLS || TLS=n), the
> CONFIG_CHELSIO_TLS_DEVICE has the dependency but AFAICT nothing prevents
> CONFIG_CHELSIO_TLS_DEVICE=m and CHELSIO_T4=y and cxgb4_main.c is under
> the latter.

You are right, adding (TLS || TLS=n) for CHELSIO_T4 resolves the build
error I am encountering.

Re: [PATCH] cxbgb4: Fix build failure when CHELSIO_TLS_DEVICE=n

[Jakub Kicinski]

On Fri, 20 Nov 2020 00:54:00 -0600 Tom Seewald wrote:
> > Seems to me that CHELSIO_T4 should depend on (TLS || TLS=n), the
> > CONFIG_CHELSIO_TLS_DEVICE has the dependency but AFAICT nothing prevents
> > CONFIG_CHELSIO_TLS_DEVICE=m and CHELSIO_T4=y and cxgb4_main.c is under
> > the latter.  
> 
> You are right, adding (TLS || TLS=n) for CHELSIO_T4 resolves the build
> error I am encountering.

Great, please submit a patch.

[PATCH] cxgb4: Fix build failure when CONFIG_TLS=m

[Tom Seewald]

After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
whenever CONFIG_TLS=m and CONFIG_CHELSIO_T4=y, the following build
failure occurs:

ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
`cxgb_select_queue':
cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'

Fix this by ensuring that if TLS is set to be a module, CHELSIO_T4 will
also be compiled as a module. As otherwise the cxgb4 driver will not be
able to access TLS' symbols.

Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
Signed-off-by: Tom Seewald <tseewald@gmail.com>
---
 drivers/net/ethernet/chelsio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/Kconfig b/drivers/net/ethernet/chelsio/Kconfig
index 87cc0ef68b31..8ba0e08e5e64 100644
--- a/drivers/net/ethernet/chelsio/Kconfig
+++ b/drivers/net/ethernet/chelsio/Kconfig
@@ -68,7 +68,7 @@ config CHELSIO_T3
 
 config CHELSIO_T4
 	tristate "Chelsio Communications T4/T5/T6 Ethernet support"
-	depends on PCI && (IPV6 || IPV6=n)
+	depends on PCI && (IPV6 || IPV6=n) && (TLS || TLS=n)
 	select FW_LOADER
 	select MDIO
 	select ZLIB_DEFLATE
-- 
2.20.1

Re: [PATCH] cxgb4: Fix build failure when CONFIG_TLS=m

[Jakub Kicinski]

On Fri, 20 Nov 2020 13:25:28 -0600 Tom Seewald wrote:
> After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> whenever CONFIG_TLS=m and CONFIG_CHELSIO_T4=y, the following build
> failure occurs:
> 
> ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> `cxgb_select_queue':
> cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
> 
> Fix this by ensuring that if TLS is set to be a module, CHELSIO_T4 will
> also be compiled as a module. As otherwise the cxgb4 driver will not be
> able to access TLS' symbols.
> 
> Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> Signed-off-by: Tom Seewald <tseewald@gmail.com>

Applied, thanks!