[PATCH net-next v3 0/3] af_unix: Fix priority inversion issue

[PATCH net-next v3 0/3] af_unix: Fix priority inversion issue

[Nam Cao]

Hi,

While auditing AF_UNIX sockets, I noticed that the sendmsg() code paths may
block on the garbage collector running as workqueue. This can cause
priority inversion and latency for real-time users.

The implementation does kindly avoid blocking "sane users". However, it is
impossible to tell whether the kernel's definition of "sane users"
accurately describes all users out there.

Digging into history and figuring out the reasons why sendmsg() needs to
wait for garbage collector, it is determined that those reasons no longer
apply.

The first patch resolves another issue reported by Sashiko in v1, paves the
way for the second patch to remove the block. The last one is a simple
post-cleanup.

v3:
  - Move unix_schedule_gc() to be after exit_task_work()

v2:
  - Add patch [1/3]
  - Rebase the other two patches onto the new patch
  - Change commit message to be more precise

Nam Cao (3):
  af_unix: Schedule the garbage collector at task exit
  af_unix: Do not wait for garbage collector in sendmsg()
  af_unix: Clean up unix_schedule_gc()

 include/net/af_unix.h |  5 +++++
 kernel/exit.c         |  7 +++++++
 net/unix/af_unix.c    |  2 +-
 net/unix/af_unix.h    |  1 -
 net/unix/garbage.c    | 16 +---------------
 5 files changed, 14 insertions(+), 17 deletions(-)

-- 
2.47.3

[PATCH net-next v3 1/3] af_unix: Schedule the garbage collector at task exit

[Nam Cao]

When a task exits while still having dead cyclic reference AF_UNIX sockets,
those sockets stay behind indefinitely until the garbage collector gets
scheduled by an unrelated reason. This can be observed with the program
below.

Resolve this issue by scheduling the garbage collector during task exit,
after the task's file descriptors have been closed.

 #include <sys/mount.h>
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <sys/wait.h>

 #include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>

static int send_fd(int unix_fd, int fd)
{
        struct msghdr msgh;
        struct cmsghdr *cmsg;
        char buf[CMSG_SPACE(sizeof(fd))];

        memset(&msgh, 0, sizeof(msgh));

        memset(buf, 0, sizeof(buf));
        msgh.msg_control = buf;
        msgh.msg_controllen = sizeof(buf);

        cmsg = CMSG_FIRSTHDR(&msgh);
        cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
        cmsg->cmsg_level = SOL_SOCKET;
        cmsg->cmsg_type = SCM_RIGHTS;

        msgh.msg_controllen = cmsg->cmsg_len;

        memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
        return sendmsg(unix_fd, &msgh, 0);
}

int main(int argc, char *argv[])
{
	int fd[2];
	int i;

	for (int n = 0; n < 100; ++n) {
		if (socketpair(PF_UNIX, SOCK_SEQPACKET, 0, fd) == -1)
			goto out_error;

		for (i = 0; i < 100; ++i) {
			if (send_fd(fd[0], fd[0]) == -1)
				goto out_error;

			if (send_fd(fd[1], fd[1]) == -1)
				goto out_error;
		}
	}

	return 0;

out_error:
	fprintf(stderr, "error: %s\n", strerror(errno));
}

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 include/net/af_unix.h | 5 +++++
 kernel/exit.c         | 7 +++++++
 net/unix/af_unix.h    | 1 -
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 34f53dde65ce..686f6f1d1c21 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -14,11 +14,16 @@
 
 #if IS_ENABLED(CONFIG_UNIX)
 struct unix_sock *unix_get_socket(struct file *filp);
+void unix_schedule_gc(struct user_struct *user);
 #else
 static inline struct unix_sock *unix_get_socket(struct file *filp)
 {
 	return NULL;
 }
+
+static inline void unix_schedule_gc(struct user_struct *user)
+{
+}
 #endif
 
 struct unix_address {
diff --git a/kernel/exit.c b/kernel/exit.c
index 2c0b1c02920f..593ac4b0105f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -71,6 +71,7 @@
 #include <linux/unwind_deferred.h>
 #include <linux/uaccess.h>
 #include <linux/pidfs.h>
+#include <net/af_unix.h>
 
 #include <uapi/linux/wait.h>
 
@@ -1009,6 +1010,12 @@ void __noreturn do_exit(long code)
 	exit_task_work(tsk);
 	exit_thread(tsk);
 
+	/*
+	 * Must be after exit_files() and exit_task_work(tsk) to ensure that
+	 * the task's AF_UNIX sockets have all been closed.
+	 */
+	unix_schedule_gc(NULL);
+
 	sched_autogroup_exit_task(tsk);
 	cgroup_task_exit(tsk);
 
diff --git a/net/unix/af_unix.h b/net/unix/af_unix.h
index 8119dbeef3a3..fc4c59893124 100644
--- a/net/unix/af_unix.h
+++ b/net/unix/af_unix.h
@@ -30,7 +30,6 @@ void unix_update_edges(struct unix_sock *receiver);
 int unix_prepare_fpl(struct scm_fp_list *fpl);
 void unix_destroy_fpl(struct scm_fp_list *fpl);
 void unix_peek_fpl(struct scm_fp_list *fpl);
-void unix_schedule_gc(struct user_struct *user);
 
 /* SOCK_DIAG */
 long unix_inq_len(struct sock *sk);
-- 
2.47.3

[PATCH net-next v3 2/3] af_unix: Do not wait for garbage collector in sendmsg()

[Nam Cao]

AF_UNIX sockets' sendmsg() schedules and blocks on the garbage collector if
user has too many inflight unix sockets and there is cyclic reference in
the system. This causes real-time issues, as cyclic reference can be
created by any task in the system, and high priority tasks who do need to
send lots of AF_UNIX sockets get blocked by the garbage collector which
runs as workqueue, causing a priority inversion scenario.

The reason for blocking on garbage collector goes back to 2008, when
it was reported that "Local/unprivileged users can cause soft lockups
and take out system processes by triggering the OOM killer":
https://bugzilla.redhat.com/show_bug.cgi?id=470201

The soft lockup was because a process can keep queueing AF_UNIX sockets to
another process that is exiting. Back in 2008, the garbage collector was
run synchronously by the exiting process, therefore keep queueing AF_UNIX
sockets blocks that process from exiting.

The solution to that issue was forcing sendmsg() to wait for ongoing
garbage collector.

The OOM killer issue was brought up again in 2010:
https://lore.kernel.org/lkml/AANLkTi=Q967xpX0KLMwX-=_4_1AKO5wjHEuJ1TrNjCj9@mail.gmail.com/

To resolve that report, beside blocking on the garbage collector, sendmsg()
also schedules the garbage collector if the number of inflight AF_UNIX
sockets in the system is too high.

Then in 2015, once again, the OOM killer problem was brought up:
https://lore.kernel.org/lkml/20151228141435.GA13351@1wt.eu/

That time, the issue was resolved by disallowing a user from having more
inflight AF_UNIX sockets than their RLIMIT_NOFILE. That was done by commit
712f4aad406b ("unix: properly account for FDs passed over unix sockets")
and commit 415e3d3e90ce ("unix: correctly track in-flight fds in sending
process user_struct").

Now, sendmsg() does not have to block on the garbage collector anymore,
because:

  - The OOM killer issue has already been addressed by checking
    RLIMIT_NOFILE.

  - The soft lockup issue is no longer relevant, because the garbage
    collector now runs asynchronously since commit d9f21b361333 ("af_unix:
    Try to run GC async.")

Therefore, remove that to prevent priority inversion. Running all the
reproducers from the mentioned bug reports after this patch, no problem is
observed.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 net/unix/garbage.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 0783555e2526..f180c59b3da9 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -300,8 +300,6 @@ int unix_prepare_fpl(struct scm_fp_list *fpl)
 	if (!fpl->edges)
 		goto err;
 
-	unix_schedule_gc(fpl->user);
-
 	return 0;
 
 err:
-- 
2.47.3

[PATCH net-next v3 3/3] af_unix: Clean up unix_schedule_gc()

[Nam Cao]

unix_schedule_gc() is only called with NULL as an argument.

Simplify by deleting the parameter.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 include/net/af_unix.h |  4 ++--
 kernel/exit.c         |  2 +-
 net/unix/af_unix.c    |  2 +-
 net/unix/garbage.c    | 14 +-------------
 4 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 686f6f1d1c21..1dfdc0720052 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -14,14 +14,14 @@
 
 #if IS_ENABLED(CONFIG_UNIX)
 struct unix_sock *unix_get_socket(struct file *filp);
-void unix_schedule_gc(struct user_struct *user);
+void unix_schedule_gc(void);
 #else
 static inline struct unix_sock *unix_get_socket(struct file *filp)
 {
 	return NULL;
 }
 
-static inline void unix_schedule_gc(struct user_struct *user)
+static inline void unix_schedule_gc(void)
 {
 }
 #endif
diff --git a/kernel/exit.c b/kernel/exit.c
index 593ac4b0105f..50c5eff07bd6 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1014,7 +1014,7 @@ void __noreturn do_exit(long code)
 	 * Must be after exit_files() and exit_task_work(tsk) to ensure that
 	 * the task's AF_UNIX sockets have all been closed.
 	 */
-	unix_schedule_gc(NULL);
+	unix_schedule_gc();
 
 	sched_autogroup_exit_task(tsk);
 	cgroup_task_exit(tsk);
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f7a9d55eee8a..759db734a866 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -733,7 +733,7 @@ static void unix_release_sock(struct sock *sk, int embrion)
 
 	/* ---- Socket is dead now and most probably destroyed ---- */
 
-	unix_schedule_gc(NULL);
+	unix_schedule_gc();
 }
 
 struct unix_peercred {
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index f180c59b3da9..d46aeb9d2051 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -635,23 +635,11 @@ static void unix_gc(struct work_struct *work)
 
 static DECLARE_WORK(unix_gc_work, unix_gc);
 
-#define UNIX_INFLIGHT_SANE_USER		(SCM_MAX_FD * 8)
-
-void unix_schedule_gc(struct user_struct *user)
+void unix_schedule_gc(void)
 {
 	if (READ_ONCE(unix_graph_state) == UNIX_GRAPH_NOT_CYCLIC)
 		return;
 
-	/* Penalise users who want to send AF_UNIX sockets
-	 * but whose sockets have not been received yet.
-	 */
-	if (user &&
-	    READ_ONCE(user->unix_inflight) < UNIX_INFLIGHT_SANE_USER)
-		return;
-
 	if (!READ_ONCE(gc_in_progress))
 		queue_work(system_dfl_wq, &unix_gc_work);
-
-	if (user && READ_ONCE(unix_graph_cyclic_sccs))
-		flush_work(&unix_gc_work);
 }
-- 
2.47.3

Re: [PATCH net-next v3 2/3] af_unix: Do not wait for garbage collector in sendmsg()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Removing `unix_schedule_gc()` from `sendmsg()` prevents the AF_UNIX garbage collector from being proactively triggered for isolated cyclic references, leading to spurious -ETOOMANYREFS failures and indefinite resource leaks.
- [Low] The `user` parameter and its associated conditional logic in `unix_schedule_gc()` are now dead code.
--

--- Patch [2]: [PATCH net-next v3 2/3] af_unix: Do not wait for garbage collector in sendmsg() ---
commit e95749602b1bd36f1394eab73582d88874294091
Author: Nam Cao <namcao@linutronix.de>
af_unix: Do not wait for garbage collector in sendmsg()

This commit removes the unix_schedule_gc() call from the af_unix sendmsg()
path to prevent priority inversion scenarios. The change relies on the
assumption that checking RLIMIT_NOFILE and running the garbage collector
asynchronously are sufficient to prevent historical soft lockup and
out-of-memory issues.

> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 0783555e25266..f180c59b3da9d 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -300,8 +300,6 @@ int unix_prepare_fpl(struct scm_fp_list *fpl)
>  	if (!fpl->edges)
>  		goto err;
>  
> -	unix_schedule_gc(fpl->user);
> -

[Severity: High]
Does removing this call prevent the af_unix garbage collector from being
proactively triggered for isolated cyclic references?

If a long-running task generates cyclic af_unix references and drops its
file descriptors, the cyclic references' f_count remains > 0, so
unix_release_sock() is never called. Without this trigger, wouldn't the
garbage collector remain unscheduled unless the task exits or closes an
unrelated, non-cyclic af_unix socket?

Could this regression lead to legitimate operations failing with
-ETOOMANYREFS for unprivileged processes, or allow a privileged process
to bypass RLIMIT_NOFILE and leak memory unboundedly?

[Severity: Low]
Does removing this call leave the user parameter in unix_schedule_gc()
as dead code?

If all other callers now unconditionally pass NULL, the conditional logic
checking user in unix_schedule_gc() will permanently evaluate to false:

net/unix/garbage.c:unix_schedule_gc() {
    ...
	if (user &&
	    READ_ONCE(user->unix_inflight) < UNIX_INFLIGHT_SANE_USER)
		return;
    ...
	if (user && READ_ONCE(unix_graph_cyclic_sccs))
		flush_work(&unix_gc_work);
    ...
}

Should the user parameter and these checks be removed as well?

>  	return 0;
>  
>  err:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784712370.git.namcao@linutronix.de?part=2

Re: [PATCH net-next v3 3/3] af_unix: Clean up unix_schedule_gc()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] An unconditional call to `unix_schedule_gc()` in `do_exit()` causes a severe system-wide GC storm and CPU exhaustion if any live cyclic AF_UNIX sockets exist.
--

--- Patch [3]: [PATCH net-next v3 3/3] af_unix: Clean up unix_schedule_gc() ---
commit fc308672f15e1ad0994af5c62a43b996e97d2e7a
Author: Nam Cao <namcao@linutronix.de>

af_unix: Clean up unix_schedule_gc()

This commit simplifies unix_schedule_gc() by removing its user argument,
since it is currently only called with NULL. The conditionals checking the
user parameter within the function body are also removed.

> diff --git a/kernel/exit.c b/kernel/exit.c
> index 593ac4b0105f2..50c5eff07bd65 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1014,7 +1014,7 @@ void __noreturn do_exit(long code)
>  	 * Must be after exit_files() and exit_task_work(tsk) to ensure that
>  	 * the task's AF_UNIX sockets have all been closed.
>  	 */
> -	unix_schedule_gc(NULL);
> +	unix_schedule_gc();
>  
>  	sched_autogroup_exit_task(tsk);
>  	cgroup_task_exit(tsk);

[ ... ]

> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index f180c59b3da9d..d46aeb9d20518 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -635,23 +635,11 @@ static void unix_gc(struct work_struct *work)
>  
>  static DECLARE_WORK(unix_gc_work, unix_gc);
>  
> -#define UNIX_INFLIGHT_SANE_USER		(SCM_MAX_FD * 8)
> -
> -void unix_schedule_gc(struct user_struct *user)
> +void unix_schedule_gc(void)
>  {
>  	if (READ_ONCE(unix_graph_state) == UNIX_GRAPH_NOT_CYCLIC)
>  		return;
>  
> -	/* Penalise users who want to send AF_UNIX sockets
> -	 * but whose sockets have not been received yet.
> -	 */
> -	if (user &&
> -	    READ_ONCE(user->unix_inflight) < UNIX_INFLIGHT_SANE_USER)
> -		return;
> -
>  	if (!READ_ONCE(gc_in_progress))
>  		queue_work(system_dfl_wq, &unix_gc_work);

[Severity: High]
Does this unconditional call to unix_schedule_gc() from do_exit() create a
risk of system-wide GC storms?

If an unprivileged local user creates a cyclic graph of AF_UNIX sockets (for
example, by sending the FDs of a socketpair to each other) and keeps the file
descriptors open, it forms a "live" cycle.

In this scenario:
1. unix_graph_state becomes UNIX_GRAPH_CYCLIC (as set in unix_walk_scc_fast).
2. When unix_gc_work runs, it ignores the live cycle but leaves
   unix_graph_state as UNIX_GRAPH_CYCLIC.
3. Once GC finishes, gc_in_progress becomes false.

Since do_exit() unconditionally calls unix_schedule_gc(), any unrelated task
exiting anywhere in the system will immediately re-queue the GC.

Could this result in CPU exhaustion and a denial of service due to heavy lock
contention on unix_gc_lock, as the GC continuously walks the inflight socket
graph?

> -
> -	if (user && READ_ONCE(unix_graph_cyclic_sccs))
> -		flush_work(&unix_gc_work);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784712370.git.namcao@linutronix.de?part=3

Re: [PATCH net-next v3 1/3] af_unix: Schedule the garbage collector at task exit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditionally triggering the global AF_UNIX garbage collector on task exit creates a severe system-wide denial of service vulnerability.
- [Medium] A TOCTOU race on `gc_in_progress` causes `do_exit()` to skip queueing the garbage collector if it is currently running, leaving the cyclic sockets leaked indefinitely.
--

--- Patch [1]: [PATCH net-next v3 1/3] af_unix: Schedule the garbage collector at task exit ---
commit 9a35b6370cf5aa7dba01e2ce8f06d95e2eb14265
Author: Nam Cao <namcao@linutronix.de>

af_unix: Schedule the garbage collector at task exit

This commit attempts to fix a memory leak involving dead cyclic reference
AF_UNIX sockets when a task exits. It does this by scheduling the garbage
collector during task exit, after the task's file descriptors have been
closed.

> diff --git a/kernel/exit.c b/kernel/exit.c
> index 2c0b1c02920f4..593ac4b0105f2 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
[ ... ]
> @@ -1009,6 +1010,12 @@ void __noreturn do_exit(long code)
>  	exit_task_work(tsk);
>  	exit_thread(tsk);
>  
> +	/*
> +	 * Must be after exit_files() and exit_task_work(tsk) to ensure that
> +	 * the task's AF_UNIX sockets have all been closed.
> +	 */
> +	unix_schedule_gc(NULL);

[Severity: High]
Does unconditionally triggering the global AF_UNIX garbage collector on task
exit create a severe system-wide denial of service vulnerability?

By passing NULL to unix_schedule_gc(), the rate-limiting check
(user->unix_inflight) is bypassed. If an unprivileged user creates a cyclic
graph of AF_UNIX sockets, every task exit system-wide would unconditionally
queue unix_gc_work.

The GC thread would then take the global unix_gc_lock spinlock and perform
an O(N) traversal of all cyclic sockets, burning CPU and blocking all
legitimate AF_UNIX operations system-wide.

[Severity: Medium]
Can a race on gc_in_progress cause do_exit() to skip queueing the garbage
collector if it is currently running, leaving the cyclic sockets leaked
indefinitely?

If a background GC run is currently active, gc_in_progress is true. If the
concurrent GC scans the exiting task's sockets before they are closed, it
considers them alive.

When the exiting task subsequently closes them and calls
unix_schedule_gc(NULL), it sees gc_in_progress is true and silently skips
queueing a new GC pass. This fails to reliably fix the socket leak it was
intended to resolve.

> +
>  	sched_autogroup_exit_task(tsk);
>  	cgroup_task_exit(tsk);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784712370.git.namcao@linutronix.de?part=1