Linux userland API discussions
 help / color / mirror / Atom feed
* Re: [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier
From: Guenter Roeck @ 2019-01-03 18:58 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
	LKML, netdev, netfilter-devel, luto, carlos, viro, dhowells, simo,
	eparis, serge, ebiederm
In-Reply-To: <20190103173613.jb5kukysslozytwg@madcap2.tricolour.ca>

Hi Richard,

On Thu, Jan 03, 2019 at 12:36:13PM -0500, Richard Guy Briggs wrote:
> On 2019-01-03 08:15, Guenter Roeck wrote:
> > Hi,
> > 
> > On Tue, Jul 31, 2018 at 04:07:35PM -0400, Richard Guy Briggs wrote:
> > > Implement kernel audit container identifier.
> > 
> > I don't see a follow-up submission of this patch series. Has it been abandoned,
> > or do I use the wrong search terms ?
> 
> Guenter, thanks for your interest in this patchset.  I haven't
> abandoned it.  I've pushed some updates to my own (ill-publicized)
> public git repo.  This effort has been going on more than 5 years with 8

Oh man :-(. Not sure if I would be that patient.

Can you point me to your repository ?

> previous revisions trying to document task namespaces and deciding that
> was insufficient.
> 

My interest is mostly thanks to having some of the patches of your series
in my incoming code review queue:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1379654/3

As background, some of the patches in the series are needed by GCP (Google
Cloud Platform) as a prerequisite for some security features. Having to
maintain out-of-tree code is always a pain, even more so in a subsystem
related to security. So it would be quite useful to understand if we are
going to be stuck with this forever or if there is a change for the code
to find its way upstream. Also, it would be useful to know if there are
some upcoming changes/improvements which should be included in our version.

Thanks,
Guenter

> For this patchset I waited 11.5 weeks (80 days, Jules Verne anyone?)
> before the primary intended maintainer did the first review, then I
> responded within 2 weeks with further questions and a followup patch
> proposal and then waited another 8 weeks for any response before adding
> another query for that followup patch proposal review at which point I
> got a rude answer saying I had disappointed and exhausted the
> maintainer's goodwill with some hints at how to proceed just before new
> year's.
> 
> I'd be delighted with other upstream review to get other angles and to
> take some of the load and responsibility off the primary maintainer.
> 
> I expect to submit a v5 within a week without having had those questions
> directly answered, but with some ideas of what to check and verify
> before I resubmit.  Most of the changes have been sitting in that branch
> for two months, already rebased one kernel version and will need
> updating again.
> 
> > Thanks,
> > Guenter
> > 
> > > This patchset is a fourth based on the proposal document (V3)
> > > posted:
> > > 	https://www.redhat.com/archives/linux-audit/2018-January/msg00014.html
> > > 
> > > The first patch is the last patch from ghak81 that is included here as a
> > > convenience.
> > > 
> > > The second patch implements the proc fs write to set the audit container
> > > identifier of a process, emitting an AUDIT_CONTAINER_OP record to announce the
> > > registration of that audit container identifier on that process.  This patch
> > > requires userspace support for record acceptance and proper type
> > > display.
> > > 
> > > The third implements the auxiliary record AUDIT_CONTAINER if an
> > > audit container identifier is identifiable with an event.  This patch
> > > requires userspace support for proper type display.
> > > 
> > > The 4th adds signal and ptrace support.
> > > 
> > > The 5th creates a local audit context to be able to bind a standalone
> > > record with a locally created auxiliary record.
> > > 
> > > The 6th patch adds audit container identifier records to the tty
> > > standalone record.
> > > 
> > > The 7th adds audit container identifier filtering to the exit,
> > > exclude and user lists.  This patch adds the AUDIT_CONTID field and
> > > requires auditctl userspace support for the --contid option.
> > > 
> > > The 8th adds network namespace audit container identifier labelling
> > > based on member tasks' audit container identifier labels.
> > > 
> > > The 9th adds audit container identifier support to standalone netfilter
> > > records that don't have a task context and lists each container to which
> > > that net namespace belongs.
> > > 
> > > The 10th implements reading the audit container identifier from the proc
> > > filesystem for debugging.  This patch isn't planned for upstream
> > > inclusion.
> > > 
> > > 
> > > Example: Set an audit container identifier of 123456 to the "sleep" task:
> > > 
> > >   sleep 2&  
> > >   child=$!
> > >   echo 123456 > /proc/$child/audit_containerid; echo $?
> > >   ausearch -ts recent -m container
> > >   echo child:$child contid:$( cat /proc/$child/audit_containerid)
> > > 
> > > This should produce a record such as:
> > > 
> > >   type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes 
> > > 
> > > 
> > > Example: Set a filter on an audit container identifier 123459 on /tmp/tmpcontainerid:
> > > 
> > >   contid=123459
> > >   key=tmpcontainerid
> > >   auditctl -a exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> > >   perl -e "sleep 1; open(my \$tmpfile, '>', \"/tmp/$key\"); close(\$tmpfile);" &
> > >   child=$!
> > >   echo $contid > /proc/$child/audit_containerid
> > >   sleep 2
> > >   ausearch -i -ts recent -k $key
> > >   auditctl -d exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> > >   rm -f /tmp/$key
> > > 
> > > This should produce an event such as:
> > > 
> > >   type=CONTAINER msg=audit(2018-06-06 12:46:31.707:26953) : op=task contid=123459 
> > >   type=PROCTITLE msg=audit(2018-06-06 12:46:31.707:26953) : proctitle=perl -e sleep 1; open(my $tmpfile, '>', "/tmp/tmpcontainerid"); close($tmpfile); 
> > >   type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=1 name=/tmp/tmpcontainerid inode=25656 dev=00:26 mode=file,644 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 
> > >   type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=0 name=/tmp/ inode=8985 dev=00:26 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 
> > >   type=CWD msg=audit(2018-06-06 12:46:31.707:26953) : cwd=/root 
> > >   type=SYSCALL msg=audit(2018-06-06 12:46:31.707:26953) : arch=x86_64 syscall=openat success=yes exit=3 a0=0xffffffffffffff9c a1=0x5621f2b81900 a2=O_WRONLY|O_CREAT|O_TRUNC a3=0x1b6 items=2 ppid=628 pid=2232 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=tmpcontainerid 
> > > 
> > > 
> > > Includes: https://github.com/linux-audit/audit-kernel/issues/81
> > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > See: https://github.com/linux-audit/audit-userspace/issues/40
> > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > 
> > > Changelog:
> > > 
> > > v4
> > > - preface set with ghak81:"collect audit task parameters"
> > > - add shallyn and sgrubb acks
> > > - rename feature bitmap macro
> > > - rename cid_valid() to audit_contid_valid()
> > > - rename AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP
> > > - delete audit_get_contid_list() from headers
> > > - move work into inner if, delete "found"
> > > - change netns contid list function names
> > > - move exports for audit_log_contid audit_alloc_local audit_free_context to non-syscall patch
> > > - list contids CSV
> > > - pass in gfp flags to audit_alloc_local() (fix audit_alloc_context callers)
> > > - use "local" in lieu of abusing in_syscall for auditsc_get_stamp()
> > > - read_lock(&tasklist_lock) around children and thread check
> > > - task_lock(tsk) should be taken before first check of tsk->audit
> > > - add spin lock to contid list in aunet
> > > - restrict /proc read to CAP_AUDIT_CONTROL
> > > - remove set again prohibition and inherited flag
> > > - delete contidion spelling fix from patchset, send to netdev/linux-wireless
> > > 
> > > v3
> > > - switched from containerid in task_struct to audit_task_info (depends on ghak81)
> > > - drop INVALID_CID in favour of only AUDIT_CID_UNSET
> > > - check for !audit_task_info, throw -ENOPROTOOPT on set
> > > - changed -EPERM to -EEXIST for parent check
> > > - return AUDIT_CID_UNSET if !audit_enabled
> > > - squash child/thread check patch into AUDIT_CONTAINER_ID patch
> > > - changed -EPERM to -EBUSY for child check
> > > - separate child and thread checks, use -EALREADY for latter
> > > - move addition of op= from ptrace/signal patch to AUDIT_CONTAINER patch
> > > - fix && to || bashism in ptrace/signal patch
> > > - uninline and export function for audit_free_context()
> > > - drop CONFIG_CHANGE, FEATURE_CHANGE, ANOM_ABEND, ANOM_SECCOMP patches
> > > - move audit_enabled check (xt_AUDIT)
> > > - switched from containerid list in struct net to net_generic's struct audit_net
> > > - move containerid list iteration into audit (xt_AUDIT)
> > > - create function to move namespace switch into audit
> > > - switched /proc/PID/ entry from containerid to audit_containerid
> > > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_alloc_context()
> > > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_log_container_info()
> > > - use xt_net(par) instead of sock_net(skb->sk) to get net
> > > - switched record and field names: initial CONTAINER_ID, aux CONTAINER, field CONTID
> > > - allow to set own contid
> > > - open code audit_set_containerid
> > > - add contid inherited flag
> > > - ccontainerid and pcontainerid eliminated due to inherited flag
> > > - change name of container list funcitons
> > > - rename containerid to contid
> > > - convert initial container record to syscall aux
> > > - fix spelling mistake of contidion in net/rfkill/core.c to avoid contid name collision
> > > 
> > > v2
> > > - add check for children and threads
> > > - add network namespace container identifier list
> > > - add NETFILTER_PKT audit container identifier logging
> > > - patch description and documentation clean-up and example
> > > - reap unused ppid
> > > 
> > > Richard Guy Briggs (10):
> > >   audit: collect audit task parameters
> > >   audit: add container id
> > >   audit: log container info of syscalls
> > >   audit: add containerid support for ptrace and signals
> > >   audit: add support for non-syscall auxiliary records
> > >   audit: add containerid support for tty_audit
> > >   audit: add containerid filtering
> > >   audit: add support for containerid to network namespaces
> > >   audit: NETFILTER_PKT: record each container ID associated with a netNS
> > >   debug audit: read container ID of a process
> > > 
> > >  drivers/tty/tty_audit.c    |   5 +-
> > >  fs/proc/base.c             |  56 ++++++++++++++
> > >  include/linux/audit.h      |  95 ++++++++++++++++++++---
> > >  include/linux/sched.h      |   5 +-
> > >  include/uapi/linux/audit.h |   8 +-
> > >  init/init_task.c           |   3 +-
> > >  init/main.c                |   2 +
> > >  kernel/audit.c             | 137 +++++++++++++++++++++++++++++++++
> > >  kernel/audit.h             |   4 +
> > >  kernel/auditfilter.c       |  47 ++++++++++++
> > >  kernel/auditsc.c           | 183 ++++++++++++++++++++++++++++++++++++++++-----
> > >  kernel/fork.c              |   4 +-
> > >  kernel/nsproxy.c           |   4 +
> > >  net/netfilter/xt_AUDIT.c   |  12 ++-
> > >  14 files changed, 526 insertions(+), 39 deletions(-)
> > > 
> > > -- 
> > > 1.8.3.1
> > > 
> 
> - RGB
> 
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635

^ permalink raw reply

* Re: [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier
From: Richard Guy Briggs @ 2019-01-03 17:36 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
	LKML, netdev, netfilter-devel, luto, carlos, viro, dhowells, simo,
	eparis, serge, ebiederm
In-Reply-To: <20190103161525.GA6551@roeck-us.net>

On 2019-01-03 08:15, Guenter Roeck wrote:
> Hi,
> 
> On Tue, Jul 31, 2018 at 04:07:35PM -0400, Richard Guy Briggs wrote:
> > Implement kernel audit container identifier.
> 
> I don't see a follow-up submission of this patch series. Has it been abandoned,
> or do I use the wrong search terms ?

Guenter, thanks for your interest in this patchset.  I haven't
abandoned it.  I've pushed some updates to my own (ill-publicized)
public git repo.  This effort has been going on more than 5 years with 8
previous revisions trying to document task namespaces and deciding that
was insufficient.

For this patchset I waited 11.5 weeks (80 days, Jules Verne anyone?)
before the primary intended maintainer did the first review, then I
responded within 2 weeks with further questions and a followup patch
proposal and then waited another 8 weeks for any response before adding
another query for that followup patch proposal review at which point I
got a rude answer saying I had disappointed and exhausted the
maintainer's goodwill with some hints at how to proceed just before new
year's.

I'd be delighted with other upstream review to get other angles and to
take some of the load and responsibility off the primary maintainer.

I expect to submit a v5 within a week without having had those questions
directly answered, but with some ideas of what to check and verify
before I resubmit.  Most of the changes have been sitting in that branch
for two months, already rebased one kernel version and will need
updating again.

> Thanks,
> Guenter
> 
> > This patchset is a fourth based on the proposal document (V3)
> > posted:
> > 	https://www.redhat.com/archives/linux-audit/2018-January/msg00014.html
> > 
> > The first patch is the last patch from ghak81 that is included here as a
> > convenience.
> > 
> > The second patch implements the proc fs write to set the audit container
> > identifier of a process, emitting an AUDIT_CONTAINER_OP record to announce the
> > registration of that audit container identifier on that process.  This patch
> > requires userspace support for record acceptance and proper type
> > display.
> > 
> > The third implements the auxiliary record AUDIT_CONTAINER if an
> > audit container identifier is identifiable with an event.  This patch
> > requires userspace support for proper type display.
> > 
> > The 4th adds signal and ptrace support.
> > 
> > The 5th creates a local audit context to be able to bind a standalone
> > record with a locally created auxiliary record.
> > 
> > The 6th patch adds audit container identifier records to the tty
> > standalone record.
> > 
> > The 7th adds audit container identifier filtering to the exit,
> > exclude and user lists.  This patch adds the AUDIT_CONTID field and
> > requires auditctl userspace support for the --contid option.
> > 
> > The 8th adds network namespace audit container identifier labelling
> > based on member tasks' audit container identifier labels.
> > 
> > The 9th adds audit container identifier support to standalone netfilter
> > records that don't have a task context and lists each container to which
> > that net namespace belongs.
> > 
> > The 10th implements reading the audit container identifier from the proc
> > filesystem for debugging.  This patch isn't planned for upstream
> > inclusion.
> > 
> > 
> > Example: Set an audit container identifier of 123456 to the "sleep" task:
> > 
> >   sleep 2&  
> >   child=$!
> >   echo 123456 > /proc/$child/audit_containerid; echo $?
> >   ausearch -ts recent -m container
> >   echo child:$child contid:$( cat /proc/$child/audit_containerid)
> > 
> > This should produce a record such as:
> > 
> >   type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes 
> > 
> > 
> > Example: Set a filter on an audit container identifier 123459 on /tmp/tmpcontainerid:
> > 
> >   contid=123459
> >   key=tmpcontainerid
> >   auditctl -a exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> >   perl -e "sleep 1; open(my \$tmpfile, '>', \"/tmp/$key\"); close(\$tmpfile);" &
> >   child=$!
> >   echo $contid > /proc/$child/audit_containerid
> >   sleep 2
> >   ausearch -i -ts recent -k $key
> >   auditctl -d exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> >   rm -f /tmp/$key
> > 
> > This should produce an event such as:
> > 
> >   type=CONTAINER msg=audit(2018-06-06 12:46:31.707:26953) : op=task contid=123459 
> >   type=PROCTITLE msg=audit(2018-06-06 12:46:31.707:26953) : proctitle=perl -e sleep 1; open(my $tmpfile, '>', "/tmp/tmpcontainerid"); close($tmpfile); 
> >   type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=1 name=/tmp/tmpcontainerid inode=25656 dev=00:26 mode=file,644 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 
> >   type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=0 name=/tmp/ inode=8985 dev=00:26 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 
> >   type=CWD msg=audit(2018-06-06 12:46:31.707:26953) : cwd=/root 
> >   type=SYSCALL msg=audit(2018-06-06 12:46:31.707:26953) : arch=x86_64 syscall=openat success=yes exit=3 a0=0xffffffffffffff9c a1=0x5621f2b81900 a2=O_WRONLY|O_CREAT|O_TRUNC a3=0x1b6 items=2 ppid=628 pid=2232 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=tmpcontainerid 
> > 
> > 
> > Includes: https://github.com/linux-audit/audit-kernel/issues/81
> > See: https://github.com/linux-audit/audit-kernel/issues/90
> > See: https://github.com/linux-audit/audit-userspace/issues/40
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > 
> > Changelog:
> > 
> > v4
> > - preface set with ghak81:"collect audit task parameters"
> > - add shallyn and sgrubb acks
> > - rename feature bitmap macro
> > - rename cid_valid() to audit_contid_valid()
> > - rename AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP
> > - delete audit_get_contid_list() from headers
> > - move work into inner if, delete "found"
> > - change netns contid list function names
> > - move exports for audit_log_contid audit_alloc_local audit_free_context to non-syscall patch
> > - list contids CSV
> > - pass in gfp flags to audit_alloc_local() (fix audit_alloc_context callers)
> > - use "local" in lieu of abusing in_syscall for auditsc_get_stamp()
> > - read_lock(&tasklist_lock) around children and thread check
> > - task_lock(tsk) should be taken before first check of tsk->audit
> > - add spin lock to contid list in aunet
> > - restrict /proc read to CAP_AUDIT_CONTROL
> > - remove set again prohibition and inherited flag
> > - delete contidion spelling fix from patchset, send to netdev/linux-wireless
> > 
> > v3
> > - switched from containerid in task_struct to audit_task_info (depends on ghak81)
> > - drop INVALID_CID in favour of only AUDIT_CID_UNSET
> > - check for !audit_task_info, throw -ENOPROTOOPT on set
> > - changed -EPERM to -EEXIST for parent check
> > - return AUDIT_CID_UNSET if !audit_enabled
> > - squash child/thread check patch into AUDIT_CONTAINER_ID patch
> > - changed -EPERM to -EBUSY for child check
> > - separate child and thread checks, use -EALREADY for latter
> > - move addition of op= from ptrace/signal patch to AUDIT_CONTAINER patch
> > - fix && to || bashism in ptrace/signal patch
> > - uninline and export function for audit_free_context()
> > - drop CONFIG_CHANGE, FEATURE_CHANGE, ANOM_ABEND, ANOM_SECCOMP patches
> > - move audit_enabled check (xt_AUDIT)
> > - switched from containerid list in struct net to net_generic's struct audit_net
> > - move containerid list iteration into audit (xt_AUDIT)
> > - create function to move namespace switch into audit
> > - switched /proc/PID/ entry from containerid to audit_containerid
> > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_alloc_context()
> > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_log_container_info()
> > - use xt_net(par) instead of sock_net(skb->sk) to get net
> > - switched record and field names: initial CONTAINER_ID, aux CONTAINER, field CONTID
> > - allow to set own contid
> > - open code audit_set_containerid
> > - add contid inherited flag
> > - ccontainerid and pcontainerid eliminated due to inherited flag
> > - change name of container list funcitons
> > - rename containerid to contid
> > - convert initial container record to syscall aux
> > - fix spelling mistake of contidion in net/rfkill/core.c to avoid contid name collision
> > 
> > v2
> > - add check for children and threads
> > - add network namespace container identifier list
> > - add NETFILTER_PKT audit container identifier logging
> > - patch description and documentation clean-up and example
> > - reap unused ppid
> > 
> > Richard Guy Briggs (10):
> >   audit: collect audit task parameters
> >   audit: add container id
> >   audit: log container info of syscalls
> >   audit: add containerid support for ptrace and signals
> >   audit: add support for non-syscall auxiliary records
> >   audit: add containerid support for tty_audit
> >   audit: add containerid filtering
> >   audit: add support for containerid to network namespaces
> >   audit: NETFILTER_PKT: record each container ID associated with a netNS
> >   debug audit: read container ID of a process
> > 
> >  drivers/tty/tty_audit.c    |   5 +-
> >  fs/proc/base.c             |  56 ++++++++++++++
> >  include/linux/audit.h      |  95 ++++++++++++++++++++---
> >  include/linux/sched.h      |   5 +-
> >  include/uapi/linux/audit.h |   8 +-
> >  init/init_task.c           |   3 +-
> >  init/main.c                |   2 +
> >  kernel/audit.c             | 137 +++++++++++++++++++++++++++++++++
> >  kernel/audit.h             |   4 +
> >  kernel/auditfilter.c       |  47 ++++++++++++
> >  kernel/auditsc.c           | 183 ++++++++++++++++++++++++++++++++++++++++-----
> >  kernel/fork.c              |   4 +-
> >  kernel/nsproxy.c           |   4 +
> >  net/netfilter/xt_AUDIT.c   |  12 ++-
> >  14 files changed, 526 insertions(+), 39 deletions(-)
> > 
> > -- 
> > 1.8.3.1
> > 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

^ permalink raw reply

* Re: [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier
From: Guenter Roeck @ 2019-01-03 16:15 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
	LKML, netdev, netfilter-devel, luto, carlos, viro, dhowells, simo,
	eparis, serge, ebiederm
In-Reply-To: <cover.1533065887.git.rgb@redhat.com>

Hi,

On Tue, Jul 31, 2018 at 04:07:35PM -0400, Richard Guy Briggs wrote:
> Implement kernel audit container identifier.
> 

I don't see a follow-up submission of this patch series. Has it been abandoned,
or do I use the wrong search terms ?

Thanks,
Guenter

> This patchset is a fourth based on the proposal document (V3)
> posted:
> 	https://www.redhat.com/archives/linux-audit/2018-January/msg00014.html
> 
> The first patch is the last patch from ghak81 that is included here as a
> convenience.
> 
> The second patch implements the proc fs write to set the audit container
> identifier of a process, emitting an AUDIT_CONTAINER_OP record to announce the
> registration of that audit container identifier on that process.  This patch
> requires userspace support for record acceptance and proper type
> display.
> 
> The third implements the auxiliary record AUDIT_CONTAINER if an
> audit container identifier is identifiable with an event.  This patch
> requires userspace support for proper type display.
> 
> The 4th adds signal and ptrace support.
> 
> The 5th creates a local audit context to be able to bind a standalone
> record with a locally created auxiliary record.
> 
> The 6th patch adds audit container identifier records to the tty
> standalone record.
> 
> The 7th adds audit container identifier filtering to the exit,
> exclude and user lists.  This patch adds the AUDIT_CONTID field and
> requires auditctl userspace support for the --contid option.
> 
> The 8th adds network namespace audit container identifier labelling
> based on member tasks' audit container identifier labels.
> 
> The 9th adds audit container identifier support to standalone netfilter
> records that don't have a task context and lists each container to which
> that net namespace belongs.
> 
> The 10th implements reading the audit container identifier from the proc
> filesystem for debugging.  This patch isn't planned for upstream
> inclusion.
> 
> 
> Example: Set an audit container identifier of 123456 to the "sleep" task:
> 
>   sleep 2&  
>   child=$!
>   echo 123456 > /proc/$child/audit_containerid; echo $?
>   ausearch -ts recent -m container
>   echo child:$child contid:$( cat /proc/$child/audit_containerid)
> 
> This should produce a record such as:
> 
>   type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes 
> 
> 
> Example: Set a filter on an audit container identifier 123459 on /tmp/tmpcontainerid:
> 
>   contid=123459
>   key=tmpcontainerid
>   auditctl -a exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
>   perl -e "sleep 1; open(my \$tmpfile, '>', \"/tmp/$key\"); close(\$tmpfile);" &
>   child=$!
>   echo $contid > /proc/$child/audit_containerid
>   sleep 2
>   ausearch -i -ts recent -k $key
>   auditctl -d exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
>   rm -f /tmp/$key
> 
> This should produce an event such as:
> 
>   type=CONTAINER msg=audit(2018-06-06 12:46:31.707:26953) : op=task contid=123459 
>   type=PROCTITLE msg=audit(2018-06-06 12:46:31.707:26953) : proctitle=perl -e sleep 1; open(my $tmpfile, '>', "/tmp/tmpcontainerid"); close($tmpfile); 
>   type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=1 name=/tmp/tmpcontainerid inode=25656 dev=00:26 mode=file,644 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 
>   type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=0 name=/tmp/ inode=8985 dev=00:26 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 
>   type=CWD msg=audit(2018-06-06 12:46:31.707:26953) : cwd=/root 
>   type=SYSCALL msg=audit(2018-06-06 12:46:31.707:26953) : arch=x86_64 syscall=openat success=yes exit=3 a0=0xffffffffffffff9c a1=0x5621f2b81900 a2=O_WRONLY|O_CREAT|O_TRUNC a3=0x1b6 items=2 ppid=628 pid=2232 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=tmpcontainerid 
> 
> 
> Includes: https://github.com/linux-audit/audit-kernel/issues/81
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/40
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> 
> Changelog:
> 
> v4
> - preface set with ghak81:"collect audit task parameters"
> - add shallyn and sgrubb acks
> - rename feature bitmap macro
> - rename cid_valid() to audit_contid_valid()
> - rename AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP
> - delete audit_get_contid_list() from headers
> - move work into inner if, delete "found"
> - change netns contid list function names
> - move exports for audit_log_contid audit_alloc_local audit_free_context to non-syscall patch
> - list contids CSV
> - pass in gfp flags to audit_alloc_local() (fix audit_alloc_context callers)
> - use "local" in lieu of abusing in_syscall for auditsc_get_stamp()
> - read_lock(&tasklist_lock) around children and thread check
> - task_lock(tsk) should be taken before first check of tsk->audit
> - add spin lock to contid list in aunet
> - restrict /proc read to CAP_AUDIT_CONTROL
> - remove set again prohibition and inherited flag
> - delete contidion spelling fix from patchset, send to netdev/linux-wireless
> 
> v3
> - switched from containerid in task_struct to audit_task_info (depends on ghak81)
> - drop INVALID_CID in favour of only AUDIT_CID_UNSET
> - check for !audit_task_info, throw -ENOPROTOOPT on set
> - changed -EPERM to -EEXIST for parent check
> - return AUDIT_CID_UNSET if !audit_enabled
> - squash child/thread check patch into AUDIT_CONTAINER_ID patch
> - changed -EPERM to -EBUSY for child check
> - separate child and thread checks, use -EALREADY for latter
> - move addition of op= from ptrace/signal patch to AUDIT_CONTAINER patch
> - fix && to || bashism in ptrace/signal patch
> - uninline and export function for audit_free_context()
> - drop CONFIG_CHANGE, FEATURE_CHANGE, ANOM_ABEND, ANOM_SECCOMP patches
> - move audit_enabled check (xt_AUDIT)
> - switched from containerid list in struct net to net_generic's struct audit_net
> - move containerid list iteration into audit (xt_AUDIT)
> - create function to move namespace switch into audit
> - switched /proc/PID/ entry from containerid to audit_containerid
> - call kzalloc with GFP_ATOMIC on in_atomic() in audit_alloc_context()
> - call kzalloc with GFP_ATOMIC on in_atomic() in audit_log_container_info()
> - use xt_net(par) instead of sock_net(skb->sk) to get net
> - switched record and field names: initial CONTAINER_ID, aux CONTAINER, field CONTID
> - allow to set own contid
> - open code audit_set_containerid
> - add contid inherited flag
> - ccontainerid and pcontainerid eliminated due to inherited flag
> - change name of container list funcitons
> - rename containerid to contid
> - convert initial container record to syscall aux
> - fix spelling mistake of contidion in net/rfkill/core.c to avoid contid name collision
> 
> v2
> - add check for children and threads
> - add network namespace container identifier list
> - add NETFILTER_PKT audit container identifier logging
> - patch description and documentation clean-up and example
> - reap unused ppid
> 
> Richard Guy Briggs (10):
>   audit: collect audit task parameters
>   audit: add container id
>   audit: log container info of syscalls
>   audit: add containerid support for ptrace and signals
>   audit: add support for non-syscall auxiliary records
>   audit: add containerid support for tty_audit
>   audit: add containerid filtering
>   audit: add support for containerid to network namespaces
>   audit: NETFILTER_PKT: record each container ID associated with a netNS
>   debug audit: read container ID of a process
> 
>  drivers/tty/tty_audit.c    |   5 +-
>  fs/proc/base.c             |  56 ++++++++++++++
>  include/linux/audit.h      |  95 ++++++++++++++++++++---
>  include/linux/sched.h      |   5 +-
>  include/uapi/linux/audit.h |   8 +-
>  init/init_task.c           |   3 +-
>  init/main.c                |   2 +
>  kernel/audit.c             | 137 +++++++++++++++++++++++++++++++++
>  kernel/audit.h             |   4 +
>  kernel/auditfilter.c       |  47 ++++++++++++
>  kernel/auditsc.c           | 183 ++++++++++++++++++++++++++++++++++++++++-----
>  kernel/fork.c              |   4 +-
>  kernel/nsproxy.c           |   4 +
>  net/netfilter/xt_AUDIT.c   |  12 ++-
>  14 files changed, 526 insertions(+), 39 deletions(-)
> 
> -- 
> 1.8.3.1
> 

^ permalink raw reply

* Re: [RFC PATCH v1 3/5] Yama: Enforces noexec mounts or file executability through O_MAYEXEC
From: Jann Horn @ 2019-01-03 11:17 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Mickaël Salaün, kernel list, Al Viro, James Morris,
	Jonathan Corbet, Kees Cook, Matthew Garrett,
	Michael Kerrisk-manpages, zohar, philippe.trebuchet, shuah,
	thibaut.sautereau, vincent.strubel, yves-alexis.perez,
	Kernel Hardening, Linux API, linux-security-module, linux-fsdevel,
	Andy Lutomirski
In-Reply-To: <f13bf750-f418-343c-baa6-9eee6e0f5e19@ssi.gouv.fr>

On Thu, Dec 13, 2018 at 3:49 PM Mickaël Salaün
<mickael.salaun@ssi.gouv.fr> wrote:
> On 12/12/2018 18:09, Jann Horn wrote:
> > On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
> >> Enable to either propagate the mount options from the underlying VFS
> >> mount to prevent execution, or to propagate the file execute permission.
> >> This may allow a script interpreter to check execution permissions
> >> before reading commands from a file.
> >>
> >> The main goal is to be able to protect the kernel by restricting
> >> arbitrary syscalls that an attacker could perform with a crafted binary
> >> or certain script languages.  It also improves multilevel isolation
> >> by reducing the ability of an attacker to use side channels with
> >> specific code.  These restrictions can natively be enforced for ELF
> >> binaries (with the noexec mount option) but require this kernel
> >> extension to properly handle scripts (e.g., Python, Perl).
> >>
> >> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
> >> behavior.  A following patch adds documentation.
[...]
> >> +{
> >> +       if (!(mask & MAY_OPENEXEC))
> >> +               return 0;
> >> +       /*
> >> +        * Match regular files and directories to make it easier to
> >> +        * modify script interpreters.
> >> +        */
> >> +       if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> >> +               return 0;
> >
> > So files are subject to checks, but loading code from things like
> > sockets is always fine?
>
> As I said in a previous email, these checks do not handle fifo either.
> This is relevant in a threat model targeting persistent attacks (and
> with additional protections/restrictions). We may want to only whitelist
> fifo, but I don't get how a socket is relevant here. Can you please clarify?

I don't think that there's a security problem here. I just think it's
weird to have the extra check when it seems to me like it isn't really
necessary - nobody is going to want to execute a socket or fifo
anyway, right?

> >
> >> +       if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
> >> +                       !(mask & MAY_EXECMOUNT))
> >> +               return -EACCES;
> >> +
> >> +       /*
> >> +        * May prefer acl_permission_check() instead of generic_permission(),
> >> +        * to not be bypassable with CAP_DAC_READ_SEARCH.
> >> +        */
> >> +       if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
> >> +               return generic_permission(inode, MAY_EXEC);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> >> +       LSM_HOOK_INIT(inode_permission, yama_inode_permission),
> >>         LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
> >>         LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
> >>         LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> >> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
> >>         return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
> >>  }
> >>
> >> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
> >> +                                         void __user *buffer, size_t *lenp,
> >> +                                         loff_t *ppos)
> >> +{
> >> +       int error;
> >> +
> >> +       if (write) {
> >> +               struct ctl_table table_copy;
> >> +               int tmp_mayexec_enforce;
> >> +
> >> +               if (!capable(CAP_MAC_ADMIN))
> >> +                       return -EPERM;
> >
> > Don't put capable() checks in sysctls, it doesn't work.
> >
>
> I tested it and the root user can indeed open the file even if the
> process doesn't have CAP_MAC_ADMIN, however writing in the sysctl file
> is denied. Btw there is a similar check in the previous function
> (yama_dointvec_minmax).

It's still wrong. If an attacker without CAP_MAC_ADMIN opens the
sysctl file, then passes the file descriptor to a setcap binary that
has CAP_MAC_ADMIN as stdout/stderr, and the setcap binary writes to
it, then the capable() check is bypassed. (But of course, to open the
sysctl file in the first place, you'd need to be root (uid 0), so the
check doesn't really matter.)

^ permalink raw reply

* [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Christian Brauner @ 2019-01-02 16:16 UTC (permalink / raw)
  To: linux-kernel, linux-api, luto, arnd, serge, keescook, akpm
  Cc: jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
	fweimer, tglx, x86, ebiederm, Christian Brauner
In-Reply-To: <20190102161654.9093-1-christian@brauner.io>

As suggested by Andrew Morton in [1] add selftests for the new
sys_pidfd_send_signal() syscall:

/* test_pidfd_send_signal_syscall_support */
Test whether the pidfd_send_signal() syscall is supported and the tests can
be run or need to be skipped.

/* test_pidfd_send_signal_simple_success */
Test whether sending a signal via a pidfd works.

/* test_pidfd_send_signal_exited_fail */
Verify that sending a signal to an already exited process fails with ESRCH.

/* test_pidfd_send_signal_recycled_pid_fail */
Verify that a recycled pid cannot be signaled via a pidfd referring to an
already exited process that had the same pid (cf. [2], [3]).

[1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
[2]: https://lore.kernel.org/lkml/20181230210245.GA30252@mail.hallyn.com/
[3]: https://lore.kernel.org/lkml/20181230232711.7aayb7vnhogbv4co@brauner.io/

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Christian Brauner <christian@brauner.io>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
/* Changelog */
v7:
- add test for pid recycling (cf. [2] and [3]) as suggested by Serge
- add test for pidfd_send_signal() syscalls support so we can skip tests if
  it isn't
v6:
- patch introduced
v5..v0:
- patch not present
---
 tools/testing/selftests/Makefile           |   1 +
 tools/testing/selftests/pidfd/Makefile     |   6 +
 tools/testing/selftests/pidfd/pidfd_test.c | 381 +++++++++++++++++++++
 3 files changed, 388 insertions(+)
 create mode 100644 tools/testing/selftests/pidfd/Makefile
 create mode 100644 tools/testing/selftests/pidfd/pidfd_test.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index eb54df682d56..84dd78a684e4 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -29,6 +29,7 @@ TARGETS += net
 TARGETS += netfilter
 TARGETS += networking/timestamping
 TARGETS += nsfs
+TARGETS += pidfd
 TARGETS += powerpc
 TARGETS += proc
 TARGETS += pstore
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
new file mode 100644
index 000000000000..deaf8073bc06
--- /dev/null
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -0,0 +1,6 @@
+CFLAGS += -g -I../../../../usr/include/
+
+TEST_GEN_PROGS := pidfd_test
+
+include ../lib.mk
+
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
new file mode 100644
index 000000000000..d59378a93782
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -0,0 +1,381 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/types.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/mount.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "../kselftest.h"
+
+static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
+					unsigned int flags)
+{
+	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
+}
+
+static int signal_received;
+
+static void set_signal_received_on_sigusr1(int sig)
+{
+	if (sig == SIGUSR1)
+		signal_received = 1;
+}
+
+/*
+ * Straightforward test to see whether pidfd_send_signal() works is to send
+ * a signal to ourself.
+ */
+static int test_pidfd_send_signal_simple_success(void)
+{
+	int pidfd, ret;
+	const char *test_name = "pidfd_send_signal send SIGUSR1";
+
+	pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
+	if (pidfd < 0)
+		ksft_exit_fail_msg(
+			"%s test: Failed to open process file descriptor\n",
+			test_name);
+
+	signal(SIGUSR1, set_signal_received_on_sigusr1);
+
+	ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
+	close(pidfd);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s test: Failed to send signal\n",
+				   test_name);
+
+	if (signal_received != 1)
+		ksft_exit_fail_msg("%s test: Failed to receive signal\n",
+				   test_name);
+
+	signal_received = 0;
+	ksft_test_result_pass("%s test: Sent signal\n", test_name);
+	return 0;
+}
+
+static int wait_for_pid(pid_t pid)
+{
+	int status, ret;
+
+again:
+	ret = waitpid(pid, &status, 0);
+	if (ret == -1) {
+		if (errno == EINTR)
+			goto again;
+
+		return -1;
+	}
+
+	if (ret != pid)
+		goto again;
+
+	if (!WIFEXITED(status))
+		return -1;
+
+	return WEXITSTATUS(status);
+}
+
+static int test_pidfd_send_signal_exited_fail(void)
+{
+	int pidfd, ret, saved_errno;
+	char buf[256];
+	pid_t pid;
+	const char *test_name = "pidfd_send_signal signal exited process";
+
+	pid = fork();
+	if (pid < 0)
+		ksft_exit_fail_msg("%s test: Failed to create new process\n",
+				   test_name);
+
+	if (pid == 0)
+		_exit(EXIT_SUCCESS);
+
+	snprintf(buf, sizeof(buf), "/proc/%d", pid);
+
+	pidfd = open(buf, O_DIRECTORY | O_CLOEXEC);
+
+	(void)wait_for_pid(pid);
+
+	if (pidfd < 0)
+		ksft_exit_fail_msg(
+			"%s test: Failed to open process file descriptor\n",
+			test_name);
+
+	ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0);
+	saved_errno = errno;
+	close(pidfd);
+	if (ret == 0)
+		ksft_exit_fail_msg(
+			"%s test: Managed to send signal to process even though it should have failed\n",
+			test_name);
+
+	if (saved_errno != ESRCH)
+		ksft_exit_fail_msg(
+			"%s test: Expected to receive ESRCH as errno value but received %d instead\n",
+			test_name, saved_errno);
+
+	ksft_test_result_pass("%s test: Failed to send signal as expected\n",
+			      test_name);
+	return 0;
+}
+
+/*
+ * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
+ * That means, when it wraps around any pid < 300 will be skipped.
+ * So we need to use a pid > 300 in order to test recycling.
+ */
+#define PID_RECYCLE 1000
+
+/*
+ * Maximum number of cycles we allow. This is equivalent to PID_MAX_DEFAULT.
+ * If users set a higher limit or we have cycled PIDFD_MAX_DEFAULT number of
+ * times then we skip the test to not go into an infinite loop or block for a
+ * long time.
+ */
+#define PIDFD_MAX_DEFAULT 0x8000
+
+/*
+ * Define a few custom error codes for the child process to clearly indicate
+ * what is happening. This way we can tell the difference between a system
+ * error, a test error, etc.
+ */
+#define PIDFD_PASS 0
+#define PIDFD_FAIL 1
+#define PIDFD_ERROR 2
+#define PIDFD_SKIP 3
+#define PIDFD_XFAIL 4
+
+static int test_pidfd_send_signal_recycled_pid_fail(void)
+{
+	int i, ret;
+	pid_t pid1;
+	const char *test_name = "pidfd_send_signal signal recycled pid";
+
+	ret = unshare(CLONE_NEWPID);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s test: Failed to unshare pid namespace\n",
+				   test_name);
+
+	ret = unshare(CLONE_NEWNS);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s test: Failed to unshare mount namespace\n",
+			test_name);
+
+	ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s test: Failed to remount / private\n",
+				   test_name);
+
+	/* pid 1 in new pid namespace */
+	pid1 = fork();
+	if (pid1 < 0)
+		ksft_exit_fail_msg("%s test: Failed to create new process\n",
+				   test_name);
+
+	if (pid1 == 0) {
+		char buf[256];
+		pid_t pid2;
+		int pidfd = -1;
+
+		(void)umount2("/proc", MNT_DETACH);
+		ret = mount("proc", "/proc", "proc", 0, NULL);
+		if (ret < 0)
+			_exit(PIDFD_ERROR);
+
+		/* grab pid PID_RECYCLE */
+		for (i = 0; i <= PIDFD_MAX_DEFAULT; i++) {
+			pid2 = fork();
+			if (pid2 < 0)
+				_exit(PIDFD_ERROR);
+
+			if (pid2 == 0)
+				_exit(PIDFD_PASS);
+
+			if (pid2 == PID_RECYCLE) {
+				snprintf(buf, sizeof(buf), "/proc/%d", pid2);
+				ksft_print_msg("pid to recycle is %d\n", pid2);
+				pidfd = open(buf, O_DIRECTORY | O_CLOEXEC);
+			}
+
+			if (wait_for_pid(pid2))
+				_exit(PIDFD_ERROR);
+
+			if (pid2 >= PID_RECYCLE)
+				break;
+		}
+
+		/*
+		 * We want to be as predictable as we can so if we haven't been
+		 * able to grab pid PID_RECYCLE skip the test.
+		 */
+		if (pid2 != PID_RECYCLE) {
+			/* skip test */
+			close(pidfd);
+			_exit(PIDFD_SKIP);
+		}
+
+		if (pidfd < 0)
+			_exit(PIDFD_ERROR);
+
+		for (i = 0; i <= PIDFD_MAX_DEFAULT; i++) {
+			char c;
+			int pipe_fds[2];
+			pid_t recycled_pid;
+			int child_ret = PIDFD_PASS;
+
+			ret = pipe2(pipe_fds, O_CLOEXEC);
+			if (ret < 0)
+				_exit(PIDFD_ERROR);
+
+			recycled_pid = fork();
+			if (recycled_pid < 0)
+				_exit(PIDFD_ERROR);
+
+			if (recycled_pid == 0) {
+				close(pipe_fds[1]);
+				(void)read(pipe_fds[0], &c, 1);
+				close(pipe_fds[0]);
+
+				_exit(PIDFD_PASS);
+			}
+
+			/*
+			 * Stop the child so we can inspect whether we have
+			 * recycled pid PID_RECYCLE.
+			 */
+			close(pipe_fds[0]);
+			ret = kill(recycled_pid, SIGSTOP);
+			close(pipe_fds[1]);
+			if (ret) {
+				(void)wait_for_pid(recycled_pid);
+				_exit(PIDFD_ERROR);
+			}
+
+			/*
+			 * We have recycled the pid. Try to signal it. This
+			 * needs to fail since this is a different process than
+			 * the one the pidfd refers to.
+			 */
+			if (recycled_pid == PID_RECYCLE) {
+				ret = sys_pidfd_send_signal(pidfd, SIGCONT,
+							    NULL, 0);
+				if (ret && errno == ESRCH)
+					child_ret = PIDFD_XFAIL;
+				else
+					child_ret = PIDFD_FAIL;
+			}
+
+			/* let the process move on */
+			ret = kill(recycled_pid, SIGCONT);
+			if (ret)
+				(void)kill(recycled_pid, SIGKILL);
+
+			if (wait_for_pid(recycled_pid))
+				_exit(PIDFD_ERROR);
+
+			switch (child_ret) {
+			case PIDFD_FAIL:
+				/* fallthrough */
+			case PIDFD_XFAIL:
+				_exit(child_ret);
+			case PIDFD_PASS:
+				break;
+			default:
+				/* not reached */
+				_exit(PIDFD_ERROR);
+			}
+
+			/*
+			 * If the user set a custom pid_max limit we could be
+			 * in the millions.
+			 * Skip the test in this case.
+			 */
+			if (recycled_pid > PIDFD_MAX_DEFAULT)
+				_exit(PIDFD_SKIP);
+		}
+
+		/* failed to recycle pid */
+		_exit(PIDFD_SKIP);
+	}
+
+	ret = wait_for_pid(pid1);
+	switch (ret) {
+	case PIDFD_FAIL:
+		ksft_exit_fail_msg(
+			"%s test: Managed to signal recycled pid %d\n",
+			test_name, PID_RECYCLE);
+	case PIDFD_PASS:
+		ksft_exit_fail_msg("%s test: Failed to recycle pid %d\n",
+				   test_name, PID_RECYCLE);
+	case PIDFD_SKIP:
+		ksft_print_msg("%s test: Skipping test\n", test_name);
+		ret = 0;
+		break;
+	case PIDFD_XFAIL:
+		ksft_test_result_pass(
+			"%s test: Failed to signal recycled pid as expected\n",
+			test_name);
+		ret = 0;
+		break;
+	default /* PIDFD_ERROR */:
+		ksft_exit_fail_msg("%s test: Error while running tests\n",
+				   test_name);
+	}
+
+	return ret;
+}
+
+static int test_pidfd_send_signal_syscall_support(void)
+{
+	int pidfd, ret;
+	const char *test_name = "pidfd_send_signal check for support";
+
+	pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
+	if (pidfd < 0)
+		ksft_exit_fail_msg(
+			"%s test: Failed to open process file descriptor\n",
+			test_name);
+
+	ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0);
+	if (ret < 0) {
+		/*
+		 * pidfd_send_signal() will currently return ENOSYS when
+		 * CONFIG_PROC_FS is not set.
+		 */
+		if (errno == ENOSYS)
+			ksft_exit_skip(
+				"%s test: pidfd_send_signal() syscall not supported (Ensure that CONFIG_PROC_FS=y is set)\n",
+				test_name);
+
+		ksft_exit_fail_msg("%s test: Failed to send signal\n",
+				   test_name);
+	}
+
+	close(pidfd);
+	ksft_test_result_pass(
+		"%s test: pidfd_send_signal() syscall is supported. Tests can be executed\n",
+		test_name);
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	ksft_print_header();
+
+	test_pidfd_send_signal_syscall_support();
+	test_pidfd_send_signal_simple_success();
+	test_pidfd_send_signal_exited_fail();
+	test_pidfd_send_signal_recycled_pid_fail();
+
+	return ksft_exit_pass();
+}
-- 
2.19.1

^ permalink raw reply related

* [PATCH v7 1/2] signal: add pidfd_send_signal() syscall
From: Christian Brauner @ 2019-01-02 16:16 UTC (permalink / raw)
  To: linux-kernel, linux-api, luto, arnd, serge, keescook, akpm
  Cc: jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
	fweimer, tglx, x86, ebiederm, Christian Brauner

The kill() syscall operates on process identifiers (pid). After a process
has exited its pid can be reused by another process. If a caller sends a
signal to a reused pid it will end up signaling the wrong process. This
issue has often surfaced and there has been a push to address this problem [1].

This patch uses file descriptors (fd) from proc/<pid> as stable handles on
struct pid. Even if a pid is recycled the handle will not change. The fd
can be used to send signals to the process it refers to.
Thus, the new syscall pidfd_send_signal() is introduced to solve this
problem. Instead of pids it operates on process fds (pidfd).

/* prototype and argument /*
long pidfd_send_signal(int pidfd, int sig, siginfo_t *info, unsigned int flags);

In addition to the pidfd and signal argument it takes an additional
siginfo_t and flags argument. If the siginfo_t argument is NULL then
pidfd_send_signal() is equivalent to kill(<positive-pid>, <signal>). If it
is not NULL pidfd_send_signal() is equivalent to rt_sigqueueinfo().
The flags argument is added to allow for future extensions of this syscall.
It currently needs to be passed as 0. Failing to do so will cause EINVAL.

/* pidfd_send_signal() replaces multiple pid-based syscalls */
The pidfd_send_signal() syscall currently takes on the job of
rt_sigqueueinfo(2) and parts of the functionality of kill(2), Namely, when a
positive pid is passed to kill(2). It will however be possible to also
replace tgkill(2) and rt_tgsigqueueinfo(2) if this syscall is extended.

