* [PATCH v1 1/4] Make anon_inodes unconditional
From: Christian Brauner @ 2019-03-26 15:55 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190326155513.26964-1-christian@brauner.io>
From: David Howells <dhowells@redhat.com>
Make the anon_inodes facility unconditional so that it can be used by core
VFS code and the pidctl() syscall.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
[christian@brauner.io: adapt commit message to mention pidctl()]
Signed-off-by: Christian Brauner <christian@brauner.io>
---
arch/arm/kvm/Kconfig | 1 -
arch/arm64/kvm/Kconfig | 1 -
arch/mips/kvm/Kconfig | 1 -
arch/powerpc/kvm/Kconfig | 1 -
arch/s390/kvm/Kconfig | 1 -
arch/x86/Kconfig | 1 -
arch/x86/kvm/Kconfig | 1 -
drivers/base/Kconfig | 1 -
drivers/char/tpm/Kconfig | 1 -
drivers/dma-buf/Kconfig | 1 -
drivers/gpio/Kconfig | 1 -
drivers/iio/Kconfig | 1 -
drivers/infiniband/Kconfig | 1 -
drivers/vfio/Kconfig | 1 -
fs/Makefile | 2 +-
fs/notify/fanotify/Kconfig | 1 -
fs/notify/inotify/Kconfig | 1 -
init/Kconfig | 10 ----------
18 files changed, 1 insertion(+), 27 deletions(-)
diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 3f5320f46de2..f591026347a5 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -22,7 +22,6 @@ config KVM
bool "Kernel-based Virtual Machine (KVM) support"
depends on MMU && OF
select PREEMPT_NOTIFIERS
- select ANON_INODES
select ARM_GIC
select ARM_GIC_V3
select ARM_GIC_V3_ITS
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index a3f85624313e..a67121d419a2 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -23,7 +23,6 @@ config KVM
depends on OF
select MMU_NOTIFIER
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
select HAVE_KVM_ARCH_TLB_FLUSH_ALL
select KVM_MMIO
diff --git a/arch/mips/kvm/Kconfig b/arch/mips/kvm/Kconfig
index 4528bc9c3cb1..eac25aef21e0 100644
--- a/arch/mips/kvm/Kconfig
+++ b/arch/mips/kvm/Kconfig
@@ -21,7 +21,6 @@ config KVM
depends on MIPS_FP_SUPPORT
select EXPORT_UASM
select PREEMPT_NOTIFIERS
- select ANON_INODES
select KVM_GENERIC_DIRTYLOG_READ_PROTECT
select HAVE_KVM_VCPU_ASYNC_IOCTL
select KVM_MMIO
diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index bfdde04e4905..f53997a8ca62 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig
@@ -20,7 +20,6 @@ if VIRTUALIZATION
config KVM
bool
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_EVENTFD
select HAVE_KVM_VCPU_ASYNC_IOCTL
select SRCU
diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index 767453faacfc..1816ee48eadd 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig
@@ -21,7 +21,6 @@ config KVM
prompt "Kernel-based Virtual Machine (KVM) support"
depends on HAVE_KVM
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
select HAVE_KVM_VCPU_ASYNC_IOCTL
select HAVE_KVM_EVENTFD
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c1f9b3cf437c..18f2c954464e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -44,7 +44,6 @@ config X86
#
select ACPI_LEGACY_TABLES_LOOKUP if ACPI
select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
- select ANON_INODES
select ARCH_32BIT_OFF_T if X86_32
select ARCH_CLOCKSOURCE_DATA
select ARCH_CLOCKSOURCE_INIT
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 72fa955f4a15..fc042419e670 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -27,7 +27,6 @@ config KVM
depends on X86_LOCAL_APIC
select PREEMPT_NOTIFIERS
select MMU_NOTIFIER
- select ANON_INODES
select HAVE_KVM_IRQCHIP
select HAVE_KVM_IRQFD
select IRQ_BYPASS_MANAGER
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 059700ea3521..03f067da12ee 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -174,7 +174,6 @@ source "drivers/base/regmap/Kconfig"
config DMA_SHARED_BUFFER
bool
default n
- select ANON_INODES
select IRQ_WORK
help
This option enables the framework for buffer-sharing between
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 536e55d3919f..f3e4bc490cf0 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -157,7 +157,6 @@ config TCG_CRB
config TCG_VTPM_PROXY
tristate "VTPM Proxy Interface"
depends on TCG_TPM
- select ANON_INODES
---help---
This driver proxies for an emulated TPM (vTPM) running in userspace.
A device /dev/vtpmx is provided that creates a device pair
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 2e5a0faa2cb1..3fc9c2efc583 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -3,7 +3,6 @@ menu "DMABUF options"
config SYNC_FILE
bool "Explicit Synchronization Framework"
default n
- select ANON_INODES
select DMA_SHARED_BUFFER
---help---
The Sync File Framework adds explicit syncronization via
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3f50526a771f..0f91600c27ae 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -12,7 +12,6 @@ config ARCH_HAVE_CUSTOM_GPIO_H
menuconfig GPIOLIB
bool "GPIO Support"
- select ANON_INODES
help
This enables GPIO support through the generic GPIO library.
You only need to enable this, if you also want to enable
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index d08aeb41cd07..1dec0fecb6ef 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -4,7 +4,6 @@
menuconfig IIO
tristate "Industrial I/O support"
- select ANON_INODES
help
The industrial I/O subsystem provides a unified framework for
drivers for many different types of embedded sensors using a
diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index a1fb840de45d..d318bab25860 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -25,7 +25,6 @@ config INFINIBAND_USER_MAD
config INFINIBAND_USER_ACCESS
tristate "InfiniBand userspace access (verbs and CM)"
- select ANON_INODES
depends on MMU
---help---
Userspace InfiniBand access support. This enables the
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 9de5ed38da83..3798d77d131c 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -22,7 +22,6 @@ menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
depends on IOMMU_API
select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM || ARM64)
- select ANON_INODES
help
VFIO provides a framework for secure userspace device drivers.
See Documentation/vfio.txt for more details.
diff --git a/fs/Makefile b/fs/Makefile
index 427fec226fae..35945f8139e6 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -25,7 +25,7 @@ obj-$(CONFIG_PROC_FS) += proc_namespace.o
obj-y += notify/
obj-$(CONFIG_EPOLL) += eventpoll.o
-obj-$(CONFIG_ANON_INODES) += anon_inodes.o
+obj-y += anon_inodes.o
obj-$(CONFIG_SIGNALFD) += signalfd.o
obj-$(CONFIG_TIMERFD) += timerfd.o
obj-$(CONFIG_EVENTFD) += eventfd.o
diff --git a/fs/notify/fanotify/Kconfig b/fs/notify/fanotify/Kconfig
index 735bfb2e9190..521dc91d2cb5 100644
--- a/fs/notify/fanotify/Kconfig
+++ b/fs/notify/fanotify/Kconfig
@@ -1,7 +1,6 @@
config FANOTIFY
bool "Filesystem wide access notification"
select FSNOTIFY
- select ANON_INODES
select EXPORTFS
default n
---help---
diff --git a/fs/notify/inotify/Kconfig b/fs/notify/inotify/Kconfig
index b981fc0c8379..0161c74e76e2 100644
--- a/fs/notify/inotify/Kconfig
+++ b/fs/notify/inotify/Kconfig
@@ -1,6 +1,5 @@
config INOTIFY_USER
bool "Inotify support for userspace"
- select ANON_INODES
select FSNOTIFY
default y
---help---
diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..be8f97e37a76 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1171,9 +1171,6 @@ config LD_DEAD_CODE_DATA_ELIMINATION
config SYSCTL
bool
-config ANON_INODES
- bool
-
config HAVE_UID16
bool
@@ -1378,14 +1375,12 @@ config HAVE_FUTEX_CMPXCHG
config EPOLL
bool "Enable eventpoll support" if EXPERT
default y
- select ANON_INODES
help
Disabling this option will cause the kernel to be built without
support for epoll family of system calls.
config SIGNALFD
bool "Enable signalfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the signalfd() system call that allows to receive signals
@@ -1395,7 +1390,6 @@ config SIGNALFD
config TIMERFD
bool "Enable timerfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the timerfd() system call that allows to receive timer
@@ -1405,7 +1399,6 @@ config TIMERFD
config EVENTFD
bool "Enable eventfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the eventfd() system call that allows to receive both
@@ -1516,7 +1509,6 @@ config KALLSYMS_BASE_RELATIVE
# syscall, maps, verifier
config BPF_SYSCALL
bool "Enable bpf() system call"
- select ANON_INODES
select BPF
select IRQ_WORK
default n
@@ -1533,7 +1525,6 @@ config BPF_JIT_ALWAYS_ON
config USERFAULTFD
bool "Enable userfaultfd() system call"
- select ANON_INODES
depends on MMU
help
Enable the userfaultfd() system call that allows to intercept and
@@ -1600,7 +1591,6 @@ config PERF_EVENTS
bool "Kernel performance events and counters"
default y if PROFILING
depends on HAVE_PERF_EVENTS
- select ANON_INODES
select IRQ_WORK
select SRCU
help
--
2.21.0
^ permalink raw reply related
* [PATCH v1 0/4] pid: add pidctl()
From: Christian Brauner @ 2019-03-26 15:55 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
This is v1 of this patchset with various minor fixes which are listed in
the individual commits. Notably, pidfds are now O_CLOEXEC by default.
The pidctl() syscalls builds on, extends, and improves translate_pid()
[4] and serves as the natural connection between the pid-based and the
pidfd-based api.
I quote Konstantins original patchset first that has already been acked
and picked up by Eric before and whose functionality is preserved in
this syscall. Multiple people have asked when this patchset will be sent
in for merging (cf. [1], [2]). It has recently been revived by
Nagarathnam Muthusamy from Oracle [3].
The intention of the original translate_pid() syscall was twofold:
1. Provide translation of pids between pid namespaces especially for the
case of deeply nested pid namespaces.
The most obvious use-case is strace which has been waiting for this
feature for a while.
2. Provide implicit pid namespace introspection
Both functionalities are preserved. The latter task has been improved
upon though. In the original version of the pachset passing pid as 1
would allow to deterimine the relationship between the pid namespaces.
This is inherhently racy. If pid 1 inside a pid namespace has died it
would report false negatives. For example, if pid 1 inside of the target
pid namespace already died, it would report that the target pid
namespace cannot be reached from the source pid namespace because it
couldn't find the pid inside of the target pid namespace and thus
falsely report to the user that the two pid namespaces are not related.
This problem is simple to avoid. In the new version we simply walk the
list of ancestors and check whether the namespace are related to each
other. By doing it this way we can reliably report what the relationship
between two pid namespace file descriptors looks like.
Additionally, this syscall has been extended to allow the retrieval of
pidfds independent of procfs. These pidfds can e.g. be used with the new
pidfd_send_signal() syscall we recently merged. The ability to retrieve
pidfds independent of procfs had already been requested in the
pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
[5]. A use-case where a kernel is compiled without procfs but where
pidfds are still useful has been outlined by Andy in [6]. Regular
anon-inode based file descriptors are used that stash a reference to
struct pid in file->private_data and drop that reference on close.
With this pidctl() has three closely related functionalities that
provide a natural connection between the pid-based and the pidfd-based
api. To clarify the semantics and to make it easier for userspace to use
the syscall it has a command argument and three commands clearly
reflecting the functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
PIDCMD_GET_PIDFD).
Embedding the retrieval of pidfds into this syscall has two main
advantages:
- pidctl provides a natural and clean connection between the traditional
pid-based and the newer pidfd-based process API
- allows the retrieval of pidfds for other pid namespaces while
enforcing that
- the caller must have been given access to two file descriptors
referring to target and source pid namespace
- the source pid namespace must be an ancestor of the target pid
namespace
- the pid must be translatable from the source pid namespace into the
target pid namespace
Note that this patchset also includes Al's and David's commit to make anon
inodes unconditional. The original intention is to make it possible to use
anon inodes in core vfs functions. pidctl() has the same requirement so
David suggested I sent this in alongside this patch. Both are informed of
this.
The syscall comes with extensive testing for all functionalities.
/* References */
[1]: https://lore.kernel.org/lkml/37b17950-b130-7933-99a1-4846c61c8555@oracle.com/
[2]: https://lore.kernel.org/lkml/20181109034919.GA21681@altlinux.org/
[3]: https://lore.kernel.org/lkml/37b17950-b130-7933-99a1-4846c61c8555@oracle.com/
[4]: 3eb39f47934f9d5a3027fe00d906a45fe3a15fad
[5]: https://lore.kernel.org/lkml/20190320203910.GA2842@avx2/
[6]: https://lore.kernel.org/lkml/CALCETrXO=V=+qEdLDVPf8eCgLZiB9bOTrUfe0V-U-tUZoeoRDA@mail.gmail.com/
Thanks!
Christian
Christian Brauner (3):
pid: add pidctl()
signal: support pidctl() with pidfd_send_signal()
tests: add pidctl() tests
David Howells (1):
Make anon_inodes unconditional
arch/arm/kvm/Kconfig | 1 -
arch/arm64/kvm/Kconfig | 1 -
arch/mips/kvm/Kconfig | 1 -
arch/powerpc/kvm/Kconfig | 1 -
arch/s390/kvm/Kconfig | 1 -
arch/x86/Kconfig | 1 -
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/x86/kvm/Kconfig | 1 -
drivers/base/Kconfig | 1 -
drivers/char/tpm/Kconfig | 1 -
drivers/dma-buf/Kconfig | 1 -
drivers/gpio/Kconfig | 1 -
drivers/iio/Kconfig | 1 -
drivers/infiniband/Kconfig | 1 -
drivers/vfio/Kconfig | 1 -
fs/Makefile | 2 +-
fs/notify/fanotify/Kconfig | 1 -
fs/notify/inotify/Kconfig | 1 -
include/linux/pid.h | 2 +
include/linux/pid_namespace.h | 8 +
include/linux/syscalls.h | 2 +
include/uapi/linux/wait.h | 14 +
init/Kconfig | 10 -
kernel/pid.c | 161 ++++++
kernel/pid_namespace.c | 25 +
kernel/signal.c | 29 +-
kernel/sys_ni.c | 3 -
tools/testing/selftests/pidfd/Makefile | 2 +-
tools/testing/selftests/pidfd/pidctl_test.c | 537 ++++++++++++++++++++
30 files changed, 765 insertions(+), 48 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidctl_test.c
--
2.21.0
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Joel Fernandes @ 2019-03-26 3:03 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Jann Horn, Christian Brauner, Daniel Colascione,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthusam>
In-Reply-To: <CAGLj2rEODAg+Y5D5Oxy3Gp-Yg6mqeisPnLQJ334TWSdrPW1L7A@mail.gmail.com>
On Mon, Mar 25, 2019 at 09:54:58PM +0000, Jonathan Kowalski wrote:
> On Mon, Mar 25, 2019 at 9:43 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Mon, Mar 25, 2019 at 10:19:26PM +0100, Jann Horn wrote:
> > > On Mon, Mar 25, 2019 at 10:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > But often you don't just want to wait for a single thing to happen;
> > > you want to wait for many things at once, and react as soon as any one
> > > of them happens. This is why the kernel has epoll and all the other
> > > "wait for event on FD" APIs. If waiting for a process isn't possible
> > > with fd-based APIs like epoll, users of this API have to spin up
> > > useless helper threads.
> >
> > This is true. I almost forgot about the polling requirement, sorry. So then a
> > new syscall it is.. About what to wait for, that can be a separate parameter
> > to pidfd_wait then.
> >
>
> This introduces a time window where the process changes state between
> "get pidfd" and "setup waitfd", it would be simpler if the pidfd
> itself supported .poll and on termination the exit status was made
> readable from the file descriptor.
It is much cleaner to add a new pidfd_wait syscall for this as discussed in
the other thread. Adding .poll directly to the pidfd would seem to complicate
the blocking configuration. Do we block until the task is a zombie or until
it is dead? That is not possible to specify easily. Also if we need to
return other types of information from the pidfd, not just exit state, then
it is not clear whether blocking on a pidfd just purely on exit status makes
sense. It is much cleaner to add a new pidfd_wait syscall giving it a pidfd,
specify what to block on (EXIT_DEAD or EXIT_ZOMBIE or both) and then return a
wait fd that can be read/blocked and returning all the needed information on
unblock.
> Further, in the clone4 patchset, there was a mechanism to autoreap
> such a process so that it does not interfere with waiting a process
> does normally. How do you intend to handle this case if anyone except
> the parent is wanting to *wait* on the process (a second process,
> without reaping, so as to not disrupt any waiting in the parent), and
> for things like libraries that finally can manage their own set of
> process when pidfd_clone is added, by excluding this process from the
> process's normal wait handling logic.
The pidfd_wait logic being discussed does not depend on or affects the
autoreap behavior. This wait is not the traditional wait family of calls.
It is for just getting notified about reading all the exit state of a task.
Once I post the code, it will be clear. And I'll CC you..
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH 02/17] fpga: dfl: fme: align PR buffer size per PR datawidth
From: Wu Hao @ 2019-03-26 0:28 UTC (permalink / raw)
To: Alan Tull; +Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Xu Yilun
In-Reply-To: <CANk1AXSUMkfQu8QnN-T0B5qeA4vaGxDTvGur9EGyLbGsBFBeKQ@mail.gmail.com>
On Mon, Mar 25, 2019 at 12:50:40PM -0500, Alan Tull wrote:
> On Sun, Mar 24, 2019 at 10:23 PM Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> Looks good, one question below.
>
> >
> > Current driver checks if input bitstream file size is aligned or
> > not per PR data width (default 32bits). It requires one additional
> > step for end user when they generate the bitstream file, padding
> > extra zeros to bitstream file to align its size per PR data width,
> > but they don't have to as hardware will drop extra padding bytes
> > automatically.
> >
> > In order to simplify the user steps, this patch aligns PR buffer
> > size per PR data width in driver, to allow user to pass unaligned
> > size bitstream files to driver.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > ---
> > drivers/fpga/dfl-fme-pr.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-fme-pr.c b/drivers/fpga/dfl-fme-pr.c
> > index d9ca955..c1fb1fe 100644
> > --- a/drivers/fpga/dfl-fme-pr.c
> > +++ b/drivers/fpga/dfl-fme-pr.c
> > @@ -74,6 +74,7 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> > struct dfl_fme *fme;
> > unsigned long minsz;
> > void *buf = NULL;
> > + size_t length;
> > int ret = 0;
> > u64 v;
> >
> > @@ -85,9 +86,6 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> > if (port_pr.argsz < minsz || port_pr.flags)
> > return -EINVAL;
> >
> > - if (!IS_ALIGNED(port_pr.buffer_size, 4))
> > - return -EINVAL;
> > -
> > /* get fme header region */
> > fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
> > FME_FEATURE_ID_HEADER);
> > @@ -103,7 +101,13 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> > port_pr.buffer_size))
> > return -EFAULT;
> >
> > - buf = vmalloc(port_pr.buffer_size);
> > + /*
> > + * align PR buffer per PR bandwidth, as HW ignores the extra padding
> > + * data automatically.
> > + */
> > + length = ALIGN(port_pr.buffer_size, 4);
> > +
> > + buf = vmalloc(length);
>
> Since it may not be completely filled, would it be worthwhile to alloc
> a zero'ed buff?
>
Hi Alan,
Thanks for the review, acutally per spec, hw doesn't care about the
extra padding data. So for now, i guess we don't need this.
Thanks
Hao
> Alan
>
^ permalink raw reply
* Re: pidfd design
From: Andy Lutomirski @ 2019-03-26 0:24 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Daniel Colascione, Jann Horn, Joel Fernandes,
Suren Baghdasaryan, Steven Rostedt, Sultan Alsawaf, Tim Murray,
Michal Hocko, Greg Kroah-Hartman, Arve Hjønnevåg,
Todd Kjos, Martijn Coenen, Ingo Molnar, Peter Zijlstra, LKML,
open list:ANDROID DRIVERS, kernel-team, Oleg Nesterov
In-Reply-To: <20190326001231.3tnhhlvzg26mof33@brauner.io>
On Mon, Mar 25, 2019 at 5:12 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Mon, Mar 25, 2019 at 05:00:17PM -0700, Andy Lutomirski wrote:
> > On Mon, Mar 25, 2019 at 4:45 PM Christian Brauner <christian@brauner.io> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 04:42:14PM -0700, Andy Lutomirski wrote:
> > > > On Mon, Mar 25, 2019 at 1:23 PM Daniel Colascione <dancol@google.com> wrote:
> > > > >
> > > > > On Mon, Mar 25, 2019 at 1:14 PM Jann Horn <jannh@google.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> > > >
> > > > > > One ioctl on procfs roots to translate pidfds into that procfs,
> > > > > > subject to both the normal lookup permission checks and only working
> > > > > > if the pidfd has a translation into the procfs:
> > > > > >
> > > > > > int proc_root_fd = open("/proc", O_RDONLY);
> > > > > > int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
> > > > > >
> > > > > > And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
> > > > > >
> > > > > > int proc_pgid_fd = open("/proc/self", O_RDONLY);
> > > > > > int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > > > > int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
> > > > > > int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > > > >
> > > >
> > > > This sounds okay to me. Or we could make it so that a procfs
> > > > directory fd also works as a pidfd, but that seems more likely to be
> > > > problematic than just allowing two-way translation like this
> > > >
> > > > > >
> > > > > > And then, as you proposed, the new sys_clone() can just return a
> > > > > > pidfd, and you can convert it into a procfs fd yourself if you want.
> > > > >
> > > > > I think that's the consensus we reached on the other thread. The
> > > > > O_DIRECTORY open on /proc/self/fd/mypidfd seems like it'd work well
> > > > > enough.
> > > >
> > > > I must have missed this particular email.
> > > >
> > > > IMO, if /proc/self/fd/mypidfd allows O_DIRECTORY open to work, then it
> > > > really ought to do function just like /proc/self/fd/mypidfd/. and
> > > > /proc/self/fd/mypidfd/status should work. And these latter two
> > > > options seem nutty.
> > > >
> > > > Also, this O_DIRECTORY thing is missing the entire point of the ioctl
> > > > interface -- it doesn't require procfs access.
> > >
> > > The other option was to encode the pid in the callers pid namespace into
> > > the pidfd's fdinfo so that you can parse it out and open /proc/<pid>.
> > > You'd just need an event on the pidfd to tell you when the process has
> > > died. Jonathan and I just discussed this.
> >
> > From an application developer's POV, the ioctl interface sounds much,
> > much nicer.
>
> Some people are strongly against ioctl()s some don't. I'm not against
> them so both options are fine with me if people can agree.
>
There are certainly non-ioctl equivalents that are functionally
equivalent. For example, there could be a syscall
procfs_open_pidfd(procfs_fd, pid_fd). I personally don't really mind
ioctl() when it's really an operation on an fd.
^ permalink raw reply
* Re: pidfd design
From: Christian Brauner @ 2019-03-26 0:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Peter Zijlstra, Tim Murray, Michal Hocko, Joel Fernandes,
Sultan Alsawaf, Jonathan Kowalski, open list:ANDROID DRIVERS,
Daniel Colascione, Suren Baghdasaryan, Ingo Molnar, kernel-team,
Todd Kjos, Kees Cook, Jann Horn, Steven Rostedt, Oleg Nesterov,
Martijn Coenen, Greg Kroah-Hartman, LKML,
Arve Hjønnevåg, Linux API
In-Reply-To: <CALCETrXfNV5marXSEDGm6F03DNC2a_M+NE94=GxEbH9oJyWkYA@mail.gmail.com>
On Mon, Mar 25, 2019 at 05:00:17PM -0700, Andy Lutomirski wrote:
> On Mon, Mar 25, 2019 at 4:45 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Mon, Mar 25, 2019 at 04:42:14PM -0700, Andy Lutomirski wrote:
> > > On Mon, Mar 25, 2019 at 1:23 PM Daniel Colascione <dancol@google.com> wrote:
> > > >
> > > > On Mon, Mar 25, 2019 at 1:14 PM Jann Horn <jannh@google.com> wrote:
> > > > >
> > > > > On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> > >
> > > > > One ioctl on procfs roots to translate pidfds into that procfs,
> > > > > subject to both the normal lookup permission checks and only working
> > > > > if the pidfd has a translation into the procfs:
> > > > >
> > > > > int proc_root_fd = open("/proc", O_RDONLY);
> > > > > int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
> > > > >
> > > > > And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
> > > > >
> > > > > int proc_pgid_fd = open("/proc/self", O_RDONLY);
> > > > > int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > > > int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
> > > > > int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > > >
> > >
> > > This sounds okay to me. Or we could make it so that a procfs
> > > directory fd also works as a pidfd, but that seems more likely to be
> > > problematic than just allowing two-way translation like this
> > >
> > > > >
> > > > > And then, as you proposed, the new sys_clone() can just return a
> > > > > pidfd, and you can convert it into a procfs fd yourself if you want.
> > > >
> > > > I think that's the consensus we reached on the other thread. The
> > > > O_DIRECTORY open on /proc/self/fd/mypidfd seems like it'd work well
> > > > enough.
> > >
> > > I must have missed this particular email.
> > >
> > > IMO, if /proc/self/fd/mypidfd allows O_DIRECTORY open to work, then it
> > > really ought to do function just like /proc/self/fd/mypidfd/. and
> > > /proc/self/fd/mypidfd/status should work. And these latter two
> > > options seem nutty.
> > >
> > > Also, this O_DIRECTORY thing is missing the entire point of the ioctl
> > > interface -- it doesn't require procfs access.
> >
> > The other option was to encode the pid in the callers pid namespace into
> > the pidfd's fdinfo so that you can parse it out and open /proc/<pid>.
> > You'd just need an event on the pidfd to tell you when the process has
> > died. Jonathan and I just discussed this.
>
> From an application developer's POV, the ioctl interface sounds much,
> much nicer.
Some people are strongly against ioctl()s some don't. I'm not against
them so both options are fine with me if people can agree.
Christian
^ permalink raw reply
* Re: [PATCH 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: Andy Lutomirski @ 2019-03-26 0:10 UTC (permalink / raw)
To: Stephen Hemminger, Linux API
Cc: Matthew Garrett, James Morris, LSM List, LKML, David Howells,
Alexei Starovoitov, Network Development, Chun-Yi Lee,
Daniel Borkmann, Kees Cook, Will Drewry
In-Reply-To: <20190325164221.5d8687bd@shemminger-XPS-13-9360>
On Mon, Mar 25, 2019 at 4:42 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Mon, 25 Mar 2019 15:09:50 -0700
> Matthew Garrett <matthewgarrett@google.com> wrote:
>
> > From: David Howells <dhowells@redhat.com>
> >
> > There are some bpf functions can be used to read kernel memory:
> > bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow
> > private keys in kernel memory (e.g. the hibernation image signing key) to
> > be read by an eBPF program and kernel memory to be altered without
> > restriction.
> >
> > Completely prohibit the use of BPF when the kernel is locked down.
> >
> > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > cc: netdev@vger.kernel.org
> > cc: Chun-Yi Lee <jlee@suse.com>
> > cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Signed-off-by: Matthew Garrett <matthewgarrett@google.com>
>
> Wouldn't this mean that Seccomp won't work in locked down mode?
I wasn't cc'd on this series, nor was linux-api, so it's awkward to review.
A while back, I suggested an approach to actually make this stuff
mergeable: submit a patch series that adds lockdown mode, enables it
by command line option (and maybe sysctl) *only* and has either no
effect or only a token effect. Then we can add actual features to
lockdown mode one at a time and review them separately.
And I'm going to complain loudly unless two things change about this
whole thing:
1. Lockdown mode becomes three states, not a boolean. The states are:
no lockdown, best-effort-to-protect-kernel-integrity, and
best-effort-to-protect-kernel-secrecy-and-integrity. And this BPF
mess illustrates why: most users will really strongly object to
turning off BPF when they actually just want to protect kernel
integrity. And as far as I know, things like Secure Boot policy will
mostly care about integrity, not secrecy, and tracing and such should
work on a normal locked-down kernel. So I think we need this knob.
2. All the proponents of this series, and the documentation, needs to
document that it's best effort. There will always be security bugs,
and there will always be things we miss.
--Andy
^ permalink raw reply
* Re: pidfd design
From: Andy Lutomirski @ 2019-03-26 0:00 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Daniel Colascione, Jann Horn, Joel Fernandes,
Suren Baghdasaryan, Steven Rostedt, Sultan Alsawaf, Tim Murray,
Michal Hocko, Greg Kroah-Hartman, Arve Hjønnevåg,
Todd Kjos, Martijn Coenen, Ingo Molnar, Peter Zijlstra, LKML,
open list:ANDROID DRIVERS, kernel-team, Oleg Nesterov
In-Reply-To: <20190325234547.wo6lyimrp52qie5p@brauner.io>
On Mon, Mar 25, 2019 at 4:45 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Mon, Mar 25, 2019 at 04:42:14PM -0700, Andy Lutomirski wrote:
> > On Mon, Mar 25, 2019 at 1:23 PM Daniel Colascione <dancol@google.com> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 1:14 PM Jann Horn <jannh@google.com> wrote:
> > > >
> > > > On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > > > One ioctl on procfs roots to translate pidfds into that procfs,
> > > > subject to both the normal lookup permission checks and only working
> > > > if the pidfd has a translation into the procfs:
> > > >
> > > > int proc_root_fd = open("/proc", O_RDONLY);
> > > > int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
> > > >
> > > > And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
> > > >
> > > > int proc_pgid_fd = open("/proc/self", O_RDONLY);
> > > > int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > > int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
> > > > int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > >
> >
> > This sounds okay to me. Or we could make it so that a procfs
> > directory fd also works as a pidfd, but that seems more likely to be
> > problematic than just allowing two-way translation like this
> >
> > > >
> > > > And then, as you proposed, the new sys_clone() can just return a
> > > > pidfd, and you can convert it into a procfs fd yourself if you want.
> > >
> > > I think that's the consensus we reached on the other thread. The
> > > O_DIRECTORY open on /proc/self/fd/mypidfd seems like it'd work well
> > > enough.
> >
> > I must have missed this particular email.
> >
> > IMO, if /proc/self/fd/mypidfd allows O_DIRECTORY open to work, then it
> > really ought to do function just like /proc/self/fd/mypidfd/. and
> > /proc/self/fd/mypidfd/status should work. And these latter two
> > options seem nutty.
> >
> > Also, this O_DIRECTORY thing is missing the entire point of the ioctl
> > interface -- it doesn't require procfs access.
>
> The other option was to encode the pid in the callers pid namespace into
> the pidfd's fdinfo so that you can parse it out and open /proc/<pid>.
> You'd just need an event on the pidfd to tell you when the process has
> died. Jonathan and I just discussed this.
>From an application developer's POV, the ioctl interface sounds much,
much nicer.
^ permalink raw reply
* Re: pidfd design
From: Christian Brauner @ 2019-03-25 23:45 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Jann Horn, Joel Fernandes, Suren Baghdasaryan,
Steven Rostedt, Sultan Alsawaf, Tim Murray, Michal Hocko,
Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Ingo Molnar, Peter Zijlstra, LKML,
open list:ANDROID DRIVERS, kernel-team, Oleg Nesterov,
Serge E. Hallyn
In-Reply-To: <CALCETrV-52TXKbAim06Fr7vvFeOvk0SczneeV_Rtqr6-LN7-GQ@mail.gmail.com>
On Mon, Mar 25, 2019 at 04:42:14PM -0700, Andy Lutomirski wrote:
> On Mon, Mar 25, 2019 at 1:23 PM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 1:14 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> > > One ioctl on procfs roots to translate pidfds into that procfs,
> > > subject to both the normal lookup permission checks and only working
> > > if the pidfd has a translation into the procfs:
> > >
> > > int proc_root_fd = open("/proc", O_RDONLY);
> > > int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
> > >
> > > And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
> > >
> > > int proc_pgid_fd = open("/proc/self", O_RDONLY);
> > > int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
> > > int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > >
>
> This sounds okay to me. Or we could make it so that a procfs
> directory fd also works as a pidfd, but that seems more likely to be
> problematic than just allowing two-way translation like this
>
> > >
> > > And then, as you proposed, the new sys_clone() can just return a
> > > pidfd, and you can convert it into a procfs fd yourself if you want.
> >
> > I think that's the consensus we reached on the other thread. The
> > O_DIRECTORY open on /proc/self/fd/mypidfd seems like it'd work well
> > enough.
>
> I must have missed this particular email.
>
> IMO, if /proc/self/fd/mypidfd allows O_DIRECTORY open to work, then it
> really ought to do function just like /proc/self/fd/mypidfd/. and
> /proc/self/fd/mypidfd/status should work. And these latter two
> options seem nutty.
>
> Also, this O_DIRECTORY thing is missing the entire point of the ioctl
> interface -- it doesn't require procfs access.
The other option was to encode the pid in the callers pid namespace into
the pidfd's fdinfo so that you can parse it out and open /proc/<pid>.
You'd just need an event on the pidfd to tell you when the process has
died. Jonathan and I just discussed this.
^ permalink raw reply
* Re: pidfd design
From: Andy Lutomirski @ 2019-03-25 23:42 UTC (permalink / raw)
To: Daniel Colascione
Cc: Jann Horn, Andy Lutomirski, Christian Brauner, Joel Fernandes,
Suren Baghdasaryan, Steven Rostedt, Sultan Alsawaf, Tim Murray,
Michal Hocko, Greg Kroah-Hartman, Arve Hjønnevåg,
Todd Kjos, Martijn Coenen, Ingo Molnar, Peter Zijlstra, LKML,
open list:ANDROID DRIVERS, kernel-team, Oleg Nesterov
In-Reply-To: <CAKOZueuNGn31QYpRa5G-OP_S=G3f9TBQ-w27giABn7UVgPQ8Ng@mail.gmail.com>
On Mon, Mar 25, 2019 at 1:23 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Mon, Mar 25, 2019 at 1:14 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> > One ioctl on procfs roots to translate pidfds into that procfs,
> > subject to both the normal lookup permission checks and only working
> > if the pidfd has a translation into the procfs:
> >
> > int proc_root_fd = open("/proc", O_RDONLY);
> > int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
> >
> > And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
> >
> > int proc_pgid_fd = open("/proc/self", O_RDONLY);
> > int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
> > int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> >
This sounds okay to me. Or we could make it so that a procfs
directory fd also works as a pidfd, but that seems more likely to be
problematic than just allowing two-way translation like this
> >
> > And then, as you proposed, the new sys_clone() can just return a
> > pidfd, and you can convert it into a procfs fd yourself if you want.
>
> I think that's the consensus we reached on the other thread. The
> O_DIRECTORY open on /proc/self/fd/mypidfd seems like it'd work well
> enough.
I must have missed this particular email.
IMO, if /proc/self/fd/mypidfd allows O_DIRECTORY open to work, then it
really ought to do function just like /proc/self/fd/mypidfd/. and
/proc/self/fd/mypidfd/status should work. And these latter two
options seem nutty.
Also, this O_DIRECTORY thing is missing the entire point of the ioctl
interface -- it doesn't require procfs access.
--Andy
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Andy Lutomirski @ 2019-03-25 23:39 UTC (permalink / raw)
To: David Howells
Cc: Daniel Colascione, Christian Brauner, Jann Horn,
Konstantin Khlebnikov, Andy Lutomirski, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, bl0pbl33p, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy
In-Reply-To: <18683.1553532974@warthog.procyon.org.uk>
On Mon, Mar 25, 2019 at 9:56 AM David Howells <dhowells@redhat.com> wrote:
>
> Daniel Colascione <dancol@google.com> wrote:
>
> > System calls are cheap.
>
> Only to a point. x86_64 will have an issue when we hit syscall 512. We're
> currently at 427.
>
I don't consider this to be a problem. I have patches to make this
problem go away forever. I just need to tidy them up a bit and
upstream them.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 23:14 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Joel Fernandes, Jann Horn, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <CAGLj2rGi_+oh=qp=kK0fOq=AEKt5b9oHZXKCWvWPPEHPquOvSA@mail.gmail.com>
On Mon, Mar 25, 2019 at 3:37 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Mon, Mar 25, 2019 at 10:07 PM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 2:55 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 9:43 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > On Mon, Mar 25, 2019 at 10:19:26PM +0100, Jann Horn wrote:
> > > > > On Mon, Mar 25, 2019 at 10:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > >
> > > > > But often you don't just want to wait for a single thing to happen;
> > > > > you want to wait for many things at once, and react as soon as any one
> > > > > of them happens. This is why the kernel has epoll and all the other
> > > > > "wait for event on FD" APIs. If waiting for a process isn't possible
> > > > > with fd-based APIs like epoll, users of this API have to spin up
> > > > > useless helper threads.
> > > >
> > > > This is true. I almost forgot about the polling requirement, sorry. So then a
> > > > new syscall it is.. About what to wait for, that can be a separate parameter
> > > > to pidfd_wait then.
> > > >
> > >
> > > This introduces a time window where the process changes state between
> > > "get pidfd" and "setup waitfd", it would be simpler if the pidfd
> > > itself supported .poll and on termination the exit status was made
> > > readable from the file descriptor.
> >
> > I don't see a race here. Process state moves in one direction, so
> > there's no race. If you want the poll to end when a process exits and
> > the process exits before you poll, the poll should finish immediately.
> > If the process exits before you even create the polling FD, whatever
> > creates the polling FD can fail with ESRCH, which is what usually
> > happens when you try to do anything with a process that's gone. Either
> > way, whatever's trying to set up the poll knows the state of the
> > process and there's no possibility of a missed wakeup.
> >
>
> poll will possibly work and return immediately, but you won't be able
> to read back anything, because for the kernel there will be no waiter
> before you open one. If you make pidfd pollable and readable (for
> parents, as an example), the poll ends immediately but there will
> something to read from the fd.
You can lose exit status this way. That's not a problem, though,
because *without* synchronization between the exiting process and the
waiting process, the waiting process can lose the race no matter what,
and *with* synchronization, there's no change of losing the
information. (It's just like a condition variable.) Today,
synchronization between a parent and child is automatic (because
zombies), but direct children work reliably with pidfd too: the
descriptor that pidfd_clone (or whatever it ends up being called)
returns will *always* be created before the child process and so will
*always* have exit status information available, even with some
non-SIGCHLD option applied.
> > > Further, in the clone4 patchset, there was a mechanism to autoreap
> > > such a process so that it does not interfere with waiting a process
> > > does normally. How do you intend to handle this case if anyone except
> > > the parent is wanting to *wait* on the process (a second process,
> > > without reaping, so as to not disrupt any waiting in the parent), and
> > > for things like libraries that finally can manage their own set of
> > > process when pidfd_clone is added, by excluding this process from the
> > > process's normal wait handling logic.
> >
> > I think that discussion is best had around pidfd_clone. pidfd waiting
> > functionality shouldn't affect wait* in any way nor affect a zombie
> > transition or reaping.
>
> So this is more akin to waitfd and traditional wait* and friends, and
> a single notification fd for possibly multiple children?
I don't know what gave you this idea. I'm talking about a model in
which you wait for one child with one open file description.
> I just wanted
> to be sure one would (should) be able to use a pidfd obtained from
> pidfd_clone without affecting the existing waitfd, and other
> primitives.
I don't think the use of pidfd *waiting*, by itself, should affect the
current fork/wait/zombie model of process exit delivery. That is, by
default, a process should transition to the zombie state, send SIGCHLD
to its parent, and then get reaped at exactly the same times it does
today, with exactly the same semantics we have today. Pidfd waiting
should be an "add on".
> The same application's components should be able to host
> their own set of children using such an API (think libraries) and
> without affecting the process.
Agreed. To be clear, you're talking about something slightly different
from pure pidfd waiting. There should be some way to *create* a
process in such a way that it's not visible to wait(-1), doesn't
generate SIGCHLD, and so on. It would be great for libraries to create
and manage worker processes transparently, even in the presence of
some other thread sitting on wait(-1). I don't think the specific form
of this option affects how pidfd waiting works though. Do you?
> > > Moreover, should anyone except the parent process be allowed to open a
> > > readable pidfd (or waitfd), without additional capabilities?
> >
> > That's a separate discussion. See
> > https://lore.kernel.org/lkml/CAKOZueussVwABQaC+O9fW+MZayccvttKQZfWg0hh-cZ+1ykXig@mail.gmail.com/, where we talked about permissions extensively. Anyone can hammer on
> > /proc today or hammer on kill(pid, 0) to learn about a process
> > exiting, so anyone should be able to wait for a process to die --- it
> > just automates what anyone can do manually. What's left is the
> > question of who should be able to learn a process's exit code. As I
> > mentioned in the linked email, process exit codes are public on
> > FreeBSD, and the simplest option is to make them public in Linux too.
>
> People might be using them in ways that convey information between the
> parent and child something else shouldn't be able to know.
In theory, yes, there's an information leak. In practice, I don't
think that it actually matters. Can you think of a concrete security
hole that would be opened by allowing anyone to inspect process exit
status? Having thought of it, have you filed for a FreeBSD CVE? :-)
That is, I'd expect that any security hole *in practice* created by
allowing public inspection of error states would have already appeared
on systems that allow public notification of error states (since most
programs are portable), so I don't think there's a problem here in
practice.
I'd much rather make exit codes knowable publicly than not: declaring
that public exit status is public sidesteps a lot of weird setuid
issues --- see the thread I linked --- and makes things like pkill(1)
more useful --- because they could report whether a process died due
to the signal pkill sent or due to some other reason.
> They might
> be relying on this assumption that it is private. I don't think
> opening it up without requiring *some* privileges is safe.
/proc/pid/stat already reports exit codes to non-parents when 1) the
process whose stat is being read has exited but has not yet been
reaped, and 2) the reader would pass a ptrace(2) exit status on the
zombie (or would have, before the zombie became a zombie). A similar
model might work as a compromise option for us here: a policy of "as
broad as /proc/pid/stat" discloses no new information except in very
minor edge case of a child of a process with SIGCHLD set to SIG_IGN.
I would still prefer completely public status information for
simplicity and flexibility reasons though.
> Also, taking this further, if the structure being returned from a read
> isn't just the exit code but something more elaborate (you have
> mentioned siginfo previously), or even statistics like getrusage, I
> would be concerned if anyone except those with CAP_SYS_PTRACE or so
> should be able to obtain such a readable pidfd/waitfd.
It seems reasonable to provide different waiters with different levels
of access.
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Scott Wood @ 2019-03-25 22:58 UTC (permalink / raw)
To: Wu Hao, atull, mdf, linux-fpga, linux-kernel
Cc: linux-api, Ananda Ravuri, Xu Yilun
In-Reply-To: <127a9356a7bf597d35dd361f2b16bf80460f0370.camel@redhat.com>
On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > In early partial reconfiguration private feature, it only
> > supports 32bit data width when writing data to hardware for
> > PR. 512bit data width PR support is an important optimization
> > for some specific solutions (e.g. XEON with FPGA integrated),
> > it allows driver to use AVX512 instruction to improve the
> > performance of partial reconfiguration. e.g. programming one
> > 100MB bitstream image via this 512bit data width PR hardware
> > only takes ~300ms, but 32bit revision requires ~3s per test
> > result.
> >
> > Please note now this optimization is only done on revision 2
> > of this PR private feature which is only used in integrated
> > solution that AVX512 is always supported.
> >
> > Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > ---
> > drivers/fpga/dfl-fme-main.c | 3 ++
> > drivers/fpga/dfl-fme-mgr.c | 75 +++++++++++++++++++++++++++++++++++++-
> > --
> > -----
> > drivers/fpga/dfl-fme-pr.c | 45 ++++++++++++++++-----------
> > drivers/fpga/dfl-fme.h | 2 ++
> > drivers/fpga/dfl.h | 5 +++
> > 5 files changed, 99 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> > index 086ad24..076d74f 100644
> > --- a/drivers/fpga/dfl-fme-main.c
> > +++ b/drivers/fpga/dfl-fme-main.c
> > @@ -21,6 +21,8 @@
> > #include "dfl.h"
> > #include "dfl-fme.h"
> >
> > +#define DRV_VERSION "0.8"
>
> What is this going to be used for? Under what circumstances will the
> driver version be bumped? What does it have to do with 512-bit writes?
>
> > +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> > +
> > +#include <asm/fpu/api.h>
> > +
> > +static inline void copy512(void *src, void __iomem *dst)
> > +{
> > + kernel_fpu_begin();
> > +
> > + asm volatile("vmovdqu64 (%0), %%zmm0;"
> > + "vmovntdq %%zmm0, (%1);"
> > + :
> > + : "r"(src), "r"(dst));
> > +
> > + kernel_fpu_end();
> > +}
>
> Shouldn't there be some sort of check that AVX512 is actually supported
> on the running system?
>
> Also, src should be const, and the asm statement should have a memory
> clobber.
>
> > +#else
> > +static inline void copy512(void *src, void __iomem *dst)
> > +{
> > + WARN_ON_ONCE(1);
> > +}
> > +#endif
>
> Likewise, this will be called if a revision 2 device is used on non-x86
> (or on x86 with an old binutils). The driver should fall back to 32-bit
> in such cases.
Sorry, I missed the comment about revision 2 only being on integrated
devices -- but will that always be the case? Seems worthwhile to check for
AVX512 support anyway. And there's still the possibility of being built
with an old binutils such that CONFIG_AS_AVX512 is not set, or running on a
kernel where avx512 was disabled via a boot option.
What about future revisions >= 2? Currently the driver will treat them as
if they were revision < 2. Is that intended?
-Scott
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Scott Wood @ 2019-03-25 22:53 UTC (permalink / raw)
To: Wu Hao, atull, mdf, linux-fpga, linux-kernel
Cc: linux-api, Ananda Ravuri, Xu Yilun
In-Reply-To: <1553483264-5379-4-git-send-email-hao.wu@intel.com>
On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> In early partial reconfiguration private feature, it only
> supports 32bit data width when writing data to hardware for
> PR. 512bit data width PR support is an important optimization
> for some specific solutions (e.g. XEON with FPGA integrated),
> it allows driver to use AVX512 instruction to improve the
> performance of partial reconfiguration. e.g. programming one
> 100MB bitstream image via this 512bit data width PR hardware
> only takes ~300ms, but 32bit revision requires ~3s per test
> result.
>
> Please note now this optimization is only done on revision 2
> of this PR private feature which is only used in integrated
> solution that AVX512 is always supported.
>
> Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> ---
> drivers/fpga/dfl-fme-main.c | 3 ++
> drivers/fpga/dfl-fme-mgr.c | 75 +++++++++++++++++++++++++++++++++++++---
> -----
> drivers/fpga/dfl-fme-pr.c | 45 ++++++++++++++++-----------
> drivers/fpga/dfl-fme.h | 2 ++
> drivers/fpga/dfl.h | 5 +++
> 5 files changed, 99 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> index 086ad24..076d74f 100644
> --- a/drivers/fpga/dfl-fme-main.c
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -21,6 +21,8 @@
> #include "dfl.h"
> #include "dfl-fme.h"
>
> +#define DRV_VERSION "0.8"
What is this going to be used for? Under what circumstances will the
driver version be bumped? What does it have to do with 512-bit writes?
> +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> +
> +#include <asm/fpu/api.h>
> +
> +static inline void copy512(void *src, void __iomem *dst)
> +{
> + kernel_fpu_begin();
> +
> + asm volatile("vmovdqu64 (%0), %%zmm0;"
> + "vmovntdq %%zmm0, (%1);"
> + :
> + : "r"(src), "r"(dst));
> +
> + kernel_fpu_end();
> +}
Shouldn't there be some sort of check that AVX512 is actually supported
on the running system?
Also, src should be const, and the asm statement should have a memory
clobber.
> +#else
> +static inline void copy512(void *src, void __iomem *dst)
> +{
> + WARN_ON_ONCE(1);
> +}
> +#endif
Likewise, this will be called if a revision 2 device is used on non-x86
(or on x86 with an old binutils). The driver should fall back to 32-bit
in such cases.
> @@ -200,21 +228,32 @@ static int fme_mgr_write(struct fpga_manager *mgr,
> pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT,
> pr_status);
> }
>
> - if (count < 4) {
> + if (count < priv->pr_datawidth) {
> dev_err(dev, "Invalid PR bitstream size\n");
> return -EINVAL;
Shouldn't this have become a WARN_ON in patch 2 given that the kernel
already pads the buffer?
> }
>
> - pr_data = 0;
> - pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
> - *(((u32 *)buf) + i));
> - writeq(pr_data, fme_pr + FME_PR_DATA);
> - count -= 4;
> + switch (priv->pr_datawidth) {
> + case 4:
> + pr_data = 0;
> + pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
> + *((u32 *)buf));
I know it's not new, but why not just "pr_data = FIELD..."? Const should
also be preserved in the cast, and you can drop one set of parentheses.
> + writeq(pr_data, fme_pr + FME_PR_DATA);
> + break;
> + case 64:
> + copy512((void *)buf, fme_pr + FME_PR_512_DATA);
> + break;
Unnecessary cast.
> + default:
> + ret = -EFAULT;
> + goto done;
How is it EFAULT? Any other value for pr_datawidth should be WARN_ON
since it's set by kernel code.
> @@ -159,13 +161,10 @@ static int fme_pr(struct platform_device *pdev,
> unsigned long arg)
> fpga_bridges_put(®ion->bridge_list);
>
> put_device(®ion->dev);
> -unlock_exit:
> - mutex_unlock(&pdata->lock);
> free_exit:
> vfree(buf);
> - if (copy_to_user((void __user *)arg, &port_pr, minsz))
> - return -EFAULT;
> -
Why is the copy_to_user being removed?
-Scott
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jonathan Kowalski @ 2019-03-25 22:37 UTC (permalink / raw)
To: Daniel Colascione
Cc: Joel Fernandes, Jann Horn, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <CAKOZuetkCKKs7sWgDNL3R5GMyXyrjLf0VZsHvh92VmYkdQcBCA@mail.gmail.com>
On Mon, Mar 25, 2019 at 10:07 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Mon, Mar 25, 2019 at 2:55 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> >
> > On Mon, Mar 25, 2019 at 9:43 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > On Mon, Mar 25, 2019 at 10:19:26PM +0100, Jann Horn wrote:
> > > > On Mon, Mar 25, 2019 at 10:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > But often you don't just want to wait for a single thing to happen;
> > > > you want to wait for many things at once, and react as soon as any one
> > > > of them happens. This is why the kernel has epoll and all the other
> > > > "wait for event on FD" APIs. If waiting for a process isn't possible
> > > > with fd-based APIs like epoll, users of this API have to spin up
> > > > useless helper threads.
> > >
> > > This is true. I almost forgot about the polling requirement, sorry. So then a
> > > new syscall it is.. About what to wait for, that can be a separate parameter
> > > to pidfd_wait then.
> > >
> >
> > This introduces a time window where the process changes state between
> > "get pidfd" and "setup waitfd", it would be simpler if the pidfd
> > itself supported .poll and on termination the exit status was made
> > readable from the file descriptor.
>
> I don't see a race here. Process state moves in one direction, so
> there's no race. If you want the poll to end when a process exits and
> the process exits before you poll, the poll should finish immediately.
> If the process exits before you even create the polling FD, whatever
> creates the polling FD can fail with ESRCH, which is what usually
> happens when you try to do anything with a process that's gone. Either
> way, whatever's trying to set up the poll knows the state of the
> process and there's no possibility of a missed wakeup.
>
poll will possibly work and return immediately, but you won't be able
to read back anything, because for the kernel there will be no waiter
before you open one. If you make pidfd pollable and readable (for
parents, as an example), the poll ends immediately but there will
something to read from the fd.
> > Further, in the clone4 patchset, there was a mechanism to autoreap
> > such a process so that it does not interfere with waiting a process
> > does normally. How do you intend to handle this case if anyone except
> > the parent is wanting to *wait* on the process (a second process,
> > without reaping, so as to not disrupt any waiting in the parent), and
> > for things like libraries that finally can manage their own set of
> > process when pidfd_clone is added, by excluding this process from the
> > process's normal wait handling logic.
>
> I think that discussion is best had around pidfd_clone. pidfd waiting
> functionality shouldn't affect wait* in any way nor affect a zombie
> transition or reaping.
So this is more akin to waitfd and traditional wait* and friends, and
a single notification fd for possibly multiple children? I just wanted
to be sure one would (should) be able to use a pidfd obtained from
pidfd_clone without affecting the existing waitfd, and other
primitives. The same application's components should be able to host
their own set of children using such an API (think libraries) and
without affecting the process.
>
> > Moreover, should anyone except the parent process be allowed to open a
> > readable pidfd (or waitfd), without additional capabilities?
>
> That's a separate discussion. See
> https://lore.kernel.org/lkml/CAKOZueussVwABQaC+O9fW+MZayccvttKQZfWg0hh-cZ+1ykXig@mail.gmail.com/, where we talked about permissions extensively. Anyone can hammer on
> /proc today or hammer on kill(pid, 0) to learn about a process
> exiting, so anyone should be able to wait for a process to die --- it
> just automates what anyone can do manually. What's left is the
> question of who should be able to learn a process's exit code. As I
> mentioned in the linked email, process exit codes are public on
> FreeBSD, and the simplest option is to make them public in Linux too.
People might be using them in ways that convey information between the
parent and child something else shouldn't be able to know. They might
be relying on this assumption that it is private. I don't think
opening it up without requiring *some* privileges is safe.
Also, taking this further, if the structure being returned from a read
isn't just the exit code but something more elaborate (you have
mentioned siginfo previously), or even statistics like getrusage, I
would be concerned if anyone except those with CAP_SYS_PTRACE or so
should be able to obtain such a readable pidfd/waitfd.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 22:07 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Joel Fernandes, Jann Horn, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <CAGLj2rEODAg+Y5D5Oxy3Gp-Yg6mqeisPnLQJ334TWSdrPW1L7A@mail.gmail.com>
On Mon, Mar 25, 2019 at 2:55 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Mon, Mar 25, 2019 at 9:43 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Mon, Mar 25, 2019 at 10:19:26PM +0100, Jann Horn wrote:
> > > On Mon, Mar 25, 2019 at 10:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > But often you don't just want to wait for a single thing to happen;
> > > you want to wait for many things at once, and react as soon as any one
> > > of them happens. This is why the kernel has epoll and all the other
> > > "wait for event on FD" APIs. If waiting for a process isn't possible
> > > with fd-based APIs like epoll, users of this API have to spin up
> > > useless helper threads.
> >
> > This is true. I almost forgot about the polling requirement, sorry. So then a
> > new syscall it is.. About what to wait for, that can be a separate parameter
> > to pidfd_wait then.
> >
>
> This introduces a time window where the process changes state between
> "get pidfd" and "setup waitfd", it would be simpler if the pidfd
> itself supported .poll and on termination the exit status was made
> readable from the file descriptor.
I don't see a race here. Process state moves in one direction, so
there's no race. If you want the poll to end when a process exits and
the process exits before you poll, the poll should finish immediately.
If the process exits before you even create the polling FD, whatever
creates the polling FD can fail with ESRCH, which is what usually
happens when you try to do anything with a process that's gone. Either
way, whatever's trying to set up the poll knows the state of the
process and there's no possibility of a missed wakeup.
> Further, in the clone4 patchset, there was a mechanism to autoreap
> such a process so that it does not interfere with waiting a process
> does normally. How do you intend to handle this case if anyone except
> the parent is wanting to *wait* on the process (a second process,
> without reaping, so as to not disrupt any waiting in the parent), and
> for things like libraries that finally can manage their own set of
> process when pidfd_clone is added, by excluding this process from the
> process's normal wait handling logic.
I think that discussion is best had around pidfd_clone. pidfd waiting
functionality shouldn't affect wait* in any way nor affect a zombie
transition or reaping.
> Moreover, should anyone except the parent process be allowed to open a
> readable pidfd (or waitfd), without additional capabilities?
That's a separate discussion. See
https://lore.kernel.org/lkml/CAKOZueussVwABQaC+O9fW+MZayccvttKQZfWg0hh-cZ+1ykXig@mail.gmail.com/,
where we talked about permissions extensively. Anyone can hammer on
/proc today or hammer on kill(pid, 0) to learn about a process
exiting, so anyone should be able to wait for a process to die --- it
just automates what anyone can do manually. What's left is the
question of who should be able to learn a process's exit code. As I
mentioned in the linked email, process exit codes are public on
FreeBSD, and the simplest option is to make them public in Linux too.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jonathan Kowalski @ 2019-03-25 21:54 UTC (permalink / raw)
To: Joel Fernandes
Cc: Jann Horn, Christian Brauner, Daniel Colascione,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathnam.muthusam>
In-Reply-To: <20190325214338.GA16969@google.com>
On Mon, Mar 25, 2019 at 9:43 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Mon, Mar 25, 2019 at 10:19:26PM +0100, Jann Horn wrote:
> > On Mon, Mar 25, 2019 at 10:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > But often you don't just want to wait for a single thing to happen;
> > you want to wait for many things at once, and react as soon as any one
> > of them happens. This is why the kernel has epoll and all the other
> > "wait for event on FD" APIs. If waiting for a process isn't possible
> > with fd-based APIs like epoll, users of this API have to spin up
> > useless helper threads.
>
> This is true. I almost forgot about the polling requirement, sorry. So then a
> new syscall it is.. About what to wait for, that can be a separate parameter
> to pidfd_wait then.
>
This introduces a time window where the process changes state between
"get pidfd" and "setup waitfd", it would be simpler if the pidfd
itself supported .poll and on termination the exit status was made
readable from the file descriptor.
Further, in the clone4 patchset, there was a mechanism to autoreap
such a process so that it does not interfere with waiting a process
does normally. How do you intend to handle this case if anyone except
the parent is wanting to *wait* on the process (a second process,
without reaping, so as to not disrupt any waiting in the parent), and
for things like libraries that finally can manage their own set of
process when pidfd_clone is added, by excluding this process from the
process's normal wait handling logic.
Moreover, should anyone except the parent process be allowed to open a
readable pidfd (or waitfd), without additional capabilities?
> Thanks.
>
> - Joel
>
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Joel Fernandes @ 2019-03-25 21:43 UTC (permalink / raw)
To: Jann Horn
Cc: Christian Brauner, Daniel Colascione, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagarathn>
In-Reply-To: <CAG48ez0K9xcSf28rXOy3k0dEL3b_4b-DfcdCO=JO7vVZeJoBcA@mail.gmail.com>
On Mon, Mar 25, 2019 at 10:19:26PM +0100, Jann Horn wrote:
> On Mon, Mar 25, 2019 at 10:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Mon, Mar 25, 2019 at 09:15:45PM +0100, Christian Brauner wrote:
> > > On Mon, Mar 25, 2019 at 01:36:14PM -0400, Joel Fernandes wrote:
> > > > On Mon, Mar 25, 2019 at 09:48:43AM -0700, Daniel Colascione wrote:
> > > > > On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.io> wrote:
> > > > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > > > I quote Konstantins original patchset first that has already been acked and
> > > > > > picked up by Eric before and whose functionality is preserved in this
> > > > > > syscall. Multiple people have asked when this patchset will be sent in
> > > > > > for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> > > > > > Muthusamy from Oracle [3].
> > > > > >
> > > > > > The intention of the original translate_pid() syscall was twofold:
> > > > > > 1. Provide translation of pids between pid namespaces
> > > > > > 2. Provide implicit pid namespace introspection
> > > > > >
> > > > > > Both functionalities are preserved. The latter task has been improved
> > > > > > upon though. In the original version of the pachset passing pid as 1
> > > > > > would allow to deterimine the relationship between the pid namespaces.
> > > > > > This is inherhently racy. If pid 1 inside a pid namespace has died it
> > > > > > would report false negatives. For example, if pid 1 inside of the target
> > > > > > pid namespace already died, it would report that the target pid
> > > > > > namespace cannot be reached from the source pid namespace because it
> > > > > > couldn't find the pid inside of the target pid namespace and thus
> > > > > > falsely report to the user that the two pid namespaces are not related.
> > > > > > This problem is simple to avoid. In the new version we simply walk the
> > > > > > list of ancestors and check whether the namespace are related to each
> > > > > > other. By doing it this way we can reliably report what the relationship
> > > > > > between two pid namespace file descriptors looks like.
> > > > > >
> > > > > > Additionally, this syscall has been extended to allow the retrieval of
> > > > > > pidfds independent of procfs. These pidfds can e.g. be used with the new
> > > > > > pidfd_send_signal() syscall we recently merged. The ability to retrieve
> > > > > > pidfds independent of procfs had already been requested in the
> > > > > > pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> > > > > > [5]. A use-case where a kernel is compiled without procfs but where
> > > > > > pidfds are still useful has been outlined by Andy in [6]. Regular
> > > > > > anon-inode based file descriptors are used that stash a reference to
> > > > > > struct pid in file->private_data and drop that reference on close.
> > > > > >
> > > > > > With this translate_pid() has three closely related but still distinct
> > > > > > functionalities. To clarify the semantics and to make it easier for
> > > > > > userspace to use the syscall it has:
> > > > > > - gained a command argument and three commands clearly reflecting the
> > > > > > distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> > > > > > PIDCMD_GET_PIDFD).
> > > > > > - been renamed to pidctl()
> > > > >
> > > > [snip]
> > > > > Also, I'm still confused about how metadata access is supposed to work
> > > > > for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> > > > > You snipped out a portion of a previous email in which I asked about
> > > > > your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> > > > > place, we have two different kinds of file descriptors for processes,
> > > > > one derived from procfs and one that's independent. The former works
> > > > > with openat(2). The latter does not. To be very specific; if I'm
> > > > > writing a function that accepts a pidfd and I get a pidfd that comes
> > > > > from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> > > > > smaps or oom_score_adj or statm for the named process in a race-free
> > > > > manner?
> > > >
> > > > This is true, that such usecase will not be supportable. But the advantage
> > > > on the other hand, is that suchs "pidfd" can be made pollable or readable in
> > > > the future. Potentially allowing us to return exit status without a new
> > > > syscall (?). And we can add IOCTLs to the pidfd descriptor which we cannot do
> > > > with proc.
> > > >
> > > > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > > > can be translated into a "pidfd" using another syscall or even a node, like
> > > > /proc/pid/handle or something. I think this is what Christian suggested in
> > > > the previous threads.
> > >
> > > Andy - and Jann who I just talked to - have proposed solutions for this.
> > > Jann's idea is similar to what you suggested, Joel. You could e.g. do an
> > > ioctl() handler for /proc that would give you a dirfd back for a given
> > > pidfd. The advantage is that pidfd_clone() can then give back pidfds
> > > without having to care in what procfs the process is supposed to live.
> > > That makes things a lot easier. But pidfds for the general case should
> > > be anon inodes. It's clean, it's simple and it is way more secure.
> >
> > That makes sense to me, it is clean and I agree let us do that.
> >
> > Also for the "blocking on pid exit status" usecase, instead of adding a new
> > syscall like pidfd_wait, lets just make that a new IOCTL to the
> > file_operations of the anon_inode pidfd file. This will lets us specify
> > exactly what to wait on (wait on death or wait on zombie) and lets us avoid
> > having a new syscall and create new fd just for waiting. Let me know if you
> > disagree, but otherwise I am thinking of modifying my patches that way and
> > avoid adding a new syscall.
>
> But often you don't just want to wait for a single thing to happen;
> you want to wait for many things at once, and react as soon as any one
> of them happens. This is why the kernel has epoll and all the other
> "wait for event on FD" APIs. If waiting for a process isn't possible
> with fd-based APIs like epoll, users of this API have to spin up
> useless helper threads.
This is true. I almost forgot about the polling requirement, sorry. So then a
new syscall it is.. About what to wait for, that can be a separate parameter
to pidfd_wait then.
Thanks.
- Joel
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jann Horn @ 2019-03-25 21:19 UTC (permalink / raw)
To: Joel Fernandes
Cc: Christian Brauner, Daniel Colascione, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagarathn>
In-Reply-To: <20190325211132.GA6494@google.com>
On Mon, Mar 25, 2019 at 10:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> On Mon, Mar 25, 2019 at 09:15:45PM +0100, Christian Brauner wrote:
> > On Mon, Mar 25, 2019 at 01:36:14PM -0400, Joel Fernandes wrote:
> > > On Mon, Mar 25, 2019 at 09:48:43AM -0700, Daniel Colascione wrote:
> > > > On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.io> wrote:
> > > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > > I quote Konstantins original patchset first that has already been acked and
> > > > > picked up by Eric before and whose functionality is preserved in this
> > > > > syscall. Multiple people have asked when this patchset will be sent in
> > > > > for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> > > > > Muthusamy from Oracle [3].
> > > > >
> > > > > The intention of the original translate_pid() syscall was twofold:
> > > > > 1. Provide translation of pids between pid namespaces
> > > > > 2. Provide implicit pid namespace introspection
> > > > >
> > > > > Both functionalities are preserved. The latter task has been improved
> > > > > upon though. In the original version of the pachset passing pid as 1
> > > > > would allow to deterimine the relationship between the pid namespaces.
> > > > > This is inherhently racy. If pid 1 inside a pid namespace has died it
> > > > > would report false negatives. For example, if pid 1 inside of the target
> > > > > pid namespace already died, it would report that the target pid
> > > > > namespace cannot be reached from the source pid namespace because it
> > > > > couldn't find the pid inside of the target pid namespace and thus
> > > > > falsely report to the user that the two pid namespaces are not related.
> > > > > This problem is simple to avoid. In the new version we simply walk the
> > > > > list of ancestors and check whether the namespace are related to each
> > > > > other. By doing it this way we can reliably report what the relationship
> > > > > between two pid namespace file descriptors looks like.
> > > > >
> > > > > Additionally, this syscall has been extended to allow the retrieval of
> > > > > pidfds independent of procfs. These pidfds can e.g. be used with the new
> > > > > pidfd_send_signal() syscall we recently merged. The ability to retrieve
> > > > > pidfds independent of procfs had already been requested in the
> > > > > pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> > > > > [5]. A use-case where a kernel is compiled without procfs but where
> > > > > pidfds are still useful has been outlined by Andy in [6]. Regular
> > > > > anon-inode based file descriptors are used that stash a reference to
> > > > > struct pid in file->private_data and drop that reference on close.
> > > > >
> > > > > With this translate_pid() has three closely related but still distinct
> > > > > functionalities. To clarify the semantics and to make it easier for
> > > > > userspace to use the syscall it has:
> > > > > - gained a command argument and three commands clearly reflecting the
> > > > > distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> > > > > PIDCMD_GET_PIDFD).
> > > > > - been renamed to pidctl()
> > > >
> > > [snip]
> > > > Also, I'm still confused about how metadata access is supposed to work
> > > > for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> > > > You snipped out a portion of a previous email in which I asked about
> > > > your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> > > > place, we have two different kinds of file descriptors for processes,
> > > > one derived from procfs and one that's independent. The former works
> > > > with openat(2). The latter does not. To be very specific; if I'm
> > > > writing a function that accepts a pidfd and I get a pidfd that comes
> > > > from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> > > > smaps or oom_score_adj or statm for the named process in a race-free
> > > > manner?
> > >
> > > This is true, that such usecase will not be supportable. But the advantage
> > > on the other hand, is that suchs "pidfd" can be made pollable or readable in
> > > the future. Potentially allowing us to return exit status without a new
> > > syscall (?). And we can add IOCTLs to the pidfd descriptor which we cannot do
> > > with proc.
> > >
> > > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > > can be translated into a "pidfd" using another syscall or even a node, like
> > > /proc/pid/handle or something. I think this is what Christian suggested in
> > > the previous threads.
> >
> > Andy - and Jann who I just talked to - have proposed solutions for this.
> > Jann's idea is similar to what you suggested, Joel. You could e.g. do an
> > ioctl() handler for /proc that would give you a dirfd back for a given
> > pidfd. The advantage is that pidfd_clone() can then give back pidfds
> > without having to care in what procfs the process is supposed to live.
> > That makes things a lot easier. But pidfds for the general case should
> > be anon inodes. It's clean, it's simple and it is way more secure.
>
> That makes sense to me, it is clean and I agree let us do that.
>
> Also for the "blocking on pid exit status" usecase, instead of adding a new
> syscall like pidfd_wait, lets just make that a new IOCTL to the
> file_operations of the anon_inode pidfd file. This will lets us specify
> exactly what to wait on (wait on death or wait on zombie) and lets us avoid
> having a new syscall and create new fd just for waiting. Let me know if you
> disagree, but otherwise I am thinking of modifying my patches that way and
> avoid adding a new syscall.
But often you don't just want to wait for a single thing to happen;
you want to wait for many things at once, and react as soon as any one
of them happens. This is why the kernel has epoll and all the other
"wait for event on FD" APIs. If waiting for a process isn't possible
with fd-based APIs like epoll, users of this API have to spin up
useless helper threads.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Daniel Colascione @ 2019-03-25 21:17 UTC (permalink / raw)
To: Joel Fernandes
Cc: Christian Brauner, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagarathnam.muthus>
In-Reply-To: <20190325211132.GA6494@google.com>
On Mon, Mar 25, 2019 at 2:11 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Mon, Mar 25, 2019 at 09:15:45PM +0100, Christian Brauner wrote:
> > On Mon, Mar 25, 2019 at 01:36:14PM -0400, Joel Fernandes wrote:
> > > On Mon, Mar 25, 2019 at 09:48:43AM -0700, Daniel Colascione wrote:
> > > > On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.io> wrote:
> > > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > > I quote Konstantins original patchset first that has already been acked and
> > > > > picked up by Eric before and whose functionality is preserved in this
> > > > > syscall. Multiple people have asked when this patchset will be sent in
> > > > > for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> > > > > Muthusamy from Oracle [3].
> > > > >
> > > > > The intention of the original translate_pid() syscall was twofold:
> > > > > 1. Provide translation of pids between pid namespaces
> > > > > 2. Provide implicit pid namespace introspection
> > > > >
> > > > > Both functionalities are preserved. The latter task has been improved
> > > > > upon though. In the original version of the pachset passing pid as 1
> > > > > would allow to deterimine the relationship between the pid namespaces.
> > > > > This is inherhently racy. If pid 1 inside a pid namespace has died it
> > > > > would report false negatives. For example, if pid 1 inside of the target
> > > > > pid namespace already died, it would report that the target pid
> > > > > namespace cannot be reached from the source pid namespace because it
> > > > > couldn't find the pid inside of the target pid namespace and thus
> > > > > falsely report to the user that the two pid namespaces are not related.
> > > > > This problem is simple to avoid. In the new version we simply walk the
> > > > > list of ancestors and check whether the namespace are related to each
> > > > > other. By doing it this way we can reliably report what the relationship
> > > > > between two pid namespace file descriptors looks like.
> > > > >
> > > > > Additionally, this syscall has been extended to allow the retrieval of
> > > > > pidfds independent of procfs. These pidfds can e.g. be used with the new
> > > > > pidfd_send_signal() syscall we recently merged. The ability to retrieve
> > > > > pidfds independent of procfs had already been requested in the
> > > > > pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> > > > > [5]. A use-case where a kernel is compiled without procfs but where
> > > > > pidfds are still useful has been outlined by Andy in [6]. Regular
> > > > > anon-inode based file descriptors are used that stash a reference to
> > > > > struct pid in file->private_data and drop that reference on close.
> > > > >
> > > > > With this translate_pid() has three closely related but still distinct
> > > > > functionalities. To clarify the semantics and to make it easier for
> > > > > userspace to use the syscall it has:
> > > > > - gained a command argument and three commands clearly reflecting the
> > > > > distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> > > > > PIDCMD_GET_PIDFD).
> > > > > - been renamed to pidctl()
> > > >
> > > [snip]
> > > > Also, I'm still confused about how metadata access is supposed to work
> > > > for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> > > > You snipped out a portion of a previous email in which I asked about
> > > > your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> > > > place, we have two different kinds of file descriptors for processes,
> > > > one derived from procfs and one that's independent. The former works
> > > > with openat(2). The latter does not. To be very specific; if I'm
> > > > writing a function that accepts a pidfd and I get a pidfd that comes
> > > > from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> > > > smaps or oom_score_adj or statm for the named process in a race-free
> > > > manner?
> > >
> > > This is true, that such usecase will not be supportable. But the advantage
> > > on the other hand, is that suchs "pidfd" can be made pollable or readable in
> > > the future. Potentially allowing us to return exit status without a new
> > > syscall (?). And we can add IOCTLs to the pidfd descriptor which we cannot do
> > > with proc.
> > >
> > > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > > can be translated into a "pidfd" using another syscall or even a node, like
> > > /proc/pid/handle or something. I think this is what Christian suggested in
> > > the previous threads.
> >
> > Andy - and Jann who I just talked to - have proposed solutions for this.
> > Jann's idea is similar to what you suggested, Joel. You could e.g. do an
> > ioctl() handler for /proc that would give you a dirfd back for a given
> > pidfd. The advantage is that pidfd_clone() can then give back pidfds
> > without having to care in what procfs the process is supposed to live.
> > That makes things a lot easier. But pidfds for the general case should
> > be anon inodes. It's clean, it's simple and it is way more secure.
>
> That makes sense to me, it is clean and I agree let us do that.
>
> Also for the "blocking on pid exit status" usecase, instead of adding a new
> syscall like pidfd_wait, lets just make that a new IOCTL to the
Please, no ioctls.
> file_operations of the anon_inode pidfd file. This will lets us specify
> exactly what to wait on (wait on death or wait on zombie) and lets us
I don't like per-open-file-description state. Ever try to set
O_NONBLOCK on standard input? It results in a broken terminal
configuration. pidfd wait mode would be similar. Processes and
intraprocess components share file descriptors all the time for
various reasons, and making the wait mode specific to the open file
description causes "spooky action at a distance" and bugs. If you need
a configurable wait mode, you should create a new open file
description that encodes that wait mode for its entire lifetime.
> avoid
> having a new syscall
Please stop using the "this lets us avoid making a new system call"
justification for interface design. System calls are cheap to add, and
going to lengths to avoid making a new system call frequently makes
interfaces worse in various ways.
> and create new fd just for waiting.
I think it's fine to make a new FD for waiting, especially if you only
need a new FD for a non-default wait mode.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jann Horn @ 2019-03-25 21:15 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Daniel Colascione, Joel Fernandes, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathn>
In-Reply-To: <CAGLj2rE7k-p=ZniyV6bDm-VbhAdzSSwxYcVV=X_Rsky5nixGvw@mail.gmail.com>
On Mon, Mar 25, 2019 at 9:40 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> On Mon, Mar 25, 2019 at 8:34 PM Jann Horn <jannh@google.com> wrote:
> >
> > [...SNIP...]
> >
> > Please don't do that. /proc/$pid/fd refers to the set of file
> > descriptors the process has open, and semantically doesn't have much
> > to do with the identity of the process. If you want to have a procfs
> > directory entry for getting a pidfd, please add a new entry. (Although
> > I don't see the point in adding a new procfs entry for this when you
> > could instead have an ioctl or syscall operating on the procfs
> > directory fd.)
>
> There is no new entry. What I was saying (and I should have been
> clearer) is that the existing entry for the fd when open'd with
> O_DIRECTORY makes the kernel resolve the symlink to /proc/<PID> of the
> process it maps to, so it would become:
>
> int dirfd = open("/proc/self/fd/3", O_DIRECTORY|O_CLOEXEC);
That still seems really weird. This magically overloads O_DIRECTORY,
which means "fail if the thing is not a directory", to suddenly have
an entirely different meaning for one magical special type of file. On
top of that, unlike an ioctl or a new syscall, it doesn't convey
explicit intent and increases the risk of confused deputy issues.
> This also means you cannot cross the filesystem boundry, the said
> process needs to have a visible entry (which would mean hidepid= and
> gid= based access controls are honored), and you can only open the
> dirfd of a process in the current ns (as the PID will not map to an
> existent process if the pidfd maps to a process not in the same or
> children pid ns, in fdinfo it lists -1 in the pid field (we might not
> even need fdinfo anymore)).
AFAICS that doesn't have anything to do with whether you do this as a
syscall, as an ioctl, or as a jumped symlink. The kernel would have to
do the same security checks in any of those cases - only a classic,
non-jumped symlink would implicitly go through the existing permission
checks. And if you implement this with a non-jumped symlink, you get
races.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jonathan Kowalski @ 2019-03-25 21:14 UTC (permalink / raw)
To: Jann Horn
Cc: Daniel Colascione, Joel Fernandes, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathn>
In-Reply-To: <CAGLj2rE7k-p=ZniyV6bDm-VbhAdzSSwxYcVV=X_Rsky5nixGvw@mail.gmail.com>
Also, extending this further, instead of new ioctl flags over to
translate a tidfd one might introduce later for thread targetted
signals (which would still be a pidfd in the struct pid terms, but
with a bit set in its reference to target the selected TID in
particular), you could resolve this neatly to the proc entry of the
task itself, which would be subject to restrictions similar to a
regular open call, minus all the races involved.
This also means you can get rid of having to support the /proc/<PID>
dir fd in pidfd_send_signal, because there is no incentive to, any
longer. The kernel now has just one pidfd object, well scoped in its
purpose, and this "feature" is tied to procfs itself, disabling which
takes away the feature as well.
Otherwise, the ioctl will be conditionally available and/or work only
when procfs is present, and you'd tie procfs to pidfds eternally as
ABI.
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Joel Fernandes @ 2019-03-25 21:11 UTC (permalink / raw)
To: Christian Brauner
Cc: Daniel Colascione, Jann Horn, khlebnikov, Andy Lutomirski,
David Howells, Serge E. Hallyn, Eric W. Biederman, Linux API,
linux-kernel, Arnd Bergmann, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, bl0pbl33p,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
nagarathnam.muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190325201544.7o2kwuie3infcblp@brauner.io>
On Mon, Mar 25, 2019 at 09:15:45PM +0100, Christian Brauner wrote:
> On Mon, Mar 25, 2019 at 01:36:14PM -0400, Joel Fernandes wrote:
> > On Mon, Mar 25, 2019 at 09:48:43AM -0700, Daniel Colascione wrote:
> > > On Mon, Mar 25, 2019 at 9:21 AM Christian Brauner <christian@brauner.io> wrote:
> > > > The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
> > > > I quote Konstantins original patchset first that has already been acked and
> > > > picked up by Eric before and whose functionality is preserved in this
> > > > syscall. Multiple people have asked when this patchset will be sent in
> > > > for merging (cf. [1], [2]). It has recently been revived by Nagarathnam
> > > > Muthusamy from Oracle [3].
> > > >
> > > > The intention of the original translate_pid() syscall was twofold:
> > > > 1. Provide translation of pids between pid namespaces
> > > > 2. Provide implicit pid namespace introspection
> > > >
> > > > Both functionalities are preserved. The latter task has been improved
> > > > upon though. In the original version of the pachset passing pid as 1
> > > > would allow to deterimine the relationship between the pid namespaces.
> > > > This is inherhently racy. If pid 1 inside a pid namespace has died it
> > > > would report false negatives. For example, if pid 1 inside of the target
> > > > pid namespace already died, it would report that the target pid
> > > > namespace cannot be reached from the source pid namespace because it
> > > > couldn't find the pid inside of the target pid namespace and thus
> > > > falsely report to the user that the two pid namespaces are not related.
> > > > This problem is simple to avoid. In the new version we simply walk the
> > > > list of ancestors and check whether the namespace are related to each
> > > > other. By doing it this way we can reliably report what the relationship
> > > > between two pid namespace file descriptors looks like.
> > > >
> > > > Additionally, this syscall has been extended to allow the retrieval of
> > > > pidfds independent of procfs. These pidfds can e.g. be used with the new
> > > > pidfd_send_signal() syscall we recently merged. The ability to retrieve
> > > > pidfds independent of procfs had already been requested in the
> > > > pidfd_send_signal patchset by e.g. Andrew [4] and later again by Alexey
> > > > [5]. A use-case where a kernel is compiled without procfs but where
> > > > pidfds are still useful has been outlined by Andy in [6]. Regular
> > > > anon-inode based file descriptors are used that stash a reference to
> > > > struct pid in file->private_data and drop that reference on close.
> > > >
> > > > With this translate_pid() has three closely related but still distinct
> > > > functionalities. To clarify the semantics and to make it easier for
> > > > userspace to use the syscall it has:
> > > > - gained a command argument and three commands clearly reflecting the
> > > > distinct functionalities (PIDCMD_QUERY_PID, PIDCMD_QUERY_PIDNS,
> > > > PIDCMD_GET_PIDFD).
> > > > - been renamed to pidctl()
> > >
> > [snip]
> > > Also, I'm still confused about how metadata access is supposed to work
> > > for these procfs-less pidfs. If I use PIDCMD_GET_PIDFD on a process,
> > > You snipped out a portion of a previous email in which I asked about
> > > your thoughts on this question. With the PIDCMD_GET_PIDFD command in
> > > place, we have two different kinds of file descriptors for processes,
> > > one derived from procfs and one that's independent. The former works
> > > with openat(2). The latter does not. To be very specific; if I'm
> > > writing a function that accepts a pidfd and I get a pidfd that comes
> > > from PIDCMD_GET_PIDFD, how am I supposed to get the equivalent of
> > > smaps or oom_score_adj or statm for the named process in a race-free
> > > manner?
> >
> > This is true, that such usecase will not be supportable. But the advantage
> > on the other hand, is that suchs "pidfd" can be made pollable or readable in
> > the future. Potentially allowing us to return exit status without a new
> > syscall (?). And we can add IOCTLs to the pidfd descriptor which we cannot do
> > with proc.
> >
> > But.. one thing we could do for Daniel usecase is if a /proc/pid directory fd
> > can be translated into a "pidfd" using another syscall or even a node, like
> > /proc/pid/handle or something. I think this is what Christian suggested in
> > the previous threads.
>
> Andy - and Jann who I just talked to - have proposed solutions for this.
> Jann's idea is similar to what you suggested, Joel. You could e.g. do an
> ioctl() handler for /proc that would give you a dirfd back for a given
> pidfd. The advantage is that pidfd_clone() can then give back pidfds
> without having to care in what procfs the process is supposed to live.
> That makes things a lot easier. But pidfds for the general case should
> be anon inodes. It's clean, it's simple and it is way more secure.
That makes sense to me, it is clean and I agree let us do that.
Also for the "blocking on pid exit status" usecase, instead of adding a new
syscall like pidfd_wait, lets just make that a new IOCTL to the
file_operations of the anon_inode pidfd file. This will lets us specify
exactly what to wait on (wait on death or wait on zombie) and lets us avoid
having a new syscall and create new fd just for waiting. Let me know if you
disagree, but otherwise I am thinking of modifying my patches that way and
avoid adding a new syscall.
thanks!
- Joel
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Christian Brauner @ 2019-03-25 20:40 UTC (permalink / raw)
To: Jann Horn
Cc: Daniel Colascione, Jonathan Kowalski, Joel Fernandes,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathna>
In-Reply-To: <CAG48ez1ZVKgwfQDYT1k4pB4-8Y8Ywv12dabh5KFFxtKmT-e7Cw@mail.gmail.com>
On Mon, Mar 25, 2019 at 09:34:00PM +0100, Jann Horn wrote:
> On Mon, Mar 25, 2019 at 9:15 PM Daniel Colascione <dancol@google.com> wrote:
> > On Mon, Mar 25, 2019 at 12:42 PM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> > > On Mon, Mar 25, 2019 at 6:57 PM Daniel Colascione <dancol@google.com> wrote:
> [...]
> > > Yes, but everything in /proc is not equivalent to an attribute, or an
> > > option, and depending on its configuration, you may not want to allow
> > > processes to even be able to see /proc for any PIDs other than those
> > > running as their own user (hidepid). This means, even if this new
> > > system call is added, to respect hidepid, it must, depending on if
> > > /proc is mounted (and what hidepid is set to, and what gid= is set
> > > to), return EPERM, because then there is a discrepancy between how the
> > > two entrypoints to acquire a process handle do access control.
> >
> > That's why I proposed that this translation mechanism accept a procfs
> > root directory --- so you'd specify *which* procfs you want and let
> > the kernel apply whatever hidepid access restrictions it wants.
> [...]
> > > > and 2) it's
> > > > "fail unsafe": IMHO, most users in practice will skip the line marked
> > > > "LIVENESS CHECK", and as a result, their code will appear to work but
> > > > contain subtle race conditions. An explicit interface to translate
> > > > from a (PIDFD, PROCFS_ROOT) tuple to a /proc/pid directory file
> > > > descriptor would be both more efficient and fail-safe.
> > > >
> > > > [1] as a separate matter, it'd be nice to have a batch version of close(2).
> > >
> > > Since /proc is full of gunk,
> >
> > People keep saying /proc is bad, but I haven't seen any serious
> > proposals for a clean replacement. :-)
> >
> > > how about adding more to it and making
> > > the magic symlink of /proc/self/fd for the pidfd to lead to the dirfd
> > > of the /proc entry of the process it maps to, when one uses
> > > O_DIRECTORY while opening it? Otherwise, it behaves as it does today.
> > > It would be equivalent to opening the proc entry with usual access
> > > restrictions (and hidepid made to work) but without the races, and
> > > because for processes outside your and children pid ns, it shouldn't
> > > work anyway, and since they wouldn't have their entry on this procfs
> > > instance, it would all just fit in nicely?
> >
> > Thanks. That'll work. It's a bit magical, but /proc/self/fd is magical
> > anyway, so that's okay.
>
> Please don't do that. /proc/$pid/fd refers to the set of file
> descriptors the process has open, and semantically doesn't have much
> to do with the identity of the process. If you want to have a procfs
> directory entry for getting a pidfd, please add a new entry. (Although
> I don't see the point in adding a new procfs entry for this when you
> could instead have an ioctl or syscall operating on the procfs
> directory fd.)
Very much agreed!
^ permalink raw reply
* Re: [PATCH 0/4] pid: add pidctl()
From: Jonathan Kowalski @ 2019-03-25 20:40 UTC (permalink / raw)
To: Jann Horn
Cc: Daniel Colascione, Joel Fernandes, Christian Brauner,
Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy <nagarathn>
In-Reply-To: <CAG48ez1ZVKgwfQDYT1k4pB4-8Y8Ywv12dabh5KFFxtKmT-e7Cw@mail.gmail.com>
On Mon, Mar 25, 2019 at 8:34 PM Jann Horn <jannh@google.com> wrote:
>
> [...SNIP...]
>
> Please don't do that. /proc/$pid/fd refers to the set of file
> descriptors the process has open, and semantically doesn't have much
> to do with the identity of the process. If you want to have a procfs
> directory entry for getting a pidfd, please add a new entry. (Although
> I don't see the point in adding a new procfs entry for this when you
> could instead have an ioctl or syscall operating on the procfs
> directory fd.)
There is no new entry. What I was saying (and I should have been
clearer) is that the existing entry for the fd when open'd with
O_DIRECTORY makes the kernel resolve the symlink to /proc/<PID> of the
process it maps to, so it would become:
int dirfd = open("/proc/self/fd/3", O_DIRECTORY|O_CLOEXEC);
This also means you cannot cross the filesystem boundry, the said
process needs to have a visible entry (which would mean hidepid= and
gid= based access controls are honored), and you can only open the
dirfd of a process in the current ns (as the PID will not map to an
existent process if the pidfd maps to a process not in the same or
children pid ns, in fdinfo it lists -1 in the pid field (we might not
even need fdinfo anymore)).
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox