Thread (14 messages) flat view 14 messages, 3 authors, 2025-01-07

Re: [PATCH v6 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process

From: Christian Brauner <brauner@kernel.org>
Date: 2024-12-02 14:31:23
Also in: linux-fsdevel, linux-kselftest, linux-mm, lkml
Subsystem: the rest · Maintainer: Linus Torvalds 

On Wed, Oct 30, 2024 at 04:37:37PM +0000, Lorenzo Stoakes wrote:
On Mon, Oct 28, 2024 at 04:06:07PM +0000, Lorenzo Stoakes wrote:
quoted
I guess I'll try to adapt that and respin a v7 when I get a chance.
Hm looking at this draft patch, it seems like a total rework of pidfd's
across the board right (now all pidfd's will need to be converted to
pid_fd)? Correct me if I'm wrong.

If only for the signal case, it seems like overkill to define a whole
pid_fd and to use this CLASS() wrapper just for this one instance.

If the intent is to convert _all_ pidfd's to use this type, it feels really
out of scope for this series and I think we'd probably instead want to go
off and do that as a separate series and put this on hold until that is
done.

If instead you mean that we ought to do something like this just for the
signal case, it feels like it'd be quite a bit of extra abstraction just
used in this one case but nowhere else, I think if you did an abstraction
like this it would _have_ to be across the board right?

I agree that the issue is with this one signal case that pins only the fd
(rather than this pid) where this 'pinning' doesn't _necessary_ mess around
with reference counts.

So we definitely must address this, but the issue you had with the first
approach was that I think (correct me if I'm wrong) I was passing a pointer
to a struct fd which is not permitted right?

Could we pass the struct fd by value to avoid this? I think we'd have to
unfortunately special-case this and probably duplicate some code which is a
pity as I liked the idea of abstracting everything to one place, but we can
obviously do that.

So I guess to TL;DR it, the options are:

1. Implement pid_fd everywhere, in which case I will leave off on
   this series and I guess, if I have time I could look at trying to
   implement that or perhaps you'd prefer to?

2. We are good for the sake of this series to special-case a pidfd_to_pid()
   implementation (used only by the pidfd_send_signal() syscall)

3. Something else, or I am misunderstanding your point :)

Let me know how you want me to proceed on this as we're at v6 already and I
want to be _really_ sure I'm doing what you want here.
I don't think we get away with abstracting it in one place without this
ending up a pretty janky api. I need to go back and think about calling
conventions for all this stuff. For now I think I'm fine with something
like the below that abstracts the api to handle mm/ cleanly and then a
special-case for pidfd_send_signal():

diff --git a/kernel/pid.c b/kernel/pid.c
index 6131543e7c09..c1857c44d1a3 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -564,15 +564,29 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
  */
 struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags)
 {
-       unsigned int f_flags;
+       unsigned int f_flags = 0;
        struct pid *pid;
        struct task_struct *task;
+       enum pid_type type;

-       pid = pidfd_get_pid(pidfd, &f_flags);
-       if (IS_ERR(pid))
-               return ERR_CAST(pid);
+       switch (pidfd) {
+       case PIDFD_SELF_THREAD:
+               type = PIDTYPE_PID;
+               pid = get_task_pid(current, type);
+               break;
+       case PIDFD_SELF_THREAD_GROUP:
+               type = PIDTYPE_TGID;
+               pid = get_task_pid(current, type);
+               break;
+       default:
+               pid = pidfd_get_pid(pidfd, &f_flags);
+               if (IS_ERR(pid))
+                       return ERR_CAST(pid);
+               type = PIDTYPE_TGID;
+               break;
+       }

-       task = get_pid_task(pid, PIDTYPE_TGID);
+       task = get_pid_task(pid, type);
        put_pid(pid);
        if (!task)
                return ERR_PTR(-ESRCH);

That would get you support for PIDFD_SELF_THREAD and
PIDFD_SELF_THREAD_GROUP for process_madvise() and process_mrelease().

And for pidfd_send_signal() we could just open code this for now:

diff --git a/kernel/signal.c b/kernel/signal.c
index 989b1cc9116a..a2e4e3a5ee42 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3990,6 +3990,45 @@ static struct pid *pidfd_to_pid(const struct file *file)
 	(PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
 	 PIDFD_SIGNAL_PROCESS_GROUP)
 
+static int do_pidfd_send_signal(struct pid *pid, int sig, enum pid_type type,
+				siginfo_t __user *info, unsigned int flags)
+{
+	kernel_siginfo_t kinfo;
+
+	switch (flags) {
+	case PIDFD_SIGNAL_THREAD:
+		type = PIDTYPE_PID;
+		break;
+	case PIDFD_SIGNAL_THREAD_GROUP:
+		type = PIDTYPE_TGID;
+		break;
+	case PIDFD_SIGNAL_PROCESS_GROUP:
+		type = PIDTYPE_PGID;
+		break;
+	}
+
+	if (info) {
+		int ret = copy_siginfo_from_user_any(&kinfo, info);
+		if (unlikely(ret))
+			return ret;
+
+		if (unlikely(sig != kinfo.si_signo))
+			return -EINVAL;
+
+		/* Only allow sending arbitrary signals to yourself. */
+		if ((task_pid(current) != pid || type > PIDTYPE_TGID) &&
+		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
+			return -EPERM;
+	} else {
+		prepare_kill_siginfo(sig, &kinfo, type);
+	}
+
+	if (type == PIDTYPE_PGID)
+		return kill_pgrp_info(sig, &kinfo, pid);
+
+	return kill_pid_info_type(sig, &kinfo, pid, type);
+}
+
 /**
  * sys_pidfd_send_signal - Signal a process through a pidfd
  * @pidfd:  file descriptor of the process
@@ -4009,7 +4048,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
 {
 	int ret;
 	struct pid *pid;
-	kernel_siginfo_t kinfo;
 	enum pid_type type;
 
 	/* Enforce flags be set to 0 until we add an extension. */
@@ -4021,56 +4059,39 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
 		return -EINVAL;
 
 	CLASS(fd, f)(pidfd);
-	if (fd_empty(f))
-		return -EBADF;
 
-	/* Is this a pidfd? */
-	pid = pidfd_to_pid(fd_file(f));
-	if (IS_ERR(pid))
-		return PTR_ERR(pid);
+	switch (pidfd) {
+	case PIDFD_SELF_THREAD:
+		pid = get_task_pid(current, PIDTYPE_PID);
+		type = PIDTYPE_PID;
+		break;
+	case PIDFD_SELF_THREAD_GROUP:
+		pid = get_task_pid(current, PIDTYPE_TGID);
+		type = PIDTYPE_TGID;
+		break;
+	default:
+		if (fd_empty(f))
+			return -EBADF;
 
-	if (!access_pidfd_pidns(pid))
-		return -EINVAL;
+		/* Is this a pidfd? */
+		pid = pidfd_to_pid(fd_file(f));
+		if (IS_ERR(pid))
+			return PTR_ERR(pid);
 
-	switch (flags) {
-	case 0:
+		if (!access_pidfd_pidns(pid))
+			return -EINVAL;
 		/* Infer scope from the type of pidfd. */
 		if (fd_file(f)->f_flags & PIDFD_THREAD)
 			type = PIDTYPE_PID;
 		else
 			type = PIDTYPE_TGID;
 		break;
-	case PIDFD_SIGNAL_THREAD:
-		type = PIDTYPE_PID;
-		break;
-	case PIDFD_SIGNAL_THREAD_GROUP:
-		type = PIDTYPE_TGID;
-		break;
-	case PIDFD_SIGNAL_PROCESS_GROUP:
-		type = PIDTYPE_PGID;
-		break;
 	}
 
-	if (info) {
-		ret = copy_siginfo_from_user_any(&kinfo, info);
-		if (unlikely(ret))
-			return ret;
-
-		if (unlikely(sig != kinfo.si_signo))
-			return -EINVAL;
-
-		/* Only allow sending arbitrary signals to yourself. */
-		if ((task_pid(current) != pid || type > PIDTYPE_TGID) &&
-		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
-			return -EPERM;
-	} else {
-		prepare_kill_siginfo(sig, &kinfo, type);
-	}
-
-	if (type == PIDTYPE_PGID)
-		return kill_pgrp_info(sig, &kinfo, pid);
-	else
-		return kill_pid_info_type(sig, &kinfo, pid, type);
+	ret = do_pidfd_send_signal(pid, sig, type, info, flags);
+	if (fd_empty(f))
+		put_pid(pid);
+	return ret;
 }
 
 static int
Other recent patches touching these files
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help