On 11/07/2017 04:42 PM, Y Song wrote:
> On Tue, Nov 7, 2017 at 2:04 PM, Alexei Starovoitov <[email protected]> wrote:
>> On 11/8/17 6:47 AM, Y Song wrote:
>>>
>>> On Tue, Nov 7, 2017 at 1:39 PM, Alexei Starovoitov <[email protected]> wrote:
>>>>
>>>> On 11/8/17 6:14 AM, Y Song wrote:
>>>>>
>>>>>
>>>>> On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>> Alexei Starovoitov wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 11/7/17 12:55 AM, Naveen N. Rao wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I thought such struct shouldn't change layout.
>>>>>>>>> If it is we need to fix include/linux/compiler-clang.h to do that
>>>>>>>>> anon struct as well.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> We considered that, but it looked to be very dependent on the version
>>>>>>>> of
>>>>>>>> gcc used to build the kernel. But, this may be a simpler approach for
>>>>>>>> the shorter term.
>>>>>>>>
>>>>>>>
>>>>>>> why it would depend on version of gcc?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> From what I can see, randomized_struct_fields_start is defined only for
>>>>>> gcc
>>>>>>>
>>>>>>>
>>>>>>> = 4.6. For older versions, it does not get mapped to an anonymous
>>>>>>
>>>>>>
>>>>>> structure. We may not care for older gcc versions, but..
>>>>>>
>>>>>> The other issue was that __randomize_layout maps to __designated_init
>>>>>> when
>>>>>> randstruct plugin is not enabled, which is in turn an attribute on gcc
>>>>>>> =
>>>>>> v5.1, but not otherwise.
>>>>>>
>>>>>>> We just need this, no?
>>>>>>>
>>>>>>> diff --git a/include/linux/compiler-clang.h
>>>>>>> b/include/linux/compiler-clang.h
>>>>>>> index de179993e039..4e29ab6187cb 100644
>>>>>>> --- a/include/linux/compiler-clang.h
>>>>>>> +++ b/include/linux/compiler-clang.h
>>>>>>> @@ -15,3 +15,6 @@
>>>>>>> * with any version that can compile the kernel
>>>>>>> */
>>>>>>> #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
>>>>>>> __COUNTER__)
>>>>>>> +
>>>>>>> +#define randomized_struct_fields_start struct {
>>>>>>> +#define randomized_struct_fields_end };
>>>>>>>
>>>>>>> since offsets are mandated by C standard.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Yes, this is what we're testing with and is probably sufficient for our
>>>>>> purposes.
>>>>>
>>>>>
>>>>>
>>>>> Just tested this with bcc. bcc actually complains. the rewriter
>>>>> is not able to rewrite prev->pid where prev is "struct task_struct
>>>>> *prev".
>>>>> I will change bcc rewriter to see whether the field value is correct or
>>>>> not.
>>>>>
>>>>> Not sure my understanding is correct or not, but I am afraid that
>>>>> the above approach for clang compiler change may not work.
>>>>> If clang calculates the field offset based on header file, the offset
>>>>> may not be the same as kernel one....
>>>>
>>>>
>>>>
>>>> why is that?
>>>> When randomization is off both gcc and clang must generate the same
>>>> offsets, since it's C standard.
>>>
>>>
>>> The patch changed compiler-clang.h, so gcc still do randomization.
>>
>>
>> gcc_plugins are off by default and randomization will not be
>> turned on for any sane distro or datacenter that cares about
>> performance and stability.
>> So imo above compiler-clang.h patch together with bcc fix would
>> be enough.
>
> Agree that short time the suggested fix should be enough.
> Long time, disto could become "insane" someday :-)
>
Yup it works with clang compiler & bcc hack. Thanks :).
I verified it with task_switch.py & runqlat.py.
Are you going to push the bcc hacks to github repo ?
Regards,
Atish
From 1583482646103147589@xxx Wed Nov 08 07:38:50 +0000 2017
X-GM-THRID: 1583027203607239623
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Tue, Nov 7, 2017 at 4:29 PM, Atish Patra <[email protected]> wrote:
> On 11/07/2017 04:42 PM, Y Song wrote:
>>
>> On Tue, Nov 7, 2017 at 2:04 PM, Alexei Starovoitov <[email protected]> wrote:
>>>
>>> On 11/8/17 6:47 AM, Y Song wrote:
>>>>
>>>>
>>>> On Tue, Nov 7, 2017 at 1:39 PM, Alexei Starovoitov <[email protected]> wrote:
>>>>>
>>>>>
>>>>> On 11/8/17 6:14 AM, Y Song wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Alexei Starovoitov wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/7/17 12:55 AM, Naveen N. Rao wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I thought such struct shouldn't change layout.
>>>>>>>>>> If it is we need to fix include/linux/compiler-clang.h to do that
>>>>>>>>>> anon struct as well.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> We considered that, but it looked to be very dependent on the
>>>>>>>>> version
>>>>>>>>> of
>>>>>>>>> gcc used to build the kernel. But, this may be a simpler approach
>>>>>>>>> for
>>>>>>>>> the shorter term.
>>>>>>>>>
>>>>>>>>
>>>>>>>> why it would depend on version of gcc?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From what I can see, randomized_struct_fields_start is defined only
>>>>>>> for
>>>>>>> gcc
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> = 4.6. For older versions, it does not get mapped to an anonymous
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> structure. We may not care for older gcc versions, but..
>>>>>>>
>>>>>>> The other issue was that __randomize_layout maps to __designated_init
>>>>>>> when
>>>>>>> randstruct plugin is not enabled, which is in turn an attribute on
>>>>>>> gcc
>>>>>>>>
>>>>>>>> =
>>>>>>>
>>>>>>> v5.1, but not otherwise.
>>>>>>>
>>>>>>>> We just need this, no?
>>>>>>>>
>>>>>>>> diff --git a/include/linux/compiler-clang.h
>>>>>>>> b/include/linux/compiler-clang.h
>>>>>>>> index de179993e039..4e29ab6187cb 100644
>>>>>>>> --- a/include/linux/compiler-clang.h
>>>>>>>> +++ b/include/linux/compiler-clang.h
>>>>>>>> @@ -15,3 +15,6 @@
>>>>>>>> * with any version that can compile the kernel
>>>>>>>> */
>>>>>>>> #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
>>>>>>>> __COUNTER__)
>>>>>>>> +
>>>>>>>> +#define randomized_struct_fields_start struct {
>>>>>>>> +#define randomized_struct_fields_end };
>>>>>>>>
>>>>>>>> since offsets are mandated by C standard.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Yes, this is what we're testing with and is probably sufficient for
>>>>>>> our
>>>>>>> purposes.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Just tested this with bcc. bcc actually complains. the rewriter
>>>>>> is not able to rewrite prev->pid where prev is "struct task_struct
>>>>>> *prev".
>>>>>> I will change bcc rewriter to see whether the field value is correct
>>>>>> or
>>>>>> not.
>>>>>>
>>>>>> Not sure my understanding is correct or not, but I am afraid that
>>>>>> the above approach for clang compiler change may not work.
>>>>>> If clang calculates the field offset based on header file, the offset
>>>>>> may not be the same as kernel one....
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> why is that?
>>>>> When randomization is off both gcc and clang must generate the same
>>>>> offsets, since it's C standard.
>>>>
>>>>
>>>>
>>>> The patch changed compiler-clang.h, so gcc still do randomization.
>>>
>>>
>>>
>>> gcc_plugins are off by default and randomization will not be
>>> turned on for any sane distro or datacenter that cares about
>>> performance and stability.
>>> So imo above compiler-clang.h patch together with bcc fix would
>>> be enough.
>>
>>
>> Agree that short time the suggested fix should be enough.
>> Long time, disto could become "insane" someday :-)
>>
>
> Yup it works with clang compiler & bcc hack. Thanks :).
> I verified it with task_switch.py & runqlat.py.
>
> Are you going to push the bcc hacks to github repo ?
I will need to have proper implementation than a hack. Yes, once done, will
push into bcc repo. Should be done soon.
>
> Regards,
> Atish
From 1583465695464556139@xxx Wed Nov 08 03:09:24 +0000 2017
X-GM-THRID: 1583027203607239623
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread