Re: [SRU][F][PATCH 1/1] UBUNTU: SAUCE: selftests/seccomp: fix 'storage size of ‘md’ isn’t known' build issue

Thu Oct 28 21:37:44 UTC 2021

Yeah it really is a bit of a mess..

So if I understand you correctly you're suggesting to go with the original
patch I proposed but with a comment explaining why we copied the definition
there? While we are there we could add ptrace definitions like in the
header file you linked, because we are effectively replacing the
linux/ptrace.h header.

- Luke

On Thu, Oct 28, 2021 at 10:53 AM Thadeu Lima de Souza Cascardo <
cascardo at canonical.com> wrote:

> On Thu, Oct 28, 2021 at 10:17:26AM -0700, Luke Nowakowski-Krijger wrote:
> > There is a way to change the search order of the headers and effectively
> > replace the global ones. We would have to do this individually however
> > because some headers in-tree reference system headers that are not in a
> > previous release, so we cant just include the whole include/uapi/linux
> dir
> > like we would want. A solution could be to say "we want X.h header in
> > file.c to be from in tree", and then a script could hook that include
> > somewhere. The only problem is if that header references a system header
> > that isnt in the release.. then we have the the same problem. And like we
> > talked about, all the headers are not self contained, which would make
> this
> > a lot easier.
> >
> > Also I'm not entirely sure how often replacing the header solves the
> > problem, maybe in 99% of cases its fine and the in-tree system header
> > references and the previous system headers don't differ by that much? In
> > the case of ptrace.h it worked and would solve this build problem better,
> > but IMO we should just solve it the hacky way and try to include
> > definitions directly since from what I have seen it doesen't seem to
> happen
> > that much and the nicer solution doesn't even solve the problem all the
> > time.
> >
> > -Luke
>
> So, I dug deep into this issue recently and failed to update the bug with
> my
> findings.
>
> Basically, glibc doesn't define lots of those structures, but adds the
> defines
> for the ptrace requests. And this really makes it a pain to do the right
> thing
> here on this test.
>
> So, I looked around searching for any real userspace users of this feature.
> Because they should be able to build whatever glibc and kernel version you
> have
> installed, right?
>
> It turns out there aren't that many. I think I found only one that had the
> seccomp_metadata structure and it was not strace or libseccomp, but criu.
>
>
> https://sources.debian.org/src/criu/3.16-2/compel/include/uapi/ptrace.h/?hl=66#L59
>
> So, I would go with a similar solution, adding a comment there, and I would
> also go upstream and fix it there too. Who knows with what glibc those
> tests
> are being tested with?
>
> Cascardo.
>
> >
> > On Wed, Oct 27, 2021 at 12:52 AM Stefan Bader <
> stefan.bader at canonical.com>
> > wrote:
> >
> > > On 26.10.21 19:19, Luke Nowakowski-Krijger wrote:
> > > > BugLink: https://bugs.launchpad.net/bugs/1896420
> > > >
> > > > There is a build issue on Bionic/5.4 kernels due to
> > > > PTRACE_SECCOMP_GET_METADATA being defined in glibc header
> sys/ptrace.h,
> > > > which then stops struct seccomp_metadata from being defined leading
> to:
> > > > seccomp_bpf.c:3028:26: error: storage size of ‘md’ isn’t known
> > > >
> > > > The solution here is to unconditonally define the seccomp_metadata
> > > > definition that we need, and remove the linux/ptrace.h header where a
> > > > definition of seccomp_metadata exists in Focal.
> > > >
> > > > Signed-off-by: Luke Nowakowski-Krijger <
> > > luke.nowakowskikrijger at canonical.com>
> > > > ---
> > >
> > > This is a general problem we have with HWE kernels. The global C
> headers
> > > are
> > > what the base kernel supports (4.15 for bionic). The way this is
> changed
> > > would
> > > lock the definition to something seperate from the 5.4 kernel itself.
> I
> > > wonder
> > > whether one could change the search order for includes in general to
> make
> > > those
> > > headers defined in the same tree take precedence over the global ones.
> > >
> > > -Stefan
> > > >   tools/testing/selftests/seccomp/seccomp_bpf.c | 3 +--
> > > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > > index e9a00d26666f..ab0da56ef7f1 100644
> > > > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > > > @@ -26,7 +26,6 @@
> > > >   #include <sys/ptrace.h>
> > > >   #include <sys/user.h>
> > > >   #include <linux/prctl.h>
> > > > -#include <linux/ptrace.h>
> > > >   #include <linux/seccomp.h>
> > > >   #include <pthread.h>
> > > >   #include <semaphore.h>
> > > > @@ -158,12 +157,12 @@ struct seccomp_data {
> > > >
> > > >   #ifndef PTRACE_SECCOMP_GET_METADATA
> > > >   #define PTRACE_SECCOMP_GET_METADATA 0x420d
> > > > +#endif
> > > >
> > > >   struct seccomp_metadata {
> > > >       __u64 filter_off;       /* Input: which filter */
> > > >       __u64 flags;             /* Output: filter's flags */
> > > >   };
> > > > -#endif
> > > >
> > > >   #ifndef SECCOMP_FILTER_FLAG_NEW_LISTENER
> > > >   #define SECCOMP_FILTER_FLAG_NEW_LISTENER    (1UL << 3)
> > > >
> > >
> > >
> > >
>
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20211028/b365c36c/attachment-0001.html>