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;
Subject: Re: [RFC PATCH] bpf: Add helpers to read useful task_struct members
To:     Y Song <ys114321@gmail.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>
 <CAH3MdRVm+6FroZ5MAENtqiMCDY5kR_74JY-fY9-Nr5Sw96pCHg@mail.gmail.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>
From:   Alexei Starovoitov <ast@fb.com>
Message-ID: <328c2dff-7d6d-0dc0-46cf-ae715845903e@fb.com>
Date:   Wed, 8 Nov 2017 07:04:02 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <CAH3MdRVm+6FroZ5MAENtqiMCDY5kR_74JY-fY9-Nr5Sw96pCHg@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Nov 2017 22:04:21.0058 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: a431223b-07bf-4c6c-e788-08d5262b81cd
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR15MB0961
X-OriginatorOrg: fb.com
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-11-07_07:,,
 signatures=0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 11/8/17 6:47 AM, Y Song wrote:
> 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.

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.


From 1583467998156655033@xxx Wed Nov 08 03:46:00 +0000 2017
X-GM-THRID: 1583027203607239623
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread