Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: add new API to enable addfd support for seccomp notifier #454

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

realsdx
Copy link

@realsdx realsdx commented Feb 3, 2025

No description provided.

@pcmoore
Copy link
Member

pcmoore commented Feb 3, 2025

Hi @realsdx, thanks for the PR :)

Unfortunately, it looks like the the CI test are failing because of a missing seccomp_notif_addfd struct declaration due to an older userspace/headers on the build/CI system. Look at how we handle seccomp_notif and seccomp_notif_resp in the "include/seccomp.h.in" file for an example on how to fix this:

/* SECCOMP_RET_USER_NOTIF was added in kernel v5.0. */
#ifndef SECCOMP_RET_USER_NOTIF
#define SECCOMP_RET_USER_NOTIF 0x7fc00000U
struct seccomp_notif {
__u64 id;
__u32 pid;
__u32 flags;
struct seccomp_data data;
};
struct seccomp_notif_resp {
__u64 id;
__s64 val;
__s32 error;
__u32 flags;
};
#endif

@pcmoore
Copy link
Member

pcmoore commented Feb 4, 2025

FWIW, if I made the following changes on top of your PR I was able to get everything building:

diff --git a/include/seccomp.h.in b/include/seccomp.h.in
index 024aca537f64..568eabaad949 100644
--- a/include/seccomp.h.in
+++ b/include/seccomp.h.in
@@ -406,6 +406,11 @@ struct seccomp_notif_resp {
        __u32 flags;
 };
 
+#endif
+
+/* seccomp_notif_addfd and ADDFD_FLAG_SETFD was added in kernel v5.10 */
+#ifndef SECCOMP_ADDFD_FLAG_SETFD
+#define SECCOMP_ADDFD_FLAG_SETFD       (1UL << 0)
 struct seccomp_notif_addfd {
        __u64 id;
        __u32 flags;
diff --git a/src/system.h b/src/system.h
index 495050fb2e75..87bf8a6e832d 100644
--- a/src/system.h
+++ b/src/system.h
@@ -180,14 +180,6 @@ struct seccomp_notif_resp {
        __u32 flags;
 };
 
-struct seccomp_notif_addfd {
-       __u64 id;
-       __u32 flags;
-       __u32 srcfd;
-       __u32 newfd;
-       __u32 newfd_flags;
-};
-
 #define SECCOMP_IOC_MAGIC               '!'
 #define SECCOMP_IO(nr)                  _IO(SECCOMP_IOC_MAGIC, nr)
 #define SECCOMP_IOR(nr, type)           _IOR(SECCOMP_IOC_MAGIC, nr, type)
@@ -201,6 +193,24 @@ struct seccomp_notif_addfd {
 #define SECCOMP_IOCTL_NOTIF_ID_VALID    SECCOMP_IOW(2, __u64)
 #endif /* SECCOMP_RET_USER_NOTIF */
 
+/* seccomp_notif_addfd and ADDFD_FLAG_SETFD was added in kernel v5.10 */
+#ifndef SECCOMP_ADDFD_FLAG_SETFD
+#define SECCOMP_ADDFD_FLAG_SETFD       (1UL << 0)
+struct seccomp_notif_addfd {
+       __u64 id;
+       __u32 flags;
+       __u32 srcfd;
+       __u32 newfd;
+       __u32 newfd_flags;
+};
+#endif
+
+/* SECCOMP_IOCTL_NOTIF_ADDFD was added in kernel v5.10 */
+#ifndef SECCOMP_IOCTL_NOTIF_ADDFD
+#define SECCOMP_IOCTL_NOTIF_ADDFD      SECCOMP_IOW(3, \
+                                                   struct seccomp_notif_addfd)
+#endif
+
 /* non-public ioctl number for backwards compat (see system.c) */
 #define SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR SECCOMP_IOR(2, __u64)

@pcmoore
Copy link
Member

pcmoore commented Feb 4, 2025

Another quick comment, we'll likely want to add a new API level to seccomp_api_get() to indicate ADDFD support.

@realsdx
Copy link
Author

realsdx commented Feb 5, 2025

Another quick comment, we'll likely want to add a new API level to seccomp_api_get() to indicate ADDFD support.

It's related to seccomp user notifiers, which I thought should be at the same level. Is the API level more dependent on the timing of kernel support, or is it about grouping features?

- add python api for seccomp addfd
- add tests for the new addfd python api
- update receive_notify & respond_notify methods to optionally accpet user provided notification fd

Signed-off-by: Sudipta Pandit <[email protected]>
@coveralls
Copy link

Coverage Status

coverage: 89.983% (-0.3%) from 90.252%
when pulling 3610e7c on realsdx:dev/addfd
into 7db46d7 on seccomp:main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants