[PATCH v3] net: bluetooth: hci_sock: Use 'const u32 *' instead of 'void *' for 2nd parameter of hci_test_bit()

hci_test_bit() does not modify 2nd parameter, so it is better to let it
be constant, or may cause build warning. The related warning (with
allmodconfig under xtensa):

  net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg':
  net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers]
          &hci_sec_filter.ocf_mask[ogf])) &&
          ^
  net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}'
   static inline int hci_test_bit(int nr, void *addr)
                     ^

hci_test_bit() always treats 2nd parameter is u32, and all callers also
know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *'
instead of 'void *'.

C language treats the array function parameter as a pointer, so the
caller need not use '&' for the 2 demotion array, or it reports warning:
'const unsigned int (*)[4]' is different with 'const unsigned int *'.


Signed-off-by: Chen Gang <redacted>
---
 net/bluetooth/hci_sock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 1d65c5b..04124ec 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c

@@ -46,9 +46,9 @@ struct hci_pinfo {
 	unsigned short    channel;
 };
 
-static inline int hci_test_bit(int nr, void *addr)
+static inline int hci_test_bit(int nr, const u32 *addr)
 {
-	return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
+	return *(addr + (nr >> 5)) & ((u32) 1 << (nr & 31));
 }
 
 /* Security filter */

@@ -107,7 +107,7 @@ static bool is_filtered_packet(struct sock *sk, struct sk_buff *skb)
 
 	flt_event = (*(__u8 *)skb->data & HCI_FLT_EVENT_BITS);
 
-	if (!hci_test_bit(flt_event, &flt->event_mask))
+	if (!hci_test_bit(flt_event, (u32 *)&flt->event_mask))
 		return true;
 
 	/* Check filter only when opcode is set */

@@ -952,7 +952,7 @@ static int hci_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 
 		if (((ogf > HCI_SFLT_MAX_OGF) ||
 		     !hci_test_bit(ocf & HCI_FLT_OCF_BITS,
-				   &hci_sec_filter.ocf_mask[ogf])) &&
+				   hci_sec_filter.ocf_mask[ogf])) &&
 		    !capable(CAP_NET_RAW)) {
 			err = -EPERM;
 			goto drop;

-- 
1.9.3

On Sat, 2015-02-07 at 21:24 +0800, Chen Gang S wrote:

hci_test_bit() does not modify 2nd parameter, so it is better to let it
be constant, or may cause build warning. The related warning (with
allmodconfig under xtensa):

  net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg':
  net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers]
          &hci_sec_filter.ocf_mask[ogf])) &&
          ^
  net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}'
   static inline int hci_test_bit(int nr, void *addr)
                     ^

hci_test_bit() always treats 2nd parameter is u32, and all callers also
know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *'
instead of 'void *'.

C language treats the array function parameter as a pointer, so the
caller need not use '&' for the 2 demotion array, or it reports warning:
'const unsigned int (*)[4]' is different with 'const unsigned int *'.

I still think you are possibly papering over potential bugs
on big-endian 64 bit systems.

unsigned long vs u32.

How are the bits actually set?

On 2/8/15 03:52, Joe Perches wrote:

On Sat, 2015-02-07 at 21:24 +0800, Chen Gang S wrote:

quoted

hci_test_bit() does not modify 2nd parameter, so it is better to let it
be constant, or may cause build warning. The related warning (with
allmodconfig under xtensa):

  net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg':
  net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers]
          &hci_sec_filter.ocf_mask[ogf])) &&
          ^
  net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}'
   static inline int hci_test_bit(int nr, void *addr)
                     ^

hci_test_bit() always treats 2nd parameter is u32, and all callers also
know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *'
instead of 'void *'.

C language treats the array function parameter as a pointer, so the
caller need not use '&' for the 2 demotion array, or it reports warning:
'const unsigned int (*)[4]' is different with 'const unsigned int *'.

I still think you are possibly papering over potential bugs
on big-endian 64 bit systems.

unsigned long vs u32.

How are the bits actually set?

From current usage of event_mask, "(u32 *) f->event_mask" is only for

