build failure of next-20220811 due to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")

build failure of next-20220811 due to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")

[Sudip Mukherjee (Codethink)]

Hi All,

Not sure if it has been reported, builds of csky and mips allmodconfig
failed to build next-20220811 with gcc-12.

mips error is:

In function 'memcmp',
    inlined from 'bacmp' at ./include/net/bluetooth/bluetooth.h:347:9,
    inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2003:15:
./include/linux/fortify-string.h:44:33: error: '__builtin_memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
   44 | #define __underlying_memcmp     __builtin_memcmp
      |                                 ^
./include/linux/fortify-string.h:420:16: note: in expansion of macro '__underlying_memcmp'
  420 |         return __underlying_memcmp(p, q, size);
      |                ^~~~~~~~~~~~~~~~~~~
In function 'memcmp',
    inlined from 'bacmp' at ./include/net/bluetooth/bluetooth.h:347:9,
    inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2004:15:
./include/linux/fortify-string.h:44:33: error: '__builtin_memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
   44 | #define __underlying_memcmp     __builtin_memcmp
      |                                 ^
./include/linux/fortify-string.h:420:16: note: in expansion of macro '__underlying_memcmp'
  420 |         return __underlying_memcmp(p, q, size);
      |                ^~~~~~~~~~~~~~~~~~~


csky error is:

In file included from net/bluetooth/l2cap_core.c:37:
In function 'bacmp',
    inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2003:15:
./include/net/bluetooth/bluetooth.h:347:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
  347 |         return memcmp(ba1, ba2, sizeof(bdaddr_t));
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'bacmp',
    inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2004:15:
./include/net/bluetooth/bluetooth.h:347:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
  347 |         return memcmp(ba1, ba2, sizeof(bdaddr_t));
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


git bisect pointed to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression").
And, reverting that commit has fixed the build failure.

I will be happy to test any patch or provide any extra log if needed.

--
Regards
Sudip

Re: build failure of next-20220811 due to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")

[Jakub Kicinski]

On Thu, 11 Aug 2022 19:53:04 +0100 Sudip Mukherjee (Codethink) wrote:
> Hi All,
> 
> Not sure if it has been reported, builds of csky and mips allmodconfig
> failed to build next-20220811 with gcc-12.

Heh, 2 minutes after I submitted it to Linus :S

> mips error is:
> 
> In function 'memcmp',
>     inlined from 'bacmp' at ./include/net/bluetooth/bluetooth.h:347:9,
>     inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2003:15:
> ./include/linux/fortify-string.h:44:33: error: '__builtin_memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]

Source is the second argument? memcmp does not really have src and dst..

Assuming it's the second one it appears to object to the:

#define BDADDR_ANY  (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})

Which, well, kinda understandable but why does it not dislike the same
construct when used in the other 70 places in the tree?

My preferred fix would be to do the same thing as we do for ethernet
i.e. open code the helper, see is_zero_ether_addr().

>    44 | #define __underlying_memcmp     __builtin_memcmp
>       |                                 ^
> ./include/linux/fortify-string.h:420:16: note: in expansion of macro '__underlying_memcmp'
>   420 |         return __underlying_memcmp(p, q, size);
>       |                ^~~~~~~~~~~~~~~~~~~
> In function 'memcmp',
>     inlined from 'bacmp' at ./include/net/bluetooth/bluetooth.h:347:9,
>     inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2004:15:
> ./include/linux/fortify-string.h:44:33: error: '__builtin_memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
>    44 | #define __underlying_memcmp     __builtin_memcmp
>       |                                 ^
> ./include/linux/fortify-string.h:420:16: note: in expansion of macro '__underlying_memcmp'
>   420 |         return __underlying_memcmp(p, q, size);
>       |                ^~~~~~~~~~~~~~~~~~~
> 
> 
> csky error is:
> 
> In file included from net/bluetooth/l2cap_core.c:37:
> In function 'bacmp',
>     inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2003:15:
> ./include/net/bluetooth/bluetooth.h:347:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
>   347 |         return memcmp(ba1, ba2, sizeof(bdaddr_t));
>       |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function 'bacmp',
>     inlined from 'l2cap_global_chan_by_psm' at net/bluetooth/l2cap_core.c:2004:15:
> ./include/net/bluetooth/bluetooth.h:347:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
>   347 |         return memcmp(ba1, ba2, sizeof(bdaddr_t));
>       |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 
> git bisect pointed to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression").
> And, reverting that commit has fixed the build failure.
> 
> I will be happy to test any patch or provide any extra log if needed.
> 
> --
> Regards
> Sudip

Re: build failure of next-20220811 due to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")

[Jakub Kicinski]

On Thu, 11 Aug 2022 19:53:04 +0100 Sudip Mukherjee (Codethink) wrote:
> Not sure if it has been reported, builds of csky and mips allmodconfig
> failed to build next-20220811 with gcc-12.

I can't repro with the cross compiler from kernel.org.
Can you test something like this?

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index e72f3b247b5e..82bf8e01f7af 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -341,6 +341,11 @@ static inline bool bdaddr_type_is_le(u8 type)
 #define BDADDR_ANY  (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
 #define BDADDR_NONE (&(bdaddr_t) {{0xff, 0xff, 0xff, 0xff, 0xff, 0xff}})
 
+static inline int ba_is_any(const bdaddr_t *ba)
+{
+	return memchr_inv(ba, sizeof(*ba), 0);
+}
+
 /* Copy, swap, convert BD Address */
 static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
 {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 77c0aac14539..a08ec272be4a 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2001,8 +2001,8 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
 			}
 
 			/* Closest match */
