There is a potential out of bounds if "ev->length" is too high or if the
number of reports are too many.
Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
Signed-off-by: Dan Carpenter <[email protected]>
---
Not tested. I suck at pointer math, and I don't know why the protocol
requires a "+ 1". Please review carefully.
net/bluetooth/hci_event.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 609fd6871c5a..ee945b3d12e1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5417,6 +5417,7 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
{
u8 num_reports = skb->data[0];
void *ptr = &skb->data[1];
+ void *end = &skb->data[skb->len];
hci_dev_lock(hdev);
@@ -5425,6 +5426,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
u8 legacy_evt_type;
u16 evt_type;
+ if (ptr + sizeof(*ev) + ev->length + 1 > end)
+ break;
evt_type = __le16_to_cpu(ev->evt_type);
legacy_evt_type = ext_evt_type_to_legacy(evt_type);
if (legacy_evt_type != LE_ADV_INVALID) {
--
2.17.1
Hi Dan,
On 3/30/19 8:25 AM, Dan Carpenter wrote:
> There is a potential out of bounds if "ev->length" is too high or if the
> number of reports are too many.
>
> Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
> Signed-off-by: Dan Carpenter <[email protected]>
Reviewed-By: Tomas Bortoli <[email protected]>
> ---
> Not tested. I suck at pointer math, and I don't know why the protocol
> requires a "+ 1". Please review carefully.
>
> net/bluetooth/hci_event.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 609fd6871c5a..ee945b3d12e1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5417,6 +5417,7 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> u8 num_reports = skb->data[0];
> void *ptr = &skb->data[1];
> + void *end = &skb->data[skb->len];
>
> hci_dev_lock(hdev);
>
> @@ -5425,6 +5426,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> u8 legacy_evt_type;
> u16 evt_type;
>
> + if (ptr + sizeof(*ev) + ev->length + 1 > end)
> + break;
Assuming that per each iteration, the while cycle, including the call to
process_adv_report() read up to (sizeof(*ev) + ev->length + 1) bytes,
(I don't understand why the +1, but, anyway..)
If the assumption is correct, then the if condition should be:
if (ptr + sizeof(*ev) + ev->length + 1 >= end)
Because as soon as we try to read from the `end` pointer on, we are
out-of-bound.. the valid skb bytes end at `end-1` (included)
Note that also hci_le_adv_report_evt() is likely to need the same fix!
> evt_type = __le16_to_cpu(ev->evt_type);
> legacy_evt_type = ext_evt_type_to_legacy(evt_type);
> if (legacy_evt_type != LE_ADV_INVALID) {
>
Kind regards,
Tomas
Hi,
sorry for the multiple emails but I have checked again the code and
looks like process_adv_report() reads from ev->data for a size of
ev->length.
I attach a patch that applies the bound check to both
hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().
Let me know your opinion,
Best regards,
Tomas
On 3/30/19 10:20 AM, Tomas Bortoli wrote:
> Hi Dan,
>
> On 3/30/19 8:25 AM, Dan Carpenter wrote:
>> There is a potential out of bounds if "ev->length" is too high or if the
>> number of reports are too many.
>>
>> Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
>> Signed-off-by: Dan Carpenter <[email protected]>
> Reviewed-By: Tomas Bortoli <[email protected]>
>
>> ---
>> Not tested. I suck at pointer math, and I don't know why the protocol
>> requires a "+ 1". Please review carefully.
>>
>> net/bluetooth/hci_event.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 609fd6871c5a..ee945b3d12e1 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -5417,6 +5417,7 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
>> {
>> u8 num_reports = skb->data[0];
>> void *ptr = &skb->data[1];
>> + void *end = &skb->data[skb->len];
>>
>> hci_dev_lock(hdev);
>>
>> @@ -5425,6 +5426,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
>> u8 legacy_evt_type;
>> u16 evt_type;
>>
>> + if (ptr + sizeof(*ev) + ev->length + 1 > end)
>> + break;
>
> Assuming that per each iteration, the while cycle, including the call to
> process_adv_report() read up to (sizeof(*ev) + ev->length + 1) bytes,
>
> (I don't understand why the +1, but, anyway..)
>
> If the assumption is correct, then the if condition should be:
>
> if (ptr + sizeof(*ev) + ev->length + 1 >= end)
>
> Because as soon as we try to read from the `end` pointer on, we are
> out-of-bound.. the valid skb bytes end at `end-1` (included)
>
> Note that also hci_le_adv_report_evt() is likely to need the same fix!
>
>
>> evt_type = __le16_to_cpu(ev->evt_type);
>> legacy_evt_type = ext_evt_type_to_legacy(evt_type);
>> if (legacy_evt_type != LE_ADV_INVALID) {
>>
>
>
> Kind regards,
> Tomas
>
On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
> Hi,
>
> sorry for the multiple emails but I have checked again the code and
> looks like process_adv_report() reads from ev->data for a size of
> ev->length.
>
> I attach a patch that applies the bound check to both
> hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().
>
You're right that both need to be fixed.
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 609fd6871c5a..275926e0753e 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5345,6 +5345,7 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> u8 num_reports = skb->data[0];
> void *ptr = &skb->data[1];
> + u8 *end = &skb->data[skb->len - 1];
^^^^^^^^^^^^
>
> hci_dev_lock(hdev);
>
> @@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> struct hci_ev_le_advertising_info *ev = ptr;
> s8 rssi;
>
> + if (ev->data + ev->length > end)
No, this isn't right. You've removed the + 1 and you've introduced an
additional "sbk->len - 1" so we're off by two... The test is supposed
to be:
start + length_read > start + length_of_buffer
So the end has to be &skb->data[skb->len]. The "+ 1" comes from later
in the function when we do:
ptr += sizeof(*ev) + ev->length + 1;
^^^^
I don't where the "+ 1" comes from, but I know the condition and the
increment should match. We could use ev->data instead of
"ptr + sizeof(*ev)" but to me, because the mysterious "+ 1" then it
seems more readable to match the increment exactly...
regards,
dan carpenter
On 4/1/19 8:32 AM, Dan Carpenter wrote:
> On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
>> Hi,
>>
>> sorry for the multiple emails but I have checked again the code and
>> looks like process_adv_report() reads from ev->data for a size of
>> ev->length.
>>
>> I attach a patch that applies the bound check to both
>> hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().
>>
>
> You're right that both need to be fixed.
>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 609fd6871c5a..275926e0753e 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -5345,6 +5345,7 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
>> {
>> u8 num_reports = skb->data[0];
>> void *ptr = &skb->data[1];
>> + u8 *end = &skb->data[skb->len - 1];
> ^^^^^^^^^^^^
>>
>> hci_dev_lock(hdev);
>>
>> @@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
>> struct hci_ev_le_advertising_info *ev = ptr;
>> s8 rssi;
>>
>> + if (ev->data + ev->length > end)
>
> No, this isn't right. You've removed the + 1 and you've introduced an
> additional "sbk->len - 1" so we're off by two... The test is supposed
> to be:
>
> start + length_read > start + length_of_buffer
>
afaict: ev->data = start and length_read = ev->length
and the right side of the condition is the upper limit. "end" as defined
in my patch is the last readable byte of skb->data (or am I wrong on
this too?)
> So the end has to be &skb->data[skb->len]. The "+ 1" comes from later
> in the function when we do:
>
> ptr += sizeof(*ev) + ev->length + 1;
> ^^^^
>
> I don't where the "+ 1" comes from, but I know the condition and the
> increment should match. We could use ev->data instead of
> "ptr + sizeof(*ev)" but to me, because the mysterious "+ 1" then it
> seems more readable to match the increment exactly...
We really have to first understand why there is that + 1 there. I agree
that the condition and the increment should match, otherwise or there is
a mistake in the error condition or the increment just skips 1 byte, not
reading the last per each cycle, for no reason (very unlikely).
Reading process_adv_report() I spotted some memcpy() and other reads of
the memory area that begins at data (ev->data) and ends at (ev->data +
length).
Could anybody clarify the logic of that inc ?
Cheers,
Tomas
On Mon, Apr 01, 2019 at 07:24:47PM +0200, Tomas Bortoli wrote:
> On 4/1/19 8:32 AM, Dan Carpenter wrote:
> > On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
> >> Hi,
> >>
> >> sorry for the multiple emails but I have checked again the code and
> >> looks like process_adv_report() reads from ev->data for a size of
> >> ev->length.
> >>
> >> I attach a patch that applies the bound check to both
> >> hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().
> >>
> >
> > You're right that both need to be fixed.
> >
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index 609fd6871c5a..275926e0753e 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -5345,6 +5345,7 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >> {
> >> u8 num_reports = skb->data[0];
> >> void *ptr = &skb->data[1];
> >> + u8 *end = &skb->data[skb->len - 1];
> > ^^^^^^^^^^^^
> >>
> >> hci_dev_lock(hdev);
> >>
> >> @@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >> struct hci_ev_le_advertising_info *ev = ptr;
> >> s8 rssi;
> >>
> >> + if (ev->data + ev->length > end)
> >
> > No, this isn't right. You've removed the + 1 and you've introduced an
> > additional "sbk->len - 1" so we're off by two... The test is supposed
> > to be:
> >
> > start + length_read > start + length_of_buffer
> >
>
> afaict: ev->data = start and length_read = ev->length
> and the right side of the condition is the upper limit. "end" as defined
> in my patch is the last readable byte of skb->data (or am I wrong on
> this too?)
>
You have:
ptr + length > &skb->data[skb->len - 1];
Imagine we "ptr = &skb->data[skb->len - 1]" that means we can read one
more byte. But in that case "if (ptr + 1 > &skb->data[skb->len - 1])"
is true so we break instead of allowing the read. Idiomatically > is
for length and >= is for indexes...
Btw, unrelated but in hci_le_adv_report_evt() if we hit the
HCI_MAX_AD_LENGTH condition we should just break as well. Everything
after that is going to be guess work and garbage. No point in trying to
parse it. IOW:
if (ptr + sizeof(*ev) + ev->length + 1 > end ||
ev->length > HCI_MAX_AD_LENGTH)
break;
I was planning to resend my patch and the fixes to hci_le_adv_report_evt()
with your Reported-by tonight as two separate patches. It just makes it
easier to backport if we have a different Fixes tag for both functions.
But then I decided to wait until tomorrow to see if anyone knew what the
+ 1 was about...
regards,
dan carpenter
Hi,
On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <[email protected]> wrote:
>
> Hi Dan,
>
> On 3/30/19 8:25 AM, Dan Carpenter wrote:
> > There is a potential out of bounds if "ev->length" is too high or if the
> > number of reports are too many.
> >
> > Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
> > Signed-off-by: Dan Carpenter <[email protected]>
> Reviewed-By: Tomas Bortoli <[email protected]>
I sent a patchset to fix all of this kind of OOB:
https://marc.info/?l=linux-netdev&m=155314874622831&w=2
Unfortunately I get no response...
Does any of you mind to look at them?
Thanks.
On Mon, Apr 01, 2019 at 11:03:53AM -0700, Cong Wang wrote:
> Hi,
>
> On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <[email protected]> wrote:
> >
> > Hi Dan,
> >
> > On 3/30/19 8:25 AM, Dan Carpenter wrote:
> > > There is a potential out of bounds if "ev->length" is too high or if the
> > > number of reports are too many.
> > >
> > > Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
> > > Signed-off-by: Dan Carpenter <[email protected]>
> > Reviewed-By: Tomas Bortoli <[email protected]>
>
> I sent a patchset to fix all of this kind of OOB:
> https://marc.info/?l=linux-netdev&m=155314874622831&w=2
>
> Unfortunately I get no response...
>
> Does any of you mind to look at them?
>
I don't know the rules... When is it ok say:
if (skb->len < sizeof(*ev))
return;
and when must we say:
if (!pskb_may_pull(skb, sizeof(*ev)))
return;
Btw, get rid of all the likely/unlikely() macros. Then the other style
comment would be don't move the "ev = (void *)skb->data;" assignments
around. It's ok to say:
struct hci_ev_pin_code_req *ev = (void *)skb->data;
struct hci_conn *conn;
if (!pskb_may_pull(skb, sizeof(*ev)))
return;
regards,
dan carpenter
On Mon, Apr 1, 2019 at 11:33 PM Dan Carpenter <[email protected]> wrote:
>
> On Mon, Apr 01, 2019 at 11:03:53AM -0700, Cong Wang wrote:
> > Hi,
> >
> > On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <[email protected]> wrote:
> > >
> > > Hi Dan,
> > >
> > > On 3/30/19 8:25 AM, Dan Carpenter wrote:
> > > > There is a potential out of bounds if "ev->length" is too high or if the
> > > > number of reports are too many.
> > > >
> > > > Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
> > > > Signed-off-by: Dan Carpenter <[email protected]>
> > > Reviewed-By: Tomas Bortoli <[email protected]>
> >
> > I sent a patchset to fix all of this kind of OOB:
> > https://marc.info/?l=linux-netdev&m=155314874622831&w=2
> >
> > Unfortunately I get no response...
> >
> > Does any of you mind to look at them?
> >
>
> I don't know the rules... When is it ok say:
>
> if (skb->len < sizeof(*ev))
> return;
>
> and when must we say:
>
> if (!pskb_may_pull(skb, sizeof(*ev)))
> return;
The rule is simple: pskb_may_pull() always knows better. In bluetooth
case, they are actually equivalent, as the skb's in bluetooth are linear.
>
> Btw, get rid of all the likely/unlikely() macros. Then the other style
> comment would be don't move the "ev = (void *)skb->data;" assignments
> around. It's ok to say:
Similarly, pskb_may_pull() may reallocate skb's, although very unlikely
for bluetooth case (skb's are linear). At least it doesn't harm anything
we move the skb->data dereference after pskb_may_pull().
I think these likely()/unlikely() are reasonable, ill-formatted packets
are rare cases, normal packets deserve such a special care. We
use likely()/unlikely() with pskb_may_pull() in many places in
networking subsystem, at least.
Anyway, if you don't mind, I can resend my patchset with you Cc'ed.
Thanks.
Hi Cong,
On 4/2/19 7:42 PM, Cong Wang wrote:
> On Mon, Apr 1, 2019 at 11:33 PM Dan Carpenter <[email protected]> wrote:
>>
>> On Mon, Apr 01, 2019 at 11:03:53AM -0700, Cong Wang wrote:
>>> Hi,
>>>
>>> On Sat, Mar 30, 2019 at 2:23 AM Tomas Bortoli <[email protected]> wrote:
>>>>
>>>> Hi Dan,
>>>>
>>>> On 3/30/19 8:25 AM, Dan Carpenter wrote:
>>>>> There is a potential out of bounds if "ev->length" is too high or if the
>>>>> number of reports are too many.
>>>>>
>>>>> Fixes: c215e9397b00 ("Bluetooth: Process extended ADV report event")
>>>>> Signed-off-by: Dan Carpenter <[email protected]>
>>>> Reviewed-By: Tomas Bortoli <[email protected]>
>>>
>>> I sent a patchset to fix all of this kind of OOB:
>>> https://marc.info/?l=linux-netdev&m=155314874622831&w=2
Reviewed-by: Tomas Bortoli <[email protected]>
All 3 looks good to me, nice work! Overall, it seems that with these 3
all the RX paths in hci_event.c are bound checked.
>>>
>>> Unfortunately I get no response...
>>>
>>> Does any of you mind to look at them?
>>>
>>
>> I don't know the rules... When is it ok say:
>>
>> if (skb->len < sizeof(*ev))
>> return;
>>
>> and when must we say:
>>
>> if (!pskb_may_pull(skb, sizeof(*ev)))
>> return;
>
>
> The rule is simple: pskb_may_pull() always knows better. In bluetooth
> case, they are actually equivalent, as the skb's in bluetooth are linear.
>
>
>>
>> Btw, get rid of all the likely/unlikely() macros. Then the other style
>> comment would be don't move the "ev = (void *)skb->data;" assignments
>> around. It's ok to say:
>
>
> Similarly, pskb_may_pull() may reallocate skb's, although very unlikely
> for bluetooth case (skb's are linear). At least it doesn't harm anything
> we move the skb->data dereference after pskb_may_pull().
>
> I think these likely()/unlikely() are reasonable, ill-formatted packets
> are rare cases, normal packets deserve such a special care. We
> use likely()/unlikely() with pskb_may_pull() in many places in
> networking subsystem, at least.
>
> Anyway, if you don't mind, I can resend my patchset with you Cc'ed.
>
> Thanks.
>
On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > Btw, get rid of all the likely/unlikely() macros. Then the other style
> > comment would be don't move the "ev = (void *)skb->data;" assignments
> > around. It's ok to say:
>
>
> Similarly, pskb_may_pull() may reallocate skb's, although very unlikely
> for bluetooth case (skb's are linear). At least it doesn't harm anything
> we move the skb->data dereference after pskb_may_pull().
>
It harms readability.
regards,
dan carpenter
On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> I think these likely()/unlikely() are reasonable, ill-formatted packets
> are rare cases, normal packets deserve such a special care. We
> use likely()/unlikely() with pskb_may_pull() in many places in
> networking subsystem, at least.
The likely()/unlikely() annotations are to help the compiler optimize
the fast path. They are not there just for decorating the code. We
should only use likely()/unlikely() where it makes a difference in
benchmarking. Otherwise it's just a style question, right (obviously)?
And it's better style to write things as simply as possible.
regards,
dan carpenter
Hi,
On Mon, Apr 1, 2019 at 10:54 PM Tomas Bortoli <[email protected]> wrote:
>
> On 4/1/19 8:32 AM, Dan Carpenter wrote:
> > On Sat, Mar 30, 2019 at 11:44:29PM +0100, Tomas Bortoli wrote:
> >> Hi,
> >>
> >> sorry for the multiple emails but I have checked again the code and
> >> looks like process_adv_report() reads from ev->data for a size of
> >> ev->length.
> >>
> >> I attach a patch that applies the bound check to both
> >> hci_le_ext_adv_report_evt() and hci_le_adv_report_evt().
> >>
> >
> > You're right that both need to be fixed.
> >
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index 609fd6871c5a..275926e0753e 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -5345,6 +5345,7 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >> {
> >> u8 num_reports = skb->data[0];
> >> void *ptr = &skb->data[1];
> >> + u8 *end = &skb->data[skb->len - 1];
> > ^^^^^^^^^^^^
> >>
> >> hci_dev_lock(hdev);
> >>
> >> @@ -5352,6 +5353,9 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >> struct hci_ev_le_advertising_info *ev = ptr;
> >> s8 rssi;
> >>
> >> + if (ev->data + ev->length > end)
> >
> > No, this isn't right. You've removed the + 1 and you've introduced an
> > additional "sbk->len - 1" so we're off by two... The test is supposed
> > to be:
> >
> > start + length_read > start + length_of_buffer
> >
>
> afaict: ev->data = start and length_read = ev->length
> and the right side of the condition is the upper limit. "end" as defined
> in my patch is the last readable byte of skb->data (or am I wrong on
> this too?)
>
> > So the end has to be &skb->data[skb->len]. The "+ 1" comes from later
> > in the function when we do:
> >
> > ptr += sizeof(*ev) + ev->length + 1;
> > ^^^^
> >
> > I don't where the "+ 1" comes from, but I know the condition and the
> > increment should match. We could use ev->data instead of
> > "ptr + sizeof(*ev)" but to me, because the mysterious "+ 1" then it
> > seems more readable to match the increment exactly...
>
> We really have to first understand why there is that + 1 there. I agree
> that the condition and the increment should match, otherwise or there is
> a mistake in the error condition or the increment just skips 1 byte, not
> reading the last per each cycle, for no reason (very unlikely).
>
> Reading process_adv_report() I spotted some memcpy() and other reads of
> the memory area that begins at data (ev->data) and ends at (ev->data +
> length).
>
> Could anybody clarify the logic of that inc ?
>
"+ 1" is required in adv_report_evt since there is one more field
"rssi" after data
so you need + 1 to point to next report, where as it is not required
in ext_adv_report_evt
since rssi is present before data. I have already raised a patch to
fix it the in ML.
Thanks,
Jaganath
On Tue, Apr 2, 2019 at 1:15 PM Dan Carpenter <[email protected]> wrote:
>
> On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > > Btw, get rid of all the likely/unlikely() macros. Then the other style
> > > comment would be don't move the "ev = (void *)skb->data;" assignments
> > > around. It's ok to say:
> >
> >
> > Similarly, pskb_may_pull() may reallocate skb's, although very unlikely
> > for bluetooth case (skb's are linear). At least it doesn't harm anything
> > we move the skb->data dereference after pskb_may_pull().
> >
>
> It harms readability.
Why? I can't see how it harms readability if you have pskb_may_pull()
in mind that it potentially reallocates skb->data.
On Tue, Apr 2, 2019 at 1:51 PM Dan Carpenter <[email protected]> wrote:
>
> On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > I think these likely()/unlikely() are reasonable, ill-formatted packets
> > are rare cases, normal packets deserve such a special care. We
> > use likely()/unlikely() with pskb_may_pull() in many places in
> > networking subsystem, at least.
>
> The likely()/unlikely() annotations are to help the compiler optimize
> the fast path. They are not there just for decorating the code. We
> should only use likely()/unlikely() where it makes a difference in
> benchmarking. Otherwise it's just a style question, right (obviously)?
That is not a requirement.
Unless you have a strong argument to believe likely()/unlikely()
doesn't help in this specific case (ill-formatted packets), we should
by default use it.
Coding style is not a strong argument, it is purely a taste. At least,
does CodingStyle forbid to use it in this case? I tried checkpatch.pl,
it has no such a complain.
Thanks.
On Wed, Apr 03, 2019 at 03:51:18PM -0700, Cong Wang wrote:
> On Tue, Apr 2, 2019 at 1:15 PM Dan Carpenter <[email protected]> wrote:
> >
> > On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > > > Btw, get rid of all the likely/unlikely() macros. Then the other style
> > > > comment would be don't move the "ev = (void *)skb->data;" assignments
> > > > around. It's ok to say:
> > >
> > >
> > > Similarly, pskb_may_pull() may reallocate skb's, although very unlikely
> > > for bluetooth case (skb's are linear). At least it doesn't harm anything
> > > we move the skb->data dereference after pskb_may_pull().
> > >
> >
> > It harms readability.
>
> Why? I can't see how it harms readability if you have pskb_may_pull()
> in mind that it potentially reallocates skb->data.
You're making the code more complicated because you're pretending that
we didn't linearize the skb data already... :/
regards,
dan carpenter
On Wed, Apr 03, 2019 at 03:55:20PM -0700, Cong Wang wrote:
> I tried checkpatch.pl, it has no such a complain.
Huh?
I sorry, I must have been very unclear if you're asking about
checkpatch. Checkpatch is a Perl script. It's basically like grep. It
has no idea about fast paths or benchmarking. Let me try explain
better.
The likely/unlikely annotations are there to help optimize branch
prediction and make the code run faster. In the real world if you can't
benchmark or measure or detect a speed improvement then it's not a real
speed improvement.
1) HCI events don't happen often enough to where the speed can be easily
benchmarked. In that situation, we just write the code as readably
as possible instead of trying to micro optimize it.
2) The compiler already does common sense branch prediction. These
conditions look straight forward so it probably gets it right. You
should take a look at the object code. The compiler probably gets
it right. I bet that these annotations don't even affect the
compiled code let alone the benchmarking output.
So in this case, we are not adding likely and unlikely for their
intended purpose of improving benchmarks. I guess we are instead adding
them just for the human readers? But most people can immediately see
that this is an error path so they don't need the annotations. In fact
the annotations obscure what the error condition is so they hurt
readability. Now there are just too many parentheses to keep track of.
if (unlikely(!pskb_may_pull(skb, sizeof(*rp))))
return;
if (!pskb_may_pull(skb, sizeof(*rp)))
return;
I know this isn't explained in CodingStyle but some things are
assumed. In drivers/ about 1.5% of if statements have likely/unlikely
annotations. It really is not normal to add it to everything willy-nilly
for no reason.
regards,
dan carpenter
On Wed, Apr 3, 2019 at 11:35 PM Dan Carpenter <[email protected]> wrote:
>
> On Wed, Apr 03, 2019 at 03:51:18PM -0700, Cong Wang wrote:
> > On Tue, Apr 2, 2019 at 1:15 PM Dan Carpenter <[email protected]> wrote:
> > >
> > > On Tue, Apr 02, 2019 at 10:42:38AM -0700, Cong Wang wrote:
> > > > > Btw, get rid of all the likely/unlikely() macros. Then the other style
> > > > > comment would be don't move the "ev = (void *)skb->data;" assignments
> > > > > around. It's ok to say:
> > > >
> > > >
> > > > Similarly, pskb_may_pull() may reallocate skb's, although very unlikely
> > > > for bluetooth case (skb's are linear). At least it doesn't harm anything
> > > > we move the skb->data dereference after pskb_may_pull().
> > > >
> > >
> > > It harms readability.
> >
> > Why? I can't see how it harms readability if you have pskb_may_pull()
> > in mind that it potentially reallocates skb->data.
>
> You're making the code more complicated because you're pretending that
> we didn't linearize the skb data already... :/
I am not pretending it, I just want to use a standard networking API
which covers all cases.
On Thu, Apr 4, 2019 at 1:06 AM Dan Carpenter <[email protected]> wrote:
>
> On Wed, Apr 03, 2019 at 03:55:20PM -0700, Cong Wang wrote:
> > I tried checkpatch.pl, it has no such a complain.
>
> Huh?
Huh +1?
>
> I sorry, I must have been very unclear if you're asking about
> checkpatch. Checkpatch is a Perl script. It's basically like grep. It
> has no idea about fast paths or benchmarking. Let me try explain
> better.
Sure, you said coding style, then I brought up checkpatch.pl.
If it hurts you, then I am sorry. How do you check your coding
style without checkpatch.pl? I am happy to learn. Personally I
just trust checkpatch.pl, because I don't want to waste my time
on coding styles, I never say I like it or not, I just need a tool.
Thanks for teaching me what checkpatch.pl is, although I have
been using it for so many years, it is really really helpful.
>
> The likely/unlikely annotations are there to help optimize branch
> prediction and make the code run faster. In the real world if you can't
> benchmark or measure or detect a speed improvement then it's not a real
> speed improvement.
Personally I disagree with you on this point. You don't have to measure,
you just have to understand the code. In this case, bluetooth has linear
and sane skb's in _normal_ case, you don't disagree, do you?
So even _if_ likely()/unlikely() is only for telling which is the normal
case, it is still helpful. In this case, we want to tell non-linear skb's are
unlikely in bluetooth, you know this is true, as linear is always the case;
and, we want to tell ill-formatted skb's are unlikely too, and you know
this is also true.
>
> 1) HCI events don't happen often enough to where the speed can be easily
> benchmarked. In that situation, we just write the code as readably
> as possible instead of trying to micro optimize it.
Same. It is not about benchmarking.
>
> 2) The compiler already does common sense branch prediction. These
> conditions look straight forward so it probably gets it right. You
I'd be surprised if the compiler could know skb's are always linear
in this particular case.
> should take a look at the object code. The compiler probably gets
> it right. I bet that these annotations don't even affect the
> compiled code let alone the benchmarking output.
If you are suggesting we should remove all likely()/unlikely()
from pskb_may_pull() callers, please go ahead to submit a patch
to remove them all, and see if David accepts it.
>
> I know this isn't explained in CodingStyle but some things are
> assumed. In drivers/ about 1.5% of if statements have likely/unlikely
> annotations. It really is not normal to add it to everything willy-nilly
> for no reason.
I don't know why you want to make the topic as broad as all
likely()/unlikely(), we are discussing likely()/unlikely() with
pskb_may_pull() together, and particularly in bluetooth.
Thanks.
I'm don't have a lot of time for these discussions. The object code is
exactly the same regardless of whether you add the annotations or not.
32c307d27583a624baa3a24eec00402e net/bluetooth/hci_event.o.no_annotation
32c307d27583a624baa3a24eec00402e net/bluetooth/hci_event.o.w_annotation
regards,
dan carpenter
I disagree,
they differ for me.
with unlikely:
f86cf90c9c1ef14063796f4d07ce64bf6952b536e9728d21ba86d3be2e7472d7
net/bluetooth/hci_event.o
without:
6c75194b981294cb933900100448ec335065a35ff9120333558d09aa3dd8b2da
net/bluetooth/hci_event.o
On 4/5/19 10:48 PM, Dan Carpenter wrote:
>
> I'm don't have a lot of time for these discussions. The object code is
> exactly the same regardless of whether you add the annotations or not.
>
> 32c307d27583a624baa3a24eec00402e net/bluetooth/hci_event.o.no_annotation
> 32c307d27583a624baa3a24eec00402e net/bluetooth/hci_event.o.w_annotation
>
> regards,
> dan carpenter
>
On Fri, Apr 05, 2019 at 11:05:53PM +0200, Tomas Bortoli wrote:
> I disagree,
>
> they differ for me.
>
> with unlikely:
> f86cf90c9c1ef14063796f4d07ce64bf6952b536e9728d21ba86d3be2e7472d7
> net/bluetooth/hci_event.o
>
> without:
> 6c75194b981294cb933900100448ec335065a35ff9120333558d09aa3dd8b2da
> net/bluetooth/hci_event.o
>
Are you sure you didn't change any line numbers or something?
regards,
dan carpenter
I only deleted one unlikely() from around an unlikely(!pskb_may_pull())
check. I made sure that the line numbers and debug symbols all stayed
exactly the same... I just re-ran my experiment with the same results.
It's weird that you're getting different object code. This stuff isn't
a new feature in GCC, it's at least 10 years old.
regards,
dan carpenter