event_mask data storage, not for calculation (always as "u32 *" for
calculation).

  [root@localhost linux-next]# grep -rn "\<event_mask\>" include/net/bluetooth net/bluetooth
  include/net/bluetooth/hci_sock.h:51:	unsigned long event_mask[2];
  include/net/bluetooth/hci_sock.h:57:	__u32  event_mask[2];
  net/bluetooth/hci_sock.c:59:	__u32 event_mask[2];
  net/bluetooth/hci_sock.c:110:	if (!hci_test_bit(flt_event, (u32 *)&flt->event_mask))
  net/bluetooth/hci_sock.c:1041:			uf.event_mask[0] = *((u32 *) f->event_mask + 0);
  net/bluetooth/hci_sock.c:1042:			uf.event_mask[1] = *((u32 *) f->event_mask + 1);
  net/bluetooth/hci_sock.c:1053:			uf.event_mask[0] &= *((u32 *) hci_sec_filter.event_mask + 0);
  net/bluetooth/hci_sock.c:1054:			uf.event_mask[1] &= *((u32 *) hci_sec_filter.event_mask + 1);
  net/bluetooth/hci_sock.c:1062:			*((u32 *) f->event_mask + 0) = uf.event_mask[0];
  net/bluetooth/hci_sock.c:1063:			*((u32 *) f->event_mask + 1) = uf.event_mask[1];
  net/bluetooth/hci_sock.c:1124:			uf.event_mask[0] = *((u32 *) f->event_mask + 0);
  net/bluetooth/hci_sock.c:1125:			uf.event_mask[1] = *((u32 *) f->event_mask + 1);

Calculation is machine endian dependency, but event_mask is always as
"u32 *" for calculation, so there is no any type cast for calculation,
it is OK.

Storage is independent from machine endian, but it depends on machine
bits. In our case, 'unsigned long' array has enough space to accept u32
array, so there is no any data overwritten, it is OK.


By the way, I intended to remain event_mask as 'unsigned long' type,
because I am not quite sure whether it is also used by another modules
in kernel (or any other systems). May we change it to u32?

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

On 2/8/15 08:00, Chen Gang S wrote:

On 2/8/15 03:52, Joe Perches wrote:

quoted

On Sat, 2015-02-07 at 21:24 +0800, Chen Gang S wrote:

quoted

hci_test_bit() does not modify 2nd parameter, so it is better to let it
be constant, or may cause build warning. The related warning (with
allmodconfig under xtensa):

  net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg':
  net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers]
          &hci_sec_filter.ocf_mask[ogf])) &&
          ^
  net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}'
   static inline int hci_test_bit(int nr, void *addr)
                     ^

hci_test_bit() always treats 2nd parameter is u32, and all callers also
know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *'
instead of 'void *'.

C language treats the array function parameter as a pointer, so the
caller need not use '&' for the 2 demotion array, or it reports warning:
'const unsigned int (*)[4]' is different with 'const unsigned int *'.

I still think you are possibly papering over potential bugs
on big-endian 64 bit systems.

unsigned long vs u32.

How are the bits actually set?

quoted

From current usage of event_mask, "(u32 *) f->event_mask" is only for

event_mask data storage, not for calculation (always as "u32 *" for
calculation).

  [root@localhost linux-next]# grep -rn "\<event_mask\>" include/net/bluetooth net/bluetooth
  include/net/bluetooth/hci_sock.h:51:	unsigned long event_mask[2];

e.g. use "unsigned char event_mask[2 * sizeof(unsigned long)]" instead
of "unsigned long event_mask[2]".

There is still no any issue within "hci_sock.c" (although I am not sure
whether this modification may cause issues in another modules outside
kernel).


Thanks.

  include/net/bluetooth/hci_sock.h:57:	__u32  event_mask[2];
  net/bluetooth/hci_sock.c:59:	__u32 event_mask[2];
  net/bluetooth/hci_sock.c:110:	if (!hci_test_bit(flt_event, (u32 *)&flt->event_mask))
  net/bluetooth/hci_sock.c:1041:			uf.event_mask[0] = *((u32 *) f->event_mask + 0);
  net/bluetooth/hci_sock.c:1042:			uf.event_mask[1] = *((u32 *) f->event_mask + 1);
  net/bluetooth/hci_sock.c:1053:			uf.event_mask[0] &= *((u32 *) hci_sec_filter.event_mask + 0);
  net/bluetooth/hci_sock.c:1054:			uf.event_mask[1] &= *((u32 *) hci_sec_filter.event_mask + 1);
  net/bluetooth/hci_sock.c:1062:			*((u32 *) f->event_mask + 0) = uf.event_mask[0];
  net/bluetooth/hci_sock.c:1063:			*((u32 *) f->event_mask + 1) = uf.event_mask[1];
  net/bluetooth/hci_sock.c:1124:			uf.event_mask[0] = *((u32 *) f->event_mask + 0);
  net/bluetooth/hci_sock.c:1125:			uf.event_mask[1] = *((u32 *) f->event_mask + 1);

Calculation is machine endian dependency, but event_mask is always as
"u32 *" for calculation, so there is no any type cast for calculation,
it is OK.

Storage is independent from machine endian, but it depends on machine
bits. In our case, 'unsigned long' array has enough space to accept u32
array, so there is no any data overwritten, it is OK.


By the way, I intended to remain event_mask as 'unsigned long' type,
because I am not quite sure whether it is also used by another modules
in kernel (or any other systems). May we change it to u32?