/* sending signals to threads (tid) and process groups (pgid) */
Specifically, the pidfd_send_signal() syscall does currently not operate on
process groups or threads. This is left for future extensions.
In order to extend the syscall to allow sending signal to threads and
process groups appropriately named flags (e.g. PIDFD_TYPE_PGID, and
PIDFD_TYPE_TID) should be added. This implies that the flags argument will
determine what is signaled and not the file descriptor itself. Put in other
words, grouping in this api is a property of the flags argument not a
property of the file descriptor (cf. [13]). Clarification for this has been
requested by Eric (cf. [19]).
When appropriate extensions through the flags argument are added then
pidfd_send_signal() can additionally replace the part of kill(2) which
operates on process groups as well as the tgkill(2) and
rt_tgsigqueueinfo(2) syscalls.
How such an extension could be implemented has been very roughly sketched
in [14], [15], and [16]. However, this should not be taken as a commitment
to a particular implementation. There might be better ways to do it.
Right now this is intentionally left out to keep this patchset as simple as
possible (cf. [4]). For example, if a pidfd for a tid from
/proc/<pid>/task/<tid> is passed EOPNOTSUPP will be returned to give
userspace a way to detect when I add support for signaling to threads (cf. [10]).

/* naming */
The syscall had various names throughout iterations of this patchset:
- procfd_signal()
- procfd_send_signal()
- taskfd_send_signal()
In the last round of reviews it was pointed out that given that if the
flags argument decides the scope of the signal instead of different types
of fds it might make sense to either settle for "procfd_" or "pidfd_" as
prefix. The community was willing to accept either (cf. [17] and [18]).
Given that one developer expressed strong preference for the "pidfd_"
prefix (cf. [13] and with other developers less opinionated about the name
we should settle for "pidfd_" to avoid further bikeshedding.

The  "_send_signal" suffix was chosen to reflect the fact that the syscall
takes on the job of multiple syscalls. It is therefore intentional that the
name is not reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the
fomer because it might imply that pidfd_send_signal() is a replacement for
kill(2), and not the latter because it is a hassle to remember the correct
spelling - especially for non-native speakers - and because it is not
descriptive enough of what the syscall actually does. The name
"pidfd_send_signal" makes it very clear that its job is to send signals.

/* zombies */
Zombies can be signaled just as any other process. No special error will be
reported since a zombie state is an unreliable state (cf. [3]). However,
this can be added as an extension through the @flags argument if the need
ever arises.

/* cross-namespace signals */
The patch currently enforces that the signaler and signalee either are in
the same pid namespace or that the signaler's pid namespace is an ancestor
of the signalee's pid namespace. This is done for the sake of simplicity
and because it is unclear to what values certain members of struct
siginfo_t would need to be set to (cf. [5], [6]).

/* compat syscalls */
It became clear that we would like to avoid adding compat syscalls
(cf. [7]).  The compat syscall handling is now done in kernel/signal.c
itself by adding __copy_siginfo_from_user_generic() which lets us avoid
compat syscalls (cf. [8]). It should be noted that the addition of
__copy_siginfo_from_user_any() is caused by a bug in the original
implementation of rt_sigqueueinfo(2) (cf. 12).
With upcoming rework for syscall handling things might improve
significantly (cf. [11]) and __copy_siginfo_from_user_any() will not gain
any additional callers.

/* testing */
This patch was tested on x64 and x86.

/* userspace usage */
An asciinema recording for the basic functionality can be found under [9].
With this patch a process can be killed via:

 #define _GNU_SOURCE
 #include <errno.h>
 #include <fcntl.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
 #include <unistd.h>

 static inline int do_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
                                         unsigned int flags)
 {
 #ifdef __NR_pidfd_send_signal
         return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
 #else
         return -ENOSYS;
 #endif
 }

 int main(int argc, char *argv[])
 {
         int fd, ret, saved_errno, sig;

         if (argc < 3)
                 exit(EXIT_FAILURE);

         fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
         if (fd < 0) {
                 printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
                 exit(EXIT_FAILURE);
         }

         sig = atoi(argv[2]);

         printf("Sending signal %d to process %s\n", sig, argv[1]);
         ret = do_pidfd_send_signal(fd, sig, NULL, 0);

         saved_errno = errno;
         close(fd);
         errno = saved_errno;

         if (ret < 0) {
                 printf("%s - Failed to send signal %d to process %s\n",
                        strerror(errno), sig, argv[1]);
                 exit(EXIT_FAILURE);
         }

         exit(EXIT_SUCCESS);
 }

/* Q&A
 * Given that it seems the same questions get asked again by people who are
 * late to the party it makes sense to add a Q&A section to the commit
 * message so it's hopefully easier to avoid duplicate threads.
 *
 * For the sake of progress please consider these arguments settled unless
 * there is a new point that desperately needs to be addressed. Please make
 * sure to check the links to the threads in this commit message whether
 * this has not already been covered.
 */
Q-01: (Florian Weimer [20], Andrew Morton [21])
      What happens when the target process has exited?
A-01: Sending the signal will fail with ESRCH (cf. [22]).

Q-02:  (Andrew Morton [21])
       Is the task_struct pinned by the fd?
A-02:  No. A reference to struct pid is kept. struct pid - as far as I
       understand - was created exactly for the reason to not require to
       pin struct task_struct (cf. [22]).

Q-03: (Andrew Morton [21])
      Does the entire procfs directory remain visible? Just one entry
      within it?
A-03: The same thing that happens right now when you hold a file descriptor
      to /proc/<pid> open (cf. [22]).

Q-04: (Andrew Morton [21])
      Does the pid remain reserved?
A-04: No. This patchset guarantees a stable handle not that pids are not
      recycled (cf. [22]).

Q-05: (Andrew Morton [21])
      Do attempts to signal that fd return errors?
A-05: See {Q,A}-01.

Q-06: (Andrew Morton [22])
      Is there a cleaner way of obtaining the fd? Another syscall perhaps.
A-06: Userspace can already trivially retrieve file descriptors from procfs
      so this is something that we will need to support anyway. Hence,
      there's no immediate need to add another syscalls just to make
      pidfd_send_signal() not dependent on the presence of procfs. However,
      adding a syscalls to get such file descriptors is planned for a
      future patchset (cf. [22]).

Q-07: (Andrew Morton [21] and others)
      This fd-for-a-process sounds like a handy thing and people may well
      think up other uses for it in the future, probably unrelated to
      signals. Are the code and the interface designed to permit such
      future applications?
