Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <fc6e7617-ee4e-b799-7565-bdfc4f7e0159@fb.com>
References: <20171103065833.8076-1-sandipan@linux.vnet.ibm.com>
 <94a4761f-1b51-8b70-fb7f-3cea91c69717@fb.com> <1509815348.8zu63uatdo.naveen@linux.ibm.com>
 <4acdc081-341d-ee91-a591-b1d331a8c8d5@fb.com> <1509982000.092la4257a.naveen@linux.ibm.com>
 <df92faa5-ca2b-a623-e074-e14b7345f8e5@fb.com> <1510042830.62prnydrlw.naveen@linux.ibm.com>
 <CAH3MdRX-XehVX5A1O7+vxR+UuG46z7mRSMOer4hsxFYdKjWKUg@mail.gmail.com> <fc6e7617-ee4e-b799-7565-bdfc4f7e0159@fb.com>
From:   Y Song <ys114321@gmail.com>
Date:   Tue, 7 Nov 2017 13:47:55 -0800
Message-ID: <CAH3MdRVm+6FroZ5MAENtqiMCDY5kR_74JY-fY9-Nr5Sw96pCHg@mail.gmail.com>
Subject: Re: [RFC PATCH] bpf: Add helpers to read useful task_struct members
To:     Alexei Starovoitov <ast@fb.com>
Cc:     "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>,
        netdev <netdev@vger.kernel.org>,
        Sandipan Das <sandipan@linux.vnet.ibm.com>,
        Brendan Gregg <brendan.d.gregg@gmail.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 7, 2017 at 1:39 PM, Alexei Starovoitov <ast@fb.com> wrote:
> On 11/8/17 6:14 AM, Y Song wrote:
>>
>> On Tue, Nov 7, 2017 at 12:37 AM, Naveen N. Rao
>> <naveen.n.rao@linux.vnet.ibm.com> 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.

>
> bcc rewriter issue is odd. I suspect it was broken from day one.
> Meaning that bcc didn't support poking into anonymous union and structs.

This seems right.

>
>> I verified that the drawf info with randomized structure config does not
>> match randomized structure member offset. Specifically, I tried
>> linux/proc_ns.h struct proc_ns_operations,
>> dwarf says:
>>   field name: offset 0
>>   field real_ns_name: offset 8
>> But if you print out the real offset at runtime, you get 40 and 16
>> respectively.
>
>
> thanks for confirming. It means that gcc randomization plugin is broken
> and has to be fixed with regard to adjusting debug info while
> randomizing the fields.
>

From 1583464824093017999@xxx Wed Nov 08 02:55:33 +0000 2017
X-GM-THRID: 1583027203607239623
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread