Minimal non-child process exit notification support

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a process to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid entry. This interface allow lmkd to give up polling and instead block and wait for process death.

Quoted 8 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 9 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a process to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid entry. This interface allow lmkd to give up polling and instead block and wait for process death.

I agree with the need for this interface (with a few caveats), but there are a few points I'd like to make:

* I don't think that making a new procfile is necessary. When you open /proc/$pid you already have a handle for the underlying process, and you can already poll to check whether the process has died (fstatat fails for instance). What if we just used an inotify event to tell userspace that the process has died -- to avoid userspace doing a poll loop?

* There is a fairly old interface called the proc_connector which gives you global fork+exec+exit events (similar to kevents from FreeBSD though much less full-featured). I was working on some patches to extend proc_connector so that it could be used inside containers as well as unprivileged users. This would be another way we could implement this.

I'm really not a huge fan of the "blocking read" semantic (though if we have to have it, can we at least provide as much information as you get from proc_connector -- such as the exit status?). Also maybe we should integrate this into the exit machinery instead of this loop...

Quoted 4 lines · Aleksa Sarai

Quoted 3 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will

successfully

Quoted 3 lines

Quoted 3 lines

complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not

one

Quoted 5 lines

Quoted 5 lines

of its children.

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually

exits

Quoted 5 lines

Quoted 5 lines

before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a process to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid entry. This interface allow lmkd to give up polling and instead

block

Quoted 3 lines

Quoted 1 line

and wait for process death.

I agree with the need for this interface (with a few caveats), but

there

Quoted 3 lines

are a few points I'd like to make:

* I don't think that making a new procfile is necessary. When you

open

Quoted 1 line

/proc/$pid you already have a handle for the underlying process,

and

Quoted 1 line

you can already poll to check whether the process has died

(fstatat

Quoted 5 lines

fails for instance). What if we just used an inotify event to tell userspace that the process has died -- to avoid userspace doing a poll loop?

* There is a fairly old interface called the proc_connector which

gives

Quoted 3 lines

you global fork+exec+exit events (similar to kevents from FreeBSD though much less full-featured). I was working on some patches to extend proc_connector so that it could be used inside containers

as

Quoted 4 lines

well as unprivileged users. This would be another way we could implement this.

I'm really not a huge fan of the "blocking read" semantic (though if

we

Quoted 1 line

have to have it, can we at least provide as much information as you

get

Quoted 1 line

from proc_connector -- such as the exit status?). Also maybe we

should

Quoted 1 line

integrate this into the exit machinery instead of this loop...

In addition, given that you've posted two patches in the similar vein but as separate patchsets -- would you mind re-sending them as a single patchset (with all the relevant folks added to Cc)?

If the idea is to extend /proc/$pid to allow for various fd-as-process-handle operations (which I agree with in principle), then they should be a single patchset. I'm also a bit cautious about how many procfiles the eventual goal is to add.

Quoted 8 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 9 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a process to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid entry. This interface allow lmkd to give up polling and instead block and wait for process death.

I agree with the need for this interface (with a few caveats), but there are a few points I'd like to make:

* I don't think that making a new procfile is necessary. When you open /proc/$pid you already have a handle for the underlying process, and you can already poll to check whether the process has died (fstatat fails for instance). What if we just used an inotify event to tell userspace that the process has died -- to avoid userspace doing a poll loop?

* There is a fairly old interface called the proc_connector which gives you global fork+exec+exit events (similar to kevents from FreeBSD though much less full-featured). I was working on some patches to extend proc_connector so that it could be used inside containers as well as unprivileged users. This would be another way we could implement this.

I'm really not a huge fan of the "blocking read" semantic (though if we have to have it, can we at least provide as much information as you get from proc_connector -- such as the exit status?).

Also maybe we should
integrate this into the exit machinery instead of this loop...

Quoted 22 lines · Aleksa Sarai

Quoted 8 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 9 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a process to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid entry. This interface allow lmkd to give up polling and instead block and wait for process death.

I agree with the need for this interface (with a few caveats), but there are a few points I'd like to make:

Quoted 6 lines

* I don't think that making a new procfile is necessary. When you open /proc/$pid you already have a handle for the underlying process, and you can already poll to check whether the process has died (fstatat fails for instance). What if we just used an inotify event to tell userspace that the process has died -- to avoid userspace doing a poll loop?