A-07: Yes (cf. [22]).

Q-08: (Andrew Morton [21] and others)
      Now I think about it, why a new syscall? This thing is looking
      rather like an ioctl?
A-08: This has been extensively discussed. It was agreed that a syscall is
      preferred for a variety or reasons. Here are just a few taken from
      prior threads. Syscalls are safer than ioctl()s especially when
      signaling to fds. Processes are a core kernel concept so a syscall
      seems more appropriate. The layout of the syscall with its four
      arguments would require the addition of a custom struct for the
      ioctl() thereby causing at least the same amount or even more
      complexity for userspace than a simple syscall. The new syscall will
      replace multiple other pid-based syscalls (see description above).
      The file-descriptors-for-processes concept introduced with this
      syscall will be extended with other syscalls in the future. See also
      [22], [23] and various other threads already linked in here.

Q-09: (Florian Weimer [24])
      What happens if you use the new interface with an O_PATH descriptor?
A-09:
      pidfds opened as O_PATH fds cannot be used to send signals to a
      process (cf. [2]). Signaling processes through pidfds is the
      equivalent of writing to a file. Thus, this is not an operation that
      operates "purely at the file descriptor level" as required by the
      open(2) manpage. See also [4].

/* References */
[1]:  https://lore.kernel.org/lkml/20181029221037.87724-1-dancol@google.com/
[2]:  https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
[3]:  https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
[4]:  https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
[5]:  https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
[6]:  https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
[7]:  https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
[8]:  https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
[9]:  https://asciinema.org/a/IQjuCHew6bnq1cr78yuMv16cy
[10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
[11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
[12]: https://lore.kernel.org/lkml/87zhtjn8ck.fsf@xmission.com/
[13]: https://lore.kernel.org/lkml/871s6u9z6u.fsf@xmission.com/
[14]: https://lore.kernel.org/lkml/20181206231742.xxi4ghn24z4h2qki@brauner.io/
[15]: https://lore.kernel.org/lkml/20181207003124.GA11160@mail.hallyn.com/
[16]: https://lore.kernel.org/lkml/20181207015423.4miorx43l3qhppfz@brauner.io/
[17]: https://lore.kernel.org/lkml/CAGXu5jL8PciZAXvOvCeCU3wKUEB_dU-O3q0tDw4uB_ojMvDEew@mail.gmail.com/
[18]: https://lore.kernel.org/lkml/20181206222746.GB9224@mail.hallyn.com/
[19]: https://lore.kernel.org/lkml/20181208054059.19813-1-christian@brauner.io/
[20]: https://lore.kernel.org/lkml/8736rebl9s.fsf@oldenburg.str.redhat.com/
[21]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
[22]: https://lore.kernel.org/lkml/20181228233725.722tdfgijxcssg76@brauner.io/
[23]: https://lwn.net/Articles/773459/
[24]: https://lore.kernel.org/lkml/8736rebl9s.fsf@oldenburg.str.redhat.com/

Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Christian Brauner <christian@brauner.io>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Serge Hallyn <serge@hallyn.com>
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
---
/* Changelog */
v7:
- patch unchanged
v6:
- Given that it seems the same questions get asked multiple times it made
  sense to add a Q&A section to the commit message so it's hopefully easier
  to avoid duplicate threads.
- Since we settled on adding flags when extending the syscalls to allow
  signaling to threads and process groups it doesn't make sense anymore to
  report EOPNOTSUPP when a file descriptor to /proc/<pid>/task/<tid> is
  passed. This means we can also remove the tgid_pidfd_to_pid() helper from
  proc_fs.h and simplify the code. We will now always return EBADF when a
  file descriptor is passed that does not refer to /proc/<pid>.
- add CONFIG_PROC_FS ifdefs for pidfd_send_signal() and add
  COND_SYSCALL(pidfd_send_signal) definition as suggested by Andrew Morgan
  in [changelog-1].
v5:
- s/may_signal_taskfd/access_taskfd_pidns/g
- make it clear that process grouping is a property of the @flags argument
  Eric has argued that he would like to know when we add thread and process
  group signal support whether grouping will be a property of the file
  descriptor or the flag argument and he would oppose this until a
  commitment has been made. It seems that the cleanest strategy is to make
  grouping a property of the @flags argument.
  He also argued that in this case the prefix of the syscall should be
  "pidfd_" (cf. https://lore.kernel.org/lkml/871s6u9z6u.fsf@xmission.com/).
- use "pidfd_" as prefix for the syscall since grouping will be a property
  of the @flags argument
- substantial rewrite of the commit message to reflect the discussion
v4:
- updated asciinema to use "taskfd_" prefix
- s/procfd_send_signal/taskfd_send_signal/g
- s/proc_is_tgid_procfd/tgid_taskfd_to_pid/b
- s/proc_is_tid_procfd/tid_taskfd_to_pid/b
- s/__copy_siginfo_from_user_generic/__copy_siginfo_from_user_any/g
- make it clear that __copy_siginfo_from_user_any() is a workaround caused
  by a bug in the original implementation of rt_sigqueueinfo()
- when spoofing signals turn them into regular kill signals if si_code is
  set to SI_USER
- make proc_is_t{g}id_procfd() return struct pid to allow proc_pid() to
  stay private to fs/proc/
v3:
- add __copy_siginfo_from_user_generic() to avoid adding compat syscalls
- s/procfd_signal/procfd_send_signal/g
- change type of flags argument from int to unsigned int
- add comment about what happens to zombies
- add proc_is_tid_procfd()
- return EOPNOTSUPP when /proc/<pid>/task/<tid> fd is passed so userspace
  has a way of knowing that tidfds are not supported currently.
v2:
- define __NR_procfd_signal in unistd.h
- wire up compat syscall
- s/proc_is_procfd/proc_is_tgid_procfd/g
- provide stubs when CONFIG_PROC_FS=n
- move proc_pid() to linux/proc_fs.h header
- use proc_pid() to grab struct pid from /proc/<pid> fd
v1:
- patch introduced

/* Changelog references */
[changelog-1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
---
 arch/x86/entry/syscalls/syscall_32.tbl |   1 +
 arch/x86/entry/syscalls/syscall_64.tbl |   1 +
 fs/proc/base.c                         |   9 ++
 include/linux/proc_fs.h                |   6 ++
 include/linux/syscalls.h               |   3 +
 include/uapi/asm-generic/unistd.h      |   4 +-
 kernel/signal.c                        | 133 +++++++++++++++++++++++--
 kernel/sys_ni.c                        |   1 +
 8 files changed, 151 insertions(+), 7 deletions(-)

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 3cf7b533b3d1..6804c1e84b36 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -398,3 +398,4 @@
 384	i386	arch_prctl		sys_arch_prctl			__ia32_compat_sys_arch_prctl
 385	i386	io_pgetevents		sys_io_pgetevents		__ia32_compat_sys_io_pgetevents
 386	i386	rseq			sys_rseq			__ia32_sys_rseq
+387	i386	pidfd_send_signal	sys_pidfd_send_signal		__ia32_sys_pidfd_send_signal
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index f0b1709a5ffb..aa4b858fa0f1 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -343,6 +343,7 @@
 332	common	statx			__x64_sys_statx
 333	common	io_pgetevents		__x64_sys_io_pgetevents
 334	common	rseq			__x64_sys_rseq
+335	common	pidfd_send_signal	__x64_sys_pidfd_send_signal
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/proc/base.c b/fs/proc/base.c
index d7fd1ca807d2..714bfc844897 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3038,6 +3038,15 @@ static const struct file_operations proc_tgid_base_operations = {
 	.llseek		= generic_file_llseek,
 };
 
+struct pid *tgid_pidfd_to_pid(const struct file *file)
+{
+	if (!d_is_dir(file->f_path.dentry) ||
+	    (file->f_op != &proc_tgid_base_operations))
+		return ERR_PTR(-EBADF);
+
+	return proc_pid(file_inode(file));
+}
+
 static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 {
 	return proc_pident_lookup(dir, dentry,
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index d0e1f1522a78..52a283ba0465 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -73,6 +73,7 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
 						    int (*show)(struct seq_file *, void *),
 						    proc_write_t write,
 						    void *data);
+extern struct pid *tgid_pidfd_to_pid(const struct file *file);
 
 #else /* CONFIG_PROC_FS */
 
@@ -114,6 +115,11 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
 #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
 #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
 
+static inline struct pid *tgid_pidfd_to_pid(const struct file *file)
+{
+	return ERR_PTR(-EBADF);
+}
+
 #endif /* CONFIG_PROC_FS */
 
 struct net;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 251979d2e709..b0d2a12bb523 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -926,6 +926,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
 			  unsigned mask, struct statx __user *buffer);
 asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
 			 int flags, uint32_t sig);
+asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
+				       siginfo_t __user *info,
+				       unsigned int flags);
 
 /*
  * Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index d90127298f12..b77538af7aca 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -740,9 +740,11 @@ __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
 __SYSCALL(__NR_rseq, sys_rseq)
 #define __NR_kexec_file_load 294
 __SYSCALL(__NR_kexec_file_load,     sys_kexec_file_load)
+#define __NR_pidfd_send_signal 295
+__SYSCALL(__NR_pidfd_send_signal, sys_pidfd_send_signal)
 
 #undef __NR_syscalls
-#define __NR_syscalls 295
+#define __NR_syscalls 296
 
 /*
  * 32 bit systems traditionally used different
diff --git a/kernel/signal.c b/kernel/signal.c
index 53e07d97ffe0..61b12c518fbc 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -19,7 +19,9 @@
 #include <linux/sched/task.h>
 #include <linux/sched/task_stack.h>
 #include <linux/sched/cputime.h>
+#include <linux/file.h>
 #include <linux/fs.h>
+#include <linux/proc_fs.h>
 #include <linux/tty.h>
 #include <linux/binfmts.h>
 #include <linux/coredump.h>
@@ -3429,6 +3431,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
 #endif
 #endif
 
+static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
+{
+	clear_siginfo(info);
+	info->si_signo = sig;
+	info->si_errno = 0;
+	info->si_code = SI_USER;
+	info->si_pid = task_tgid_vnr(current);
+	info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
+}
+
 /**
  *  sys_kill - send a signal to a process
  *  @pid: the PID of the process
@@ -3438,16 +3450,125 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
 {
 	struct kernel_siginfo info;
 
-	clear_siginfo(&info);
-	info.si_signo = sig;
-	info.si_errno = 0;
-	info.si_code = SI_USER;
-	info.si_pid = task_tgid_vnr(current);
-	info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+	prepare_kill_siginfo(sig, &info);
 
 	return kill_something_info(sig, &info, pid);
 }
 
+#ifdef CONFIG_PROC_FS
+/*
+ * Verify that the signaler and signalee either are in the same pid namespace
+ * or that the signaler's pid namespace is an ancestor of the signalee's pid
+ * namespace.
+ */
+static bool access_pidfd_pidns(struct pid *pid)
+{
+	struct pid_namespace *active = task_active_pid_ns(current);
+	struct pid_namespace *p = ns_of_pid(pid);
+
+	for (;;) {
+		if (!p)
+			return false;
+		if (p == active)
+			break;
+		p = p->parent;
+	}
+
+	return true;
+}
+
+static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, siginfo_t *info)
+{
+#ifdef CONFIG_COMPAT
+	/*
+	 * Avoid hooking up compat syscalls and instead handle necessary
+	 * conversions here. Note, this is a stop-gap measure and should not be
+	 * considered a generic solution.
+	 */
+	if (in_compat_syscall())
+		return copy_siginfo_from_user32(
+			kinfo, (struct compat_siginfo __user *)info);
+#endif
+	return copy_siginfo_from_user(kinfo, info);
+}
+
+/**
+ * sys_pidfd_send_signal - send a signal to a process through a task file
+ *                          descriptor
+ * @pidfd:  the file descriptor of the process
+ * @sig:    signal to be sent
+ * @info:   the signal info
+ * @flags:  future flags to be passed
+ *
+ * The syscall currently only signals via PIDTYPE_PID which covers
+ * kill(<positive-pid>, <signal>. It does not signal threads or process
+ * groups.
+ * In order to extend the syscall to threads and process groups the @flags
+ * argument should be used. In essence, the @flags argument will determine
+ * what is signaled and not the file descriptor itself. Put in other words,
+ * grouping is a property of the flags argument not a property of the file
+ * descriptor.
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
+		siginfo_t __user *, info, unsigned int, flags)
+{
+	int ret;
+	struct fd f;
+	struct pid *pid;
+	kernel_siginfo_t kinfo;
+
+	/* Enforce flags be set to 0 until we add an extension. */
+	if (flags)
+		return -EINVAL;
+
+	f = fdget_raw(pidfd);
+	if (!f.file)
+		return -EBADF;
+
+	/* Is this a pidfd? */
+	pid = tgid_pidfd_to_pid(f.file);
+	if (IS_ERR(pid)) {
+		ret = PTR_ERR(pid);
+		goto err;
+	}
+
+	ret = -EINVAL;
+	if (!access_pidfd_pidns(pid))
+		goto err;
+
+	if (info) {
+		ret = copy_siginfo_from_user_any(&kinfo, info);
+		if (unlikely(ret))
+			goto err;
+
+		ret = -EINVAL;
+		if (unlikely(sig != kinfo.si_signo))
+			goto err;
+
+		if ((task_pid(current) != pid) &&
+		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) {
+			/* Only allow sending arbitrary signals to yourself. */
+			ret = -EPERM;
+			if (kinfo.si_code != SI_USER)
+				goto err;
+
+			/* Turn this into a regular kill signal. */
+			prepare_kill_siginfo(sig, &kinfo);
+		}
+	} else {
+		prepare_kill_siginfo(sig, &kinfo);
+	}
+
+	ret = kill_pid_info(sig, &kinfo, pid);
+
+err:
+	fdput(f);
+	return ret;
+}
+#endif /* CONFIG_PROC_FS */
+
 static int
 do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
 {
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index ab9d0e3c6d50..f905f4f9f677 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -163,6 +163,7 @@ COND_SYSCALL(syslog);
 /* kernel/sched/core.c */
 
 /* kernel/signal.c */
+COND_SYSCALL(pidfd_send_signal);
 
 /* kernel/sys.c */
 COND_SYSCALL(setregid);
-- 
2.19.1

^ permalink raw reply related

* Re: d_off field in struct dirent and 32-on-64 emulation
From: Adhemerval Zanella @ 2019-01-02 13:16 UTC (permalink / raw)
  To: Joseph Myers
  Cc: Florian Weimer, linux-fsdevel, linux-api, linux-ext4,
	linux-kernel, v9fs-developer, libc-alpha, qemu-devel, ericvh,
	rminnich, lucho, hpa, arnd
In-Reply-To: <alpine.DEB.2.21.1812311657050.27287@digraph.polyomino.org.uk>



On 31/12/2018 15:03, Joseph Myers wrote:
> On Fri, 28 Dec 2018, Adhemerval Zanella wrote:
> 
>>>> Currently we only have nios2 and csky (unfortunately).  But since generic 
>>>> definition for off_t and off64_t still assumes non-LFS support, all new
>>>> 32-bits ports potentially might carry the issue.
>>>
>>> For csky, we could still change the type of the non-standard d_off
>>> field to long long int.  This way, only telldir would have to fail
>>> when truncation is necessary, as mentioned below:
>>
>> I think it makes no sense to continue making non-LFS as default for
>> newer 32 bits ports, the support will be emulated with LFS syscalls.
> 
> Any new 32-bit port that uses 64-bit time_t will also use 64-bit offsets 
> (because we don't have any glibc configurations that support the 
> combination of 64-bit time with 32-bit offsets, and don't want to add 
> them).  That should apply for RISC-V 32-bit at least.
> 
> I've filed <https://sourceware.org/bugzilla/show_bug.cgi?id=24050> for 
> missing overflow checks in telldir when the default off_t is wider than 
> long int (currently just applies to x32; not sure why we don't see glibc 
> test failures on x32 resulting from the quiet truncation, as the issue is 
> certainly there in the source code).
> 

What about csky? Should we still make it use 32-bit offsets as default
configuration even when kernel does not support it natively?

^ permalink raw reply

* Re: [PATCH v6 2/2] selftests: add tests for pidfd_send_signal()
From: Serge E. Hallyn @ 2019-01-01 17:02 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Serge E. Hallyn, linux-kernel, linux-api, luto, arnd, keescook,
	akpm, jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
	fweimer, tglx, x86, ebiederm
In-Reply-To: <20190101150743.ikrrfcogqec2oqeh@brauner.io>

On Tue, Jan 01, 2019 at 04:07:44PM +0100, Christian Brauner wrote:
> On Mon, Dec 31, 2018 at 12:27:13AM +0100, Christian Brauner wrote:
> > On Sun, Dec 30, 2018 at 03:02:45PM -0600, Serge Hallyn wrote:
> > > On Sat, Dec 29, 2018 at 11:27:56PM +0100, Christian Brauner wrote:
> > > > As suggested by Andrew Morton in [1] add selftests for the new
> > > > sys_pidfd_send_signal() syscall.
> > > > This tests whether we can send a signal to an existing process and whether
> > > > sending a signal to a process that has already exited fails with ESRCH.
> > > > 
> > > > [1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
> > > > 
> > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Serge Hallyn <serge@hallyn.com>
> > > 
> > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > 
> > > Not saying you need to do this, but it would be neat if you could test
> > > sending to a pid which has been recycled :)
> > 
> > Yeah, I thought about it but it's a little weird code. First of all, we
> > can't set /proc/sys/kernel/pid_max to a very low value since this is a
> > system wide setting. So we need to recycle a lot via fork(). Something
> > along the lines of:
> > - unshare pid namespace
> > - fork to create pid 1 in new pid namespace
> > - cycle with fork() until pid > 300 since pids lower than 300 are
> >   reserved by the kernel.
> >   (That means if we simply use the first fork() after we created pid 1 we
> >   would never be able to recycle the pid since we skip over it. :))
> > - get pidfd to the pid > 300 we just created
> > - wait on the pid > 300
> > - cycle via fork() until we have reached the same pid > 300 again
> > - send SIGSTOP to that recycled process
> > - test that we cannot send SIGCONT to this SIGSTOPed task via the pidfd we
> >   received before
> > - send SIGCONT to the SIGSTOPed recycled pid and exit
> 
> Ok, I have something like this in my tree now that tests for pid
> recycling. I'm going to send it out tomorrow since I reckon Andrew and
> others will be off today.
> But fwiw it sits in https://github.com/brauner/linux/commits/2018-12-02/procfds