Thanks.

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Hi Chen,

quoted

quoted

quoted

hci_test_bit() does not modify 2nd parameter, so it is better to let it
be constant, or may cause build warning. The related warning (with
allmodconfig under xtensa):

 net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg':
 net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers]
         &hci_sec_filter.ocf_mask[ogf])) &&
         ^
 net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}'
  static inline int hci_test_bit(int nr, void *addr)
                    ^

hci_test_bit() always treats 2nd parameter is u32, and all callers also
know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *'
instead of 'void *'.

C language treats the array function parameter as a pointer, so the
caller need not use '&' for the 2 demotion array, or it reports warning:
'const unsigned int (*)[4]' is different with 'const unsigned int *'.

I still think you are possibly papering over potential bugs
on big-endian 64 bit systems.

unsigned long vs u32.

How are the bits actually set?

quoted

From current usage of event_mask, "(u32 *) f->event_mask" is only for

event_mask data storage, not for calculation (always as "u32 *" for
calculation).

 [root@localhost linux-next]# grep -rn "\<event_mask\>" include/net/bluetooth net/bluetooth
 include/net/bluetooth/hci_sock.h:51:	unsigned long event_mask[2];

e.g. use "unsigned char event_mask[2 * sizeof(unsigned long)]" instead
of "unsigned long event_mask[2]".

There is still no any issue within "hci_sock.c" (although I am not sure
whether this modification may cause issues in another modules outside
kernel).

what about writing a test case for userspace that ensures that things are working correctly. As I said before, we left it this way since it is part of the API.

Regards

Marcel

On 2/9/15 04:17, Marcel Holtmann wrote:

Hi Chen,

quoted

quoted

quoted

quoted

hci_test_bit() does not modify 2nd parameter, so it is better to let it
be constant, or may cause build warning. The related warning (with
allmodconfig under xtensa):

 net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg':
 net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers]
         &hci_sec_filter.ocf_mask[ogf])) &&
         ^
 net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}'
  static inline int hci_test_bit(int nr, void *addr)
                    ^

hci_test_bit() always treats 2nd parameter is u32, and all callers also
know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *'
instead of 'void *'.

C language treats the array function parameter as a pointer, so the
caller need not use '&' for the 2 demotion array, or it reports warning:
'const unsigned int (*)[4]' is different with 'const unsigned int *'.

I still think you are possibly papering over potential bugs
on big-endian 64 bit systems.

unsigned long vs u32.

How are the bits actually set?

quoted

From current usage of event_mask, "(u32 *) f->event_mask" is only for

event_mask data storage, not for calculation (always as "u32 *" for
calculation).

 [root@localhost linux-next]# grep -rn "\<event_mask\>" include/net/bluetooth net/bluetooth
 include/net/bluetooth/hci_sock.h:51:	unsigned long event_mask[2];

e.g. use "unsigned char event_mask[2 * sizeof(unsigned long)]" instead
of "unsigned long event_mask[2]".

There is still no any issue within "hci_sock.c" (although I am not sure
whether this modification may cause issues in another modules outside
kernel).

what about writing a test case for userspace that ensures that things are working correctly. As I said before, we left it this way since it is part of the API.

If it is really the API which can be used outside kernel, what you said
sounds reasonable to me. But I guess, except the related orgnizations/
company/members, most of kernel members can not give a suitable test:

 - It is an API, but we only know kernel part implementation, and we
   also know that the kernel part implementation intends to use
   "unsigned long event_mask[2]" and "u32 *" type cast.

 - We don't know the other part implementation (we event don't know
   whether it is open source). And also it is out of most of kernel
   members' current border (e.g. me).

 - If the other part implementation match what kernel part has done, it
   is OK, else it should cause issue.

So at present, in kernel part, we can only say the original authors
intended to do like this. And only within kernel part, it can not cause
issue. I guess, original authors originally knew what we talk about.

This patch is for fixing building warnings without any negative effect.
And most of us are not the suitable members to continue analyzing. So
at present, for me, we can accept this patch.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

From: Chen Gang
...

So at present, in kernel part, we can only say the original authors
intended to do like this. And only within kernel part, it can not cause
issue. I guess, original authors originally knew what we talk about.

I've just searched for hci_u*filter it is all horrid.
Look at the code for get/set_sockopt of HCI_FILTER.
Someone seems to have done a complete 'bodge job' of fixing the user interface
for apps (32bit and 64bit) on 64bit kernels.

This patch is for fixing building warnings without any negative effect.
And most of us are not the suitable members to continue analyzing. So
at present, for me, we can accept this patch.

And, not uncommonly, it has shown up a 'bag of worms'.

If you change 'hci_filter' to contain u32[2] then you can drop
all of the casts and the temporary structures in the sockopt code.
Just be aware of the tail-padding.

	David

Hi David,

quoted

So at present, in kernel part, we can only say the original authors
intended to do like this. And only within kernel part, it can not cause
issue. I guess, original authors originally knew what we talk about.

I've just searched for hci_u*filter it is all horrid.
Look at the code for get/set_sockopt of HCI_FILTER.
Someone seems to have done a complete 'bodge job' of fixing the user interface
for apps (32bit and 64bit) on 64bit kernels.

we are actually fully aware of that. Just keep in mind that this code is from 2.4.6 kernel and with that most likely 14 years old by now. Unfortunately it ended up as userspace API.

quoted

This patch is for fixing building warnings without any negative effect.
And most of us are not the suitable members to continue analyzing. So
at present, for me, we can accept this patch.

And, not uncommonly, it has shown up a 'bag of worms'.

If you change 'hci_filter' to contain u32[2] then you can drop
all of the casts and the temporary structures in the sockopt code.
Just be aware of the tail-padding.

I am happy to accept patches for this, but we might want to get some unit tests into BlueZ userspace code first to make sure that nothing breaks.

Regards

Marcel

From: Marcel Holtmann

Hi David,

quoted

quoted

So at present, in kernel part, we can only say the original authors
intended to do like this. And only within kernel part, it can not cause
issue. I guess, original authors originally knew what we talk about.

I've just searched for hci_u*filter it is all horrid.
Look at the code for get/set_sockopt of HCI_FILTER.
Someone seems to have done a complete 'bodge job' of fixing the user interface
for apps (32bit and 64bit) on 64bit kernels.

we are actually fully aware of that. Just keep in mind that this code is from 2.4.6 kernel and with
that most likely 14 years old by now. Unfortunately it ended up as userspace API.

AFAICT the userspace API is always __u32[2] the long[2] is never exposed
to userspace (certainly not through the socket option code).

Quite why all the casts were added - instead of changing the type
is probably hidden in the annals of the source repo.
Some serious archaeology might be in order.

	David

On 2/10/15 00:57, David Laight wrote:

From: Marcel Holtmann

quoted

Hi David,

quoted

quoted

So at present, in kernel part, we can only say the original authors
intended to do like this. And only within kernel part, it can not cause
issue. I guess, original authors originally knew what we talk about.

I've just searched for hci_u*filter it is all horrid.
Look at the code for get/set_sockopt of HCI_FILTER.
Someone seems to have done a complete 'bodge job' of fixing the user interface
for apps (32bit and 64bit) on 64bit kernels.

we are actually fully aware of that. Just keep in mind that this code is from 2.4.6 kernel and with
that most likely 14 years old by now. Unfortunately it ended up as userspace API.

AFAICT the userspace API is always __u32[2] the long[2] is never exposed
to userspace (certainly not through the socket option code).

Quite why all the casts were added - instead of changing the type
is probably hidden in the annals of the source repo.
Some serious archaeology might be in order.

For me, I guess what you said above sounds reasonable. But excuse me,
I am not familiar with the related modules outside Linux kernel (at
present, it is out of my border).

I guess, I am not the suitable member to make the related fix patch for
this issue. Welcome any other members to send the fix patch for it.

And for me, if "hci_u*filter" is related with the modules outside kernel
(I guess, it should be), I recommend to put it to the related "uapi"
header.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Hi Chen,

quoted

quoted

quoted

quoted

So at present, in kernel part, we can only say the original authors
intended to do like this. And only within kernel part, it can not cause
issue. I guess, original authors originally knew what we talk about.

I've just searched for hci_u*filter it is all horrid.
Look at the code for get/set_sockopt of HCI_FILTER.
Someone seems to have done a complete 'bodge job' of fixing the user interface
for apps (32bit and 64bit) on 64bit kernels.

we are actually fully aware of that. Just keep in mind that this code is from 2.4.6 kernel and with
that most likely 14 years old by now. Unfortunately it ended up as userspace API.

AFAICT the userspace API is always __u32[2] the long[2] is never exposed
to userspace (certainly not through the socket option code).

Quite why all the casts were added - instead of changing the type
is probably hidden in the annals of the source repo.
Some serious archaeology might be in order.

For me, I guess what you said above sounds reasonable. But excuse me,
I am not familiar with the related modules outside Linux kernel (at
present, it is out of my border).

I guess, I am not the suitable member to make the related fix patch for
this issue. Welcome any other members to send the fix patch for it.

And for me, if "hci_u*filter" is related with the modules outside kernel
(I guess, it should be), I recommend to put it to the related "uapi"
header.

since that is such old code, the "uapi" is essentially what we have as libbluetooth from the BlueZ userspace code.

Regards

Marcel