LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Christian Brauner <christian@brauner.io>
To: Florian Weimer <fweimer@redhat.com>
Cc: ebiederm@xmission.com, linux-kernel@vger.kernel.org,
	serge@hallyn.com, jannh@google.com, luto@kernel.org,
	akpm@linux-foundation.org, oleg@redhat.com, cyphar@cyphar.com,
	viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
	linux-api@vger.kernel.org, dancol@google.com,
	timmurray@google.com, linux-man@vger.kernel.org,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v2] signal: add procfd_signal() syscall
Date: Mon, 3 Dec 2018 19:02:29 +0100
Message-ID: <20181203180224.fkvw4kajtbvru2ku@brauner.io> (raw)
In-Reply-To: <8736rebl9s.fsf@oldenburg.str.redhat.com>

On Mon, Dec 03, 2018 at 05:57:51PM +0100, Florian Weimer wrote:
> * Christian Brauner:
> 
> > Ok, I finally have access to source code again. Scratch what I said above!
> > I looked at the code and tested it. If the process has exited but not
> > yet waited upon aka is a zombie procfd_send_signal() will return 0. This
> > is identical to kill(2) behavior. It should've been sort-of obvious
> > since when a process is in zombie state /proc/<pid> will still be around
> > which means that struct pid must still be around.
> 
> Should we make this state more accessible, by providing a different
> error code?

No, I don't think we want that. Imho, It's not really helpful. Signals
are still delivered to zombies. If zombie state were to always mean that
no-one is going to wait on this thread anymore then it would make sense
to me. But given that zombie can also mean that someone put a
sleep(1000) right before their wait() call in the parent it seems odd to
report back that it is a zombie.

> 
> Will the system call ever return ESRCH, given that you have a handle for
> the process?

Yes, whenever you signal a process that has already been waited upon:
- get procfd handle referring to <proc>
- <proc> exits and is waited upon
- procfd_send_signal(procfd, ...) returns -1 with errno == ESRCH

> 
> >> >Looking at the rt_tgsigqueueinfo interface, is there a way to implement
> >> >the “tg” part with the current procfd_signal interface?  Would you use
> >> >openat to retrieve the Tgid: line from "status"?
> > 
> > Yes, the tg part can be implemented.
> 
> I meant on top of the existing interface.

See below.

> 
> > As I pointed out in another mail my I is to make this work by using
> > file descriptors for /proc/<pid>/task/<tid>.  I don't want this in the
> > initial patchset though.  I prefer to slowly add those features once
> > we have gotten the basic functionality in.
> 
> Do you want to land all this in one kernel release?  I wonder how
> applications are supposed to discover kernel support if functionality is
> split across several kernel releases.  If you get EINVAL or EBADF, it
> may not be obvious what is going on.

Sigh, I get that but I really don't want to have to land this in one big
chunk. I want this syscall to go in in a as soon as we can to fulfill
the most basic need: having a way that guarantees us that we signal the
process that we intended to signal.

The thread case is easy to implement on top of it. But I suspect we will
quibble about the exact semantics for a long time. Even now we have been
on multiple - justified - detrous. That's all pefectly fine and
expected. But if we have the basic functionality in we have time to do
all of that. We might even land it in the same kernel release still. I
really don't want to come of as tea-party-kernel-conservative here but I
have time-and-time again seen that making something fancy and cover ever
interesting feature in one patchset takes a very very long time.

If you care about userspace being able to detect that case I can return
EOPNOTSUPP when a tid descriptor is passed.

> 
> What happens if you use the new interface with an O_PATH descriptor?

You get EINVAL. When an O_PATH file descriptor is created the kernel
will set file->f_op = &empty_fops at which point the check I added 
        if (!proc_is_tgid_procfd(f.file))
                goto err;
will fail. Imho this is correct behavior since technically signaling a
struct pid is the equivalent of writing to a file and hence doesn't
purely operate on the file descriptor level.

Christian

  reply index

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-20 10:51 Christian Brauner
2018-11-20 10:51 ` [PATCH v2] procfd_signal.2: document procfd_signal syscall Christian Brauner
2018-11-22  8:00 ` [PATCH v2] signal: add procfd_signal() syscall Serge E. Hallyn
2018-11-22  8:23 ` Aleksa Sarai
2018-11-28 14:05 ` Arnd Bergmann
2018-11-29 12:28 ` Florian Weimer
2018-11-29 16:54   ` Andy Lutomirski
2018-11-29 19:16     ` Christian Brauner
2018-11-29 19:22       ` Andy Lutomirski
2018-11-29 19:55         ` Christian Brauner
2018-11-29 20:14           ` Andy Lutomirski
2018-11-29 21:02             ` Arnd Bergmann
2018-11-29 21:35               ` Christian Brauner
2018-11-29 21:40                 ` Arnd Bergmann
2018-11-30  2:40                   ` Aleksa Sarai
2018-12-01  1:25                   ` Christian Brauner
2018-11-30  5:13               ` ebiederm
2018-11-30  6:56                 ` Christian Brauner
2018-11-30 11:41                   ` Arnd Bergmann
2018-11-30 16:35                     ` Andy Lutomirski
2018-11-30 21:57                       ` Christian Brauner
2018-11-30 22:09                       ` Arnd Bergmann
2018-11-30 22:26                         ` Christian Brauner
2018-11-30 23:05                           ` Daniel Colascione
2018-11-30 23:12                             ` Arnd Bergmann
2018-11-30 23:15                               ` Arnd Bergmann
2018-11-30 23:37                               ` Christian Brauner
2018-11-30 23:46                                 ` Andy Lutomirski
2018-12-01  1:20                                   ` Christian Brauner
2018-11-30 23:53                         ` Andy Lutomirski
2018-12-01  8:51                           ` Arnd Bergmann
2018-12-01  9:17                             ` Christian Brauner
2018-12-01 10:27                             ` Arnd Bergmann
2018-12-01 13:41                       ` ebiederm
2018-12-01 14:46                     ` ebiederm
2018-12-01 15:28                       ` ebiederm
2018-12-01 15:52                         ` Andy Lutomirski
2018-12-01 16:27                           ` Christian Brauner
2018-12-02  0:06                           ` ebiederm
2018-12-02  1:14                             ` Andy Lutomirski
2018-12-02  8:52                         ` Christian Brauner
2018-11-30 23:52   ` Christian Brauner
2018-12-02 10:03     ` Christian Brauner
2018-12-03 16:57       ` Florian Weimer
2018-12-03 18:02         ` Christian Brauner [this message]
2018-12-04  6:03           ` Aleksa Sarai
2018-12-04 12:55           ` Florian Weimer
2018-12-04 13:26             ` Christian Brauner
2018-12-06 18:54             ` Andy Lutomirski
2018-12-06 18:56               ` Florian Weimer
2018-12-06 19:03                 ` Christian Brauner
2018-12-25  5:32                   ` Lai Jiangshan
2018-12-25  7:11                     ` Lai Jiangshan
2018-12-25 12:07                       ` Aleksa Sarai

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181203180224.fkvw4kajtbvru2ku@brauner.io \
    --to=christian@brauner.io \
    --cc=akpm@linux-foundation.org \
    --cc=cyphar@cyphar.com \
    --cc=dancol@google.com \
    --cc=ebiederm@xmission.com \
    --cc=fweimer@redhat.com \
    --cc=jannh@google.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-man@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=oleg@redhat.com \
    --cc=serge@hallyn.com \
    --cc=timmurray@google.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git