Re: [PATCH] Bluetooth: hci_event: potential out of bounds parsing ADV events

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 4, 2019 at 1:06 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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.



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux