meson: optionally build seccomp if it supports notify #339

haircommander · 2022-05-27T18:10:57Z

Signed-off-by: Peter Hunt pehunt@redhat.com

haircommander · 2022-05-27T18:11:06Z

theoretically this fixes #337

eli-schwartz · 2022-05-27T20:33:16Z

meson.build

 endif
 if sd_journal.found()
 	add_project_arguments('-DUSE_JOURNALD=1', language : 'c')
 endif

+if run_command('./hack/seccomp-notify.sh', check: false).returncode() == 0


This relies on a shell script to do compile checks, which doesn't respect $CC (there may not necessarily be a cc on $PATH, though there likely is) or $PKG_CONFIG (which may point to pkgconf).

Maybe it should be ported to something Meson understands natively?

I have attempted to make the script respect those variables, PTAL

That's not something Meson understands natively. Also that doesn't help if someone configures Meson using a machine file (e.g. cross-compilation).

eli-schwartz · 2022-05-27T20:34:21Z

meson.build

 endif
 if sd_journal.found()
 	add_project_arguments('-DUSE_JOURNALD=1', language : 'c')
 endif

+if run_command('./hack/seccomp-notify.sh', check: false).returncode() == 0
+  seccomp = dependency('libseccomp', required : false)


You don't seem to ever add this dependency anywhere?

fun fact: I've never used meson before! thanks for the heads up :-)

kloczek · 2022-05-27T21:02:31Z

IMO that is not correct solution.
if libseccomp API is used independently from libsystemd is used simple in meson detection should be two checks for those two libraries.
How it was found in #337 containers uses directly libseccomp API even if it was not possible to detect libsystemd so meson detection should not rely on indirect dependencies

Using some additional script to check availability of the seccomp_notif_sizes () is as well incorrect because all what dhould be checked is just minimum version of the pkgconfig module.

kloczek · 2022-05-27T21:03:42Z

Just checked and it is not possible to build libseccomp without that seccomp_notif_sizes () routine so checking it is pointless.

haircommander · 2022-06-01T15:45:37Z

the only reason we link with libseccomp is to use seccomp notify, which requires that structure

haircommander · 2022-06-01T15:56:35Z

I've updated with some suggestions, thanks for the reviews y'all

Using some additional script to check availability of the seccomp_notif_sizes () is as well incorrect because all what dhould be checked is just minimum version of the pkgconfig module.

the problem is we need to know if seccomp notify is supported, but I believe packaged libseccomps don't necessarily correctly reflect that with pkg-config (for instance if support was backported) hence the additional check by compiling

kloczek · 2022-06-01T16:44:55Z

the problem is we need to know if seccomp notify is supported, but I believe packaged libseccomps don't necessarily correctly reflect that with pkg-config (for instance if support was backported) hence the additional check by compiling

Just checked last libseccomp 2.5.4 and in last version I see

Add a function, get_notify_fd(), to the Python bindings to get the nofication file descriptor

If it is about that bit all what woud be good to do is require libseccomps >= 2.5.4 and looks like that script can be dropped.

Yet another small thing which I fould looking on patch of that PR .
hack/seccomp-notify.sh .. if that script really is needed which I'm still not sure, shebang can be changed from /bin/bash to /usr/bin/sh as looks like it does not contain anything bash specyfic.

shenghaoyang · 2022-06-01T18:27:24Z

Maybe it would be cleaner to use the built-in test features of meson? See https://mesonbuild.com/Compiler-properties.html - there's a brute force "does snippet compile" function and a separate helper for sizeof: https://mesonbuild.com/Compiler-properties.html#expression-size. Should do the trick for structure presence detection.

Bonus: since it uses the same C compiler meson is setup to use, there shouldn't be any issues with the check being done with one compiler and the build performed by another.

kloczek · 2022-06-01T18:31:15Z

To be honest I've not been looking on that aspect closer.
I'm only dropping you some small advices and comments. 😊
Decisions are always yours 😋

eli-schwartz · 2022-06-01T18:33:08Z

Maybe it would be cleaner to use the built-in test features of meson?

Yes, that's what I suggested above too.

haircommander · 2022-06-01T18:35:50Z

thanks for the help everyone, I have attempted with the sizeof function

eli-schwartz · 2022-06-01T18:38:36Z

but I believe packaged libseccomps don't necessarily correctly reflect that with pkg-config (for instance if support was backported)

That's completely insane, which distro is going around willy-nilly breaking ABI by backporting something that isn't a bugfix into a different version?

Also, even if some distro is insane enough to do this, there's zero downside of simply not recognizing the unannounced breaking change. Simply don't compile with that feature, and conmon still works. Anyone who wants to use the notify functionality has to use a system that isn't a Frankensystem.

Also, the current script requires the right version anyway, so that wouldn't actually work. The script quote clearly requires at least 2.5.0, and then additionally does a compile check.

eli-schwartz · 2022-06-01T18:41:30Z

meson.build

 endif
 if sd_journal.found()
 	add_project_arguments('-DUSE_JOURNALD=1', language : 'c')
 endif

+seccomp = dependency('libseccomp', required : false)


Suggested change

seccomp = dependency('libseccomp', required : false)

seccomp = dependency('libseccomp', version: '>=2.5.0', required : false)

The previous build system enforced this restriction.

haircommander · 2022-06-01T18:57:37Z

I think that frankensystem was ubuntu: #275

since 16.04 is out of service, and ubuntu 18.04 is on its way, maybe we won't need it much longer

For now, risk seems low to keep it

eli-schwartz · 2022-06-01T21:42:38Z

the problem is we need to know if seccomp notify is supported, but I believe packaged libseccomps don't necessarily correctly reflect that with pkg-config (for instance if support was backported) hence the additional check by compiling

I took a look at this, the actual problem is that those definitions come from Linux not seccomp, and seccomp will simply define them locally if you don't have a new enough kernel. This allows seccomp to build even on systems too old to support this feature, on the theory that you can later upgrade the kernel and use said feature.

There's no backport of anything.

Again: seccomp doesn't provide the struct you're testing in the first place, the only test here is if you have a 5.0 kernel (Ubuntu 16.04 does not). So the test is fine.

But of course it should be done directly in Meson... Which it now does. Yay. :)

rhatdan · 2022-07-25T13:24:38Z

@haircommander What is going on with this?

kloczek · 2022-07-25T16:02:09Z

IM that PR soes not make any sense because notify is supported since exact version of the libseccomp so checking dependencies usimg only dependency() covers 100% what is needed.
I have my own version fix of that issue:

--- a/meson.build
+++ b/meson.build
@@ -33,6 +33,10 @@
                       language : 'c')

 glib = dependency('glib-2.0')
+seccomp = dependency('libseccomp', version : '>= 2.5.2')
+if seccomp.found()
+  add_project_arguments('-DUSE_SECCOMP=1', language : 'c')
+endif

 cc = meson.get_compiler('c')
 null_dep = dependency('', required : false)
@@ -85,7 +86,7 @@
             'src/utils.h',
             'src/seccomp_notify.c',
             'src/seccomp_notify.h'],
-           dependencies : [glib, libdl, sd_journal],
+           dependencies : [glib, libdl, sd_journal, seccomp]
            install : true,
            install_dir : join_paths(get_option('libexecdir'), 'podman'),
 )

With that patch hack/seccomp-notify.sh can be deleted.

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander · 2022-07-25T17:01:28Z

adapted as suggested. @rhatdan PTAL and I'll cut v2.1.3

kloczek · 2022-07-25T17:08:25Z

With that this shell backed sritpt can be removed as well 😋 (as no longer needed)

haircommander · 2022-07-25T17:21:53Z

I'd prefer to keep it for now as it's still used in the makefile :)

