linux-kernel - Re: [PATCH v3 04/13] recordmcount: Rewrite error/success handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <B522B4E8-B107-4763-AA39-FA71A193C02C@vmware.com>
Date:   Fri, 26 Jul 2019 19:23:15 +0000
From:   Matt Helsley <mhelsley@...are.com>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v3 04/13] recordmcount: Rewrite error/success handling



> On Jul 26, 2019, at 11:43 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> On Fri, 26 Jul 2019 18:37:11 +0000
> Matt Helsley <mhelsley@...are.com> wrote:
> 
>>>> diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
>>>> index c1e1b04b4871..909a3e4775c2 100644
>>>> --- a/scripts/recordmcount.h
>>>> +++ b/scripts/recordmcount.h
>>>> @@ -24,7 +24,9 @@
>>>> #undef mcount_adjust
>>>> #undef sift_rel_mcount
>>>> #undef nop_mcount
>>>> +#undef missing_sym
>>>> #undef find_secsym_ndx
>>>> +#undef already_has_rel_mcount  
>>> 
>>> Why do we need these as defines? Can't you just have a single:
>>> 
>>> const char *already_has_mcount = "success";
>>> 
>>> in recordmcount.c before recordmcount.h is included?
>>> 
>>> And same for missing_sym.  
>> 
>> Yes, that’s a good point. I’ve been trying to separate the changes to
>> the functions from moving parts out but in this case it would make
>> just as much sense to add them to recordmcount.c in the first place.
>> 
>> Ultimately, this ugliness gets removed as the next series removes
>> recordmcount.h entirely and one of the steps is moving
>> find_secsym_ndx() out while eliminating these redundant pieces.
> 
> Yeah, this code will be cleaned up later, but let's have the steps in
> between look fine as well.

Makes sense.

> 
> 
>> 
>>> 
>>> Another, probably more robust way of doing this, is change
>>> find_secsym_ndx() to return 0 on success and -1 on missing symbol,
>>> and just pass a pointer by reference to fill the recsym (which
>>> doesn't have to be a constant).  
>> 
>> That’s easy enough to do  and  I do like separating the error/success
>> return from returning  the index. I can send that out now or tack it
>> onto the next RFC series I’m about to send which completes the
>> conversion if that’s preferable.
>> 
>> Yeah, the original code applies “const” in lots of places -- I
>> presume it’s an attempt to eek out every last bit of performance from
>> the compiler.
> 
> As I said before, I've applied patches 1-3, so you don't need to resend
> them. I finished looking at the rest, and only this patch needs to be
> fixed, and since you are resending, could you fix the "upside-down
> x-mas" tree declaration I mentioned in patch 8.

Will do.

Cheers,
      -Matt