netdev - Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390

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]

Date:   Tue, 2 Jul 2019 10:53:34 -0700
From:   Y Song <ys114321@...il.com>
To:     Ilya Leoshkevich <iii@...ux.ibm.com>
Cc:     bpf <bpf@...r.kernel.org>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390

On Tue, Jul 2, 2019 at 9:58 AM Ilya Leoshkevich <iii@...ux.ibm.com> wrote:
>
> > Am 02.07.2019 um 18:42 schrieb Y Song <ys114321@...il.com>:
> >
> > On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@...ux.ibm.com> wrote:
> >>
> >> -#elif defined(__s390x__)
> >> -       #define bpf_target_s930x
> >
> > I see in some other places (e.g., bcc) where
> > macro __s390x__ is also used to indicate a s390 architecture.
> > Could you explain the difference between __s390__ and
> > __s390x__?
>
> __s390__ is defined for 32-bit and 64-bit variants, __s390x__ is defined
> for 64-bit variant only.

Thanks.

>
> >> #if defined(bpf_target_x86)
> >>
> >> +#ifdef __KERNEL__
> >
> > In samples/bpf/,  __KERNEL__ is defined at clang options and
> > in selftests/bpf/, the __KERNEL__ is not defined.
> >
> > I checked x86 pt_regs definition with and without __KERNEL__.
> > They are identical except some register name difference.
> > I am wondering whether we can unify into all without
> > __KERNEL__. Is __KERNEL__ really needed?
>
> Right now removing it causes the build to fail, but the errors look
> fixable. However, I wonder whether there is a plan regarding this:
> should eBPF programs be built with user headers, kernel headers,
> or both? Status quo appears to be "both", so I’ve decided to stick with
> that in this patch.

Your patch is okay in the sense it maintains the current behavor.
I think it is okay since user level and kernel pt_regs layout are the same
except certain names are different.

>
> >> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
> >> +struct pt_regs;
> >> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> >
> > Is user_pt_regs a recent change or has been there for quite some time?
> > I am asking since bcc did not use user_pt_regs yet.
>
> It was added in late 2017 in commit 466698e654e8 ("s390/bpf: correct
> broken uapi for BPF_PROG_TYPE_PERF_EVENT program type“).

Thanks.