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 <[email protected]>
---
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:
>> 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:
>> 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?
>>
>
>>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,
>>>> 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];
>
> 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,
>
>>>>> 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];
>>
>> 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,
>> 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.
>> 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,
>
> >> 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
>> Hi David,
>>
>>>> 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,
>>>>> 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