I'm trying to make a simple interface. The basic unix data access model is that a userspace application wants information (e.g., next bunch of bytes in a file, next packet from a socket, next signal from a signal FD, etc.), and tells the kernel so by making a system call on a file descriptor. Ordinarily, the kernel returns to userspace with the requested information when it's available, potentially after blocking until the information is available. Sometimes userspace doesn't want to block, so it adds O_NONBLOCK to the open file mode, and in this mode, the kernel can tell the userspace requestor "try again later", but the source of truth is still that ordinarily-blocking system call. How does userspace know when to try again in the "try again later" case? By using select/poll/epoll/whatever, which suggests a good time for that "try again later" retry, but is not dispositive about it, since that ordinarily-blocking system call is still the sole source of truth, and that poll is allowed to report spurious readabilty.

The event file I'm proposing is so ordinary, in fact, that it works from the shell. Without some specific technical reason to do something different, we shouldn't do something unusual.

Given that we *can*, cheaply, provide a clean and consistent API to userspace, why would we instead want to inflict some exotic and hard-to-use interface on userspace instead? Asking that userspace poll on a directory file descriptor and, when poll returns, check by looking for certain errors (we'd have to spec which ones) from fstatat is awkward. /proc/pid is a directory. In what other context does the kernel ask userspace to use a directory this way?

Quoted 3 lines

I'm really not a huge fan of the "blocking read" semantic (though if we have to have it, can we at least provide as much information as you get from proc_connector -- such as the exit status?).

[...]
The exit status in /proc/pid/stat is zeroed out for readers that fail do_task_stat's ptrace_may_access call. (Falsifying the exit status in stat seems a privilege check fails seems like a bad idea from a correctness POV.)

Should open() on exithand perform the same ptrace_may_access privilege check? What if the process *becomes* untraceable during its lifetime (e.g., with setuid). Should that read() on the exithand FD still yield a siginfo_t? Just having exithand yield EOF all the time punts the privilege problem to a later discussion because this approach doesn't leak information. We can always add an "exithand_full" or something that actually yields a siginfo_t.

Another option would be to make exithand's read() always yield a siginfo_t, but have the open() just fail if the caller couldn't ptrace_may_access it. But why shouldn't you be able to wait on other processes? If you can see it in /proc, you should be able to wait on it exiting.

Quoted 2 lines

Also maybe we should
integrate this into the exit machinery instead of this loop...

I don't know what you mean. It's already integrated into the exit
machinery: it's what runs the waitqueue.

Quoted 30 lines · Daniel Colascione

Quoted 22 lines · Aleksa Sarai

Quoted 8 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 9 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a process to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid entry. This interface allow lmkd to give up polling and instead block and wait for process death.

I agree with the need for this interface (with a few caveats), but there are a few points I'd like to make:

Quoted 6 lines

* I don't think that making a new procfile is necessary. When you open /proc/$pid you already have a handle for the underlying process, and you can already poll to check whether the process has died (fstatat fails for instance). What if we just used an inotify event to tell userspace that the process has died -- to avoid userspace doing a poll loop?

Quoted 16 lines

I'm trying to make a simple interface. The basic unix data access model is that a userspace application wants information (e.g., next bunch of bytes in a file, next packet from a socket, next signal from a signal FD, etc.), and tells the kernel so by making a system call on a file descriptor. Ordinarily, the kernel returns to userspace with the requested information when it's available, potentially after blocking until the information is available. Sometimes userspace doesn't want to block, so it adds O_NONBLOCK to the open file mode, and in this mode, the kernel can tell the userspace requestor "try again later", but the source of truth is still that ordinarily-blocking system call. How does userspace know when to try again in the "try again later" case? By using select/poll/epoll/whatever, which suggests a good time for that "try again later" retry, but is not dispositive about it, since that ordinarily-blocking system call is still the sole source of truth, and that poll is allowed to report spurious readabilty.

inotify gives you an event if a file or directory is deleted. A pid dying semantically is similar to the idea of a /proc/$pid being deleted. I don't see how a blocking read on a new procfile is simpler than using the existing notification-on-file-events infrastructure -- not to mention that the idea of "this file blocks until the thing we are indirectly referencing by this file is gone" seems to me to be a really strange interface.

Sure, it uses read(2) -- but is that the only constraint on designing simple interfaces?

Quoted 3 lines

The event file I'm proposing is so ordinary, in fact, that it works from the shell. Without some specific technical reason to do something different, we shouldn't do something unusual.

inotify-tools are available on effectively every distribution.

Quoted 7 lines

Given that we *can*, cheaply, provide a clean and consistent API to userspace, why would we instead want to inflict some exotic and hard-to-use interface on userspace instead? Asking that userspace poll on a directory file descriptor and, when poll returns, check by looking for certain errors (we'd have to spec which ones) from fstatat is awkward. /proc/pid is a directory. In what other context does the kernel ask userspace to use a directory this way?

I'm not sure you understood my proposal. I said that we need an interface to do this, and I was trying to explain (by noting what the current way of doing it would be) what I think the interface should be.

Quoted 8 lines

Quoted 3 lines

I'm really not a huge fan of the "blocking read" semantic (though if we have to have it, can we at least provide as much information as you get from proc_connector -- such as the exit status?).

[...]
The exit status in /proc/pid/stat is zeroed out for readers that fail do_task_stat's ptrace_may_access call. (Falsifying the exit status in stat seems a privilege check fails seems like a bad idea from a correctness POV.)

It's not clear to me what the purpose of that field is within procfs for *dead* proceses -- which is what we're discussing here. As far as I can tell, you will get an ESRCH when you try to read it. When testing this it also looked like you didn't even get the exit_status as a zombie but I might be mistaken.

So while it is masked for !ptrace_may_access, it's also zero (or unreadable) for almost every case outside of stopped processes (AFAICS). Am I missing something?

Quoted 7 lines

Should open() on exithand perform the same ptrace_may_access privilege check? What if the process *becomes* untraceable during its lifetime (e.g., with setuid). Should that read() on the exithand FD still yield a siginfo_t? Just having exithand yield EOF all the time punts the privilege problem to a later discussion because this approach doesn't leak information. We can always add an "exithand_full" or something that actually yields a siginfo_t.

I agree that read(2) makes this hard. I don't think we should use it.

But if we have to use it, I would like us to have feature parity with features that FreeBSD had 18 years ago.

Quoted 5 lines

Another option would be to make exithand's read() always yield a siginfo_t, but have the open() just fail if the caller couldn't ptrace_may_access it. But why shouldn't you be able to wait on other processes? If you can see it in /proc, you should be able to wait on it exiting.

I would suggest looking at FreeBSD's kevent semantics for inspiration (or at least to see an alternative way of doing things). In particular, EVFILT_PROC+NOTE_EXIT -- which is attached to a particular process. I wonder what their view is on these sorts of questions.

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a proces to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid

entry. This interface allow lmkd to give up polling and instead block and wait for process death.

Quoted 9 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 7 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a proces to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid

Any idea why it needs to wait and then send SIGKILL? Why not do SIGKILL and look for errno == ESRCH in a loop with a delay.

Quoted 9 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 7 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a proces to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid

Any idea why it needs to wait and then send SIGKILL? Why not do SIGKILL and look for errno == ESRCH in a loop with a delay.

Quoted 2 lines

entry. This interface allow lmkd to give up polling and instead block and wait for process death.

Can we use ptrace(2) for the exit notifications? I am assuming you already though about it but I'm curious what is the reason this is better.

Thanks for taking a look.

Quoted 19 lines · Joel Fernandes

Quoted 9 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 7 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a proces to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid

Quoted 2 lines

Any idea why it needs to wait and then send SIGKILL? Why not do SIGKILL and look for errno == ESRCH in a loop with a delay.

I want to get polling loops out of the system. Polling loops are bad for wakeup attribution, bad for power, bad for priority inheritance, and bad for latency. There's no right answer to the question "How long should I wait before checking $CONDITION again?". If we can have an explicit waitqueue interface to something, we should. Besides, PID polling is vulnerable to PID reuse, whereas this mechanism (just like anything based on struct pid) is immune to it.

Quoted 26 lines · Daniel Colascione

Thanks for taking a look.

Quoted 19 lines · Joel Fernandes

Quoted 9 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 7 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a proces to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid

Quoted 2 lines

Any idea why it needs to wait and then send SIGKILL? Why not do SIGKILL and look for errno == ESRCH in a loop with a delay.

Quoted 7 lines

I want to get polling loops out of the system. Polling loops are bad for wakeup attribution, bad for power, bad for priority inheritance, and bad for latency. There's no right answer to the question "How long should I wait before checking $CONDITION again?". If we can have an explicit waitqueue interface to something, we should. Besides, PID polling is vulnerable to PID reuse, whereas this mechanism (just like anything based on struct pid) is immune to it.

The argument sounds Ok to me. I would also more details in the commit message about the alternate methods to do this (such as kill polling or ptrace) and why they don't work well etc so no one asks any questions. Like maybe under a "other ways to do this" section. A bit of googling also showed a netlink way of doing it without polling (though I don't look into that much and wouldn't be surprised if its more complicated)

Also I guess when you send a patch, it'd be good to pass "--cc-cmd='./scripts/get_maintainer.pl" to git-send-email so it automatically CCs the maintainers who maintain this.

Quoted 35 lines · Joel Fernandes

Quoted 26 lines · Daniel Colascione

Thanks for taking a look.

Quoted 19 lines · Joel Fernandes

Quoted 9 lines · Daniel Colascione

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 7 lines

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a proces to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid

Quoted 2 lines

Any idea why it needs to wait and then send SIGKILL? Why not do SIGKILL and look for errno == ESRCH in a loop with a delay.

Quoted 7 lines

I want to get polling loops out of the system. Polling loops are bad for wakeup attribution, bad for power, bad for priority inheritance, and bad for latency. There's no right answer to the question "How long should I wait before checking $CONDITION again?". If we can have an explicit waitqueue interface to something, we should. Besides, PID polling is vulnerable to PID reuse, whereas this mechanism (just like anything based on struct pid) is immune to it.

Quoted 7 lines

The argument sounds Ok to me. I would also more details in the commit message about the alternate methods to do this (such as kill polling or ptrace) and why they don't work well etc so no one asks any questions. Like maybe under a "other ways to do this" section. A bit of googling also showed a netlink way of doing it without polling (though I don't look into that much and wouldn't be surprised if its more complicated)

Thanks for taking a look. I'll add to the commit message.

Re: netlink isn't enabled everywhere and is subject to lossy buffy overruns, AIUI. You could also monitor process exit by setting up ftrace and watching events, or by installing BPF that watched for process exit and sent a perf event. :-) All of these interfaces feel like abusing a "monitoring" API for controlling system operations, and this kind of abuse tends to have ugly failure modes. I'm looking for something a bit more explicit and robust.

Sent: 29 October 2018 17:53

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

From: Daniel Colascione

Quoted 9 lines

Sent: 29 October 2018 17:53

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Why do you need an extra file?

It ought to be possible to use poll() to wait for POLLERR having set 'events' to zero on any of the nodes in /proc/pid - or even on the directory itself.

Indeed, to avoid killing the wrong process you need to have opened some node of /proc/pid/* (maybe cmdline) before sending the kill signal.

Sent: 31 October 2018 12:56

Quoted 11 lines · David Laight

From: Daniel Colascione

Quoted 9 lines

Sent: 29 October 2018 17:53

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 1 line

Why do you need an extra file?

Because no current file suffices.

Quoted 3 lines

It ought to be possible to use poll() to wait for POLLERR having set 'events' to zero on any of the nodes in /proc/pid - or even on the directory itself.

That doesn't actually work today. And waiting on a directory with POLLERR would be very weird, since directories in general don't do things like blocking reads or poll support. A separate file with self-contained, well-defined semantics is cleaner.

Quoted 3 lines

Indeed, to avoid killing the wrong process you need to have opened some node of /proc/pid/* (maybe cmdline) before sending the kill signal.

The kernel really needs better documentation of the semantics of procfs file descriptors. You're not the only person to think, mistakenly, that keeping a reference to a /proc/$PID/something FD reserves $PID and prevents it being used for another process. Procfs FDs do no such thing. kill(2) is unsafe whether or not /proc/pid/cmdline or any other /proc file is open.

From: Daniel Colascione

Quoted 13 lines

Sent: 31 October 2018 12:56

Quoted 11 lines · David Laight

From: Daniel Colascione

Quoted 9 lines

Sent: 29 October 2018 17:53

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 3 lines

Quoted 1 line

Why do you need an extra file?

Because no current file suffices.

That doesn't stop you making something work on any/all of the existing files.

Quoted 8 lines

Quoted 3 lines

It ought to be possible to use poll() to wait for POLLERR having set 'events' to zero on any of the nodes in /proc/pid - or even on the directory itself.

That doesn't actually work today. And waiting on a directory with POLLERR would be very weird, since directories in general don't do things like blocking reads or poll support. A separate file with self-contained, well-defined semantics is cleaner.

Device drivers will (well ought to) return POLLERR when a device is removed.
Making procfs behave the same way wouldn't be too stupid.

Quoted 10 lines

Quoted 3 lines

Indeed, to avoid killing the wrong process you need to have opened some node of /proc/pid/* (maybe cmdline) before sending the kill signal.

The kernel really needs better documentation of the semantics of procfs file descriptors. You're not the only person to think, mistakenly, that keeping a reference to a /proc/$PID/something FD reserves $PID and prevents it being used for another process. Procfs FDs do no such thing. kill(2) is unsafe whether or not /proc/pid/cmdline or any other /proc file is open.

Interesting.
Linux 'fixed' the problem of pid reuse in the kernel by adding (IIRC) 'struct pid' that reference counts the pid stopping reuse.

Quoted 11 lines

Quoted 10 lines

Quoted 3 lines

Indeed, to avoid killing the wrong process you need to have opened some node of /proc/pid/* (maybe cmdline) before sending the kill signal.

The kernel really needs better documentation of the semantics of procfs file descriptors. You're not the only person to think, mistakenly, that keeping a reference to a /proc/$PID/something FD reserves $PID and prevents it being used for another process. Procfs FDs do no such thing. kill(2) is unsafe whether or not /proc/pid/cmdline or any other /proc file is open.

Quoted 3 lines

Interesting.
Linux 'fixed' the problem of pid reuse in the kernel by adding (IIRC) 'struct pid' that reference counts the pid stopping reuse.

This is incorrect if you mean numeric pids. See the end of these comments in include/linux/pid.h . A pid value can be reused, it just works Ok because it causes a new struct pid allocation. That doesn't mean there isn't a numeric reuse. There's also no where in pid_alloc() where we prevent the numeric reuse AFAICT.

From: Daniel Colascione

Quoted 13 lines

Sent: 31 October 2018 12:56

Quoted 11 lines · David Laight

From: Daniel Colascione

Quoted 9 lines

Sent: 29 October 2018 17:53

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Quoted 3 lines

Quoted 1 line

Why do you need an extra file?

Because no current file suffices.

That doesn't stop you making something work on any/all of the existing files.

Quoted 4 lines

Quoted 3 lines

It ought to be possible to use poll() to wait for POLLERR having set 'events' to zero on any of the nodes in /proc/pid - or even on the directory itself.

Quoted 4 lines

That doesn't actually work today. And waiting on a directory with POLLERR would be very weird, since directories in general don't do things like blocking reads or poll support. A separate file with self-contained, well-defined semantics is cleaner.

Device drivers will (well ought to) return POLLERR when a device is removed.
Making procfs behave the same way wouldn't be too stupid.

Quoted 10 lines

Quoted 3 lines

Indeed, to avoid killing the wrong process you need to have opened some node of /proc/pid/* (maybe cmdline) before sending the kill signal.

The kernel really needs better documentation of the semantics of procfs file descriptors. You're not the only person to think, mistakenly, that keeping a reference to a /proc/$PID/something FD reserves $PID and prevents it being used for another process. Procfs FDs do no such thing. kill(2) is unsafe whether or not /proc/pid/cmdline or any other /proc file is open.

Interesting.
Linux 'fixed' the problem of pid reuse in the kernel by adding (IIRC) 'struct pid' that reference counts the pid stopping reuse.

But since the pids are still allocated sequentially userspace can still reference a pid that is freed and immediately reused.
I'd have thought that procfs nodes held a reference count on the 'struct pid'. There's probably no reason why it shouldn't.

Annoyingly non-GPL drivers can't release references to 'struct pid' so are very constrained about which processes they can signal.

I also managed to use a stale 'struct pid' and kill the wrong process
- much more likely that the pid number being reused.

If you look at the NetBSD pid allocator you'll see that it uses the low pid bits to index an array and the high bits as a sequence number. The array slots are also reused LIFO, so you always need a significant number of pid allocate/free before a number is reused.
The non-sequential allocation also makes it significantly more difficult to predict when a pid will be reused.
The table size is doubled when it gets nearly full.

This patch adds a new file under /proc/pid, /proc/pid/exithand. Attempting to read from an exithand file will block until the corresponding process exits, at which point the read will successfully complete with EOF. The file descriptor supports both blocking operations and poll(2). It's intended to be a minimal interface for allowing a program to wait for the exit of a process that is not one of its children.

Why might we want this interface? Android's lmkd kills processes in order to free memory in response to various memory pressure signals. It's desirable to wait until a killed process actually exits before moving on (if needed) to killing the next process. Since the processes that lmkd kills are not lmkd's children, lmkd currently lacks a way to wait for a proces to actually die after being sent SIGKILL; today, lmkd resorts to polling the proc filesystem pid entry. This interface allow lmkd to give up polling and instead block and wait for process death.

Signed-off-by Daniel Colascione <[email protected]>

---
 fs/proc/Makefile             |   1 +
 fs/proc/base.c               |   1 +
 fs/proc/exithand.c           | 117 +++++++++++++++++++++++++++++++++++
 fs/proc/internal.h           |   4 ++
 include/linux/sched/signal.h |   7 +++
 kernel/exit.c                |   2 +
 kernel/signal.c              |   3 +
 7 files changed, 135 insertions(+)
 create mode 100644 fs/proc/exithand.c

fs/proc/Makefile +1 -0

index ead487e80510..21322280a2c1 100644
@@ -27,6 +27,7 @@ proc-y        += softirqs.o
 proc-y += namespaces.o
 proc-y += self.o
 proc-y += thread_self.o
+proc-y  += exithand.o
 proc-$(CONFIG_PROC_SYSCTL)     += proc_sysctl.o
 proc-$(CONFIG_NET)             += proc_net.o
 proc-$(CONFIG_PROC_KCORE)      += kcore.o

fs/proc/base.c +1 -0

index 7e9f07bf260d..31bc6bbb6dc4 100644
@@ -3006,6 +3006,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_LIVEPATCH
        ONE("patch_state",  S_IRUSR, proc_pid_patch_state),
 #endif
+       REG("exithand", S_IRUGO, proc_tgid_exithand_operations),
 };
 
 static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)

fs/proc/exithand.c new file +117 -0

new file mode 100644
index 000000000000..358b08da6a08
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Synchronous exit notification of non-child processes
+ *
+ * Simple file descriptor /proc/pid/exithand. Read blocks (and poll
+ * reports non-readable) until process either dies or becomes
+ * a zombie.
+ */
+#include <linux/printk.h>
+#include <linux/sched/signal.h>
+#include <linux/poll.h>
+#include "internal.h"
+
+static int proc_tgid_exithand_open(struct inode *inode, struct file *file)
+{
+       struct task_struct* task = get_proc_task(inode);
+       /* If get_proc_task failed, it means the task is dead, which
+        * is fine, since a subsequent read will return
+        * immediately.  */
+       if (task && !thread_group_leader(task))
+               return -EINVAL;
+       return 0;
+}
+
+static ssize_t proc_tgid_exithand_read(struct file * file,
+                                      char __user * buf,
+                                      size_t count, loff_t *ppos)
+{
+       struct task_struct* task = NULL;
+       wait_queue_entry_t wait;
+       ssize_t res = 0;
+       bool locked = false;
+
+       for (;;) {
+               /* Retrieve the task from the struct pid each time
+                * through the loop in case the exact struct task
+                * changes underneath us (e.g., if in exec.c, the
+                * execing process kills the group leader and starts
+                * using its PID).  The struct signal should be the
+                * same though even in this case.
+                */
+               task = get_proc_task(file_inode(file));
+               res = 0;
+               if (!task)
+                       goto out;  /* No task?  Must have died.  */
+
+               BUG_ON(!thread_group_leader(task));
+
+               /* Synchronizes with exit.c machinery. */
+               read_lock(&tasklist_lock);
+               locked = true;
+
+               res = 0;
+               if (task->exit_state)
+                       goto out;
+
+               res = -EAGAIN;
+               if (file->f_flags & O_NONBLOCK)
+                       goto out;
+
+               /* Tell exit.c to go to the trouble of waking our
+                * runqueue when this process gets around to
+                * exiting. */
+               task->signal->exithand_is_interested = true;
+
+               /* Even if the task identity changes, task->signal
+                * should be invariant across the wait, making it safe
+                * to go remove our wait record from the wait queue
+                * after we come back from schedule.  */
+
+               init_waitqueue_entry(&wait, current);
+               add_wait_queue(&wait_exithand, &wait);
+
+               read_unlock(&tasklist_lock);
+               locked = false;
+
+               put_task_struct(task);
+               task = NULL;
+
+               set_current_state(TASK_INTERRUPTIBLE);
+               schedule();
+               set_current_state(TASK_RUNNING);
+               remove_wait_queue(&wait_exithand, &wait);
+
+               res = -ERESTARTSYS;
+               if (signal_pending(current))
+                       goto out;
+       }
+out:
+       if (locked)
+               read_unlock(&tasklist_lock);
+       if (task)
+               put_task_struct(task);
+       return res;
+}
+
+static __poll_t proc_tgid_exithand_poll(struct file *file, poll_table *wait)
+{
+       __poll_t mask = 0;
+       struct task_struct* task = get_proc_task(file_inode(file));
+       if (!task) {
+               mask |= POLLIN;
+       } else if (READ_ONCE(task->exit_state)) {
+               mask |= POLLIN;
+       } else {
+               read_lock(&tasklist_lock);
+               task->signal->exithand_is_interested = true;
+               read_unlock(&tasklist_lock);
+               poll_wait(file, &wait_exithand, wait);
+       }
+       return mask;
+}
+
+const struct file_operations proc_tgid_exithand_operations = {
+       .open           = proc_tgid_exithand_open,
+       .read           = proc_tgid_exithand_read,
+       .poll           = proc_tgid_exithand_poll,
+};

fs/proc/internal.h +4 -0

index 5185d7f6a51e..1009d20475bc 100644
@@ -304,3 +304,7 @@ extern unsigned long task_statm(struct mm_struct *,
                                unsigned long *, unsigned long *,
                                unsigned long *, unsigned long *);
 extern void task_mem(struct seq_file *, struct mm_struct *);
+
+/* exithand.c */
+
+extern const struct file_operations proc_tgid_exithand_operations;

include/linux/sched/signal.h +7 -0

index 13789d10a50e..44131cb6c7f4 100644
@@ -74,6 +74,10 @@ struct multiprocess_signals {
        struct hlist_node node;
 };
 
+/* Need to stick the waitq for exithand outside process structures in
+ * case a process disappears across a poll.  */
+extern wait_queue_head_t wait_exithand;
+
 /*
  * NOTE! "signal_struct" does not have its own
  * locking, because a shared signal_struct always

@@ -87,6 +91,9 @@ struct signal_struct {
        int                     nr_threads;
        struct list_head        thread_head;
 
+       /* Protected with tasklist_lock.  */
+       bool                    exithand_is_interested;
+
        wait_queue_head_t       wait_chldexit;  /* for wait4() */
 
        /* current thread group signal load-balancing target: */

kernel/exit.c +2 -0

index 0e21e6d21f35..44a4e3796f8b 100644
@@ -1485,6 +1485,8 @@ void __wake_up_parent(struct task_struct *p, struct task_struct *parent)
 {
        __wake_up_sync_key(&parent->signal->wait_chldexit,
                                TASK_INTERRUPTIBLE, 1, p);
+       if (p->signal->exithand_is_interested)
+               __wake_up_sync(&wait_exithand, TASK_INTERRUPTIBLE, 0);
 }
 
 static long do_wait(struct wait_opts *wo)

kernel/signal.c +3 -1

index 17565240b1c6..e156d48da70a 100644
@@ -454,6 +454,9 @@ void flush_sigqueue(struct sigpending *queue)
        }
 }
 
+wait_queue_head_t wait_exithand =
+       __WAIT_QUEUE_HEAD_INITIALIZER(wait_exithand);
+
 /*
  * Flush all pending signals for this kthread.
  */
--

2.19.1.568.g152ad8e336-goog

I had an old patch to do much the same thing:

https://lore.kernel.org/patchwork/patch/345098/

Can you comment as to how your API compares to my old patch?

You’re using
some fairly gnarly global synchronization

, and that seems unnecessary