rhatdan · 2022-07-25T19:56:27Z

/lgtm

haircommander mentioned this pull request May 27, 2022

2.1.1: build fails because incorrect checking dependency for libsystemd #337

Open

eli-schwartz suggested changes May 27, 2022

View reviewed changes

haircommander force-pushed the meson-seccmop branch from 7c53053 to a2fa046 Compare June 1, 2022 15:54

haircommander force-pushed the meson-seccmop branch from a2fa046 to ef619b8 Compare June 1, 2022 18:34

eli-schwartz suggested changes Jun 1, 2022

View reviewed changes

haircommander force-pushed the meson-seccmop branch from ef619b8 to aa372fc Compare June 1, 2022 18:58

meson: build with seccomp if available

a627951

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander force-pushed the meson-seccmop branch from aa372fc to a627951 Compare July 25, 2022 17:01

rhatdan merged commit 7074480 into containers:main Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

meson: optionally build seccomp if it supports notify #339

meson: optionally build seccomp if it supports notify #339

haircommander commented May 27, 2022

haircommander commented May 27, 2022

eli-schwartz May 27, 2022

haircommander Jun 1, 2022

eli-schwartz Jun 1, 2022

eli-schwartz May 27, 2022

haircommander Jun 1, 2022

kloczek commented May 27, 2022

kloczek commented May 27, 2022

haircommander commented Jun 1, 2022 •

edited

haircommander commented Jun 1, 2022

kloczek commented Jun 1, 2022

shenghaoyang commented Jun 1, 2022

kloczek commented Jun 1, 2022

eli-schwartz commented Jun 1, 2022

haircommander commented Jun 1, 2022

eli-schwartz commented Jun 1, 2022 •

edited

eli-schwartz Jun 1, 2022

haircommander Jun 1, 2022

haircommander commented Jun 1, 2022

eli-schwartz commented Jun 1, 2022 •

edited

rhatdan commented Jul 25, 2022

kloczek commented Jul 25, 2022

haircommander commented Jul 25, 2022

kloczek commented Jul 25, 2022

haircommander commented Jul 25, 2022

rhatdan commented Jul 25, 2022

	seccomp = dependency('libseccomp', required : false)
	seccomp = dependency('libseccomp', version: '>=2.5.0', required : false)

meson: optionally build seccomp if it supports notify #339

meson: optionally build seccomp if it supports notify #339

Conversation

haircommander commented May 27, 2022

haircommander commented May 27, 2022

eli-schwartz May 27, 2022

Choose a reason for hiding this comment

haircommander Jun 1, 2022

Choose a reason for hiding this comment

eli-schwartz Jun 1, 2022

Choose a reason for hiding this comment

eli-schwartz May 27, 2022

Choose a reason for hiding this comment

haircommander Jun 1, 2022

Choose a reason for hiding this comment

kloczek commented May 27, 2022

kloczek commented May 27, 2022

haircommander commented Jun 1, 2022 • edited

haircommander commented Jun 1, 2022

kloczek commented Jun 1, 2022

shenghaoyang commented Jun 1, 2022

kloczek commented Jun 1, 2022

eli-schwartz commented Jun 1, 2022

haircommander commented Jun 1, 2022

eli-schwartz commented Jun 1, 2022 • edited

eli-schwartz Jun 1, 2022

Choose a reason for hiding this comment

haircommander Jun 1, 2022

Choose a reason for hiding this comment

haircommander commented Jun 1, 2022

eli-schwartz commented Jun 1, 2022 • edited

rhatdan commented Jul 25, 2022

kloczek commented Jul 25, 2022

haircommander commented Jul 25, 2022

kloczek commented Jul 25, 2022

haircommander commented Jul 25, 2022

rhatdan commented Jul 25, 2022

haircommander commented Jun 1, 2022 •

edited

eli-schwartz commented Jun 1, 2022 •

edited

eli-schwartz commented Jun 1, 2022 •

edited