Thanks, that shows off the advantages of the new syscall :)

-serge

^ permalink raw reply

* Re: [PATCH v6 2/2] selftests: add tests for pidfd_send_signal()
From: Christian Brauner @ 2019-01-01 15:07 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: linux-kernel, linux-api, luto, arnd, keescook, akpm, jannh, oleg,
	cyphar, viro, linux-fsdevel, dancol, timmurray, fweimer, tglx,
	x86, ebiederm
In-Reply-To: <20181230232711.7aayb7vnhogbv4co@brauner.io>

On Mon, Dec 31, 2018 at 12:27:13AM +0100, Christian Brauner wrote:
> On Sun, Dec 30, 2018 at 03:02:45PM -0600, Serge Hallyn wrote:
> > On Sat, Dec 29, 2018 at 11:27:56PM +0100, Christian Brauner wrote:
> > > As suggested by Andrew Morton in [1] add selftests for the new
> > > sys_pidfd_send_signal() syscall.
> > > This tests whether we can send a signal to an existing process and whether
> > > sending a signal to a process that has already exited fails with ESRCH.
> > > 
> > > [1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
> > > 
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Serge Hallyn <serge@hallyn.com>
> > 
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > 
> > Not saying you need to do this, but it would be neat if you could test
> > sending to a pid which has been recycled :)
> 
> Yeah, I thought about it but it's a little weird code. First of all, we
> can't set /proc/sys/kernel/pid_max to a very low value since this is a
> system wide setting. So we need to recycle a lot via fork(). Something
> along the lines of:
> - unshare pid namespace
> - fork to create pid 1 in new pid namespace
> - cycle with fork() until pid > 300 since pids lower than 300 are
>   reserved by the kernel.
>   (That means if we simply use the first fork() after we created pid 1 we
>   would never be able to recycle the pid since we skip over it. :))
> - get pidfd to the pid > 300 we just created
> - wait on the pid > 300
> - cycle via fork() until we have reached the same pid > 300 again
> - send SIGSTOP to that recycled process
> - test that we cannot send SIGCONT to this SIGSTOPed task via the pidfd we
>   received before
> - send SIGCONT to the SIGSTOPed recycled pid and exit

Ok, I have something like this in my tree now that tests for pid
recycling. I'm going to send it out tomorrow since I reckon Andrew and
others will be off today.
But fwiw it sits in https://github.com/brauner/linux/commits/2018-12-02/procfds

> 
> Christian
> 
> > 
> > > Cc: Jann Horn <jannh@google.com>
> > > Cc: Andy Lutomirsky <luto@kernel.org>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Oleg Nesterov <oleg@redhat.com>
> > > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Florian Weimer <fweimer@redhat.com>
> > > Signed-off-by: Christian Brauner <christian@brauner.io>
> > > ---
> > > /* Changelog */
> > > v6:
> > > - patch introduced
> > > v5..v0:
> > > - patch not present
> > > ---
> > >  tools/testing/selftests/Makefile           |   1 +
> > >  tools/testing/selftests/pidfd/Makefile     |   6 +
> > >  tools/testing/selftests/pidfd/pidfd_test.c | 130 +++++++++++++++++++++
> > >  3 files changed, 137 insertions(+)
> > >  create mode 100644 tools/testing/selftests/pidfd/Makefile
> > >  create mode 100644 tools/testing/selftests/pidfd/pidfd_test.c
> > > 
> > > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > > index 24b9934fb269..63b0d8a0ebf7 100644
> > > --- a/tools/testing/selftests/Makefile
> > > +++ b/tools/testing/selftests/Makefile
> > > @@ -27,6 +27,7 @@ TARGETS += net
> > >  TARGETS += netfilter
> > >  TARGETS += networking/timestamping
> > >  TARGETS += nsfs
> > > +TARGETS += pidfd
> > >  TARGETS += powerpc
> > >  TARGETS += proc
> > >  TARGETS += pstore
> > > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > > new file mode 100644
> > > index 000000000000..deaf8073bc06
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/pidfd/Makefile
> > > @@ -0,0 +1,6 @@
> > > +CFLAGS += -g -I../../../../usr/include/
> > > +
> > > +TEST_GEN_PROGS := pidfd_test
> > > +
> > > +include ../lib.mk
> > > +
> > > diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
> > > new file mode 100644
> > > index 000000000000..edcd59979b10
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/pidfd/pidfd_test.c
> > > @@ -0,0 +1,130 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#define _GNU_SOURCE
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <linux/types.h>
> > > +#include <signal.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <syscall.h>
> > > +#include <sys/wait.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "../kselftest.h"
> > > +
> > > +static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> > > +					unsigned int flags)
> > > +{
> > > +	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> > > +}
> > > +
> > > +static int signal_received;
> > > +
> > > +static void do_exit_success(int sig)
> > > +{
> > > +	signal_received = 1;
> > > +}
> > > +
> > > +/*
> > > + * Straightforward test to see whether pidfd_send_signal() works is to send
> > > + * a signal to ourselves.
> > > + */
> > > +static int test_pidfd_send_signal_simple_success(void)
> > > +{
> > > +	int pidfd, ret;
> > > +	const char *test_name = "pidfd_send_signal send SIGUSR1";
> > > +
> > > +	pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
> > > +	if (pidfd < 0)
> > > +		ksft_exit_fail_msg(
> > > +			"%s test: Failed to open process file descriptor\n",
> > > +			test_name);
> > > +
> > > +	signal(SIGUSR1, do_exit_success);
> > > +
> > > +	ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
> > > +	close(pidfd);
> > > +	if (ret < 0)
> > > +		ksft_exit_fail_msg("%s test: Failed to send signal\n",
> > > +				   test_name);
> > > +
> > > +	if (signal_received != 1)
> > > +		ksft_exit_fail_msg("%s test: Failed to receive signal\n",
> > > +				   test_name);
> > > +
> > > +	signal_received = 0;
> > > +	ksft_test_result_pass("%s test: Sent signal\n", test_name);
> > > +	return 0;
> > > +}
> > > +
> > > +static void wait_for_pid(pid_t pid)
> > > +{
> > > +	int status, ret;
> > > +
> > > +again:
> > > +	ret = waitpid(pid, &status, 0);
> > > +	if (ret == -1) {
> > > +		if (errno == EINTR)
> > > +			goto again;
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	if (ret != pid)
> > > +		goto again;
> > > +}
> > > +
> > > +static int test_pidfd_send_signal_exited_fail(void)
> > > +{
> > > +	int pidfd, ret, saved_errno;
> > > +	char buf[256];
> > > +	pid_t pid;
> > > +	const char *test_name = "pidfd_send_signal signal exited process";
> > > +
> > > +	pid = fork();
> > > +	if (pid < 0)
> > > +		ksft_exit_fail_msg("%s test: Failed to create new process\n",
> > > +				   test_name);
> > > +
> > > +	if (pid == 0)
> > > +		_exit(EXIT_SUCCESS);
> > > +
> > > +	snprintf(buf, sizeof(buf), "/proc/%d", pid);
> > > +
> > > +	pidfd = open(buf, O_DIRECTORY | O_CLOEXEC);
> > > +
> > > +	wait_for_pid(pid);
> > > +
> > > +	if (pidfd < 0)
> > > +		ksft_exit_fail_msg(
> > > +			"%s test: Failed to open process file descriptor\n",
> > > +			test_name);
> > > +
> > > +	ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0);
> > > +	saved_errno = errno;
> > > +	close(pidfd);
> > > +	if (ret == 0)
> > > +		ksft_exit_fail_msg(
> > > +			"%s test: Managed to send signal to process even though it should have failed\n",
> > > +			test_name);
> > > +
> > > +	if (saved_errno != ESRCH)
> > > +		ksft_exit_fail_msg(
> > > +			"%s test: Expected to receive ESRCH as errno value but received %d instead\n",
> > > +			test_name, saved_errno);
> > > +
> > > +	ksft_test_result_pass("%s test: Failed to send signal as expected\n",
> > > +			      test_name);
> > > +	return 0;
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +	ksft_print_header();
> > > +
> > > +	test_pidfd_send_signal_simple_success();
> > > +	test_pidfd_send_signal_exited_fail();
> > > +
> > > +	return ksft_exit_pass();
> > > +}
> > > -- 
> > > 2.19.1

^ permalink raw reply

* Re: d_off field in struct dirent and 32-on-64 emulation
From: Joseph Myers @ 2018-12-31 17:03 UTC (permalink / raw)
  To: Adhemerval Zanella
  Cc: Florian Weimer, linux-fsdevel, linux-api, linux-ext4,
	linux-kernel, v9fs-developer, libc-alpha, qemu-devel, ericvh,
	rminnich, lucho, hpa, arnd
In-Reply-To: <ae0530c9-5c46-5560-9734-1eacaf173b8d@linaro.org>

On Fri, 28 Dec 2018, Adhemerval Zanella wrote:

> >> Currently we only have nios2 and csky (unfortunately).  But since generic 
> >> definition for off_t and off64_t still assumes non-LFS support, all new
> >> 32-bits ports potentially might carry the issue.
> > 
> > For csky, we could still change the type of the non-standard d_off
> > field to long long int.  This way, only telldir would have to fail
> > when truncation is necessary, as mentioned below:
> 
> I think it makes no sense to continue making non-LFS as default for
> newer 32 bits ports, the support will be emulated with LFS syscalls.

Any new 32-bit port that uses 64-bit time_t will also use 64-bit offsets 
(because we don't have any glibc configurations that support the 
combination of 64-bit time with 32-bit offsets, and don't want to add 
them).  That should apply for RISC-V 32-bit at least.

I've filed <https://sourceware.org/bugzilla/show_bug.cgi?id=24050> for 
missing overflow checks in telldir when the default off_t is wider than 
long int (currently just applies to x32; not sure why we don't see glibc 
test failures on x32 resulting from the quiet truncation, as the issue is 
certainly there in the source code).

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply

* Re: [PATCH v6 2/2] selftests: add tests for pidfd_send_signal()
From: Christian Brauner @ 2018-12-30 23:27 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: linux-kernel, linux-api, luto, arnd, keescook, akpm, jannh, oleg,
	cyphar, viro, linux-fsdevel, dancol, timmurray, fweimer, tglx,
	x86, ebiederm
In-Reply-To: <20181230210245.GA30252@mail.hallyn.com>

On Sun, Dec 30, 2018 at 03:02:45PM -0600, Serge Hallyn wrote:
> On Sat, Dec 29, 2018 at 11:27:56PM +0100, Christian Brauner wrote:
> > As suggested by Andrew Morton in [1] add selftests for the new
> > sys_pidfd_send_signal() syscall.
> > This tests whether we can send a signal to an existing process and whether
> > sending a signal to a process that has already exited fails with ESRCH.
> > 
> > [1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Serge Hallyn <serge@hallyn.com>
> 
> Acked-by: Serge Hallyn <serge@hallyn.com>
> 
> Not saying you need to do this, but it would be neat if you could test
> sending to a pid which has been recycled :)

Yeah, I thought about it but it's a little weird code. First of all, we
can't set /proc/sys/kernel/pid_max to a very low value since this is a
system wide setting. So we need to recycle a lot via fork(). Something
along the lines of:
- unshare pid namespace
- fork to create pid 1 in new pid namespace
- cycle with fork() until pid > 300 since pids lower than 300 are
  reserved by the kernel.
  (That means if we simply use the first fork() after we created pid 1 we
  would never be able to recycle the pid since we skip over it. :))
- get pidfd to the pid > 300 we just created
- wait on the pid > 300
- cycle via fork() until we have reached the same pid > 300 again
- send SIGSTOP to that recycled process
- test that we cannot send SIGCONT to this SIGSTOPed task via the pidfd we
  received before
- send SIGCONT to the SIGSTOPed recycled pid and exit

Christian

> 
> > Cc: Jann Horn <jannh@google.com>
> > Cc: Andy Lutomirsky <luto@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Florian Weimer <fweimer@redhat.com>
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > /* Changelog */
> > v6:
> > - patch introduced
> > v5..v0:
> > - patch not present
> > ---
> >  tools/testing/selftests/Makefile           |   1 +
> >  tools/testing/selftests/pidfd/Makefile     |   6 +
> >  tools/testing/selftests/pidfd/pidfd_test.c | 130 +++++++++++++++++++++
> >  3 files changed, 137 insertions(+)
> >  create mode 100644 tools/testing/selftests/pidfd/Makefile
> >  create mode 100644 tools/testing/selftests/pidfd/pidfd_test.c
> > 
> > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> > index 24b9934fb269..63b0d8a0ebf7 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -27,6 +27,7 @@ TARGETS += net
> >  TARGETS += netfilter
> >  TARGETS += networking/timestamping
> >  TARGETS += nsfs
> > +TARGETS += pidfd
> >  TARGETS += powerpc
> >  TARGETS += proc
> >  TARGETS += pstore
> > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > new file mode 100644
> > index 000000000000..deaf8073bc06
> > --- /dev/null
> > +++ b/tools/testing/selftests/pidfd/Makefile
> > @@ -0,0 +1,6 @@
> > +CFLAGS += -g -I../../../../usr/include/
> > +
> > +TEST_GEN_PROGS := pidfd_test
> > +
> > +include ../lib.mk
> > +
> > diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
> > new file mode 100644
> > index 000000000000..edcd59979b10
> > --- /dev/null
> > +++ b/tools/testing/selftests/pidfd/pidfd_test.c
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#define _GNU_SOURCE
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <linux/types.h>
> > +#include <signal.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <syscall.h>
> > +#include <sys/wait.h>
> > +#include <unistd.h>
> > +
> > +#include "../kselftest.h"
> > +
> > +static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> > +					unsigned int flags)
> > +{
> > +	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> > +}
> > +
> > +static int signal_received;
> > +
> > +static void do_exit_success(int sig)
> > +{
> > +	signal_received = 1;
> > +}
> > +
> > +/*
> > + * Straightforward test to see whether pidfd_send_signal() works is to send
> > + * a signal to ourselves.
> > + */
> > +static int test_pidfd_send_signal_simple_success(void)
> > +{
> > +	int pidfd, ret;
> > +	const char *test_name = "pidfd_send_signal send SIGUSR1";
> > +
> > +	pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
> > +	if (pidfd < 0)
> > +		ksft_exit_fail_msg(
> > +			"%s test: Failed to open process file descriptor\n",
> > +			test_name);
> > +
> > +	signal(SIGUSR1, do_exit_success);
> > +
> > +	ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
> > +	close(pidfd);
> > +	if (ret < 0)
> > +		ksft_exit_fail_msg("%s test: Failed to send signal\n",
> > +				   test_name);
> > +
> > +	if (signal_received != 1)
> > +		ksft_exit_fail_msg("%s test: Failed to receive signal\n",
> > +				   test_name);
> > +
> > +	signal_received = 0;
> > +	ksft_test_result_pass("%s test: Sent signal\n", test_name);
> > +	return 0;
> > +}
> > +
> > +static void wait_for_pid(pid_t pid)
> > +{
> > +	int status, ret;
> > +
> > +again:
> > +	ret = waitpid(pid, &status, 0);
> > +	if (ret == -1) {
> > +		if (errno == EINTR)
> > +			goto again;
> > +
> > +		return;
> > +	}
> > +
> > +	if (ret != pid)
> > +		goto again;
> > +}
> > +
> > +static int test_pidfd_send_signal_exited_fail(void)
> > +{
> > +	int pidfd, ret, saved_errno;
> > +	char buf[256];
> > +	pid_t pid;
> > +	const char *test_name = "pidfd_send_signal signal exited process";
> > +
> > +	pid = fork();
> > +	if (pid < 0)
> > +		ksft_exit_fail_msg("%s test: Failed to create new process\n",
> > +				   test_name);
> > +
> > +	if (pid == 0)
> > +		_exit(EXIT_SUCCESS);
> > +
> > +	snprintf(buf, sizeof(buf), "/proc/%d", pid);
> > +
> > +	pidfd = open(buf, O_DIRECTORY | O_CLOEXEC);
> > +
> > +	wait_for_pid(pid);
> > +
> > +	if (pidfd < 0)
> > +		ksft_exit_fail_msg(
> > +			"%s test: Failed to open process file descriptor\n",
> > +			test_name);
> > +
> > +	ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0);
> > +	saved_errno = errno;
> > +	close(pidfd);
> > +	if (ret == 0)
> > +		ksft_exit_fail_msg(
> > +			"%s test: Managed to send signal to process even though it should have failed\n",
> > +			test_name);
> > +
> > +	if (saved_errno != ESRCH)
> > +		ksft_exit_fail_msg(
> > +			"%s test: Expected to receive ESRCH as errno value but received %d instead\n",
> > +			test_name, saved_errno);
> > +
> > +	ksft_test_result_pass("%s test: Failed to send signal as expected\n",
> > +			      test_name);
> > +	return 0;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	ksft_print_header();
> > +
> > +	test_pidfd_send_signal_simple_success();
> > +	test_pidfd_send_signal_exited_fail();
> > +
> > +	return ksft_exit_pass();
> > +}
> > -- 
> > 2.19.1

^ permalink raw reply

* Re: [PATCH v6 2/2] selftests: add tests for pidfd_send_signal()
From: Serge E. Hallyn @ 2018-12-30 21:02 UTC (permalink / raw)
  To: Christian Brauner
  Cc: linux-kernel, linux-api, luto, arnd, serge, keescook, akpm, jannh,
	oleg, cyphar, viro, linux-fsdevel, dancol, timmurray, fweimer,
	tglx, x86, ebiederm
In-Reply-To: <20181229222756.30027-2-christian@brauner.io>

