1. Implement SCM_PIDFD, a new type of CMSG type analogical to SCM_CREDENTIALS,
but it contains pidfd instead of plain pid, which allows programmers not
to care about PID reuse problem.
2. Add SO_PEERPIDFD which allows to get pidfd of peer socket holder pidfd.
This thing is direct analog of SO_PEERCRED which allows to get plain PID.
3. Add SCM_PIDFD / SO_PEERPIDFD kselftest
Idea comes from UAPI kernel group:
https://uapi-group.org/kernel-features/
Big thanks to Christian Brauner and Lennart Poettering for productive
discussions about this and Luca Boccassi for testing and reviewing this.
=== Motivation behind this patchset
Eric Dumazet raised a question:
> It seems that we already can use pidfd_open() (since linux-5.3), and
> pass the resulting fd in af_unix SCM_RIGHTS message ?
Yes, it's possible, but it means that from the receiver side we need
to trust the sent pidfd (in SCM_RIGHTS),
or always use combination of SCM_RIGHTS+SCM_CREDENTIALS, then we can
extract pidfd from SCM_RIGHTS,
then acquire plain pid from pidfd and after compare it with the pid
from SCM_CREDENTIALS.
A few comments from other folks regarding this.
Christian Brauner wrote:
>Let me try and provide some of the missing background.
>There are a range of use-cases where we would like to authenticate a
>client through sockets without being susceptible to PID recycling
>attacks. Currently, we can't do this as the race isn't fully fixable.
>We can only apply mitigations.
>What this patchset will allows us to do is to get a pidfd without the
>client having to send us an fd explicitly via SCM_RIGHTS. As that's
>already possibly as you correctly point out.
>But for protocols like polkit this is quite important. Every message is
>standalone and we would need to force a complete protocol change where
>we would need to require that every client allocate and send a pidfd via
>SCM_RIGHTS. That would also mean patching through all polkit users.
>For something like systemd-journald where we provide logging facilities
>and want to add metadata to the log we would also immensely benefit from
>being able to get a receiver-side controlled pidfd.
>With the message type we envisioned we don't need to change the sender
>at all and can be safe against pid recycling.
>Link: https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/154
>Link: https://uapi-group.org/kernel-features
Lennart Poettering wrote:
>So yes, this is of course possible, but it would mean the pidfd would
>have to be transported as part of the user protocol, explicitly sent
>by the sender. (Moreover, the receiver after receiving the pidfd would
>then still have to somehow be able to prove that the pidfd it just
>received actually refers to the peer's process and not some random
>process. – this part is actually solvable in userspace, but ugly)
>The big thing is simply that we want that the pidfd is associated
>*implicity* with each AF_UNIX connection, not explicitly. A lot of
>userspace already relies on this, both in the authentication area
>(polkit) as well as in the logging area (systemd-journald). Right now
>using the PID field from SO_PEERCREDS/SCM_CREDENTIALS is racy though
>and very hard to get right. Making this available as pidfd too, would
>solve this raciness, without otherwise changing semantics of it all:
>receivers can still enable the creds stuff as they wish, and the data
>is then implicitly appended to the connections/datagrams the sender
>initiates.
>Or to turn this around: things like polkit are typically used to
>authenticate arbitrary dbus methods calls: some service implements a
>dbus method call, and when an unprivileged client then issues that
>call, it will take the client's info, go to polkit and ask it if this
>is ok. If we wanted to send the pidfd as part of the protocol we
>basically would have to extend every single method call to contain the
>client's pidfd along with it as an additional argument, which would be
>a massive undertaking: it would change the prototypes of basically
>*all* methods a service defines… And that's just ugly.
>Note that Alex' patch set doesn't expose anything that wasn't exposed
>before, or attach, propagate what wasn't before. All it does, is make
>the field already available anyway (the struct ucred .pid field)
>available also in a better way (as a pidfd), to solve a variety of
>races, with no effect on the protocol actually spoken within the
>AF_UNIX transport. It's a seamless improvement of the status quo.
===
Git tree:
https://github.com/mihalicyn/linux/tree/scm_pidfd
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Kuniyuki Iwashima <[email protected]>
Cc: Lennart Poettering <[email protected]>
Cc: Luca Boccassi <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Stanislav Fomichev <[email protected]>
Tested-by: Luca Boccassi <[email protected]>
Alexander Mikhalitsyn (3):
scm: add SO_PASSPIDFD and SCM_PIDFD
net: core: add getsockopt SO_PEERPIDFD
selftests: net: add SCM_PIDFD / SO_PEERPIDFD test
arch/alpha/include/uapi/asm/socket.h | 3 +
arch/mips/include/uapi/asm/socket.h | 3 +
arch/parisc/include/uapi/asm/socket.h | 3 +
arch/sparc/include/uapi/asm/socket.h | 3 +
include/linux/net.h | 1 +
include/linux/socket.h | 1 +
include/net/scm.h | 43 +-
include/uapi/asm-generic/socket.h | 3 +
net/core/sock.c | 48 ++
net/mptcp/sockopt.c | 3 +
net/unix/af_unix.c | 34 +-
tools/include/uapi/asm-generic/socket.h | 3 +
tools/testing/selftests/net/.gitignore | 1 +
tools/testing/selftests/net/af_unix/Makefile | 3 +-
.../testing/selftests/net/af_unix/scm_pidfd.c | 430 ++++++++++++++++++
15 files changed, 574 insertions(+), 8 deletions(-)
create mode 100644 tools/testing/selftests/net/af_unix/scm_pidfd.c
--
2.34.1
Implement SCM_PIDFD, a new type of CMSG type analogical to SCM_CREDENTIALS,
but it contains pidfd instead of plain pid, which allows programmers not
to care about PID reuse problem.
We mask SO_PASSPIDFD feature if CONFIG_UNIX is not builtin because
it depends on a pidfd_prepare() API which is not exported to the kernel
modules.
Idea comes from UAPI kernel group:
https://uapi-group.org/kernel-features/
Big thanks to Christian Brauner and Lennart Poettering for productive
discussions about this.
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Kuniyuki Iwashima <[email protected]>
Cc: Lennart Poettering <[email protected]>
Cc: Luca Boccassi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Tested-by: Luca Boccassi <[email protected]>
Reviewed-by: Kuniyuki Iwashima <[email protected]>
Reviewed-by: Christian Brauner <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
---
v6:
- disable feature when CONFIG_UNIX=n/m (pidfd_prepare API is not exported to modules)
v5:
- no changes
v4:
- fixed silent fd_install if writting of CMSG to the userspace fails (pointed by Christian)
v2:
According to review comments from Kuniyuki Iwashima and Christian Brauner:
- use pidfd_create(..) retval as a result
- whitespace change
---
arch/alpha/include/uapi/asm/socket.h | 2 ++
arch/mips/include/uapi/asm/socket.h | 2 ++
arch/parisc/include/uapi/asm/socket.h | 2 ++
arch/sparc/include/uapi/asm/socket.h | 2 ++
include/linux/net.h | 1 +
include/linux/socket.h | 1 +
include/net/scm.h | 43 +++++++++++++++++++++++--
include/uapi/asm-generic/socket.h | 2 ++
net/core/sock.c | 15 +++++++++
net/mptcp/sockopt.c | 3 ++
net/unix/af_unix.c | 18 ++++++++---
tools/include/uapi/asm-generic/socket.h | 2 ++
12 files changed, 86 insertions(+), 7 deletions(-)
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
index 739891b94136..ff310613ae64 100644
--- a/arch/alpha/include/uapi/asm/socket.h
+++ b/arch/alpha/include/uapi/asm/socket.h
@@ -137,6 +137,8 @@
#define SO_RCVMARK 75
+#define SO_PASSPIDFD 76
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64
diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
index 18f3d95ecfec..762dcb80e4ec 100644
--- a/arch/mips/include/uapi/asm/socket.h
+++ b/arch/mips/include/uapi/asm/socket.h
@@ -148,6 +148,8 @@
#define SO_RCVMARK 75
+#define SO_PASSPIDFD 76
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64
diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
index f486d3dfb6bb..df16a3e16d64 100644
--- a/arch/parisc/include/uapi/asm/socket.h
+++ b/arch/parisc/include/uapi/asm/socket.h
@@ -129,6 +129,8 @@
#define SO_RCVMARK 0x4049
+#define SO_PASSPIDFD 0x404A
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64
diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
index 2fda57a3ea86..6e2847804fea 100644
--- a/arch/sparc/include/uapi/asm/socket.h
+++ b/arch/sparc/include/uapi/asm/socket.h
@@ -130,6 +130,8 @@
#define SO_RCVMARK 0x0054
+#define SO_PASSPIDFD 0x0055
+
#if !defined(__KERNEL__)
diff --git a/include/linux/net.h b/include/linux/net.h
index b73ad8e3c212..c234dfbe7a30 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -43,6 +43,7 @@ struct net;
#define SOCK_PASSSEC 4
#define SOCK_SUPPORT_ZC 5
#define SOCK_CUSTOM_SOCKOPT 6
+#define SOCK_PASSPIDFD 7
#ifndef ARCH_HAS_SOCKET_TYPES
/**
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 13c3a237b9c9..6bf90f251910 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -177,6 +177,7 @@ static inline size_t msg_data_left(struct msghdr *msg)
#define SCM_RIGHTS 0x01 /* rw: access rights (array of int) */
#define SCM_CREDENTIALS 0x02 /* rw: struct ucred */
#define SCM_SECURITY 0x03 /* rw: security label */
+#define SCM_PIDFD 0x04 /* ro: pidfd (int) */
struct ucred {
__u32 pid;
diff --git a/include/net/scm.h b/include/net/scm.h
index 585adc1346bd..13b188422370 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,12 +120,46 @@ static inline bool scm_has_secdata(struct socket *sock)
}
#endif /* CONFIG_SECURITY_NETWORK */
+#if IS_BUILTIN(CONFIG_UNIX)
+static __inline__ void scm_pidfd_recv(struct msghdr *msg, struct scm_cookie *scm)
+{
+ struct file *pidfd_file = NULL;
+ int pidfd;
+
+ /*
+ * put_cmsg() doesn't return an error if CMSG is truncated,
+ * that's why we need to opencode these checks here.
+ */
+ if ((msg->msg_controllen <= sizeof(struct cmsghdr)) ||
+ (msg->msg_controllen - sizeof(struct cmsghdr)) < sizeof(int)) {
+ msg->msg_flags |= MSG_CTRUNC;
+ return;
+ }
+
+ WARN_ON_ONCE(!scm->pid);
+ pidfd = pidfd_prepare(scm->pid, 0, &pidfd_file);
+
+ if (put_cmsg(msg, SOL_SOCKET, SCM_PIDFD, sizeof(int), &pidfd)) {
+ if (pidfd_file) {
+ put_unused_fd(pidfd);
+ fput(pidfd_file);
+ }
+
+ return;
+ }
+
+ if (pidfd_file)
+ fd_install(pidfd, pidfd_file);
+}
+#endif
+
static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg,
struct scm_cookie *scm, int flags)
{
if (!msg->msg_control) {
- if (test_bit(SOCK_PASSCRED, &sock->flags) || scm->fp ||
- scm_has_secdata(sock))
+ if (test_bit(SOCK_PASSCRED, &sock->flags) ||
+ test_bit(SOCK_PASSPIDFD, &sock->flags) ||
+ scm->fp || scm_has_secdata(sock))
msg->msg_flags |= MSG_CTRUNC;
scm_destroy(scm);
return;
@@ -141,6 +175,11 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg,
put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(ucreds), &ucreds);
}
+#if IS_BUILTIN(CONFIG_UNIX)
+ if (test_bit(SOCK_PASSPIDFD, &sock->flags))
+ scm_pidfd_recv(msg, scm);
+#endif
+
scm_destroy_cred(scm);
scm_passec(sock, msg, scm);
diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
index 638230899e98..b76169fdb80b 100644
--- a/include/uapi/asm-generic/socket.h
+++ b/include/uapi/asm-generic/socket.h
@@ -132,6 +132,8 @@
#define SO_RCVMARK 75
+#define SO_PASSPIDFD 76
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
diff --git a/net/core/sock.c b/net/core/sock.c
index 5440e67bcfe3..f6c415ef151f 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1246,6 +1246,15 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
clear_bit(SOCK_PASSCRED, &sock->flags);
break;
+#if IS_BUILTIN(CONFIG_UNIX)
+ case SO_PASSPIDFD:
+ if (valbool)
+ set_bit(SOCK_PASSPIDFD, &sock->flags);
+ else
+ clear_bit(SOCK_PASSPIDFD, &sock->flags);
+ break;
+#endif
+
case SO_TIMESTAMP_OLD:
case SO_TIMESTAMP_NEW:
case SO_TIMESTAMPNS_OLD:
@@ -1732,6 +1741,12 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
v.val = !!test_bit(SOCK_PASSCRED, &sock->flags);
break;
+#if IS_BUILTIN(CONFIG_UNIX)
+ case SO_PASSPIDFD:
+ v.val = !!test_bit(SOCK_PASSPIDFD, &sock->flags);
+ break;
+#endif
+
case SO_PEERCRED:
{
struct ucred peercred;
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index d4258869ac48..5a80eb23089f 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -355,6 +355,9 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname,
case SO_BROADCAST:
case SO_BSDCOMPAT:
case SO_PASSCRED:
+#if IS_BUILTIN(CONFIG_UNIX)
+ case SO_PASSPIDFD:
+#endif
case SO_PASSSEC:
case SO_RXQ_OVFL:
case SO_WIFI_STATUS:
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index cc695c9f09ec..aac40106d036 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1361,7 +1361,8 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr,
if (err)
goto out;
- if (test_bit(SOCK_PASSCRED, &sock->flags) &&
+ if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
+ test_bit(SOCK_PASSPIDFD, &sock->flags)) &&
!unix_sk(sk)->addr) {
err = unix_autobind(sk);
if (err)
@@ -1469,7 +1470,8 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
if (err)
goto out;
- if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr) {
+ if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
+ test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) {
err = unix_autobind(sk);
if (err)
goto out;
@@ -1670,6 +1672,8 @@ static void unix_sock_inherit_flags(const struct socket *old,
{
if (test_bit(SOCK_PASSCRED, &old->flags))
set_bit(SOCK_PASSCRED, &new->flags);
+ if (test_bit(SOCK_PASSPIDFD, &old->flags))
+ set_bit(SOCK_PASSPIDFD, &new->flags);
if (test_bit(SOCK_PASSSEC, &old->flags))
set_bit(SOCK_PASSSEC, &new->flags);
}
@@ -1819,8 +1823,10 @@ static bool unix_passcred_enabled(const struct socket *sock,
const struct sock *other)
{
return test_bit(SOCK_PASSCRED, &sock->flags) ||
+ test_bit(SOCK_PASSPIDFD, &sock->flags) ||
!other->sk_socket ||
- test_bit(SOCK_PASSCRED, &other->sk_socket->flags);
+ test_bit(SOCK_PASSCRED, &other->sk_socket->flags) ||
+ test_bit(SOCK_PASSPIDFD, &other->sk_socket->flags);
}
/*
@@ -1922,7 +1928,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
goto out;
}
- if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr) {
+ if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
+ test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) {
err = unix_autobind(sk);
if (err)
goto out;
@@ -2824,7 +2831,8 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
/* Never glue messages from different writers */
if (!unix_skb_scm_eq(skb, &scm))
break;
- } else if (test_bit(SOCK_PASSCRED, &sock->flags)) {
+ } else if (test_bit(SOCK_PASSCRED, &sock->flags) ||
+ test_bit(SOCK_PASSPIDFD, &sock->flags)) {
/* Copy credentials */
scm_set_cred(&scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid);
unix_set_secdata(&scm, skb);
diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h
index 8756df13be50..fbbc4bf53ee3 100644
--- a/tools/include/uapi/asm-generic/socket.h
+++ b/tools/include/uapi/asm-generic/socket.h
@@ -121,6 +121,8 @@
#define SO_RCVMARK 75
+#define SO_PASSPIDFD 76
+
#if !defined(__KERNEL__)
#if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
--
2.34.1
Add SO_PEERPIDFD which allows to get pidfd of peer socket holder pidfd.
This thing is direct analog of SO_PEERCRED which allows to get plain PID.
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Kuniyuki Iwashima <[email protected]>
Cc: Lennart Poettering <[email protected]>
Cc: Luca Boccassi <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Stanislav Fomichev <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Christian Brauner <[email protected]>
Tested-by: Luca Boccassi <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
---
v5:
- started using (struct proto)->bpf_bypass_getsockopt hook
v4:
- return -ESRCH if sk->sk_peer_pid is NULL from getsockopt() syscall
- return errors from pidfd_prepare() as it is from getsockopt() syscall
v3:
- fixed possible fd leak (thanks to Christian Brauner)
v2:
According to review comments from Kuniyuki Iwashima and Christian Brauner:
- use pidfd_create(..) retval as a result
- whitespace change
---
arch/alpha/include/uapi/asm/socket.h | 1 +
arch/mips/include/uapi/asm/socket.h | 1 +
arch/parisc/include/uapi/asm/socket.h | 1 +
arch/sparc/include/uapi/asm/socket.h | 1 +
include/uapi/asm-generic/socket.h | 1 +
net/core/sock.c | 33 +++++++++++++++++++++++++
net/unix/af_unix.c | 16 ++++++++++++
tools/include/uapi/asm-generic/socket.h | 1 +
8 files changed, 55 insertions(+)
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
index ff310613ae64..e94f621903fe 100644
--- a/arch/alpha/include/uapi/asm/socket.h
+++ b/arch/alpha/include/uapi/asm/socket.h
@@ -138,6 +138,7 @@
#define SO_RCVMARK 75
#define SO_PASSPIDFD 76
+#define SO_PEERPIDFD 77
#if !defined(__KERNEL__)
diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
index 762dcb80e4ec..60ebaed28a4c 100644
--- a/arch/mips/include/uapi/asm/socket.h
+++ b/arch/mips/include/uapi/asm/socket.h
@@ -149,6 +149,7 @@
#define SO_RCVMARK 75
#define SO_PASSPIDFD 76
+#define SO_PEERPIDFD 77
#if !defined(__KERNEL__)
diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
index df16a3e16d64..be264c2b1a11 100644
--- a/arch/parisc/include/uapi/asm/socket.h
+++ b/arch/parisc/include/uapi/asm/socket.h
@@ -130,6 +130,7 @@
#define SO_RCVMARK 0x4049
#define SO_PASSPIDFD 0x404A
+#define SO_PEERPIDFD 0x404B
#if !defined(__KERNEL__)
diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
index 6e2847804fea..682da3714686 100644
--- a/arch/sparc/include/uapi/asm/socket.h
+++ b/arch/sparc/include/uapi/asm/socket.h
@@ -131,6 +131,7 @@
#define SO_RCVMARK 0x0054
#define SO_PASSPIDFD 0x0055
+#define SO_PEERPIDFD 0x0056
#if !defined(__KERNEL__)
diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
index b76169fdb80b..8ce8a39a1e5f 100644
--- a/include/uapi/asm-generic/socket.h
+++ b/include/uapi/asm-generic/socket.h
@@ -133,6 +133,7 @@
#define SO_RCVMARK 75
#define SO_PASSPIDFD 76
+#define SO_PEERPIDFD 77
#if !defined(__KERNEL__)
diff --git a/net/core/sock.c b/net/core/sock.c
index f6c415ef151f..dee8438985dd 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1762,6 +1762,39 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
goto lenout;
}
+ case SO_PEERPIDFD:
+ {
+ struct pid *peer_pid;
+ struct file *pidfd_file = NULL;
+ int pidfd;
+
+ if (len > sizeof(pidfd))
+ len = sizeof(pidfd);
+
+ spin_lock(&sk->sk_peer_lock);
+ peer_pid = get_pid(sk->sk_peer_pid);
+ spin_unlock(&sk->sk_peer_lock);
+
+ if (!peer_pid)
+ return -ESRCH;
+
+ pidfd = pidfd_prepare(peer_pid, 0, &pidfd_file);
+ put_pid(peer_pid);
+ if (pidfd < 0)
+ return pidfd;
+
+ if (copy_to_sockptr(optval, &pidfd, len) ||
+ copy_to_sockptr(optlen, &len, sizeof(int))) {
+ put_unused_fd(pidfd);
+ fput(pidfd_file);
+
+ return -EFAULT;
+ }
+
+ fd_install(pidfd, pidfd_file);
+ return 0;
+ }
+
case SO_PEERGROUPS:
{
const struct cred *cred;
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index aac40106d036..ea24843fb017 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -921,11 +921,26 @@ static void unix_unhash(struct sock *sk)
*/
}
+static bool unix_bpf_bypass_getsockopt(int level, int optname)
+{
+ if (level == SOL_SOCKET) {
+ switch (optname) {
+ case SO_PEERPIDFD:
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ return false;
+}
+
struct proto unix_dgram_proto = {
.name = "UNIX",
.owner = THIS_MODULE,
.obj_size = sizeof(struct unix_sock),
.close = unix_close,
+ .bpf_bypass_getsockopt = unix_bpf_bypass_getsockopt,
#ifdef CONFIG_BPF_SYSCALL
.psock_update_sk_prot = unix_dgram_bpf_update_proto,
#endif
@@ -937,6 +952,7 @@ struct proto unix_stream_proto = {
.obj_size = sizeof(struct unix_sock),
.close = unix_close,
.unhash = unix_unhash,
+ .bpf_bypass_getsockopt = unix_bpf_bypass_getsockopt,
#ifdef CONFIG_BPF_SYSCALL
.psock_update_sk_prot = unix_stream_bpf_update_proto,
#endif
diff --git a/tools/include/uapi/asm-generic/socket.h b/tools/include/uapi/asm-generic/socket.h
index fbbc4bf53ee3..54d9c8bf7c55 100644
--- a/tools/include/uapi/asm-generic/socket.h
+++ b/tools/include/uapi/asm-generic/socket.h
@@ -122,6 +122,7 @@
#define SO_RCVMARK 75
#define SO_PASSPIDFD 76
+#define SO_PEERPIDFD 77
#if !defined(__KERNEL__)
--
2.34.1
Basic test to check consistency between:
- SCM_CREDENTIALS and SCM_PIDFD
- SO_PEERCRED and SO_PEERPIDFD
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Kuniyuki Iwashima <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
---
v3:
- started using kselftest lib (thanks to Kuniyuki Iwashima for suggestion/review)
- now test covers abstract sockets too and SOCK_DGRAM sockets
---
tools/testing/selftests/net/.gitignore | 1 +
tools/testing/selftests/net/af_unix/Makefile | 3 +-
.../testing/selftests/net/af_unix/scm_pidfd.c | 430 ++++++++++++++++++
3 files changed, 433 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/net/af_unix/scm_pidfd.c
diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index 80f06aa62034..83fd1ebd34ec 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -26,6 +26,7 @@ reuseport_bpf_cpu
reuseport_bpf_numa
reuseport_dualstack
rxtimestamp
+scm_pidfd
sk_bind_sendto_listen
sk_connect_zero_addr
socket
diff --git a/tools/testing/selftests/net/af_unix/Makefile b/tools/testing/selftests/net/af_unix/Makefile
index 1e4b397cece6..221c387a7d7f 100644
--- a/tools/testing/selftests/net/af_unix/Makefile
+++ b/tools/testing/selftests/net/af_unix/Makefile
@@ -1,3 +1,4 @@
-TEST_GEN_PROGS := diag_uid test_unix_oob unix_connect
+CFLAGS += $(KHDR_INCLUDES)
+TEST_GEN_PROGS := diag_uid test_unix_oob unix_connect scm_pidfd
include ../../lib.mk
diff --git a/tools/testing/selftests/net/af_unix/scm_pidfd.c b/tools/testing/selftests/net/af_unix/scm_pidfd.c
new file mode 100644
index 000000000000..a86222143d79
--- /dev/null
+++ b/tools/testing/selftests/net/af_unix/scm_pidfd.c
@@ -0,0 +1,430 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+#define _GNU_SOURCE
+#include <error.h>
+#include <limits.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/socket.h>
+#include <linux/socket.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/un.h>
+#include <sys/signal.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include "../../kselftest_harness.h"
+
+#define clean_errno() (errno == 0 ? "None" : strerror(errno))
+#define log_err(MSG, ...) \
+ fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", __FILE__, __LINE__, \
+ clean_errno(), ##__VA_ARGS__)
+
+#ifndef SCM_PIDFD
+#define SCM_PIDFD 0x04
+#endif
+
+static void child_die()
+{
+ exit(1);
+}
+
+static int safe_int(const char *numstr, int *converted)
+{
+ char *err = NULL;
+ long sli;
+
+ errno = 0;
+ sli = strtol(numstr, &err, 0);
+ if (errno == ERANGE && (sli == LONG_MAX || sli == LONG_MIN))
+ return -ERANGE;
+
+ if (errno != 0 && sli == 0)
+ return -EINVAL;
+
+ if (err == numstr || *err != '\0')
+ return -EINVAL;
+
+ if (sli > INT_MAX || sli < INT_MIN)
+ return -ERANGE;
+
+ *converted = (int)sli;
+ return 0;
+}
+
+static int char_left_gc(const char *buffer, size_t len)
+{
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ if (buffer[i] == ' ' || buffer[i] == '\t')
+ continue;
+
+ return i;
+ }
+
+ return 0;
+}
+
+static int char_right_gc(const char *buffer, size_t len)
+{
+ int i;
+
+ for (i = len - 1; i >= 0; i--) {
+ if (buffer[i] == ' ' || buffer[i] == '\t' ||
+ buffer[i] == '\n' || buffer[i] == '\0')
+ continue;
+
+ return i + 1;
+ }
+
+ return 0;
+}
+
+static char *trim_whitespace_in_place(char *buffer)
+{
+ buffer += char_left_gc(buffer, strlen(buffer));
+ buffer[char_right_gc(buffer, strlen(buffer))] = '\0';
+ return buffer;
+}
+
+/* borrowed (with all helpers) from pidfd/pidfd_open_test.c */
+static pid_t get_pid_from_fdinfo_file(int pidfd, const char *key, size_t keylen)
+{
+ int ret;
+ char path[512];
+ FILE *f;
+ size_t n = 0;
+ pid_t result = -1;
+ char *line = NULL;
+
+ snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", pidfd);
+
+ f = fopen(path, "re");
+ if (!f)
+ return -1;
+
+ while (getline(&line, &n, f) != -1) {
+ char *numstr;
+
+ if (strncmp(line, key, keylen))
+ continue;
+
+ numstr = trim_whitespace_in_place(line + 4);
+ ret = safe_int(numstr, &result);
+ if (ret < 0)
+ goto out;
+
+ break;
+ }
+
+out:
+ free(line);
+ fclose(f);
+ return result;
+}
+
+static int cmsg_check(int fd)
+{
+ struct msghdr msg = { 0 };
+ struct cmsghdr *cmsg;
+ struct iovec iov;
+ struct ucred *ucred = NULL;
+ int data = 0;
+ char control[CMSG_SPACE(sizeof(struct ucred)) +
+ CMSG_SPACE(sizeof(int))] = { 0 };
+ int *pidfd = NULL;
+ pid_t parent_pid;
+ int err;
+
+ iov.iov_base = &data;
+ iov.iov_len = sizeof(data);
+
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = control;
+ msg.msg_controllen = sizeof(control);
+
+ err = recvmsg(fd, &msg, 0);
+ if (err < 0) {
+ log_err("recvmsg");
+ return 1;
+ }
+
+ if (msg.msg_flags & (MSG_TRUNC | MSG_CTRUNC)) {
+ log_err("recvmsg: truncated");
+ return 1;
+ }
+
+ for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != NULL;
+ cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+ if (cmsg->cmsg_level == SOL_SOCKET &&
+ cmsg->cmsg_type == SCM_PIDFD) {
+ if (cmsg->cmsg_len < sizeof(*pidfd)) {
+ log_err("CMSG parse: SCM_PIDFD wrong len");
+ return 1;
+ }
+
+ pidfd = (void *)CMSG_DATA(cmsg);
+ }
+
+ if (cmsg->cmsg_level == SOL_SOCKET &&
+ cmsg->cmsg_type == SCM_CREDENTIALS) {
+ if (cmsg->cmsg_len < sizeof(*ucred)) {
+ log_err("CMSG parse: SCM_CREDENTIALS wrong len");
+ return 1;
+ }
+
+ ucred = (void *)CMSG_DATA(cmsg);
+ }
+ }
+
+ /* send(pfd, "x", sizeof(char), 0) */
+ if (data != 'x') {
+ log_err("recvmsg: data corruption");
+ return 1;
+ }
+
+ if (!pidfd) {
+ log_err("CMSG parse: SCM_PIDFD not found");
+ return 1;
+ }
+
+ if (!ucred) {
+ log_err("CMSG parse: SCM_CREDENTIALS not found");
+ return 1;
+ }
+
+ /* pidfd from SCM_PIDFD should point to the parent process PID */
+ parent_pid =
+ get_pid_from_fdinfo_file(*pidfd, "Pid:", sizeof("Pid:") - 1);
+ if (parent_pid != getppid()) {
+ log_err("wrong SCM_PIDFD %d != %d", parent_pid, getppid());
+ return 1;
+ }
+
+ return 0;
+}
+
+struct sock_addr {
+ char sock_name[32];
+ struct sockaddr_un listen_addr;
+ socklen_t addrlen;
+};
+
+FIXTURE(scm_pidfd)
+{
+ int server;
+ pid_t client_pid;
+ int startup_pipe[2];
+ struct sock_addr server_addr;
+ struct sock_addr *client_addr;
+};
+
+FIXTURE_VARIANT(scm_pidfd)
+{
+ int type;
+ bool abstract;
+};
+
+FIXTURE_VARIANT_ADD(scm_pidfd, stream_pathname)
+{
+ .type = SOCK_STREAM,
+ .abstract = 0,
+};
+
+FIXTURE_VARIANT_ADD(scm_pidfd, stream_abstract)
+{
+ .type = SOCK_STREAM,
+ .abstract = 1,
+};
+
+FIXTURE_VARIANT_ADD(scm_pidfd, dgram_pathname)
+{
+ .type = SOCK_DGRAM,
+ .abstract = 0,
+};
+
+FIXTURE_VARIANT_ADD(scm_pidfd, dgram_abstract)
+{
+ .type = SOCK_DGRAM,
+ .abstract = 1,
+};
+
+FIXTURE_SETUP(scm_pidfd)
+{
+ self->client_addr = mmap(NULL, sizeof(*self->client_addr), PROT_READ | PROT_WRITE,
+ MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+ ASSERT_NE(MAP_FAILED, self->client_addr);
+}
+
+FIXTURE_TEARDOWN(scm_pidfd)
+{
+ close(self->server);
+
+ kill(self->client_pid, SIGKILL);
+ waitpid(self->client_pid, NULL, 0);
+
+ if (!variant->abstract) {
+ unlink(self->server_addr.sock_name);
+ unlink(self->client_addr->sock_name);
+ }
+}
+
+static void fill_sockaddr(struct sock_addr *addr, bool abstract)
+{
+ char *sun_path_buf = (char *)&addr->listen_addr.sun_path;
+
+ addr->listen_addr.sun_family = AF_UNIX;
+ addr->addrlen = offsetof(struct sockaddr_un, sun_path);
+ snprintf(addr->sock_name, sizeof(addr->sock_name), "scm_pidfd_%d", getpid());
+ addr->addrlen += strlen(addr->sock_name);
+ if (abstract) {
+ *sun_path_buf = '\0';
+ addr->addrlen++;
+ sun_path_buf++;
+ } else {
+ unlink(addr->sock_name);
+ }
+ memcpy(sun_path_buf, addr->sock_name, strlen(addr->sock_name));
+}
+
+static void client(FIXTURE_DATA(scm_pidfd) *self,
+ const FIXTURE_VARIANT(scm_pidfd) *variant)
+{
+ int err;
+ int cfd;
+ socklen_t len;
+ struct ucred peer_cred;
+ int peer_pidfd;
+ pid_t peer_pid;
+ int on = 0;
+
+ cfd = socket(AF_UNIX, variant->type, 0);
+ if (cfd < 0) {
+ log_err("socket");
+ child_die();
+ }
+
+ if (variant->type == SOCK_DGRAM) {
+ fill_sockaddr(self->client_addr, variant->abstract);
+
+ if (bind(cfd, (struct sockaddr *)&self->client_addr->listen_addr, self->client_addr->addrlen)) {
+ log_err("bind");
+ child_die();
+ }
+ }
+
+ if (connect(cfd, (struct sockaddr *)&self->server_addr.listen_addr,
+ self->server_addr.addrlen) != 0) {
+ log_err("connect");
+ child_die();
+ }
+
+ on = 1;
+ if (setsockopt(cfd, SOL_SOCKET, SO_PASSCRED, &on, sizeof(on))) {
+ log_err("Failed to set SO_PASSCRED");
+ child_die();
+ }
+
+ if (setsockopt(cfd, SOL_SOCKET, SO_PASSPIDFD, &on, sizeof(on))) {
+ log_err("Failed to set SO_PASSPIDFD");
+ child_die();
+ }
+
+ close(self->startup_pipe[1]);
+
+ if (cmsg_check(cfd)) {
+ log_err("cmsg_check failed");
+ child_die();
+ }
+
+ /* skip further for SOCK_DGRAM as it's not applicable */
+ if (variant->type == SOCK_DGRAM)
+ return;
+
+ len = sizeof(peer_cred);
+ if (getsockopt(cfd, SOL_SOCKET, SO_PEERCRED, &peer_cred, &len)) {
+ log_err("Failed to get SO_PEERCRED");
+ child_die();
+ }
+
+ len = sizeof(peer_pidfd);
+ if (getsockopt(cfd, SOL_SOCKET, SO_PEERPIDFD, &peer_pidfd, &len)) {
+ log_err("Failed to get SO_PEERPIDFD");
+ child_die();
+ }
+
+ /* pid from SO_PEERCRED should point to the parent process PID */
+ if (peer_cred.pid != getppid()) {
+ log_err("peer_cred.pid != getppid(): %d != %d", peer_cred.pid, getppid());
+ child_die();
+ }
+
+ peer_pid = get_pid_from_fdinfo_file(peer_pidfd,
+ "Pid:", sizeof("Pid:") - 1);
+ if (peer_pid != peer_cred.pid) {
+ log_err("peer_pid != peer_cred.pid: %d != %d", peer_pid, peer_cred.pid);
+ child_die();
+ }
+}
+
+TEST_F(scm_pidfd, test)
+{
+ int err;
+ int pfd;
+ int child_status = 0;
+
+ self->server = socket(AF_UNIX, variant->type, 0);
+ ASSERT_NE(-1, self->server);
+
+ fill_sockaddr(&self->server_addr, variant->abstract);
+
+ err = bind(self->server, (struct sockaddr *)&self->server_addr.listen_addr, self->server_addr.addrlen);
+ ASSERT_EQ(0, err);
+
+ if (variant->type == SOCK_STREAM) {
+ err = listen(self->server, 1);
+ ASSERT_EQ(0, err);
+ }
+
+ err = pipe(self->startup_pipe);
+ ASSERT_NE(-1, err);
+
+ self->client_pid = fork();
+ ASSERT_NE(-1, self->client_pid);
+ if (self->client_pid == 0) {
+ close(self->server);
+ close(self->startup_pipe[0]);
+ client(self, variant);
+ exit(0);
+ }
+ close(self->startup_pipe[1]);
+
+ if (variant->type == SOCK_STREAM) {
+ pfd = accept(self->server, NULL, NULL);
+ ASSERT_NE(-1, pfd);
+ } else {
+ pfd = self->server;
+ }
+
+ /* wait until the child arrives at checkpoint */
+ read(self->startup_pipe[0], &err, sizeof(int));
+ close(self->startup_pipe[0]);
+
+ if (variant->type == SOCK_DGRAM) {
+ err = sendto(pfd, "x", sizeof(char), 0, (struct sockaddr *)&self->client_addr->listen_addr, self->client_addr->addrlen);
+ ASSERT_NE(-1, err);
+ } else {
+ err = send(pfd, "x", sizeof(char), 0);
+ ASSERT_NE(-1, err);
+ }
+
+ close(pfd);
+ waitpid(self->client_pid, &child_status, 0);
+ ASSERT_EQ(0, WIFEXITED(child_status) ? WEXITSTATUS(child_status) : 1);
+}
+
+TEST_HARNESS_MAIN
--
2.34.1
On Mon, 22 May 2023 15:24:37 +0200 Alexander Mikhalitsyn wrote:
> v6:
> - disable feature when CONFIG_UNIX=n/m (pidfd_prepare API is not exported to modules)
IMHO hiding the code under #if IS_BUILTIN(CONFIG_UNRELATED) is
surprising to the user and.. ugly?
Can we move scm_pidfd_recv() into a C source and export that?
That should be less controversial than exporting pidfd_prepare()
directly?
On Mon, May 22, 2023 at 01:34:09PM -0700, Jakub Kicinski wrote:
> On Mon, 22 May 2023 15:24:37 +0200 Alexander Mikhalitsyn wrote:
> > v6:
> > - disable feature when CONFIG_UNIX=n/m (pidfd_prepare API is not exported to modules)
>
> IMHO hiding the code under #if IS_BUILTIN(CONFIG_UNRELATED) is
> surprising to the user and.. ugly?
>
> Can we move scm_pidfd_recv() into a C source and export that?
> That should be less controversial than exporting pidfd_prepare()
> directly?
I really would like to avoid that because it will just mean that someone
else will abuse that function and then make an argument why we should
export the other function.
I think it would be ok if we required that unix support is built in
because it's not unprecedented either and we're not breaking anything.
Bpf has the same requirement:
#if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL)
struct bpf_unix_iter_state {
struct seq_net_private p;
unsigned int cur_sk;
unsigned int end_sk;
unsigned int max_sk;
struct sock **batch;
bool st_bucket_done;
};
and
#if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
DEFINE_BPF_ITER_FUNC(unix, struct bpf_iter_meta *meta,
struct unix_sock *unix_sk, uid_t uid)
On Tue, 23 May 2023 at 10:49, Christian Brauner <[email protected]> wrote:
>
> On Mon, May 22, 2023 at 01:34:09PM -0700, Jakub Kicinski wrote:
> > On Mon, 22 May 2023 15:24:37 +0200 Alexander Mikhalitsyn wrote:
> > > v6:
> > > - disable feature when CONFIG_UNIX=n/m (pidfd_prepare API is not exported to modules)
> >
> > IMHO hiding the code under #if IS_BUILTIN(CONFIG_UNRELATED) is
> > surprising to the user and.. ugly?
> >
> > Can we move scm_pidfd_recv() into a C source and export that?
> > That should be less controversial than exporting pidfd_prepare()
> > directly?
>
> I really would like to avoid that because it will just mean that someone
> else will abuse that function and then make an argument why we should
> export the other function.
>
> I think it would be ok if we required that unix support is built in
> because it's not unprecedented either and we're not breaking anything.
> Bpf has the same requirement:
>
> #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL)
> struct bpf_unix_iter_state {
> struct seq_net_private p;
> unsigned int cur_sk;
> unsigned int end_sk;
> unsigned int max_sk;
> struct sock **batch;
> bool st_bucket_done;
> };
>
> and
>
> #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
> DEFINE_BPF_ITER_FUNC(unix, struct bpf_iter_meta *meta,
> struct unix_sock *unix_sk, uid_t uid)
Some data points: Debian, Ubuntu, Fedora, RHEL, CentOS, Archlinux all
ship with CONFIG_UNIX=y, so a missing SCM_PIDFD in unlikely to have a
widespread impact, and if it does, it might encourage someone to
review their kconfig.
As mentioned on the v5 thread, we are waiting for this API to get the
userspace side sorted (systemd/dbus/dbus-broker/polkit), so I'd be
really grateful if we could start with the simplest and most
conservative approach (which seems to be the current one in v6 to me),
and then eventually later decide whether to export more functions, or
to deprecate CONFIG_UNIX=m, or something else entirely, as that
doesn't really affect the shape of the UAPI, just the details of its
availability. Thank you.
Kind regards,
Luca Boccassi
On Tue, 23 May 2023 11:44:01 +0100 Luca Boccassi wrote:
> > I really would like to avoid that because it will just mean that someone
> > else will abuse that function and then make an argument why we should
> > export the other function.
> >
> > I think it would be ok if we required that unix support is built in
> > because it's not unprecedented either and we're not breaking anything.
> > Bpf has the same requirement:
> >
> > #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL)
> > struct bpf_unix_iter_state {
> > struct seq_net_private p;
> > unsigned int cur_sk;
> > unsigned int end_sk;
> > unsigned int max_sk;
> > struct sock **batch;
> > bool st_bucket_done;
> > };
> >
> > and
> >
> > #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
> > DEFINE_BPF_ITER_FUNC(unix, struct bpf_iter_meta *meta,
> > struct unix_sock *unix_sk, uid_t uid)
Don't think we should bring BPF into arguments about uAPI consistency :S
> Some data points: Debian, Ubuntu, Fedora, RHEL, CentOS, Archlinux all
> ship with CONFIG_UNIX=y, so a missing SCM_PIDFD in unlikely to have a
> widespread impact, and if it does, it might encourage someone to
> review their kconfig.
IDK how you can argue that everyone sets UNIX to =y so hiding SCM_PIDFD
is fine and at the same time not be okay with making UNIX a bool :S
> As mentioned on the v5 thread, we are waiting for this API to get the
> userspace side sorted (systemd/dbus/dbus-broker/polkit), so I'd be
> really grateful if we could start with the simplest and most
> conservative approach (which seems to be the current one in v6 to me),
> and then eventually later decide whether to export more functions, or
> to deprecate CONFIG_UNIX=m, or something else entirely, as that
> doesn't really affect the shape of the UAPI, just the details of its
> availability. Thank you.
Just throw in a patch to make UNIX a bool and stop arguing then.
On Tue, May 23, 2023 at 11:08 PM Jakub Kicinski <[email protected]> wrote:
>
> On Tue, 23 May 2023 11:44:01 +0100 Luca Boccassi wrote:
> > > I really would like to avoid that because it will just mean that someone
> > > else will abuse that function and then make an argument why we should
> > > export the other function.
> > >
> > > I think it would be ok if we required that unix support is built in
> > > because it's not unprecedented either and we're not breaking anything.
> > > Bpf has the same requirement:
> > >
> > > #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL)
> > > struct bpf_unix_iter_state {
> > > struct seq_net_private p;
> > > unsigned int cur_sk;
> > > unsigned int end_sk;
> > > unsigned int max_sk;
> > > struct sock **batch;
> > > bool st_bucket_done;
> > > };
> > >
> > > and
> > >
> > > #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
> > > DEFINE_BPF_ITER_FUNC(unix, struct bpf_iter_meta *meta,
> > > struct unix_sock *unix_sk, uid_t uid)
>
> Don't think we should bring BPF into arguments about uAPI consistency :S
>
> > Some data points: Debian, Ubuntu, Fedora, RHEL, CentOS, Archlinux all
> > ship with CONFIG_UNIX=y, so a missing SCM_PIDFD in unlikely to have a
> > widespread impact, and if it does, it might encourage someone to
> > review their kconfig.
>
> IDK how you can argue that everyone sets UNIX to =y so hiding SCM_PIDFD
> is fine and at the same time not be okay with making UNIX a bool :S
>
> > As mentioned on the v5 thread, we are waiting for this API to get the
> > userspace side sorted (systemd/dbus/dbus-broker/polkit), so I'd be
> > really grateful if we could start with the simplest and most
> > conservative approach (which seems to be the current one in v6 to me),
> > and then eventually later decide whether to export more functions, or
> > to deprecate CONFIG_UNIX=m, or something else entirely, as that
> > doesn't really affect the shape of the UAPI, just the details of its
> > availability. Thank you.
>
> Just throw in a patch to make UNIX a bool and stop arguing then.
Dear Jakub,
Thanks for your attention to these patch series!
I'm ready to prepare/send a patch to make CONFIG_UNIX bool.
I will send SO_PEERPIDFD as an independent patch too, because it
doesn't require this change with CONFIG_UNIX
and we can avoid waiting until CONFIG_UNIX change will be merged.
I've a feeling that the discussion around making CONFIG_UNIX to be a
boolean won't be easy and fast ;-)
Kind regards,
Alex
On Wed, 24 May 2023 at 11:43, Aleksandr Mikhalitsyn
<[email protected]> wrote:
>
> On Tue, May 23, 2023 at 11:08 PM Jakub Kicinski <[email protected]> wrote:
> >
> > On Tue, 23 May 2023 11:44:01 +0100 Luca Boccassi wrote:
> > > > I really would like to avoid that because it will just mean that someone
> > > > else will abuse that function and then make an argument why we should
> > > > export the other function.
> > > >
> > > > I think it would be ok if we required that unix support is built in
> > > > because it's not unprecedented either and we're not breaking anything.
> > > > Bpf has the same requirement:
> > > >
> > > > #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL)
> > > > struct bpf_unix_iter_state {
> > > > struct seq_net_private p;
> > > > unsigned int cur_sk;
> > > > unsigned int end_sk;
> > > > unsigned int max_sk;
> > > > struct sock **batch;
> > > > bool st_bucket_done;
> > > > };
> > > >
> > > > and
> > > >
> > > > #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
> > > > DEFINE_BPF_ITER_FUNC(unix, struct bpf_iter_meta *meta,
> > > > struct unix_sock *unix_sk, uid_t uid)
> >
> > Don't think we should bring BPF into arguments about uAPI consistency :S
> >
> > > Some data points: Debian, Ubuntu, Fedora, RHEL, CentOS, Archlinux all
> > > ship with CONFIG_UNIX=y, so a missing SCM_PIDFD in unlikely to have a
> > > widespread impact, and if it does, it might encourage someone to
> > > review their kconfig.
> >
> > IDK how you can argue that everyone sets UNIX to =y so hiding SCM_PIDFD
> > is fine and at the same time not be okay with making UNIX a bool :S
> >
> > > As mentioned on the v5 thread, we are waiting for this API to get the
> > > userspace side sorted (systemd/dbus/dbus-broker/polkit), so I'd be
> > > really grateful if we could start with the simplest and most
> > > conservative approach (which seems to be the current one in v6 to me),
> > > and then eventually later decide whether to export more functions, or
> > > to deprecate CONFIG_UNIX=m, or something else entirely, as that
> > > doesn't really affect the shape of the UAPI, just the details of its
> > > availability. Thank you.
> >
> > Just throw in a patch to make UNIX a bool and stop arguing then.
>
> Dear Jakub,
>
> Thanks for your attention to these patch series!
>
> I'm ready to prepare/send a patch to make CONFIG_UNIX bool.
>
> I will send SO_PEERPIDFD as an independent patch too, because it
> doesn't require this change with CONFIG_UNIX
> and we can avoid waiting until CONFIG_UNIX change will be merged.
> I've a feeling that the discussion around making CONFIG_UNIX to be a
> boolean won't be easy and fast ;-)
Thank you, that sounds great to me, I can start using SO_PEERPIDFD
independently of SCM_PIDFD, there's no hard dependency between the
two.
Kind regards,
Luca Boccassi
On Wed, 24 May 2023 11:47:50 +0100 Luca Boccassi wrote:
> > I will send SO_PEERPIDFD as an independent patch too, because it
> > doesn't require this change with CONFIG_UNIX
> > and we can avoid waiting until CONFIG_UNIX change will be merged.
> > I've a feeling that the discussion around making CONFIG_UNIX to be a
> > boolean won't be easy and fast ;-)
>
> Thank you, that sounds great to me, I can start using SO_PEERPIDFD
> independently of SCM_PIDFD, there's no hard dependency between the
> two.
How about you put the UNIX -> bool patch at the end of the series,
(making it a 4 patch series) and if there's a discussion about it
I'll just skip it and apply the first 3 patches?
In the (IMHO more likely) case that there isn't a discussion it saves
me from remembering to chase you to send that patch ;)
On Wed, May 24, 2023 at 5:19 PM Jakub Kicinski <[email protected]> wrote:
>
> On Wed, 24 May 2023 11:47:50 +0100 Luca Boccassi wrote:
> > > I will send SO_PEERPIDFD as an independent patch too, because it
> > > doesn't require this change with CONFIG_UNIX
> > > and we can avoid waiting until CONFIG_UNIX change will be merged.
> > > I've a feeling that the discussion around making CONFIG_UNIX to be a
> > > boolean won't be easy and fast ;-)
> >
> > Thank you, that sounds great to me, I can start using SO_PEERPIDFD
> > independently of SCM_PIDFD, there's no hard dependency between the
> > two.
>
> How about you put the UNIX -> bool patch at the end of the series,
> (making it a 4 patch series) and if there's a discussion about it
> I'll just skip it and apply the first 3 patches?
Sure, I will do that!
>
> In the (IMHO more likely) case that there isn't a discussion it saves
> me from remembering to chase you to send that patch ;)
Thanks a lot, Jakub!
Kind regards,
Alex
On Sun, Jun 4, 2023 at 8:02 PM Jakub Kicinski <[email protected]> wrote:
>
> On Wed, 24 May 2023 17:45:25 +0200 Aleksandr Mikhalitsyn wrote:
> > > How about you put the UNIX -> bool patch at the end of the series,
> > > (making it a 4 patch series) and if there's a discussion about it
> > > I'll just skip it and apply the first 3 patches?
> >
> > Sure, I will do that!
>
> Hi Aleksandr! Did you disappear? Have I missed v7?
Dear Jakub,
of course I'm not, I've just got distracted with other things last
week. Will send -v7 this week!
Thanks for paying attention to the series ;-)
Kind regards,
Alex
On Wed, 24 May 2023 17:45:25 +0200 Aleksandr Mikhalitsyn wrote:
> > How about you put the UNIX -> bool patch at the end of the series,
> > (making it a 4 patch series) and if there's a discussion about it
> > I'll just skip it and apply the first 3 patches?
>
> Sure, I will do that!
Hi Aleksandr! Did you disappear? Have I missed v7?
On Sun, Jun 4, 2023 at 8:07 PM Aleksandr Mikhalitsyn
<[email protected]> wrote:
>
> On Sun, Jun 4, 2023 at 8:02 PM Jakub Kicinski <[email protected]> wrote:
> >
> > On Wed, 24 May 2023 17:45:25 +0200 Aleksandr Mikhalitsyn wrote:
> > > > How about you put the UNIX -> bool patch at the end of the series,
> > > > (making it a 4 patch series) and if there's a discussion about it
> > > > I'll just skip it and apply the first 3 patches?
> > >
> > > Sure, I will do that!
> >
> > Hi Aleksandr! Did you disappear? Have I missed v7?
>
> Dear Jakub,
>
> of course I'm not, I've just got distracted with other things last
> week. Will send -v7 this week!
> Thanks for paying attention to the series ;-)
>
> Kind regards,
> Alex
Dear Jakub,
I've sent -v7:
https://lore.kernel.org/all/[email protected]/
Kind regards,
Alex