netdev - Re: [PATCH v3 bpf-next 01/10] treewide: remove struct-pass-by-value from tracepoints arguments

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] [day] [month] [year] [list]

Message-ID: <22c5240e-af3e-fff1-2be0-3c5692d611aa@fb.com>
Date:   Thu, 22 Mar 2018 15:52:44 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>
CC:     David Miller <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Peter Zijlstra <peterz@...radead.org>,
        Network Development <netdev@...r.kernel.org>,
        kernel-team <kernel-team@...com>,
        Linux API <linux-api@...r.kernel.org>
Subject: Re: [PATCH v3 bpf-next 01/10] treewide: remove struct-pass-by-value
 from tracepoints arguments

On 3/22/18 1:52 PM, Linus Torvalds wrote:
> On Thu, Mar 22, 2018 at 1:48 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
>>
>> OK, but instead of changing it to pass by reference, why not just pass
>> the value. That is:
>>
>>  static void xen_set_pte_atomic(pte_t *ptep, pte_t pte)
>>  {
>> -       trace_xen_mmu_set_pte_atomic(ptep, pte);
>> +       trace_xen_mmu_set_pte_atomic(ptep, native_pte_val(pte));
>>         set_64bit((u64 *)ptep, native_pte_val(pte));
>>  }
>
> That looks simple and clean, and makes sense since the function itself
> then uses that value anyway.
>
> Certainly simpler than my monster define.

memcpy to avoid warning is a nice trick.
Generate code looks good on variety of compilers and architectures.
It also works with s390 tracepoints that pass this struct:
struct subchannel_id {
         u32 cssid : 8;
         u32 : 4;
         u32 m : 1;
         u32 ssid : 2;
         u32 one : 1;
         u32 sch_no : 16;
} __attribute__ ((packed, aligned(4)));

by value into a bunch of trace_* functions.
So at the end this 'monster define' will reduce patch 1 a lot.

Passing less than 8 byte structs by value is ok.
If we disallow them for tracepoints it would be seen as annoying
programming convention, because one dude (me) couldn't figure out
how to cast them.
Since you found the way to do this cast, let's go with it.
It will be hidden in one file (regardless whether it's ugly or not)
whereas small struct dereferences will be everywhere,
so one hack is better.