On Sat, Dec 29, 2018 at 11:27:56PM +0100, Christian Brauner wrote:
> As suggested by Andrew Morton in [1] add selftests for the new
> sys_pidfd_send_signal() syscall.
> This tests whether we can send a signal to an existing process and whether
> sending a signal to a process that has already exited fails with ESRCH.
> 
> [1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge Hallyn <serge@hallyn.com>

Acked-by: Serge Hallyn <serge@hallyn.com>

Not saying you need to do this, but it would be neat if you could test
sending to a pid which has been recycled :)

> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> /* Changelog */
> v6:
> - patch introduced
> v5..v0:
> - patch not present
> ---
>  tools/testing/selftests/Makefile           |   1 +
>  tools/testing/selftests/pidfd/Makefile     |   6 +
>  tools/testing/selftests/pidfd/pidfd_test.c | 130 +++++++++++++++++++++
>  3 files changed, 137 insertions(+)
>  create mode 100644 tools/testing/selftests/pidfd/Makefile
>  create mode 100644 tools/testing/selftests/pidfd/pidfd_test.c
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 24b9934fb269..63b0d8a0ebf7 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -27,6 +27,7 @@ TARGETS += net
>  TARGETS += netfilter
>  TARGETS += networking/timestamping
>  TARGETS += nsfs
> +TARGETS += pidfd
>  TARGETS += powerpc
>  TARGETS += proc
>  TARGETS += pstore
> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> new file mode 100644
> index 000000000000..deaf8073bc06
> --- /dev/null
> +++ b/tools/testing/selftests/pidfd/Makefile
> @@ -0,0 +1,6 @@
> +CFLAGS += -g -I../../../../usr/include/
> +
> +TEST_GEN_PROGS := pidfd_test
> +
> +include ../lib.mk
> +
> diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
> new file mode 100644
> index 000000000000..edcd59979b10
> --- /dev/null
> +++ b/tools/testing/selftests/pidfd/pidfd_test.c
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/types.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <syscall.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "../kselftest.h"
> +
> +static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> +					unsigned int flags)
> +{
> +	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> +}
> +
> +static int signal_received;
> +
> +static void do_exit_success(int sig)
> +{
> +	signal_received = 1;
> +}
> +
> +/*
> + * Straightforward test to see whether pidfd_send_signal() works is to send
> + * a signal to ourselves.
> + */
> +static int test_pidfd_send_signal_simple_success(void)
> +{
> +	int pidfd, ret;
> +	const char *test_name = "pidfd_send_signal send SIGUSR1";
> +
> +	pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
> +	if (pidfd < 0)
> +		ksft_exit_fail_msg(
> +			"%s test: Failed to open process file descriptor\n",
> +			test_name);
> +
> +	signal(SIGUSR1, do_exit_success);
> +
> +	ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
> +	close(pidfd);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("%s test: Failed to send signal\n",
> +				   test_name);
> +
> +	if (signal_received != 1)
> +		ksft_exit_fail_msg("%s test: Failed to receive signal\n",
> +				   test_name);
> +
> +	signal_received = 0;
> +	ksft_test_result_pass("%s test: Sent signal\n", test_name);
> +	return 0;
> +}
> +
> +static void wait_for_pid(pid_t pid)
> +{
> +	int status, ret;
> +
> +again:
> +	ret = waitpid(pid, &status, 0);
> +	if (ret == -1) {
> +		if (errno == EINTR)
> +			goto again;
> +
> +		return;
> +	}
> +
> +	if (ret != pid)
> +		goto again;
> +}
> +
> +static int test_pidfd_send_signal_exited_fail(void)
> +{
> +	int pidfd, ret, saved_errno;
> +	char buf[256];
> +	pid_t pid;
> +	const char *test_name = "pidfd_send_signal signal exited process";
> +
> +	pid = fork();
> +	if (pid < 0)
> +		ksft_exit_fail_msg("%s test: Failed to create new process\n",
> +				   test_name);
> +
> +	if (pid == 0)
> +		_exit(EXIT_SUCCESS);
> +
> +	snprintf(buf, sizeof(buf), "/proc/%d", pid);
> +
> +	pidfd = open(buf, O_DIRECTORY | O_CLOEXEC);
> +
> +	wait_for_pid(pid);
> +
> +	if (pidfd < 0)
> +		ksft_exit_fail_msg(
> +			"%s test: Failed to open process file descriptor\n",
> +			test_name);
> +
> +	ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0);
> +	saved_errno = errno;
> +	close(pidfd);
> +	if (ret == 0)
> +		ksft_exit_fail_msg(
> +			"%s test: Managed to send signal to process even though it should have failed\n",
> +			test_name);
> +
> +	if (saved_errno != ESRCH)
> +		ksft_exit_fail_msg(
> +			"%s test: Expected to receive ESRCH as errno value but received %d instead\n",
> +			test_name, saved_errno);
> +
> +	ksft_test_result_pass("%s test: Failed to send signal as expected\n",
> +			      test_name);
> +	return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	ksft_print_header();
> +
> +	test_pidfd_send_signal_simple_success();
> +	test_pidfd_send_signal_exited_fail();
> +
> +	return ksft_exit_pass();
> +}
> -- 
> 2.19.1

^ permalink raw reply

* Re: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Peter Maydell @ 2018-12-30 13:59 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Matthew Wilcox, Andreas Dilger, Florian Weimer, linux-fsdevel,
	Linux API, Ext4 Developers List, Latchesar Ionkov, libc-alpha,
	Arnd Bergmann, Eric Van Hensbergen, H. Peter Anvin,
	lkml - Kernel Mailing List, QEMU Developers, Ron Minnich,
	V9FS Developers
In-Reply-To: <CALCETrUJn4cV+o3scP1EzYS=JA--5KUXvjuxZDbsnhXra1vrwA@mail.gmail.com>

On Sat, 29 Dec 2018 at 16:49, Andy Lutomirski <luto@kernel.org> wrote:
> > Could you use a prctl to set whether you were running in 32 or 64 bit
> > mode?  Or do you change which kind of task you're emulating too often
> > to make this a good idea?

QEMU's linux-user mode always only runs the single process,
which is a fixed guest architecture. But it also wants to
make system calls on its own behalf, as well as the ones it
is passing through from the guest, and I suspect it would
confuse the host libc if we changed the semantics of those
under its feet.

> How would this work?  We already have the separate
> COMPAT_DEFINE_SYSCALL entries *and* in_compat_syscall(). Now we’d have
> a third degree of freedom.
>
> Either the arches people care about should add reasonable ways to
> issue 32-bit syscalls from 64-bit mode or there should be an explicit
> way to ask for the 32-bit directory offsets.

The first of those is not sufficient for QEMU if done
as a per-architecture thing, because there may not even be
a 32-bit syscall interface on the host kernel. The second
sounds better -- there's nothing conceptually architecture
specific about what we want to do or which is tied to the
idea of whether there's a 32-bit compat mode in the host
architecture or not.

thanks
-- PMM

^ permalink raw reply

* [PATCH v6 2/2] selftests: add tests for pidfd_send_signal()
From: Christian Brauner @ 2018-12-29 22:27 UTC (permalink / raw)
  To: linux-kernel, linux-api, luto, arnd, serge, keescook, akpm
  Cc: jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
	fweimer, tglx, x86, ebiederm, Christian Brauner
In-Reply-To: <20181229222756.30027-1-christian@brauner.io>

As suggested by Andrew Morton in [1] add selftests for the new
sys_pidfd_send_signal() syscall.
This tests whether we can send a signal to an existing process and whether
sending a signal to a process that has already exited fails with ESRCH.

[1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* Changelog */
v6:
- patch introduced
v5..v0:
- patch not present
---
 tools/testing/selftests/Makefile           |   1 +
 tools/testing/selftests/pidfd/Makefile     |   6 +
 tools/testing/selftests/pidfd/pidfd_test.c | 130 +++++++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 tools/testing/selftests/pidfd/Makefile
 create mode 100644 tools/testing/selftests/pidfd/pidfd_test.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 24b9934fb269..63b0d8a0ebf7 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -27,6 +27,7 @@ TARGETS += net
 TARGETS += netfilter
 TARGETS += networking/timestamping
 TARGETS += nsfs
+TARGETS += pidfd
 TARGETS += powerpc
 TARGETS += proc
 TARGETS += pstore
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
new file mode 100644
index 000000000000..deaf8073bc06
--- /dev/null
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -0,0 +1,6 @@
+CFLAGS += -g -I../../../../usr/include/
+
+TEST_GEN_PROGS := pidfd_test
+
+include ../lib.mk
+
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
new file mode 100644
index 000000000000..edcd59979b10
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/types.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "../kselftest.h"
+
+static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
+					unsigned int flags)
+{
+	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
+}
+
+static int signal_received;
+
+static void do_exit_success(int sig)
+{
+	signal_received = 1;
+}
+
+/*
+ * Straightforward test to see whether pidfd_send_signal() works is to send
+ * a signal to ourselves.
+ */
+static int test_pidfd_send_signal_simple_success(void)
+{
+	int pidfd, ret;
+	const char *test_name = "pidfd_send_signal send SIGUSR1";
+
+	pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
+	if (pidfd < 0)
+		ksft_exit_fail_msg(
+			"%s test: Failed to open process file descriptor\n",
+			test_name);
+
+	signal(SIGUSR1, do_exit_success);
+
+	ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
+	close(pidfd);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s test: Failed to send signal\n",
+				   test_name);
+
+	if (signal_received != 1)
+		ksft_exit_fail_msg("%s test: Failed to receive signal\n",
+				   test_name);
+
+	signal_received = 0;
+	ksft_test_result_pass("%s test: Sent signal\n", test_name);
+	return 0;
+}
+
+static void wait_for_pid(pid_t pid)
+{
+	int status, ret;
+
+again:
+	ret = waitpid(pid, &status, 0);
+	if (ret == -1) {
+		if (errno == EINTR)
+			goto again;
+
+		return;
+	}
+
+	if (ret != pid)
+		goto again;
+}
+
+static int test_pidfd_send_signal_exited_fail(void)
+{
+	int pidfd, ret, saved_errno;
+	char buf[256];
+	pid_t pid;
+	const char *test_name = "pidfd_send_signal signal exited process";
+
+	pid = fork();
+	if (pid < 0)
+		ksft_exit_fail_msg("%s test: Failed to create new process\n",
+				   test_name);
+
+	if (pid == 0)
+		_exit(EXIT_SUCCESS);
+
+	snprintf(buf, sizeof(buf), "/proc/%d", pid);
+
+	pidfd = open(buf, O_DIRECTORY | O_CLOEXEC);
+
+	wait_for_pid(pid);
+
+	if (pidfd < 0)
+		ksft_exit_fail_msg(
+			"%s test: Failed to open process file descriptor\n",
+			test_name);
+
+	ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0);
+	saved_errno = errno;
+	close(pidfd);
+	if (ret == 0)
+		ksft_exit_fail_msg(
+			"%s test: Managed to send signal to process even though it should have failed\n",
+			test_name);
+
+	if (saved_errno != ESRCH)
+		ksft_exit_fail_msg(
+			"%s test: Expected to receive ESRCH as errno value but received %d instead\n",
+			test_name, saved_errno);
+
+	ksft_test_result_pass("%s test: Failed to send signal as expected\n",
+			      test_name);
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	ksft_print_header();
+
+	test_pidfd_send_signal_simple_success();
+	test_pidfd_send_signal_exited_fail();
+
+	return ksft_exit_pass();
+}
-- 
2.19.1

^ permalink raw reply related

* [PATCH v6 1/2] signal: add pidfd_send_signal() syscall
From: Christian Brauner @ 2018-12-29 22:27 UTC (permalink / raw)
  To: linux-kernel, linux-api, luto, arnd, serge, keescook, akpm
  Cc: jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
	fweimer, tglx, x86, ebiederm, Christian Brauner

The kill() syscall operates on process identifiers (pid). After a process
has exited its pid can be reused by another process. If a caller sends a
signal to a reused pid it will end up signaling the wrong process. This
issue has often surfaced and there has been a push to address this problem [1].

This patch uses file descriptors (fd) from proc/<pid> as stable handles on
struct pid. Even if a pid is recycled the handle will not change. The fd
can be used to send signals to the process it refers to.
Thus, the new syscall pidfd_send_signal() is introduced to solve this
problem. Instead of pids it operates on process fds (pidfd).

/* prototype and argument /*
long pidfd_send_signal(int pidfd, int sig, siginfo_t *info, unsigned int flags);

In addition to the pidfd and signal argument it takes an additional
siginfo_t and flags argument. If the siginfo_t argument is NULL then
pidfd_send_signal() is equivalent to kill(<positive-pid>, <signal>). If it
is not NULL pidfd_send_signal() is equivalent to rt_sigqueueinfo().
The flags argument is added to allow for future extensions of this syscall.
It currently needs to be passed as 0. Failing to do so will cause EINVAL.

/* pidfd_send_signal() replaces multiple pid-based syscalls */
The pidfd_send_signal() syscall currently takes on the job of
rt_sigqueueinfo(2) and parts of the functionality of kill(2), Namely, when a
positive pid is passed to kill(2). It will however be possible to also
replace tgkill(2) and rt_tgsigqueueinfo(2) if this syscall is extended.

/* sending signals to threads (tid) and process groups (pgid) */
Specifically, the pidfd_send_signal() syscall does currently not operate on
process groups or threads. This is left for future extensions.
In order to extend the syscall to allow sending signal to threads and
process groups appropriately named flags (e.g. PIDFD_TYPE_PGID, and
PIDFD_TYPE_TID) should be added. This implies that the flags argument will
determine what is signaled and not the file descriptor itself. Put in other
words, grouping in this api is a property of the flags argument not a
property of the file descriptor (cf. [13]). Clarification for this has been
requested by Eric (cf. [19]).
When appropriate extensions through the flags argument are added then
pidfd_send_signal() can additionally replace the part of kill(2) which
operates on process groups as well as the tgkill(2) and
rt_tgsigqueueinfo(2) syscalls.
How such an extension could be implemented has been very roughly sketched
in [14], [15], and [16]. However, this should not be taken as a commitment
to a particular implementation. There might be better ways to do it.
Right now this is intentionally left out to keep this patchset as simple as
possible (cf. [4]). For example, if a pidfd for a tid from
/proc/<pid>/task/<tid> is passed EOPNOTSUPP will be returned to give
userspace a way to detect when I add support for signaling to threads (cf. [10]).

/* naming */
The syscall had various names throughout iterations of this patchset:
- procfd_signal()
- procfd_send_signal()
- taskfd_send_signal()
In the last round of reviews it was pointed out that given that if the
flags argument decides the scope of the signal instead of different types
of fds it might make sense to either settle for "procfd_" or "pidfd_" as
prefix. The community was willing to accept either (cf. [17] and [18]).
Given that one developer expressed strong preference for the "pidfd_"
prefix (cf. [13] and with other developers less opinionated about the name
we should settle for "pidfd_" to avoid further bikeshedding.

The  "_send_signal" suffix was chosen to reflect the fact that the syscall
takes on the job of multiple syscalls. It is therefore intentional that the
name is not reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the
fomer because it might imply that pidfd_send_signal() is a replacement for
kill(2), and not the latter because it is a hassle to remember the correct
spelling - especially for non-native speakers - and because it is not
descriptive enough of what the syscall actually does. The name
"pidfd_send_signal" makes it very clear that its job is to send signals.

/* zombies */
Zombies can be signaled just as any other process. No special error will be
reported since a zombie state is an unreliable state (cf. [3]). However,
this can be added as an extension through the @flags argument if the need
ever arises.

/* cross-namespace signals */
The patch currently enforces that the signaler and signalee either are in
the same pid namespace or that the signaler's pid namespace is an ancestor
of the signalee's pid namespace. This is done for the sake of simplicity
and because it is unclear to what values certain members of struct
siginfo_t would need to be set to (cf. [5], [6]).

/* compat syscalls */
It became clear that we would like to avoid adding compat syscalls
(cf. [7]).  The compat syscall handling is now done in kernel/signal.c
itself by adding __copy_siginfo_from_user_generic() which lets us avoid
compat syscalls (cf. [8]). It should be noted that the addition of
__copy_siginfo_from_user_any() is caused by a bug in the original
implementation of rt_sigqueueinfo(2) (cf. 12).
With upcoming rework for syscall handling things might improve
significantly (cf. [11]) and __copy_siginfo_from_user_any() will not gain
any additional callers.

/* testing */
This patch was tested on x64 and x86.

/* userspace usage */
An asciinema recording for the basic functionality can be found under [9].
With this patch a process can be killed via:

 #define _GNU_SOURCE
 #include <errno.h>
 #include <fcntl.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
 #include <unistd.h>

 static inline int do_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
                                         unsigned int flags)
 {
 #ifdef __NR_pidfd_send_signal
         return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
 #else
         return -ENOSYS;
 #endif
 }

 int main(int argc, char *argv[])
 {
         int fd, ret, saved_errno, sig;

         if (argc < 3)
                 exit(EXIT_FAILURE);

         fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
         if (fd < 0) {
                 printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
                 exit(EXIT_FAILURE);
         }

         sig = atoi(argv[2]);

         printf("Sending signal %d to process %s\n", sig, argv[1]);
         ret = do_pidfd_send_signal(fd, sig, NULL, 0);

         saved_errno = errno;
         close(fd);
         errno = saved_errno;

         if (ret < 0) {
                 printf("%s - Failed to send signal %d to process %s\n",
                        strerror(errno), sig, argv[1]);
                 exit(EXIT_FAILURE);
         }

         exit(EXIT_SUCCESS);
 }

/* Q&A
 * Given that it seems the same questions get asked again by people who are
 * late to the party it makes sense to add a Q&A section to the commit
 * message so it's hopefully easier to avoid duplicate threads.
 *
 * For the sake of progress please consider these arguments settled unless
 * there is a new point that desperately needs to be addressed. Please make
 * sure to check the links to the threads in this commit message whether
 * this has not already been covered.
 */
Q-01: (Florian Weimer [20], Andrew Morton [21])
      What happens when the target process has exited?
A-01: Sending the signal will fail with ESRCH (cf. [22]).

Q-02:  (Andrew Morton [21])
       Is the task_struct pinned by the fd?
A-02:  No. A reference to struct pid is kept. struct pid - as far as I
       understand - was created exactly for the reason to not require to
       pin struct task_struct (cf. [22]).

Q-03: (Andrew Morton [21])
      Does the entire procfs directory remain visible? Just one entry
      within it?
A-03: The same thing that happens right now when you hold a file descriptor
      to /proc/<pid> open (cf. [22]).

Q-04: (Andrew Morton [21])
      Does the pid remain reserved?
A-04: No. This patchset guarantees a stable handle not that pids are not
      recycled (cf. [22]).

Q-05: (Andrew Morton [21])
      Do attempts to signal that fd return errors?
A-05: See {Q,A}-01.

Q-06: (Andrew Morton [22])
      Is there a cleaner way of obtaining the fd? Another syscall perhaps.
A-06: Userspace can already trivially retrieve file descriptors from procfs
      so this is something that we will need to support anyway. Hence,
      there's no immediate need to add another syscalls just to make
      pidfd_send_signal() not dependent on the presence of procfs. However,
      adding a syscalls to get such file descriptors is planned for a
      future patchset (cf. [22]).

Q-07: (Andrew Morton [21] and others)
      This fd-for-a-process sounds like a handy thing and people may well
      think up other uses for it in the future, probably unrelated to
      signals. Are the code and the interface designed to permit such
      future applications?
A-07: Yes (cf. [22]).

Q-08: (Andrew Morton [21] and others)
      Now I think about it, why a new syscall? This thing is looking
      rather like an ioctl?
A-08: This has been extensively discussed. It was agreed that a syscall is
      preferred for a variety or reasons. Here are just a few taken from
      prior threads. Syscalls are safer than ioctl()s especially when
      signaling to fds. Processes are a core kernel concept so a syscall
      seems more appropriate. The layout of the syscall with its four
      arguments would require the addition of a custom struct for the
      ioctl() thereby causing at least the same amount or even more
      complexity for userspace than a simple syscall. The new syscall will
      replace multiple other pid-based syscalls (see description above).
      The file-descriptors-for-processes concept introduced with this
      syscall will be extended with other syscalls in the future. See also
      [22], [23] and various other threads already linked in here.

Q-09: (Florian Weimer [24])
      What happens if you use the new interface with an O_PATH descriptor?
A-09:
      pidfds opened as O_PATH fds cannot be used to send signals to a
      process (cf. [2]). Signaling processes through pidfds is the
      equivalent of writing to a file. Thus, this is not an operation that
      operates "purely at the file descriptor level" as required by the
      open(2) manpage. See also [4].

/* References */
[1]:  https://lore.kernel.org/lkml/20181029221037.87724-1-dancol@google.com/
[2]:  https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
[3]:  https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
[4]:  https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
[5]:  https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
[6]:  https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
[7]:  https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
[8]:  https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
[9]:  https://asciinema.org/a/IQjuCHew6bnq1cr78yuMv16cy
[10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
[11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
[12]: https://lore.kernel.org/lkml/87zhtjn8ck.fsf@xmission.com/
[13]: https://lore.kernel.org/lkml/871s6u9z6u.fsf@xmission.com/
[14]: https://lore.kernel.org/lkml/20181206231742.xxi4ghn24z4h2qki@brauner.io/
[15]: https://lore.kernel.org/lkml/20181207003124.GA11160@mail.hallyn.com/
[16]: https://lore.kernel.org/lkml/20181207015423.4miorx43l3qhppfz@brauner.io/
[17]: https://lore.kernel.org/lkml/CAGXu5jL8PciZAXvOvCeCU3wKUEB_dU-O3q0tDw4uB_ojMvDEew@mail.gmail.com/
[18]: https://lore.kernel.org/lkml/20181206222746.GB9224@mail.hallyn.com/
[19]: https://lore.kernel.org/lkml/20181208054059.19813-1-christian@brauner.io/
[20]: https://lore.kernel.org/lkml/8736rebl9s.fsf@oldenburg.str.redhat.com/
[21]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
[22]: https://lore.kernel.org/lkml/20181228233725.722tdfgijxcssg76@brauner.io/
[23]: https://lwn.net/Articles/773459/
[24]: https://lore.kernel.org/lkml/8736rebl9s.fsf@oldenburg.str.redhat.com/

Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Christian Brauner <christian@brauner.io>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Serge Hallyn <serge@hallyn.com>
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
---
/* Changelog */
v6:
- Given that it seems the same questions get asked multiple times it made
  sense to add a Q&A section to the commit message so it's hopefully easier
  to avoid duplicate threads.
- Since we settled on adding flags when extending the syscalls to allow
  signaling to threads and process groups it doesn't make sense anymore to
  report EOPNOTSUPP when a file descriptor to /proc/<pid>/task/<tid> is
  passed. This means we can also remove the tgid_pidfd_to_pid() helper from
  proc_fs.h and simplify the code. We will now always return EBADF when a
  file descriptor is passed that does not refer to /proc/<pid>.
- add CONFIG_PROC_FS ifdefs for pidfd_send_signal() and add
  COND_SYSCALL(pidfd_send_signal) definition as suggested by Andrew Morgan
  in [changelog-1].
v5:
- s/may_signal_taskfd/access_taskfd_pidns/g
- make it clear that process grouping is a property of the @flags argument
  Eric has argued that he would like to know when we add thread and process
  group signal support whether grouping will be a property of the file
  descriptor or the flag argument and he would oppose this until a
  commitment has been made. It seems that the cleanest strategy is to make
  grouping a property of the @flags argument.
  He also argued that in this case the prefix of the syscall should be
  "pidfd_" (cf. https://lore.kernel.org/lkml/871s6u9z6u.fsf@xmission.com/).
- use "pidfd_" as prefix for the syscall since grouping will be a property
  of the @flags argument
- substantial rewrite of the commit message to reflect the discussion
v4:
- updated asciinema to use "taskfd_" prefix
- s/procfd_send_signal/taskfd_send_signal/g
- s/proc_is_tgid_procfd/tgid_taskfd_to_pid/b
- s/proc_is_tid_procfd/tid_taskfd_to_pid/b
- s/__copy_siginfo_from_user_generic/__copy_siginfo_from_user_any/g
- make it clear that __copy_siginfo_from_user_any() is a workaround caused
  by a bug in the original implementation of rt_sigqueueinfo()
- when spoofing signals turn them into regular kill signals if si_code is
  set to SI_USER
- make proc_is_t{g}id_procfd() return struct pid to allow proc_pid() to
  stay private to fs/proc/
v3:
- add __copy_siginfo_from_user_generic() to avoid adding compat syscalls
- s/procfd_signal/procfd_send_signal/g
- change type of flags argument from int to unsigned int
- add comment about what happens to zombies
- add proc_is_tid_procfd()
- return EOPNOTSUPP when /proc/<pid>/task/<tid> fd is passed so userspace
  has a way of knowing that tidfds are not supported currently.
v2:
- define __NR_procfd_signal in unistd.h
- wire up compat syscall
- s/proc_is_procfd/proc_is_tgid_procfd/g
- provide stubs when CONFIG_PROC_FS=n
- move proc_pid() to linux/proc_fs.h header
- use proc_pid() to grab struct pid from /proc/<pid> fd
v1:
- patch introduced

/* Changelog references */
[changelog-1]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
---
 arch/x86/entry/syscalls/syscall_32.tbl |   1 +
 arch/x86/entry/syscalls/syscall_64.tbl |   1 +
 fs/proc/base.c                         |   9 ++
 include/linux/proc_fs.h                |   6 ++
 include/linux/syscalls.h               |   3 +
 include/uapi/asm-generic/unistd.h      |   4 +-
 kernel/signal.c                        | 133 +++++++++++++++++++++++--
 kernel/sys_ni.c                        |   1 +
 8 files changed, 151 insertions(+), 7 deletions(-)

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 3cf7b533b3d1..6804c1e84b36 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -398,3 +398,4 @@
 384	i386	arch_prctl		sys_arch_prctl			__ia32_compat_sys_arch_prctl
 385	i386	io_pgetevents		sys_io_pgetevents		__ia32_compat_sys_io_pgetevents
 386	i386	rseq			sys_rseq			__ia32_sys_rseq
+387	i386	pidfd_send_signal	sys_pidfd_send_signal		__ia32_sys_pidfd_send_signal
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index f0b1709a5ffb..aa4b858fa0f1 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -343,6 +343,7 @@
 332	common	statx			__x64_sys_statx
 333	common	io_pgetevents		__x64_sys_io_pgetevents
 334	common	rseq			__x64_sys_rseq
+335	common	pidfd_send_signal	__x64_sys_pidfd_send_signal
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ce3465479447..9b812b777faa 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3038,6 +3038,15 @@ static const struct file_operations proc_tgid_base_operations = {
 	.llseek		= generic_file_llseek,
 };
 
+struct pid *tgid_pidfd_to_pid(const struct file *file)
+{
+	if (!d_is_dir(file->f_path.dentry) ||
+	    (file->f_op != &proc_tgid_base_operations))
+		return ERR_PTR(-EBADF);
+
+	return proc_pid(file_inode(file));
+}
+
 static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 {
 	return proc_pident_lookup(dir, dentry,
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index d0e1f1522a78..52a283ba0465 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -73,6 +73,7 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
 						    int (*show)(struct seq_file *, void *),
 						    proc_write_t write,
 						    void *data);
+extern struct pid *tgid_pidfd_to_pid(const struct file *file);
 
 #else /* CONFIG_PROC_FS */
 
@@ -114,6 +115,11 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
 #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
 #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
 
+static inline struct pid *tgid_pidfd_to_pid(const struct file *file)
+{
+	return ERR_PTR(-EBADF);
+}
+
 #endif /* CONFIG_PROC_FS */
 
 struct net;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 2ac3d13a915b..fd85b9045a9f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -907,6 +907,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
 			  unsigned mask, struct statx __user *buffer);
 asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
 			 int flags, uint32_t sig);
+asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
+				       siginfo_t __user *info,
+				       unsigned int flags);
 
 /*
  * Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index d90127298f12..b77538af7aca 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -740,9 +740,11 @@ __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
 __SYSCALL(__NR_rseq, sys_rseq)
 #define __NR_kexec_file_load 294
 __SYSCALL(__NR_kexec_file_load,     sys_kexec_file_load)
+#define __NR_pidfd_send_signal 295
+__SYSCALL(__NR_pidfd_send_signal, sys_pidfd_send_signal)
 
 #undef __NR_syscalls
-#define __NR_syscalls 295
+#define __NR_syscalls 296
 
 /*
  * 32 bit systems traditionally used different
diff --git a/kernel/signal.c b/kernel/signal.c
index 9a32bc2088c9..a108368905c7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -19,7 +19,9 @@
 #include <linux/sched/task.h>
 #include <linux/sched/task_stack.h>
 #include <linux/sched/cputime.h>
+#include <linux/file.h>
 #include <linux/fs.h>
+#include <linux/proc_fs.h>
 #include <linux/tty.h>
 #include <linux/binfmts.h>
 #include <linux/coredump.h>
@@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
 }
 #endif
 
+static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
+{
+	clear_siginfo(info);
+	info->si_signo = sig;
+	info->si_errno = 0;
+	info->si_code = SI_USER;
+	info->si_pid = task_tgid_vnr(current);
+	info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
+}
+
 /**
  *  sys_kill - send a signal to a process
  *  @pid: the PID of the process
@@ -3295,16 +3307,125 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
 {
 	struct kernel_siginfo info;
 
-	clear_siginfo(&info);
-	info.si_signo = sig;
-	info.si_errno = 0;
-	info.si_code = SI_USER;
-	info.si_pid = task_tgid_vnr(current);
-	info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+	prepare_kill_siginfo(sig, &info);
 
 	return kill_something_info(sig, &info, pid);
 }
 
+#ifdef CONFIG_PROC_FS
+/*
+ * Verify that the signaler and signalee either are in the same pid namespace
+ * or that the signaler's pid namespace is an ancestor of the signalee's pid
+ * namespace.
+ */
+static bool access_pidfd_pidns(struct pid *pid)
+{
+	struct pid_namespace *active = task_active_pid_ns(current);
+	struct pid_namespace *p = ns_of_pid(pid);
+
+	for (;;) {
+		if (!p)
+			return false;
+		if (p == active)
+			break;
+		p = p->parent;
+	}
+
+	return true;
+}
+
+static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, siginfo_t *info)
+{
+#ifdef CONFIG_COMPAT
+	/*
+	 * Avoid hooking up compat syscalls and instead handle necessary
+	 * conversions here. Note, this is a stop-gap measure and should not be
+	 * considered a generic solution.
+	 */
+	if (in_compat_syscall())
+		return copy_siginfo_from_user32(
+			kinfo, (struct compat_siginfo __user *)info);
+#endif
+	return copy_siginfo_from_user(kinfo, info);
+}
+
+/**
+ * sys_pidfd_send_signal - send a signal to a process through a task file
+ *                          descriptor
+ * @pidfd:  the file descriptor of the process
+ * @sig:    signal to be sent
+ * @info:   the signal info
+ * @flags:  future flags to be passed
+ *
+ * The syscall currently only signals via PIDTYPE_PID which covers
+ * kill(<positive-pid>, <signal>. It does not signal threads or process
+ * groups.
+ * In order to extend the syscall to threads and process groups the @flags
+ * argument should be used. In essence, the @flags argument will determine
+ * what is signaled and not the file descriptor itself. Put in other words,
+ * grouping is a property of the flags argument not a property of the file
+ * descriptor.
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
+		siginfo_t __user *, info, unsigned int, flags)
+{
+	int ret;
+	struct fd f;
+	struct pid *pid;
+	kernel_siginfo_t kinfo;
+
+	/* Enforce flags be set to 0 until we add an extension. */
+	if (flags)
+		return -EINVAL;
+
+	f = fdget_raw(pidfd);
+	if (!f.file)
+		return -EBADF;
+
+	/* Is this a pidfd? */
+	pid = tgid_pidfd_to_pid(f.file);
+	if (IS_ERR(pid)) {
+		ret = PTR_ERR(pid);
+		goto err;
+	}
+
+	ret = -EINVAL;
+	if (!access_pidfd_pidns(pid))
+		goto err;
+
+	if (info) {
+		ret = copy_siginfo_from_user_any(&kinfo, info);
+		if (unlikely(ret))
+			goto err;
+
+		ret = -EINVAL;
+		if (unlikely(sig != kinfo.si_signo))
+			goto err;
+
+		if ((task_pid(current) != pid) &&
+		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) {
+			/* Only allow sending arbitrary signals to yourself. */
+			ret = -EPERM;
+			if (kinfo.si_code != SI_USER)
+				goto err;
+
+			/* Turn this into a regular kill signal. */
+			prepare_kill_siginfo(sig, &kinfo);
+		}
+	} else {
+		prepare_kill_siginfo(sig, &kinfo);
+	}
+
+	ret = kill_pid_info(sig, &kinfo, pid);
+
+err:
+	fdput(f);
+	return ret;
+}
+#endif /* CONFIG_PROC_FS */
+
 static int
 do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
 {
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index df556175be50..e0af0a166bac 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -163,6 +163,7 @@ COND_SYSCALL(syslog);
 /* kernel/sched/core.c */
 
 /* kernel/signal.c */
+COND_SYSCALL(pidfd_send_signal);
 
 /* kernel/sys.c */
 COND_SYSCALL(setregid);
-- 
2.19.1

^ permalink raw reply related

* Re: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Andy Lutomirski @ 2018-12-29 16:49 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Peter Maydell, Andreas Dilger, Florian Weimer, linux-fsdevel,
	Linux API, Ext4 Developers List, Latchesar Ionkov, libc-alpha,
	Arnd Bergmann, Eric Van Hensbergen, H. Peter Anvin,
	lkml - Kernel Mailing List, QEMU Developers, Ron Minnich,
	V9FS Developers
In-Reply-To: <20181229015453.GA6310@bombadil.infradead.org>

> On Dec 28, 2018, at 6:54 PM, Matthew Wilcox <willy@infradead.org> wrote:
>
>> On Sat, Dec 29, 2018 at 12:12:27AM +0000, Peter Maydell wrote:
>> On Fri, 28 Dec 2018 at 23:16, Andreas Dilger <adilger@dilger.ca> wrot
>>> On Dec 28, 2018, at 4:18 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> The problem is that there is no 32-bit API in some cases
>>>> (unless I have misunderstood the kernel code) -- not all
>>>> host architectures implement compat syscalls or allow them
>>>> to be called from 64-bit processes or implement all the older
>>>> syscall variants that had smaller offets. If there was a guaranteed
>>>> "this syscall always exists and always gives me 32-bit offsets"
>>>> we could use it.
>>>
>>> The "32bitapi" mount option would use 32-bit hash for seekdir
>>> and telldir, regardless of what kernel API was used.  That would
>>> just set the FMODE_32BITHASH flag in the file->f_mode for all files.
>>
>> A mount option wouldn't be much use to QEMU -- we can't tell
>> our users how to mount their filesystems, which they're
>> often doing lots of other things with besides running QEMU.
>> (Otherwise we could just tell them "don't use ext4", which
>> would also solve the problem :-)) We need something we can
>> use at the individual-syscall level.
>
> Could you use a prctl to set whether you were running in 32 or 64 bit
> mode?  Or do you change which kind of task you're emulating too often
> to make this a good idea?


How would this work?  We already have the separate
COMPAT_DEFINE_SYSCALL entries *and* in_compat_syscall(). Now we’d have
a third degree of freedom.

Either the arches people care about should add reasonable ways to
issue 32-bit syscalls from 64-bit mode or there should be an explicit
way to ask for the 32-bit directory offsets.

^ permalink raw reply