-			src_any = !bacmp(&c->src, BDADDR_ANY);
-			dst_any = !bacmp(&c->dst, BDADDR_ANY);
+			src_any = !ba_is_any(&c->src);
+			dst_any = !ba_is_any(&c->dst);
 			if ((src_match && dst_any) || (src_any && dst_match) ||
 			    (src_any && dst_any))
 				c1 = c;

Re: build failure of next-20220811 due to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")

[Luiz Augusto von Dentz]

Hi Jakub,

On Thu, Aug 11, 2022 at 12:59 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 11 Aug 2022 19:53:04 +0100 Sudip Mukherjee (Codethink) wrote:
> > Not sure if it has been reported, builds of csky and mips allmodconfig
> > failed to build next-20220811 with gcc-12.
>
> I can't repro with the cross compiler from kernel.org.
> Can you test something like this?
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index e72f3b247b5e..82bf8e01f7af 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -341,6 +341,11 @@ static inline bool bdaddr_type_is_le(u8 type)
>  #define BDADDR_ANY  (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
>  #define BDADDR_NONE (&(bdaddr_t) {{0xff, 0xff, 0xff, 0xff, 0xff, 0xff}})
>
> +static inline int ba_is_any(const bdaddr_t *ba)
> +{
> +       return memchr_inv(ba, sizeof(*ba), 0);
> +}

So we can't use something like BDADDR_ANY to compare? Anyway afaik
these were already present before the patch so I do wonder what had
trigger it show now or perhaps it was being suppressed before and
since we change it now start showing again?

>  /* Copy, swap, convert BD Address */
>  static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
>  {
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 77c0aac14539..a08ec272be4a 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2001,8 +2001,8 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
>                         }
>
>                         /* Closest match */
> -                       src_any = !bacmp(&c->src, BDADDR_ANY);
> -                       dst_any = !bacmp(&c->dst, BDADDR_ANY);
> +                       src_any = !ba_is_any(&c->src);
> +                       dst_any = !ba_is_any(&c->dst);
>                         if ((src_match && dst_any) || (src_any && dst_match) ||
>                             (src_any && dst_any))
>                                 c1 = c;



-- 
Luiz Augusto von Dentz

Re: build failure of next-20220811 due to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")

[Jakub Kicinski]

On Thu, 11 Aug 2022 13:20:52 -0700 Luiz Augusto von Dentz wrote:
> > +static inline int ba_is_any(const bdaddr_t *ba)
> > +{
> > +       return memchr_inv(ba, sizeof(*ba), 0);
> > +}  
> 
> So we can't use something like BDADDR_ANY to compare? Anyway afaik
> these were already present before the patch so I do wonder what had
> trigger it show now or perhaps it was being suppressed before and
> since we change it now start showing again?

Yeah, I mentioned that in my previous reply as well, a quick grep
counts 70 instances, IDK what makes the l2cap code different :S
Then again I don't know how the compiler deals with passing a pointer
to a constant to an inline function.... so I figured memchr_inv()
could help us avoid hitting compiler bugs.

Re: build failure of next-20220811 due to 332f1795ca20 ("Bluetooth: L2CAP: Fix l2cap_global_chan_by_psm regression")

[Sudip Mukherjee]

On Thu, Aug 11, 2022 at 8:46 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 11 Aug 2022 19:53:04 +0100 Sudip Mukherjee (Codethink) wrote:
> > Not sure if it has been reported, builds of csky and mips allmodconfig
> > failed to build next-20220811 with gcc-12.
>
> I can't repro with the cross compiler from kernel.org.
> Can you test something like this?

With this patch I get new failure:

In file included from net/bluetooth/l2cap_core.c:37:
./include/net/bluetooth/bluetooth.h: In function 'ba_is_any':
./include/net/bluetooth/bluetooth.h:346:16: error: returning 'void *'
from a function with return type 'int' makes integer from pointer
without a cast [-Werror=int-conversion]
  346 |         return memchr_inv(ba, sizeof(*ba), 0);

So for a quick test, I modified it a little (just a typecast) which worked.

diff --git a/include/net/bluetooth/bluetooth.h
b/include/net/bluetooth/bluetooth.h
index e72f3b247b5e..19bdd2520070 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -341,6 +341,11 @@ static inline bool bdaddr_type_is_le(u8 type)
 #define BDADDR_ANY  (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
 #define BDADDR_NONE (&(bdaddr_t) {{0xff, 0xff, 0xff, 0xff, 0xff, 0xff}})

+static inline int ba_is_any(const bdaddr_t *ba)
+{
+       return (int) memchr_inv(ba, sizeof(*ba), 0);
+}
+
 /* Copy, swap, convert BD Address */
 static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
 {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index cbe0cae73434..67c5d923bc6c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2000,8 +2000,8 @@ static struct l2cap_chan
*l2cap_global_chan_by_psm(int state, __le16 psm,
                        }

                        /* Closest match */
-                       src_any = !bacmp(&c->src, BDADDR_ANY);
-                       dst_any = !bacmp(&c->dst, BDADDR_ANY);
+                       src_any = !ba_is_any(&c->src);
+                       dst_any = !ba_is_any(&c->dst);
                        if ((src_match && dst_any) || (src_any && dst_match) ||
                            (src_any && dst_any))
                                c1 = c;



-- 
Regards
Sudip