* Re: [PATCH v6 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-12-29 15:41 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Jann Horn, James Bottomley, kernel list, Linux API, containers,
	dima, Al Viro, linux-fsdevel, Andrew Morton
In-Reply-To: <36933e07-d7b3-49be-4ad9-2028fa1977f7@vivier.eu>

Ping

Thanks,
Laurent

Le 29/11/2018 à 14:05, Laurent Vivier a écrit :
> Le 01/11/2018 à 15:16, Eric W. Biederman a écrit :
>> Laurent Vivier <laurent@vivier.eu> writes:
>>
>>> On 01/11/2018 04:51, Jann Horn wrote:
>>>> On Thu, Nov 1, 2018 at 3:59 AM James Bottomley
>>>> <James.Bottomley@hansenpartnership.com> wrote:
>>>>>
>>>>> On Tue, 2018-10-16 at 11:52 +0200, Laurent Vivier wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Any comment on this last version?
>>>>>>
>>>>>> Any chance to be merged?
>>>>>
>>>>> I've got a use case for this:  I went to one of the Graphene talks in
>>>>> Edinburgh and it struck me that we seem to keep reinventing the type of
>>>>> sandboxing that qemu-user already does.  However if you want to do an
>>>>> x86 on x86 sandbox, you can't currently use the binfmt_misc mechanism
>>>>> because that has you running *every* binary on the system emulated.
>>>>> Doing it per user namespace fixes this problem and allows us to at
>>>>> least cut down on all the pointless duplication.
>>>>
>>>> Waaaaaait. What? qemu-user does not do "sandboxing". qemu-user makes
>>>> your code slower and *LESS* secure. As far as I know, qemu-user is
>>>> only intended for purposes like development and testing.
>>>>
>>>
>>> I think the idea here is not to run qemu, but to use an interpreter
>>> (something like gVisor) into a container to control the binaries
>>> execution inside the container without using this interpreter on the
>>> host itself (container and host shares the same binfmt_misc
>>> magic/mask).
>>
>> Please remind me of this patchset after the merge window is over, and if
>> there are no issues I will take it via my user namespace branch.
>>
>> Last I looked I had a concern that some of the permission check issues
>> were being papered over by using override cred instead of fixing the
>> deaper code.  Sometimes they are necessary but seeing work-arounds
>> instead of fixes for problems tends to be a maintenance issue, possibly
>> with security consequences.  Best is if the everyone agrees on how all
>> of the interfaces work so their are no surprises.
> 
> I don't know where we are in the merge window, but is there something I
> can do to have this merged?
> 
> Thanks,
> Laurent
> 

^ permalink raw reply

* Re: [V9fs-developer] [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Dominique Martinet @ 2018-12-29  4:04 UTC (permalink / raw)
  To: Theodore Y. Ts'o, Peter Maydell, Andreas Dilger,
	Florian Weimer, linux-fsdevel, Linux API, Ext4 Developers List,
	lucho, libc-alpha, Arnd Bergmann, ericvh, hpa,
	lkml - Kernel Mailing List, QEMU Developers, rminnich,
	v9fs-developer
In-Reply-To: <20181229031416.GH5864@mit.edu>

Theodore Y. Ts'o wrote on Fri, Dec 28, 2018:
> On Sat, Dec 29, 2018 at 03:37:21AM +0100, Dominique Martinet wrote:
> > > Are there going to be cases where a process or a thread will sometimes
> > > want the 64-bit interface, and sometimes want the 32-bit interface?
> > > Or is it always going to be one or the other?  I wonder if we could
> > > simply add a new flag to the process personality(2) flags.
> > 
> > That would likely work for qemu user, but the qemu system+9p case is
> > going to be more painful..
> > More precisely, the 9p protocol does not plan for anything other than
> > 64bit offset so if the vfs needs to hand out a 32bit offset we'll need
> > to make a correspondance table between the 32bit offsets we hand off and
> > the 64bit ones to use; unless some flag can be passed at lopen to tell
> > the server to always hand out 32bit offsets for this directory... And if
> > we do that then 9p servers will need a way to use both APIs in parallel
> > for both types of directories.
> 
> How about if we add a fcntl(2) mediated flag, which is tied to a
> struct file?  Would that be more or less painful for 9p and qemu
> system+9p?

Hmm. 9P2000.L doesn't have anything akin to fcntl either, the only two
obvious places where we could pass a flag is lopen (which already
handles a bunch of linux-specific flags, e.g. passing O_LARGEFILE
O_NOATIME etc will just forward these through for qemu/diod at least),
or adding a new parameter to the 9p readdir.

The former would let us get away without modifying the protocol as
servers will just ignore flags they don't handle on implementations I
checked, so it'd definitely be the least effort choice from what I can
tell.


On the other hand a fcntl would solve the server-side problem, it'd
allow the server to request appropriately-sized offsets per fd, so it's
a good start; we "just" need to figure how to translate that on the wire.

-- 
Dominique Martinet | Asmadeus

^ permalink raw reply

* Re: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Theodore Y. Ts'o @ 2018-12-29  3:14 UTC (permalink / raw)
  To: Peter Maydell, Andreas Dilger, Florian Weimer, linux-fsdevel,
	Linux API, Ext4 Developers List, lucho, libc-alpha, Arnd Bergmann,
	ericvh, hpa, lkml - Kernel Mailing List, QEMU Developers,
	rminnich, v9fs-developer
In-Reply-To: <20181229023721.GA9291@nautica>

On Sat, Dec 29, 2018 at 03:37:21AM +0100, Dominique Martinet wrote:
> > Are there going to be cases where a process or a thread will sometimes
> > want the 64-bit interface, and sometimes want the 32-bit interface?
> > Or is it always going to be one or the other?  I wonder if we could
> > simply add a new flag to the process personality(2) flags.
> 
> That would likely work for qemu user, but the qemu system+9p case is
> going to be more painful..
> More precisely, the 9p protocol does not plan for anything other than
> 64bit offset so if the vfs needs to hand out a 32bit offset we'll need
> to make a correspondance table between the 32bit offsets we hand off and
> the 64bit ones to use; unless some flag can be passed at lopen to tell
> the server to always hand out 32bit offsets for this directory... And if
> we do that then 9p servers will need a way to use both APIs in parallel
> for both types of directories.

How about if we add a fcntl(2) mediated flag, which is tied to a
struct file?  Would that be more or less painful for 9p and qemu
system+9p?

       	      	    	    	    	 	 - Ted

^ permalink raw reply

* Re: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Dominique Martinet @ 2018-12-29  2:37 UTC (permalink / raw)
  To: Theodore Y. Ts'o, Peter Maydell, Andreas Dilger,
	Florian Weimer, linux-fsdevel, Linux API, Ext4 Developers List,
	lucho, libc-alpha, Arnd Bergmann, ericvh, hpa,
	lkml - Kernel Mailing List, QEMU Developers, rminnich,
	v9fs-developer
In-Reply-To: <20181229021157.GG5864@mit.edu>

Theodore Y. Ts'o wrote on Fri, Dec 28, 2018:
> > The problem is that there is no 32-bit API in some cases
> > (unless I have misunderstood the kernel code) -- not all
> > host architectures implement compat syscalls or allow them
> > to be called from 64-bit processes or implement all the older
> > syscall variants that had smaller offets. If there was a guaranteed
> > "this syscall always exists and always gives me 32-bit offsets"
> > we could use it.
> 
> Are there going to be cases where a process or a thread will sometimes
> want the 64-bit interface, and sometimes want the 32-bit interface?
> Or is it always going to be one or the other?  I wonder if we could
> simply add a new flag to the process personality(2) flags.

That would likely work for qemu user, but the qemu system+9p case is
going to be more painful..
More precisely, the 9p protocol does not plan for anything other than
64bit offset so if the vfs needs to hand out a 32bit offset we'll need
to make a correspondance table between the 32bit offsets we hand off and
the 64bit ones to use; unless some flag can be passed at lopen to tell
the server to always hand out 32bit offsets for this directory... And if
we do that then 9p servers will need a way to use both APIs in parallel
for both types of directories.

(Although I'd rather not have to do either in the first place, keeping
track of all used offsets just in case seems like a waste even if we
only do it for processes in 32bit mode, and a new flag would be a
protocol change with 9p not being designed to catter for subtle protocol
changes so would be rather painful to roll out)


No bright idea here, sorry.
-- 
Dominique Martinet | Asmadeus

^ permalink raw reply

* Re: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Theodore Y. Ts'o @ 2018-12-29  2:11 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Andreas Dilger, Florian Weimer, linux-fsdevel, Linux API,
	Ext4 Developers List, lucho, libc-alpha, Arnd Bergmann, ericvh,
	hpa, lkml - Kernel Mailing List, QEMU Developers, rminnich,
	v9fs-developer
In-Reply-To: <CAFEAcA9W+JK7_TrtTnL1P2ES1knNPJX9wcUvhfLwxLq9augq1w@mail.gmail.com>

On Fri, Dec 28, 2018 at 11:18:18AM +0000, Peter Maydell wrote:
> In general inodes and offsets start from 0 and work up --
> so almost all of the time they don't actually overflow.
> The problem with ext4 directory hash "offsets" is that they
> overflow all the time and immediately, so instead of "works
> unless you have a weird edge case" like all the other filesystems,h
> it's "never works".

Actually, XFS uses the inode number to encode the location of the
inode (it doesn't have a fixed inode table, so it's effectively the
block number shifted left by 3 or 4 bits, with the low bits indicating
the slot in the 4k block).  It has a hack to provide backwards
compatibility for 32-bit API's, but there is a similar, "oh, we're on
a non-paleolithic CPU, let's use the full 64-bits" sort of logic that
ext4 has.

> The problem is that there is no 32-bit API in some cases
> (unless I have misunderstood the kernel code) -- not all
> host architectures implement compat syscalls or allow them
> to be called from 64-bit processes or implement all the older
> syscall variants that had smaller offets. If there was a guaranteed
> "this syscall always exists and always gives me 32-bit offsets"
> we could use it.

Are there going to be cases where a process or a thread will sometimes
want the 64-bit interface, and sometimes want the 32-bit interface?
Or is it always going to be one or the other?  I wonder if we could
simply add a new flag to the process personality(2) flags.

> Yes, that has been suggested, but it seemed a bit dubious
> to bake in knowledge of ext4's internal implementation details.
> Can we rely on this as an ABI promise that will always work
> for all versions of all file systems going forwards?

Yeah, that seems dubious because I'm pretty sure there are other file
systems that may have their own 32/64-bit quirks.

						- Ted

^ permalink raw reply

* Re: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Matthew Wilcox @ 2018-12-29  1:54 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Andreas Dilger, Florian Weimer, linux-fsdevel, Linux API,
	Ext4 Developers List, Latchesar Ionkov, libc-alpha, Arnd Bergmann,
	Eric Van Hensbergen, hpa, lkml - Kernel Mailing List,
	QEMU Developers, rminnich, v9fs-developer
In-Reply-To: <CAFEAcA8Yq=CEyAUer1TcaSzuzcdNi3OUi4c67B6e+OkhmcKvbg@mail.gmail.com>

On Sat, Dec 29, 2018 at 12:12:27AM +0000, Peter Maydell wrote:
> On Fri, 28 Dec 2018 at 23:16, Andreas Dilger <adilger@dilger.ca> wrot
> > On Dec 28, 2018, at 4:18 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > The problem is that there is no 32-bit API in some cases
> > > (unless I have misunderstood the kernel code) -- not all
> > > host architectures implement compat syscalls or allow them
> > > to be called from 64-bit processes or implement all the older
> > > syscall variants that had smaller offets. If there was a guaranteed
> > > "this syscall always exists and always gives me 32-bit offsets"
> > > we could use it.
> >
> > The "32bitapi" mount option would use 32-bit hash for seekdir
> > and telldir, regardless of what kernel API was used.  That would
> > just set the FMODE_32BITHASH flag in the file->f_mode for all files.
> 
> A mount option wouldn't be much use to QEMU -- we can't tell
> our users how to mount their filesystems, which they're
> often doing lots of other things with besides running QEMU.
> (Otherwise we could just tell them "don't use ext4", which
> would also solve the problem :-)) We need something we can
> use at the individual-syscall level.

Could you use a prctl to set whether you were running in 32 or 64 bit
mode?  Or do you change which kind of task you're emulating too often
to make this a good idea?

^ permalink raw reply

* Re: [GIT PULL] y2038: more syscalls and cleanups
From: pr-tracker-bot @ 2018-12-29  1:30 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linux-sh list, y2038 Mailman List, Linux API,
	Linux Kernel Mailing List, Deepa Dinamani,
	Linux FS-devel Mailing List, Thomas Gleixner, Linus Torvalds
In-Reply-To: <CAK8P3a1t0RiYcHtpwk3jwma0SvsS3NE5PAvTbb2zR4n+wkh69A@mail.gmail.com>

The pull request you sent on Fri, 21 Dec 2018 00:09:43 +0100:

> (unable to parse the git remote)

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/b12a9124eeb71d766a3e3eb594ebbb3fefc66902

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

^ permalink raw reply

* Re: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
From: Peter Maydell @ 2018-12-29  0:12 UTC (permalink / raw)
  To: Andreas Dilger
  Cc: Florian Weimer, linux-fsdevel, Linux API, Ext4 Developers List,
	Latchesar Ionkov, libc-alpha, Arnd Bergmann, Eric Van Hensbergen,
	hpa, lkml - Kernel Mailing List, QEMU Developers, rminnich,
	v9fs-developer
In-Reply-To: <1EF1B31A-83D8-4642-BEBF-F56E45485223@dilger.ca>

On Fri, 28 Dec 2018 at 23:16, Andreas Dilger <adilger@dilger.ca> wrot
> On Dec 28, 2018, at 4:18 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > The problem is that there is no 32-bit API in some cases
> > (unless I have misunderstood the kernel code) -- not all
> > host architectures implement compat syscalls or allow them
> > to be called from 64-bit processes or implement all the older
> > syscall variants that had smaller offets. If there was a guaranteed
> > "this syscall always exists and always gives me 32-bit offsets"
> > we could use it.
>
> The "32bitapi" mount option would use 32-bit hash for seekdir
> and telldir, regardless of what kernel API was used.  That would
> just set the FMODE_32BITHASH flag in the file->f_mode for all files.

A mount option wouldn't be much use to QEMU -- we can't tell
our users how to mount their filesystems, which they're
often doing lots of other things with besides running QEMU.
(Otherwise we could just tell them "don't use ext4", which
would also solve the problem :-)) We need something we can
use at the individual-syscall level.

thanks
-- PMM

^ permalink raw reply

* Re: [PATCH v5 RESEND] signal: add pidfd_send_signal() syscall
From: Christian Brauner @ 2018-12-28 23:37 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, linux-api, luto, arnd, serge, keescook, jannh, oleg,
	cyphar, viro, linux-fsdevel, dancol, timmurray, fweimer, tglx,
	x86, ebiederm
In-Reply-To: <20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org>

On Fri, Dec 28, 2018 at 03:20:12PM -0800, Andrew Morton wrote:
> On Fri, 28 Dec 2018 23:48:53 +0100 Christian Brauner <christian@brauner.io> wrote:
> 
> > The kill() syscall operates on process identifiers (pid). After a process
> > has exited its pid can be reused by another process. If a caller sends a
> > signal to a reused pid it will end up signaling the wrong process. This
> > issue has often surfaced and there has been a push to address this problem [1].
> > 
> > This patch uses file descriptors (fd) from proc/<pid> as stable handles on
> > struct pid. Even if a pid is recycled the handle will not change. The fd
> > can be used to send signals to the process it refers to.
> > Thus, the new syscall pidfd_send_signal() is introduced to solve this
> > problem. Instead of pids it operates on process fds (pidfd).

So most of the questions you ask have been extensively discussed in
prior threads. All of them have links above in the long commit message.

> 
> I can't see a description of what happens when the target process has
> exited.  Is the task_struct pinned by the fd?  Does the entire procfs

A reference to struct pid is kept. struct pid - as far as I understand -
was created exactly for the reason to not require to pin struct
task_struct. This is also noted in the comments to struct pid:
https://elixir.bootlin.com/linux/v4.20-rc1/source/include/linux/pid.h#L16

> directory remain visible?  Just one entry within it?  Does the pid

The same thing that happens when you currently keep an fd to /proc/<pid>
open.

> remain reserved?  Do attempts to signal that fd return errors? 

The pid does not remain reserved. Which leads back to your question
about what happens when you try to signal a process that has exited: you
get ESRCH.

> etcetera.  These behaviors should be described in the changelog and
> manipulate please.

I can add those questions to the changelog too.

> 
> The code in signal.c appears to be compiled in even when
> CONFIG_PROC_FS=y.  Can we add the appropriate ifdefs and an entry in
> sys_ni.c?

Without having looked super close at this from the top of my head I'd
say, yes we can.

> 
> A selftest in toole/testing/selftests would be nice.  And it will be
> helpful to architecture maintainers as they wire this up.

I can extend the sample program in the commit message to a selftest.

> 
> The feature doesn't have its own Kconfig setting.  Perhaps it should?
> It should presumably depend on PROC_FS.

Not sure why we should do this.

> 
> I must say that I dislike the linkage to procfs.  procfs is a
> high-level thing which is manipulated using syscalls.  To turn around
> and make a syscall dependent upon the presence of procfs seems just ...
> wrong.  Is there a cleaner way of obtaining the fd?  Another syscall
> perhaps.

We may do something like this in the future. There's another syscall
lined up at some point translate_pid() which we may extend to also give
back an fd to a process. For now the open() on /proc/<pid> is the
cleanest way of doing this.

> 
> This fd-for-a-process sounds like a handy thing and people may well
> think up other uses for it in the future, probably unrelated to
> signals.  Are the code and the interface designed to permit such future
> applications?  I guess "no" - it presently assumes that anything which

This too has been discussed in prior threads linked in the commit
message. Yes, it does permit of such extension and we have already
publicly discussed future extensions (e.g. wait maybe a new clone
syscall).

> is written to that fd is a signal.  Perhaps there should be a tag at
> the start of the message (which is what it is) which identifies the
> message's type?
> 
> Now I think about it, why a new syscall?  This thing is looking rather
> like an ioctl?

Again, we have had a lengthy discussion about this and by now we all
agree that a dedicated syscall makes more sense than an ioctl() for
security reasons, because processes are a core-kernel concept, and we
intend to extend this api with syscalls in the future (e.g. wait etc.
also discussed in prior threads linked in here). There's also a summary
article on lwn about parts of this (https://lwn.net/Articles/773459/).

Thanks!
Christian

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox