* Re: [PATCH] mm: extend memfd with ability to create secret memory
From: kbuild test robot @ 2019-12-08 14:10 UTC (permalink / raw)
To: Mike Rapoport
Cc: kbuild-all, Andy Lutomirski, LKML, Alexey Dobriyan, Andrew Morton,
Arnd Bergmann, Borislav Petkov, Dave Hansen, James Bottomley,
Peter Zijlstra, Steven Rostedt, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Linux API, Linux-MM, X86 ML, Mike Rapoport,
Alan Cox, Reshetova, Elena, Tycho Andersen, Chr
In-Reply-To: <20191205153400.GA25575@rapoport-lnx>
[-- Attachment #1: Type: text/plain, Size: 3891 bytes --]
Hi Mike,
I love your patch! Yet something to improve:
[auto build test ERROR on mmotm/master]
[also build test ERROR on linux/master v5.4]
[cannot apply to arm-soc/for-next linus/master next-20191208]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Mike-Rapoport/mm-extend-memfd-with-ability-to-create-secret-memory/20191207-130906
base: git://git.cmpxchg.org/linux-mmotm.git master
config: arm64-randconfig-a001-20191208 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
mm/secretmem.c: In function 'secretmem_fault':
>> mm/secretmem.c:45:9: error: implicit declaration of function 'set_pages_array_uc'; did you mean 'set_page_dirty_lock'? [-Werror=implicit-function-declaration]
err = set_pages_array_uc(&page, 1);
^~~~~~~~~~~~~~~~~~
set_page_dirty_lock
mm/secretmem.c: In function 'secretmem_close':
>> mm/secretmem.c:75:4: error: implicit declaration of function 'set_pages_array_wb'; did you mean 'set_page_dirty'? [-Werror=implicit-function-declaration]
set_pages_array_wb(&page, 1);
^~~~~~~~~~~~~~~~~~
set_page_dirty
cc1: some warnings being treated as errors
vim +45 mm/secretmem.c
21
22 static vm_fault_t secretmem_fault(struct vm_fault *vmf)
23 {
24 struct secretmem_state *state = vmf->vma->vm_file->private_data;
25 struct address_space *mapping = vmf->vma->vm_file->f_mapping;
26 pgoff_t offset = vmf->pgoff;
27 unsigned long addr;
28 struct page *page;
29 int err;
30
31 page = find_get_page(mapping, offset);
32 if (!page) {
33 page = pagecache_get_page(mapping, offset,
34 FGP_CREAT|FGP_FOR_MMAP,
35 vmf->gfp_mask);
36 if (!page)
37 return vmf_error(-ENOMEM);
38
39 __SetPageUptodate(page);
40 }
41
42 if (state->mode == SECRETMEM_EXCLUSIVE)
43 err = set_direct_map_invalid_noflush(page);
44 else if (state->mode == SECRETMEM_UNCACHED)
> 45 err = set_pages_array_uc(&page, 1);
46 else
47 BUG();
48
49 if (err) {
50 delete_from_page_cache(page);
51 return vmf_error(err);
52 }
53
54 addr = (unsigned long)page_address(page);
55 flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
56
57 vmf->page = page;
58 return 0;
59 }
60
61 static void secretmem_close(struct vm_area_struct *vma)
62 {
63 struct secretmem_state *state = vma->vm_file->private_data;
64 struct address_space *mapping = vma->vm_file->f_mapping;
65 struct page *page;
66 pgoff_t index;
67
68 xa_for_each(&mapping->i_pages, index, page) {
69 get_page(page);
70 lock_page(page);
71
72 if (state->mode == SECRETMEM_EXCLUSIVE)
73 set_direct_map_default_noflush(page);
74 else if (state->mode == SECRETMEM_UNCACHED)
> 75 set_pages_array_wb(&page, 1);
76 else
77 BUG();
78
79 __ClearPageDirty(page);
80 delete_from_page_cache(page);
81
82 unlock_page(page);
83 put_page(page);
84 }
85 }
86
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37879 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 04/10] pipe: Use head and tail pointers for the ring, not cursor and length [ver #2]
From: Johannes Hirte @ 2019-12-08 17:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, Rasmus Villemoes, Greg Kroah-Hartman,
Peter Zijlstra, Nicolas Dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wjEa5oNcQ9+9fai1Awqktf+hzz_HZmChi8HZJWcL62+Cw@mail.gmail.com>
On 2019 Dez 06, Linus Torvalds wrote:
> On Fri, Dec 6, 2019 at 4:00 PM Johannes Hirte
> <johannes.hirte@datenkhaos.de> wrote:
> >
> > Tested with 5.4.0-11505-g347f56fb3890 and still the same wrong behavior.
>
> Ok, we'll continue looking.
>
> That said, your version string is strange.
>
> Commit 347f56fb3890 should be "v5.4.0-13174-g347f56fb3890", the fact
> that you have "11505" confuses me.
>
> The hash is what matters, but I wonder what is going on that you have
> the commit count in that version string so wrong.
>
> Linus
Yes, something got messed up here. After last pull, git describe says:
drm-next-2019-12-06-11662-g9455d25f4e3b
whereas with a fresh cloned repo I get:
v5.4-13331-g9455d25f4e3b
I assume the later is right. With this version the bug seems to be
fixed, regardless of the commit count. Tested with different websites
and firefox works as expected again.
--
Regards,
Johannes Hirte
^ permalink raw reply
* Re: [RFC PATCH 04/10] pipe: Use head and tail pointers for the ring, not cursor and length [ver #2]
From: Linus Torvalds @ 2019-12-08 18:10 UTC (permalink / raw)
To: Johannes Hirte
Cc: David Howells, Rasmus Villemoes, Greg Kroah-Hartman,
Peter Zijlstra, Nicolas Dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <20191208175602.GA1844@latitude>
On Sun, Dec 8, 2019 at 9:56 AM Johannes Hirte
<johannes.hirte@datenkhaos.de> wrote:
>
> whereas with a fresh cloned repo I get:
>
> v5.4-13331-g9455d25f4e3b
>
> I assume the later is right. With this version the bug seems to be
> fixed, regardless of the commit count. Tested with different websites
> and firefox works as expected again.
Ok, good. It was almost certainly the select/poll race fix - Mariusz
Ceier reported a problem with youtube hanging, and confirmed that the
poll/select fix seems to have cleared that one up. I suspect that your
hang on fb and spiegel.de were the same thing.
So I think the pipe code is stable again with current -git. Thanks for
reporting and testing,
Linus
^ permalink raw reply
* Re: [PATCH v6 0/2] add performance reporting support to FPGA DFL drivers
From: Wu Hao @ 2019-12-09 2:45 UTC (permalink / raw)
To: Will Deacon
Cc: mdf, mark.rutland, linux-fpga, linux-kernel, linux-api, atull,
gregkh
In-Reply-To: <20191125080839.GA6227@hao-dev>
On Mon, Nov 25, 2019 at 04:08:39PM +0800, Wu Hao wrote:
> On Mon, Nov 25, 2019 at 08:01:28AM +0000, Will Deacon wrote:
> > On Mon, Nov 25, 2019 at 11:34:12AM +0800, Wu Hao wrote:
> > > Hi Will and Mark,
> > >
> > > Could you please help us on review this patchset? as this patchset mainly
> > > introduced a new perf driver following the similar way as drivers/perf/*.
> >
> > Why is it not under drivers/perf/, then?
>
> Hi Will
>
> Thanks for the quick response. This is one sub feature for DFL based FPGAs,
> and we plan to put this sub feature together with others, including related
> documentation. It only registers a standard perf pmu for its userspace
> interfaces.
>
> >
> > > This patchset has been submitted for a long time but didn't receive any
> > > comment after v4. we appreciate any review comments! thanks in advance. :)
> >
> > Hmm, not sure I saw the previous versions. Guessing I wasn't on cc?
>
> We switched to perf API from v4, and started ccing you and Mark from v5. :)
Hi Will
Did you get a chance to look into this patchset?
Thanks
Hao
>
> Thanks
> Hao
>
> >
> > Will
^ permalink raw reply
* [PATCH v2 0/4] Add ptrace get_fd request
From: Sargun Dhillon @ 2019-12-09 7:04 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro
This patchest introduces a mechanism to capture file descriptors from other
processes via ptrace. Although this can be achieved using SCM_RIGHTS, and
parasitic code injection, this offers a slightly more straightforward
mechainsm. It also does not mutate the tracee in any way, nor does it
require that the tracee is stopped, as to avoid causing issues with
attaching debuggers or runtimes that expect syscalls to be preemptible, or
return within a specific amount of time.
It has an options mechanism that's only usable to set CLOEXEC on the fd,
but I'm thinking that it could be extended to other aspects. For example,
for sockets, one could want to scrub the cgroup information.
In the future, the API may not require ptrace attachment, or seizing, but
right now it does as a matter of safety.
Changes since the RFC v1:
* Introduce a new helper to fs/file.c to fetch a file descriptor from
any process. It largely uses the code suggested by Oleg, with a few
changes to fix locking
* It uses an extensible options struct to supply the FD, and option.
* I added a sample, using the code from the user-ptrace sample
Sargun Dhillon (4):
vfs, fdtable: Add get_task_file helper
ptrace: add PTRACE_GETFD request to fetch file descriptors from
tracees
samples: split generalized user-trap code into helper file
samples: Add example of using PTRACE_GETFD in conjunction with user
trap
fs/file.c | 19 +++
include/linux/fdtable.h | 10 ++
include/uapi/linux/ptrace.h | 15 +++
kernel/ptrace.c | 35 +++++-
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 15 ++-
samples/seccomp/user-trap-helper.c | 84 +++++++++++++
samples/seccomp/user-trap-helper.h | 13 ++
samples/seccomp/user-trap-ptrace.c | 193 +++++++++++++++++++++++++++++
samples/seccomp/user-trap.c | 85 +------------
10 files changed, 382 insertions(+), 88 deletions(-)
create mode 100644 samples/seccomp/user-trap-helper.c
create mode 100644 samples/seccomp/user-trap-helper.h
create mode 100644 samples/seccomp/user-trap-ptrace.c
--
2.20.1
^ permalink raw reply
* [PATCH v2 1/4] vfs, fdtable: Add get_task_file helper
From: Sargun Dhillon @ 2019-12-09 7:06 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro
This introduces a function which can be used to fetch a file, given an
arbitrary task. As long as the user holds a reference (refcnt) to the
task_struct it is safe to call, and will either return NULL on failure,
or a pointer to the file, with a refcnt.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
fs/file.c | 19 +++++++++++++++++++
include/linux/fdtable.h | 10 ++++++++++
2 files changed, 29 insertions(+)
diff --git a/fs/file.c b/fs/file.c
index 3da91a112bab..98601a503a0f 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1015,3 +1015,22 @@ int iterate_fd(struct files_struct *files, unsigned n,
return res;
}
EXPORT_SYMBOL(iterate_fd);
+
+struct file *get_task_file(struct task_struct *task, unsigned int fd)
+{
+ struct file *file = NULL;
+
+ task_lock(task);
+ rcu_read_lock();
+
+ if (task->files) {
+ file = fcheck_files(task->files, fd);
+ if (file && !get_file_rcu(file))
+ file = NULL;
+ }
+
+ rcu_read_unlock();
+ task_unlock(task);
+
+ return file;
+}
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index f07c55ea0c22..eacb1a56df44 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -115,6 +115,16 @@ int iterate_fd(struct files_struct *, unsigned,
int (*)(const void *, struct file *, unsigned),
const void *);
+/*
+ * get_task_file - get a reference to a file from another task
+ * @task: the task to get the file descriptor from
+ * @fd: the file descriptor number to fetch
+ *
+ * returns NULL on failure, or pointer to the file on success, with a reference
+ * It requires that the task is pinned prior to calling it.
+ */
+struct file *get_task_file(struct task_struct *task, unsigned int fd);
+
extern int __alloc_fd(struct files_struct *files,
unsigned start, unsigned end, unsigned flags);
extern void __fd_install(struct files_struct *files,
--
2.20.1
^ permalink raw reply related
* [PATCH v2 2/4] ptrace: add PTRACE_GETFD request to fetch file descriptors from tracees
From: Sargun Dhillon @ 2019-12-09 7:06 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro
PTRACE_GETFD is a generic ptrace API that allows the tracer to
get file descriptors from the tracee.
One reason to use this is to allow sandboxers to take actions on file
descriptors on the behalf of a tracee. For example, this can be
combined with seccomp-bpf's user notification to ptrace on-demand and
capture an fd without requiring the tracer to always be attached to
the process. The tracer can then take a privileged action on behalf
of the tracee, like binding a socket to a privileged port.
It works whether or not the tracee is stopped. The only prior requirement
is that the tracer is attached to the process via PTRACE_ATTACH or
PTRACE_SEIZE. Stopping the process breaks certain runtimes that expect
to be able to preempt syscalls (quickly). In addition, it is meant to be
used in an on-demand fashion to avoid breaking debuggers.
The ptrace call takes a pointer to ptrace_getfd_args in data, and the
size of the structure in addr. There is an options field, which can
be used to state whether the fd should be opened with CLOEXEC, or not.
This options field may be extended in the future to include the ability
to clear cgroup information about the file descriptor at a later point.
If the structure is from a newer kernel, and includes members which
make it larger than the structure that's known to this kernel version,
E2BIG will be returned.
The requirement that the tracer has attached to the tracee prior to the
capture of the file descriptor may be lifted at a later point.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
include/uapi/linux/ptrace.h | 15 +++++++++++++++
kernel/ptrace.c | 35 +++++++++++++++++++++++++++++++++--
2 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index a71b6e3b03eb..c84655bcc453 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -101,6 +101,21 @@ struct ptrace_syscall_info {
};
};
+/*
+ * This gets a file descriptor from a process. It requires that the process
+ * has either been attached to. It does not require that the process is
+ * stopped.
+ */
+#define PTRACE_GETFD 0x420f
+
+/* options to pass in to ptrace_getfd_args */
+#define PTRACE_GETFD_O_CLOEXEC (1 << 0) /* open the fd with cloexec */
+
+struct ptrace_getfd_args {
+ __u32 fd; /* the tracee's file descriptor to get */
+ __u32 options;
+} __attribute__((packed));
+
/*
* These values are stored in task->ptrace_message
* by tracehook_report_syscall_* to describe the current syscall-stop.
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index cb9ddcc08119..8f619dceac6f 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -31,6 +31,7 @@
#include <linux/cn_proc.h>
#include <linux/compat.h>
#include <linux/sched/signal.h>
+#include <linux/fdtable.h>
#include <asm/syscall.h> /* for syscall_get_* */
@@ -994,6 +995,33 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
}
#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
+static int ptrace_getfd(struct task_struct *child, unsigned long user_size,
+ void __user *datavp)
+{
+ struct ptrace_getfd_args args;
+ unsigned int fd_flags = 0;
+ struct file *file;
+ int ret;
+
+ ret = copy_struct_from_user(&args, sizeof(args), datavp, user_size);
+ if (ret)
+ goto out;
+ if ((args.options & ~(PTRACE_GETFD_O_CLOEXEC)) != 0)
+ return -EINVAL;
+ if (args.options & PTRACE_GETFD_O_CLOEXEC)
+ fd_flags &= O_CLOEXEC;
+ file = get_task_file(child, args.fd);
+ if (!file)
+ return -EBADF;
+ ret = get_unused_fd_flags(fd_flags);
+ if (ret >= 0)
+ fd_install(ret, file);
+ else
+ fput(file);
+out:
+ return ret;
+}
+
int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
{
@@ -1222,7 +1250,9 @@ int ptrace_request(struct task_struct *child, long request,
case PTRACE_SECCOMP_GET_METADATA:
ret = seccomp_get_metadata(child, addr, datavp);
break;
-
+ case PTRACE_GETFD:
+ ret = ptrace_getfd(child, addr, datavp);
+ break;
default:
break;
}
@@ -1265,7 +1295,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
}
ret = ptrace_check_attach(child, request == PTRACE_KILL ||
- request == PTRACE_INTERRUPT);
+ request == PTRACE_INTERRUPT ||
+ request == PTRACE_GETFD);
if (ret < 0)
goto out_put_task_struct;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 3/4] samples: split generalized user-trap code into helper file
From: Sargun Dhillon @ 2019-12-09 7:06 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro
This moves the code for setting up a syscall interceptor with user
notification and sending the user notification file descriptor over a
socket using SCM_RIGHTS into a file that can be shared between multiple
samples.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
samples/seccomp/Makefile | 6 ++-
samples/seccomp/user-trap-helper.c | 84 +++++++++++++++++++++++++++++
samples/seccomp/user-trap-helper.h | 13 +++++
samples/seccomp/user-trap.c | 85 +-----------------------------
4 files changed, 103 insertions(+), 85 deletions(-)
create mode 100644 samples/seccomp/user-trap-helper.c
create mode 100644 samples/seccomp/user-trap-helper.h
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index 009775b52538..82b7347318d1 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -16,9 +16,13 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
bpf-direct-objs := bpf-direct.o
+
+HOSTCFLAGS_user-trap-helper.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap-helper.o += -idirafter $(objtree)/include
+
HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
-user-trap-objs := user-trap.o
+user-trap-objs := user-trap.o user-trap-helper.o
# Try to match the kernel target.
ifndef CONFIG_64BIT
diff --git a/samples/seccomp/user-trap-helper.c b/samples/seccomp/user-trap-helper.c
new file mode 100644
index 000000000000..f91ae9d947c5
--- /dev/null
+++ b/samples/seccomp/user-trap-helper.c
@@ -0,0 +1,84 @@
+#include <linux/seccomp.h>
+#include <linux/filter.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stddef.h>
+#include <sys/types.h>
+#include <sys/syscall.h>
+#include <sys/socket.h>
+#include "user-trap-helper.h"
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
+int user_trap_syscall(int nr, unsigned int flags)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
+}
+
+int send_fd(int sock, int fd)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+ *((int *)CMSG_DATA(cmsg)) = fd;
+ msg.msg_controllen = cmsg->cmsg_len;
+
+ if (sendmsg(sock, &msg, 0) < 0) {
+ perror("sendmsg");
+ return -1;
+ }
+
+ return 0;
+}
+
+int recv_fd(int sock)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+
+ if (recvmsg(sock, &msg, 0) < 0) {
+ perror("recvmsg");
+ return -1;
+ }
+
+ cmsg = CMSG_FIRSTHDR(&msg);
+
+ return *((int *)CMSG_DATA(cmsg));
+}
diff --git a/samples/seccomp/user-trap-helper.h b/samples/seccomp/user-trap-helper.h
new file mode 100644
index 000000000000..a5ebda25fdfe
--- /dev/null
+++ b/samples/seccomp/user-trap-helper.h
@@ -0,0 +1,13 @@
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <errno.h>
+
+static inline int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
+
+int user_trap_syscall(int nr, unsigned int flags);
+int send_fd(int sock, int fd);
+int recv_fd(int sock);
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
index 6d0125ca8af7..1b6526587456 100644
--- a/samples/seccomp/user-trap.c
+++ b/samples/seccomp/user-trap.c
@@ -5,101 +5,18 @@
#include <errno.h>
#include <fcntl.h>
#include <string.h>
-#include <stddef.h>
#include <sys/sysmacros.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/mman.h>
-#include <sys/syscall.h>
#include <sys/user.h>
#include <sys/ioctl.h>
-#include <sys/ptrace.h>
#include <sys/mount.h>
#include <linux/limits.h>
-#include <linux/filter.h>
#include <linux/seccomp.h>
-
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
-
-static int seccomp(unsigned int op, unsigned int flags, void *args)
-{
- errno = 0;
- return syscall(__NR_seccomp, op, flags, args);
-}
-
-static int send_fd(int sock, int fd)
-{
- struct msghdr msg = {};
- struct cmsghdr *cmsg;
- char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
- struct iovec io = {
- .iov_base = &c,
- .iov_len = 1,
- };
-
- msg.msg_iov = &io;
- msg.msg_iovlen = 1;
- msg.msg_control = buf;
- msg.msg_controllen = sizeof(buf);
- cmsg = CMSG_FIRSTHDR(&msg);
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- cmsg->cmsg_len = CMSG_LEN(sizeof(int));
- *((int *)CMSG_DATA(cmsg)) = fd;
- msg.msg_controllen = cmsg->cmsg_len;
-
- if (sendmsg(sock, &msg, 0) < 0) {
- perror("sendmsg");
- return -1;
- }
-
- return 0;
-}
-
-static int recv_fd(int sock)
-{
- struct msghdr msg = {};
- struct cmsghdr *cmsg;
- char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
- struct iovec io = {
- .iov_base = &c,
- .iov_len = 1,
- };
-
- msg.msg_iov = &io;
- msg.msg_iovlen = 1;
- msg.msg_control = buf;
- msg.msg_controllen = sizeof(buf);
-
- if (recvmsg(sock, &msg, 0) < 0) {
- perror("recvmsg");
- return -1;
- }
-
- cmsg = CMSG_FIRSTHDR(&msg);
-
- return *((int *)CMSG_DATA(cmsg));
-}
-
-static int user_trap_syscall(int nr, unsigned int flags)
-{
- struct sock_filter filter[] = {
- BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
- offsetof(struct seccomp_data, nr)),
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
- BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
- BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
- };
-
- struct sock_fprog prog = {
- .len = (unsigned short)ARRAY_SIZE(filter),
- .filter = filter,
- };
-
- return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
-}
+#include "user-trap-helper.h"
static int handle_req(struct seccomp_notif *req,
struct seccomp_notif_resp *resp, int listener)
--
2.20.1
^ permalink raw reply related
* [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Sargun Dhillon @ 2019-12-09 7:06 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro
This sample adds the usage of SECCOMP_RET_USER_NOTIF together with ptrace
PTRACE_GETFD. It shows trapping a syscall, and handling it by extracting
the FD into the parent process without stopping the child process.
Although, in this example, there's no explicit policy separation in
the two processes, it can be generalized into the example of a transparent
proxy.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 9 +-
samples/seccomp/user-trap-ptrace.c | 193 +++++++++++++++++++++++++++++
3 files changed, 202 insertions(+), 1 deletion(-)
create mode 100644 samples/seccomp/user-trap-ptrace.c
diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
index d1e2e817d556..169bc130ec39 100644
--- a/samples/seccomp/.gitignore
+++ b/samples/seccomp/.gitignore
@@ -2,3 +2,4 @@ bpf-direct
bpf-fancy
dropper
user-trap
+user-trap-ptrace
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index 82b7347318d1..c0f3ef713f5b 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef CROSS_COMPILE
-hostprogs-y := bpf-fancy dropper bpf-direct user-trap
+hostprogs-y := bpf-fancy dropper bpf-direct user-trap user-trap-ptrace
HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
@@ -24,6 +24,11 @@ HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
user-trap-objs := user-trap.o user-trap-helper.o
+HOSTCFLAGS_user-trap-ptrace.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap-ptrace.o += -idirafter $(objtree)/include
+user-trap-ptrace-objs := user-trap-ptrace.o user-trap-helper.o
+
+
# Try to match the kernel target.
ifndef CONFIG_64BIT
@@ -39,10 +44,12 @@ HOSTCFLAGS_dropper.o += $(MFLAG)
HOSTCFLAGS_bpf-helper.o += $(MFLAG)
HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
HOSTCFLAGS_user-trap.o += $(MFLAG)
+HOSTCFLAGS_user-trap-ptrace.o += $(MFLAG)
HOSTLDLIBS_bpf-direct += $(MFLAG)
HOSTLDLIBS_bpf-fancy += $(MFLAG)
HOSTLDLIBS_dropper += $(MFLAG)
HOSTLDLIBS_user-trap += $(MFLAG)
+HOSTLDLIBS_user-trap-ptrace += $(MFLAG)
endif
always := $(hostprogs-y)
endif
diff --git a/samples/seccomp/user-trap-ptrace.c b/samples/seccomp/user-trap-ptrace.c
new file mode 100644
index 000000000000..5cca1cb4916c
--- /dev/null
+++ b/samples/seccomp/user-trap-ptrace.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/seccomp.h>
+#include <linux/ptrace.h>
+#include <linux/prctl.h>
+#include <sys/socket.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/ioctl.h>
+#include <assert.h>
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <netinet/in.h>
+#include "user-trap-helper.h"
+
+#define CHILD_PORT_TRY_BIND 80
+#define CHILD_PORT_ACTUAL_BIND 4998
+
+static int ptrace(long request, long pid, void *addr, unsigned long data)
+{
+ errno = 0;
+ return syscall(__NR_ptrace, request, pid, addr, data);
+}
+
+static int ptrace_getfd(long pid, struct ptrace_getfd_args *args)
+{
+ errno = 0;
+ return syscall(__NR_ptrace, PTRACE_GETFD, pid, sizeof(*args), args);
+}
+
+static int tracee(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = htons(CHILD_PORT_TRY_BIND),
+ .sin_addr = {
+ .s_addr = htonl(INADDR_ANY)
+ }
+ };
+ socklen_t addrlen = sizeof(addr);
+ int sock, ret = 1;
+
+ sock = socket(AF_INET, SOCK_STREAM, 0);
+ if (sock == -1) {
+ perror("socket");
+ goto out;
+ }
+
+
+ if (bind(sock, (struct sockaddr *) &addr, sizeof(addr))) {
+ perror("bind");
+ goto out;
+ }
+
+ printf("Child successfully performed bind operation\n");
+ if (getsockname(sock, (struct sockaddr *) &addr, &addrlen)) {
+ perror("getsockname");
+ goto out;
+ }
+
+
+ printf("Socket bound to port %d\n", ntohs(addr.sin_port));
+ assert(ntohs(addr.sin_port) == CHILD_PORT_ACTUAL_BIND);
+
+ ret = 0;
+out:
+ return ret;
+}
+
+static int handle_req(int listener)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = htons(4998),
+ .sin_addr = {
+ .s_addr = htonl(INADDR_LOOPBACK)
+ }
+ };
+ struct ptrace_getfd_args getfd_args = {
+ .options = PTRACE_GETFD_O_CLOEXEC
+ };
+ struct seccomp_notif_sizes sizes;
+ struct seccomp_notif_resp *resp;
+ struct seccomp_notif *req;
+ int fd, ret = 1;
+
+ if (seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes) < 0) {
+ perror("seccomp(GET_NOTIF_SIZES)");
+ goto out;
+ }
+ req = malloc(sizes.seccomp_notif);
+ if (!req)
+ goto out;
+ memset(req, 0, sizeof(*req));
+
+ resp = malloc(sizes.seccomp_notif_resp);
+ if (!resp)
+ goto out_free_req;
+ memset(resp, 0, sizeof(*resp));
+
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
+ perror("ioctl recv");
+ goto out;
+ }
+ printf("Child tried to call bind with fd: %lld\n", req->data.args[0]);
+ getfd_args.fd = req->data.args[0];
+ fd = ptrace_getfd(req->pid, &getfd_args);
+ if (fd == -1) {
+ perror("ptrace_getfd");
+ goto out_free_resp;
+ }
+ if (bind(fd, (struct sockaddr *) &addr, sizeof(addr))) {
+ perror("bind");
+ goto out_free_resp;
+ }
+
+ resp->id = req->id;
+ resp->error = 0;
+ resp->val = 0;
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, resp) < 0) {
+ perror("ioctl send");
+ goto out_free_resp;
+ }
+
+ ret = 0;
+out_free_resp:
+ free(resp);
+out_free_req:
+ free(req);
+out:
+ return ret;
+}
+
+int main(void)
+{
+ int listener, sk_pair[2], ret = 1;
+ pid_t pid;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ perror("socketpair");
+ goto out;
+ }
+
+ pid = fork();
+ if (pid < 0) {
+ perror("fork");
+ goto close_pair;
+ }
+ if (pid == 0) {
+ if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+ perror("prctl(NO_NEW_PRIVS)");
+ exit(1);
+ }
+ listener = user_trap_syscall(__NR_bind,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ if (listener < 0) {
+ perror("seccomp");
+ exit(1);
+ }
+ if (send_fd(sk_pair[1], listener) < 0)
+ exit(1);
+ close(listener);
+ exit(tracee());
+ }
+
+ if (ptrace(PTRACE_SEIZE, pid, 0, PTRACE_O_EXITKILL)) {
+ perror("ptrace(PTRACE_SEIZE)");
+ goto kill_child;
+ }
+
+ listener = recv_fd(sk_pair[0]);
+ if (listener < 0)
+ goto kill_child;
+
+ if (handle_req(listener))
+ goto kill_child;
+
+ /* Wait for child to finish */
+ waitpid(pid, NULL, 0);
+
+ ret = 0;
+ goto close_pair;
+
+kill_child:
+ kill(pid, SIGKILL);
+close_pair:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+out:
+ return ret;
+}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 2/4] ptrace: add PTRACE_GETFD request to fetch file descriptors from tracees
From: Christian Brauner @ 2019-12-09 9:39 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro
In-Reply-To: <20191209070621.GA32450@ircssh-2.c.rugged-nimbus-611.internal>
On Mon, Dec 09, 2019 at 07:06:24AM +0000, Sargun Dhillon wrote:
> PTRACE_GETFD is a generic ptrace API that allows the tracer to
> get file descriptors from the tracee.
>
> One reason to use this is to allow sandboxers to take actions on file
> descriptors on the behalf of a tracee. For example, this can be
> combined with seccomp-bpf's user notification to ptrace on-demand and
> capture an fd without requiring the tracer to always be attached to
> the process. The tracer can then take a privileged action on behalf
> of the tracee, like binding a socket to a privileged port.
>
> It works whether or not the tracee is stopped. The only prior requirement
> is that the tracer is attached to the process via PTRACE_ATTACH or
> PTRACE_SEIZE. Stopping the process breaks certain runtimes that expect
> to be able to preempt syscalls (quickly). In addition, it is meant to be
> used in an on-demand fashion to avoid breaking debuggers.
>
> The ptrace call takes a pointer to ptrace_getfd_args in data, and the
> size of the structure in addr. There is an options field, which can
> be used to state whether the fd should be opened with CLOEXEC, or not.
> This options field may be extended in the future to include the ability
> to clear cgroup information about the file descriptor at a later point.
> If the structure is from a newer kernel, and includes members which
> make it larger than the structure that's known to this kernel version,
> E2BIG will be returned.
>
> The requirement that the tracer has attached to the tracee prior to the
> capture of the file descriptor may be lifted at a later point.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> include/uapi/linux/ptrace.h | 15 +++++++++++++++
> kernel/ptrace.c | 35 +++++++++++++++++++++++++++++++++--
> 2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index a71b6e3b03eb..c84655bcc453 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -101,6 +101,21 @@ struct ptrace_syscall_info {
> };
> };
>
> +/*
> + * This gets a file descriptor from a process. It requires that the process
> + * has either been attached to. It does not require that the process is
> + * stopped.
> + */
> +#define PTRACE_GETFD 0x420f
> +
> +/* options to pass in to ptrace_getfd_args */
> +#define PTRACE_GETFD_O_CLOEXEC (1 << 0) /* open the fd with cloexec */
Hey Sargun,
Thanks for the patch!
Why not simply accept O_CLOEXEC as flag? If that's not possible for some
reason I'd say
#define PTRACE_GETFD_O_CLOEXEC O_CLOEXEC /* open the fd with cloexec */
is the right thing to do. This is fairly common:
include/uapi/linux/timerfd.h:#define TFD_CLOEXEC O_CLOEXEC
include/uapi/drm/drm.h:#define DRM_CLOEXEC O_CLOEXEC
include/linux/userfaultfd_k.h:#define UFFD_CLOEXEC O_CLOEXEC
include/linux/eventfd.h:#define EFD_CLOEXEC O_CLOEXEC
include/uapi/linux/eventpoll.h:#define EPOLL_CLOEXEC O_CLOEXEC
include/uapi/linux/inotify.h:/* For O_CLOEXEC and O_NONBLOCK */
include/uapi/linux/inotify.h:#define IN_CLOEXEC O_CLOEXEC
include/uapi/linux/mount.h:#define OPEN_TREE_CLOEXEC O_CLOEXEC /* Close the file on execve() */
You can also add a compile-time assert to ptrace like we did for
fs/namespace.c's OPEN_TREE_CLOEXEC:
BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
And I'd remove the _O if you go with a separate flag, i.e.:
#define PTRACE_GETFD_CLOEXEC O_CLOEXEC /* open the fd with cloexec */
> +
> +struct ptrace_getfd_args {
> + __u32 fd; /* the tracee's file descriptor to get */
> + __u32 options;
Nit and I'm not set on it at all but "flags" might just be better.
> +} __attribute__((packed));
What's the benefit in using __attribute__((packed)) here? Seems to me that:
+struct ptrace_getfd_args {
+ __u32 fd; /* the tracee's file descriptor to get */
+ __u32 options;
+};
would just work fine.
> +
> /*
> * These values are stored in task->ptrace_message
> * by tracehook_report_syscall_* to describe the current syscall-stop.
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index cb9ddcc08119..8f619dceac6f 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -31,6 +31,7 @@
> #include <linux/cn_proc.h>
> #include <linux/compat.h>
> #include <linux/sched/signal.h>
> +#include <linux/fdtable.h>
>
> #include <asm/syscall.h> /* for syscall_get_* */
>
> @@ -994,6 +995,33 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> }
> #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
>
> +static int ptrace_getfd(struct task_struct *child, unsigned long user_size,
> + void __user *datavp)
> +{
> + struct ptrace_getfd_args args;
> + unsigned int fd_flags = 0;
> + struct file *file;
> + int ret;
> +
> + ret = copy_struct_from_user(&args, sizeof(args), datavp, user_size);
> + if (ret)
> + goto out;
Why is this goto out and not just return ret?
> + if ((args.options & ~(PTRACE_GETFD_O_CLOEXEC)) != 0)
> + return -EINVAL;
> + if (args.options & PTRACE_GETFD_O_CLOEXEC)
> + fd_flags &= O_CLOEXEC;
> + file = get_task_file(child, args.fd);
> + if (!file)
> + return -EBADF;
> + ret = get_unused_fd_flags(fd_flags);
Why isn't that whole thing just:
ret = get_unused_fd_flags(fd_flags & {PTRACE_GETFD_}O_CLOEXEC);
> + if (ret >= 0)
> + fd_install(ret, file);
> + else
> + fput(file);
> +out:
> + return ret;
> +}
So sm like:
static int ptrace_getfd(struct task_struct *child, unsigned long user_size,
void __user *datavp)
{
struct ptrace_getfd_args args;
unsigned int fd_flags = 0;
struct file *file;
int ret;
ret = copy_struct_from_user(&args, sizeof(args), datavp, user_size);
if (ret)
return ret;
if ((args.options & ~(PTRACE_GETFD_O_CLOEXEC)) != 0)
return -EINVAL;
file = get_task_file(child, args.fd);
if (!file)
return -EBADF;
/* PTRACE_GETFD_CLOEXEC == O_CLOEXEC */
ret = get_unused_fd_flags(fd_flags & PTRACE_GETFD_O_CLOEXEC);
if (ret >= 0)
fd_install(ret, file);
else
fput(file);
return ret;
}
^ permalink raw reply
* Re: [PATCH v2 2/4] ptrace: add PTRACE_GETFD request to fetch file descriptors from tracees
From: Sargun Dhillon @ 2019-12-09 10:55 UTC (permalink / raw)
To: Christian Brauner
Cc: LKML, Linux Containers, Linux API, linux-fsdevel, Tycho Andersen,
Jann Horn, cyphar, oleg, Andy Lutomirski, viro
In-Reply-To: <20191209093944.g6lgt2cqkec7eaym@wittgenstein>
On Mon, Dec 9, 2019 at 1:39 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> Hey Sargun,
>
> Thanks for the patch!
Thanks for your review.
>
> Why not simply accept O_CLOEXEC as flag? If that's not possible for some
> reason I'd say
>
I did this initially. My plan is to use this options field for other
(future) things as well, like
clearing (cgroup) metadata on sockets (assuming we figure out a safe
way to do it).
If we use O_CLOEXEC, it takes up an arbitrary bit which is different
on different
platforms, and working around that seems messy
Another way around this would be to have two members. One member which
is something
like fdflags, that just takes the fd flags, like O_CLOEXEC, and then
later on, we can add
an options member to enable these future use cases.
What do you think?
> #define PTRACE_GETFD_O_CLOEXEC O_CLOEXEC /* open the fd with cloexec */
>
> is the right thing to do. This is fairly common:
>
> include/uapi/linux/timerfd.h:#define TFD_CLOEXEC O_CLOEXEC
> include/uapi/drm/drm.h:#define DRM_CLOEXEC O_CLOEXEC
> include/linux/userfaultfd_k.h:#define UFFD_CLOEXEC O_CLOEXEC
> include/linux/eventfd.h:#define EFD_CLOEXEC O_CLOEXEC
> include/uapi/linux/eventpoll.h:#define EPOLL_CLOEXEC O_CLOEXEC
> include/uapi/linux/inotify.h:/* For O_CLOEXEC and O_NONBLOCK */
> include/uapi/linux/inotify.h:#define IN_CLOEXEC O_CLOEXEC
> include/uapi/linux/mount.h:#define OPEN_TREE_CLOEXEC O_CLOEXEC /* Close the file on execve() */
>
> You can also add a compile-time assert to ptrace like we did for
> fs/namespace.c's OPEN_TREE_CLOEXEC:
> BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
>
> And I'd remove the _O if you go with a separate flag, i.e.:
>
> #define PTRACE_GETFD_CLOEXEC O_CLOEXEC /* open the fd with cloexec */
>
> > +
> > +struct ptrace_getfd_args {
> > + __u32 fd; /* the tracee's file descriptor to get */
> > + __u32 options;
>
> Nit and I'm not set on it at all but "flags" might just be better.
>
> > +} __attribute__((packed));
>
> What's the benefit in using __attribute__((packed)) here? Seems to me that:
>
1) Are we always to assume that the compiler will give us 4-byte
alignment (paranoia)
2) If we're to add new non-4-byte aligned members later on, is it
kosher to add packed
later on?
> +struct ptrace_getfd_args {
> + __u32 fd; /* the tracee's file descriptor to get */
> + __u32 options;
> +};
>
> would just work fine.
>
> > +
> > /*
> > * These values are stored in task->ptrace_message
> > * by tracehook_report_syscall_* to describe the current syscall-stop.
> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index cb9ddcc08119..8f619dceac6f 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -31,6 +31,7 @@
> > #include <linux/cn_proc.h>
> > #include <linux/compat.h>
> > #include <linux/sched/signal.h>
> > +#include <linux/fdtable.h>
> >
> > #include <asm/syscall.h> /* for syscall_get_* */
> >
> > @@ -994,6 +995,33 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> > }
> > #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
> >
> > +static int ptrace_getfd(struct task_struct *child, unsigned long user_size,
> > + void __user *datavp)
> > +{
> > + struct ptrace_getfd_args args;
> > + unsigned int fd_flags = 0;
> > + struct file *file;
> > + int ret;
> > +
> > + ret = copy_struct_from_user(&args, sizeof(args), datavp, user_size);
> > + if (ret)
> > + goto out;
>
> Why is this goto out and not just return ret?
>
> > + if ((args.options & ~(PTRACE_GETFD_O_CLOEXEC)) != 0)
> > + return -EINVAL;
> > + if (args.options & PTRACE_GETFD_O_CLOEXEC)
> > + fd_flags &= O_CLOEXEC;
> > + file = get_task_file(child, args.fd);
> > + if (!file)
> > + return -EBADF;
> > + ret = get_unused_fd_flags(fd_flags);
>
> Why isn't that whole thing just:
>
> ret = get_unused_fd_flags(fd_flags & {PTRACE_GETFD_}O_CLOEXEC);
>
> > + if (ret >= 0)
> > + fd_install(ret, file);
> > + else
> > + fput(file);
> > +out:
> > + return ret;
> > +}
>
> So sm like:
>
> static int ptrace_getfd(struct task_struct *child, unsigned long user_size,
> void __user *datavp)
> {
> struct ptrace_getfd_args args;
> unsigned int fd_flags = 0;
> struct file *file;
> int ret;
>
> ret = copy_struct_from_user(&args, sizeof(args), datavp, user_size);
> if (ret)
> return ret;
>
> if ((args.options & ~(PTRACE_GETFD_O_CLOEXEC)) != 0)
> return -EINVAL;
>
> file = get_task_file(child, args.fd);
> if (!file)
> return -EBADF;
>
> /* PTRACE_GETFD_CLOEXEC == O_CLOEXEC */
> ret = get_unused_fd_flags(fd_flags & PTRACE_GETFD_O_CLOEXEC);
Wouldn't this always be 0, since fd_flags is always 0?
> if (ret >= 0)
> fd_install(ret, file);
> else
> fput(file);
>
> return ret;
> }
^ permalink raw reply
* Re: [PATCH v2 2/4] ptrace: add PTRACE_GETFD request to fetch file descriptors from tracees
From: Christian Brauner @ 2019-12-09 11:23 UTC (permalink / raw)
To: Sargun Dhillon
Cc: LKML, Linux Containers, Linux API, linux-fsdevel, Tycho Andersen,
Jann Horn, cyphar, oleg, Andy Lutomirski, viro
In-Reply-To: <CAMp4zn8_CxB6C=4Myw7DrmWg5w3Qm+FwYVTQLnbCEBJXL4UKzg@mail.gmail.com>
On Mon, Dec 09, 2019 at 02:55:40AM -0800, Sargun Dhillon wrote:
> On Mon, Dec 9, 2019 at 1:39 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > Hey Sargun,
> >
> > Thanks for the patch!
> Thanks for your review.
>
> >
> > Why not simply accept O_CLOEXEC as flag? If that's not possible for some
> > reason I'd say
> >
> I did this initially. My plan is to use this options field for other
> (future) things as well, like
> clearing (cgroup) metadata on sockets (assuming we figure out a safe
> way to do it).
> If we use O_CLOEXEC, it takes up an arbitrary bit which is different
> on different
> platforms, and working around that seems messy
>
> Another way around this would be to have two members. One member which
> is something
> like fdflags, that just takes the fd flags, like O_CLOEXEC, and then
> later on, we can add
> an options member to enable these future use cases.
That honestly sounds cleaner to me. I really don't like this flag
explosion for CLOEXEC. Every new kernel api we add that deals with fds
comes with their own set of CLOEXEC flags. The minimal thing to do is
to make sure that at least
NEW_CLOEXEC_THINGY == O_CLOEXEC.
>
> What do you think?
> > #define PTRACE_GETFD_O_CLOEXEC O_CLOEXEC /* open the fd with cloexec */
> >
> > is the right thing to do. This is fairly common:
> >
> > include/uapi/linux/timerfd.h:#define TFD_CLOEXEC O_CLOEXEC
> > include/uapi/drm/drm.h:#define DRM_CLOEXEC O_CLOEXEC
> > include/linux/userfaultfd_k.h:#define UFFD_CLOEXEC O_CLOEXEC
> > include/linux/eventfd.h:#define EFD_CLOEXEC O_CLOEXEC
> > include/uapi/linux/eventpoll.h:#define EPOLL_CLOEXEC O_CLOEXEC
> > include/uapi/linux/inotify.h:/* For O_CLOEXEC and O_NONBLOCK */
> > include/uapi/linux/inotify.h:#define IN_CLOEXEC O_CLOEXEC
> > include/uapi/linux/mount.h:#define OPEN_TREE_CLOEXEC O_CLOEXEC /* Close the file on execve() */
> >
> > You can also add a compile-time assert to ptrace like we did for
> > fs/namespace.c's OPEN_TREE_CLOEXEC:
> > BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
> >
> > And I'd remove the _O if you go with a separate flag, i.e.:
> >
> > #define PTRACE_GETFD_CLOEXEC O_CLOEXEC /* open the fd with cloexec */
> >
> > > +
> > > +struct ptrace_getfd_args {
> > > + __u32 fd; /* the tracee's file descriptor to get */
> > > + __u32 options;
> >
> > Nit and I'm not set on it at all but "flags" might just be better.
> >
> > > +} __attribute__((packed));
> >
> > What's the benefit in using __attribute__((packed)) here? Seems to me that:
> >
> 1) Are we always to assume that the compiler will give us 4-byte
> alignment (paranoia)
> 2) If we're to add new non-4-byte aligned members later on, is it
> kosher to add packed
> later on?
Using explicit padding is the more common way we do this (e.g. struct
open_how, struct statx and a lot more add explicit padding to guarantee
alignment.).
>
> > +struct ptrace_getfd_args {
> > + __u32 fd; /* the tracee's file descriptor to get */
> > + __u32 options;
> > +};
> >
> > would just work fine.
> >
> > > +
> > > /*
> > > * These values are stored in task->ptrace_message
> > > * by tracehook_report_syscall_* to describe the current syscall-stop.
> > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > > index cb9ddcc08119..8f619dceac6f 100644
> > > --- a/kernel/ptrace.c
> > > +++ b/kernel/ptrace.c
> > > @@ -31,6 +31,7 @@
> > > #include <linux/cn_proc.h>
> > > #include <linux/compat.h>
> > > #include <linux/sched/signal.h>
> > > +#include <linux/fdtable.h>
> > >
> > > #include <asm/syscall.h> /* for syscall_get_* */
> > >
> > > @@ -994,6 +995,33 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> > > }
> > > #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
> > >
> > > +static int ptrace_getfd(struct task_struct *child, unsigned long user_size,
> > > + void __user *datavp)
> > > +{
> > > + struct ptrace_getfd_args args;
> > > + unsigned int fd_flags = 0;
> > > + struct file *file;
> > > + int ret;
> > > +
> > > + ret = copy_struct_from_user(&args, sizeof(args), datavp, user_size);
> > > + if (ret)
> > > + goto out;
> >
> > Why is this goto out and not just return ret?
> >
> > > + if ((args.options & ~(PTRACE_GETFD_O_CLOEXEC)) != 0)
> > > + return -EINVAL;
> > > + if (args.options & PTRACE_GETFD_O_CLOEXEC)
> > > + fd_flags &= O_CLOEXEC;
> > > + file = get_task_file(child, args.fd);
> > > + if (!file)
> > > + return -EBADF;
> > > + ret = get_unused_fd_flags(fd_flags);
> >
> > Why isn't that whole thing just:
> >
> > ret = get_unused_fd_flags(fd_flags & {PTRACE_GETFD_}O_CLOEXEC);
> >
> > > + if (ret >= 0)
> > > + fd_install(ret, file);
> > > + else
> > > + fput(file);
> > > +out:
> > > + return ret;
> > > +}
> >
> > So sm like:
> >
> > static int ptrace_getfd(struct task_struct *child, unsigned long user_size,
> > void __user *datavp)
> > {
> > struct ptrace_getfd_args args;
> > unsigned int fd_flags = 0;
> > struct file *file;
> > int ret;
> >
> > ret = copy_struct_from_user(&args, sizeof(args), datavp, user_size);
> > if (ret)
> > return ret;
> >
> > if ((args.options & ~(PTRACE_GETFD_O_CLOEXEC)) != 0)
> > return -EINVAL;
> >
> > file = get_task_file(child, args.fd);
> > if (!file)
> > return -EBADF;
> >
> > /* PTRACE_GETFD_CLOEXEC == O_CLOEXEC */
> > ret = get_unused_fd_flags(fd_flags & PTRACE_GETFD_O_CLOEXEC);
> Wouldn't this always be 0, since fd_flags is always 0?
You're right, I missed this. Maybe rather:
if (args.options & PTRACE_GETFD_O_CLOEXEC)
fd_flags |= O_CLOEXEC;
Another question is if we shouldn't just make them cloexec by default?
The notifier fds and pidfds already are.
Christian
^ permalink raw reply
* Re: [v4 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
From: Vlastimil Babka @ 2019-12-09 15:21 UTC (permalink / raw)
To: Yang Shi, fabecassis, jhubbard, mhocko, cl, mgorman, akpm
Cc: linux-mm, linux-kernel, stable, Linux API
In-Reply-To: <1575584353-125392-1-git-send-email-yang.shi@linux.alibaba.com>
Boo, v4 and nobody suggested CCing linux-api yet?
Doing that and not trimming the reply, for reference.
On 12/5/19 11:19 PM, Yang Shi wrote:
> Felix Abecassis reports move_pages() would return random status if the
> pages are already on the target node by the below test program:
>
> ---8<---
>
> int main(void)
> {
> const long node_id = 1;
> const long page_size = sysconf(_SC_PAGESIZE);
> const int64_t num_pages = 8;
>
> unsigned long nodemask = 1 << node_id;
> long ret = set_mempolicy(MPOL_BIND, &nodemask, sizeof(nodemask));
> if (ret < 0)
> return (EXIT_FAILURE);
>
> void **pages = malloc(sizeof(void*) * num_pages);
> for (int i = 0; i < num_pages; ++i) {
> pages[i] = mmap(NULL, page_size, PROT_WRITE | PROT_READ,
> MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS,
> -1, 0);
> if (pages[i] == MAP_FAILED)
> return (EXIT_FAILURE);
> }
>
> ret = set_mempolicy(MPOL_DEFAULT, NULL, 0);
> if (ret < 0)
> return (EXIT_FAILURE);
>
> int *nodes = malloc(sizeof(int) * num_pages);
> int *status = malloc(sizeof(int) * num_pages);
> for (int i = 0; i < num_pages; ++i) {
> nodes[i] = node_id;
> status[i] = 0xd0; /* simulate garbage values */
> }
>
> ret = move_pages(0, num_pages, pages, nodes, status, MPOL_MF_MOVE);
> printf("move_pages: %ld\n", ret);
> for (int i = 0; i < num_pages; ++i)
> printf("status[%d] = %d\n", i, status[i]);
> }
> ---8<---
>
> Then running the program would return nonsense status values:
> $ ./move_pages_bug
> move_pages: 0
> status[0] = 208
> status[1] = 208
> status[2] = 208
> status[3] = 208
> status[4] = 208
> status[5] = 208
> status[6] = 208
> status[7] = 208
>
> This is because the status is not set if the page is already on the
> target node, but move_pages() should return valid status as long as it
> succeeds. The valid status may be errno or node id.
>
> We can't simply initialize status array to zero since the pages may be
> not on node 0. Fix it by updating status with node id which the page is
> already on.
>
> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
> Reported-by: Felix Abecassis <fabecassis@nvidia.com>
> Tested-by: Felix Abecassis <fabecassis@nvidia.com>
> Suggested-by: Michal Hocko <mhocko@suse.com>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> Acked-by: Christoph Lameter <cl@linux.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: <stable@vger.kernel.org> 4.17+
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> v4: * Fixed the comments from Christopher and John and added their Acked-by
> and Reviewed-by.
> v3: * Adopted the suggestion from Michal.
> v2: * Correted the return value when add_page_for_migration() returns 1.
>
> mm/migrate.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index a8f87cb..6b44818f 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1512,9 +1512,11 @@ static int do_move_pages_to_node(struct mm_struct *mm,
> /*
> * Resolves the given address to a struct page, isolates it from the LRU and
> * puts it to the given pagelist.
> - * Returns -errno if the page cannot be found/isolated or 0 when it has been
> - * queued or the page doesn't need to be migrated because it is already on
> - * the target node
> + * Returns:
> + * errno - if the page cannot be found/isolated
> + * 0 - when it doesn't have to be migrated because it is already on the
> + * target node
> + * 1 - when it has been queued
> */
> static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> int node, struct list_head *pagelist, bool migrate_all)
> @@ -1553,7 +1555,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> if (PageHuge(page)) {
> if (PageHead(page)) {
> isolate_huge_page(page, pagelist);
> - err = 0;
> + err = 1;
> }
> } else {
> struct page *head;
> @@ -1563,7 +1565,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> if (err)
> goto out_putpage;
>
> - err = 0;
> + err = 1;
> list_add_tail(&head->lru, pagelist);
> mod_node_page_state(page_pgdat(head),
> NR_ISOLATED_ANON + page_is_file_cache(head),
> @@ -1640,8 +1642,17 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> */
> err = add_page_for_migration(mm, addr, current_node,
> &pagelist, flags & MPOL_MF_MOVE_ALL);
> - if (!err)
> +
> + if (!err) {
> + /* The page is already on the target node */
> + err = store_status(status, i, current_node, 1);
> + if (err)
> + goto out_flush;
> continue;
> + } else if (err > 0) {
> + /* The page is successfully queued for migration */
> + continue;
> + }
>
> err = store_status(status, i, err, 1);
> if (err)
>
^ permalink raw reply
* Re: [PATCH v2 1/4] vfs, fdtable: Add get_task_file helper
From: Oleg Nesterov @ 2019-12-09 15:52 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, christian.brauner, luto, viro
In-Reply-To: <20191209070609.GA32438@ircssh-2.c.rugged-nimbus-611.internal>
On 12/09, Sargun Dhillon wrote:
>
> +struct file *get_task_file(struct task_struct *task, unsigned int fd)
> +{
> + struct file *file = NULL;
> +
> + task_lock(task);
> + rcu_read_lock();
> +
> + if (task->files) {
> + file = fcheck_files(task->files, fd);
> + if (file && !get_file_rcu(file))
> + file = NULL;
> + }
On second thought this is not exactly right, get_file_rcu() can fail if
get_task_file() races with dup2(), in this case we need to do fcheck_files()
again. And this is what __fget() already does, so may be the patch below
makes more sense?
I will leave this to other reviewers, but suddenly I recall that I have
already sent the patch which adds a similar helper a while ago.
See https://lore.kernel.org/lkml/20180915160423.GA31461@redhat.com/
In short, get_files_struct() should be avoided because it can race with
exec() and break POSIX locks which use ->fl_owner = files_struct.
Oleg.
--- x/fs/file.c
+++ x/fs/file.c
@@ -706,9 +706,9 @@ void do_close_on_exec(struct files_struc
spin_unlock(&files->file_lock);
}
-static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+static struct file *__fget_files(struct files_struct *files, unsigned int fd,
+ fmode_t mask, unsigned int refs)
{
- struct files_struct *files = current->files;
struct file *file;
rcu_read_lock();
@@ -729,6 +729,23 @@ loop:
return file;
}
+struct file *fget_task(struct task_struct *task, unsigned int fd)
+{
+ struct file *file;
+
+ task_lock(task);
+ if (task->files)
+ file = __fget_files(task->files, fd, 0, 1);
+ task_unlock(task);
+
+ return file;
+}
+
+static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+{
+ return __fget_files(current->files, fd, mask, refs);
+}
+
struct file *fget_many(unsigned int fd, unsigned int refs)
{
return __fget(fd, FMODE_PATH, refs);
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Oleg Nesterov @ 2019-12-09 19:30 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, christian.brauner, luto, viro
In-Reply-To: <20191209070646.GA32477@ircssh-2.c.rugged-nimbus-611.internal>
On 12/09, Sargun Dhillon wrote:
>
> +#define CHILD_PORT_TRY_BIND 80
> +#define CHILD_PORT_ACTUAL_BIND 4998
...
> +static int handle_req(int listener)
> +{
> + struct sockaddr_in addr = {
> + .sin_family = AF_INET,
> + .sin_port = htons(4998),
then I think
.sin_port = htons(CHILD_PORT_ACTUAL_BIND);
would be more clear...
> + .sin_addr = {
> + .s_addr = htonl(INADDR_LOOPBACK)
> + }
> + };
> + struct ptrace_getfd_args getfd_args = {
> + .options = PTRACE_GETFD_O_CLOEXEC
> + };
> + struct seccomp_notif_sizes sizes;
> + struct seccomp_notif_resp *resp;
> + struct seccomp_notif *req;
> + int fd, ret = 1;
> +
> + if (seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes) < 0) {
> + perror("seccomp(GET_NOTIF_SIZES)");
> + goto out;
> + }
> + req = malloc(sizes.seccomp_notif);
> + if (!req)
> + goto out;
> + memset(req, 0, sizeof(*req));
> +
> + resp = malloc(sizes.seccomp_notif_resp);
> + if (!resp)
> + goto out_free_req;
> + memset(resp, 0, sizeof(*resp));
> +
> + if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
> + perror("ioctl recv");
> + goto out;
> + }
> + printf("Child tried to call bind with fd: %lld\n", req->data.args[0]);
> + getfd_args.fd = req->data.args[0];
> + fd = ptrace_getfd(req->pid, &getfd_args);
and iiuc otherwise you do not need to ptrace the child. So you could remove
ptrace(PTRACE_SEIZE) in main() and just do
ptrace(PTRACE_SEIZE, req->pid);
fd = ptrace_getfd(req->pid, &getfd_args);
ptrace(PTRACE_DETACH, req->pid);
here. However, PTRACE_DETACH won't work, it needs the stopped tracee. We can
add PTRACE_DETACH_ASYNC, but this makes me think that PTRACE_GETFD has nothing
to do with ptrace.
May be a new syscall which does ptrace_may_access() + get_task_file() will make
more sense?
Oleg.
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Christian Brauner @ 2019-12-09 19:49 UTC (permalink / raw)
To: Oleg Nesterov, Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, luto, viro
In-Reply-To: <20191209192959.GB10721@redhat.com>
On December 9, 2019 8:30:00 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com> wrote:
>On 12/09, Sargun Dhillon wrote:
>>
>> +#define CHILD_PORT_TRY_BIND 80
>> +#define CHILD_PORT_ACTUAL_BIND 4998
>
>...
>
>> +static int handle_req(int listener)
>> +{
>> + struct sockaddr_in addr = {
>> + .sin_family = AF_INET,
>> + .sin_port = htons(4998),
>
>then I think
> .sin_port = htons(CHILD_PORT_ACTUAL_BIND);
>
>would be more clear...
>
>> + .sin_addr = {
>> + .s_addr = htonl(INADDR_LOOPBACK)
>> + }
>> + };
>> + struct ptrace_getfd_args getfd_args = {
>> + .options = PTRACE_GETFD_O_CLOEXEC
>> + };
>> + struct seccomp_notif_sizes sizes;
>> + struct seccomp_notif_resp *resp;
>> + struct seccomp_notif *req;
>> + int fd, ret = 1;
>> +
>> + if (seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes) < 0) {
>> + perror("seccomp(GET_NOTIF_SIZES)");
>> + goto out;
>> + }
>> + req = malloc(sizes.seccomp_notif);
>> + if (!req)
>> + goto out;
>> + memset(req, 0, sizeof(*req));
>> +
>> + resp = malloc(sizes.seccomp_notif_resp);
>> + if (!resp)
>> + goto out_free_req;
>> + memset(resp, 0, sizeof(*resp));
>> +
>> + if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
>> + perror("ioctl recv");
>> + goto out;
>> + }
>> + printf("Child tried to call bind with fd: %lld\n",
>req->data.args[0]);
>> + getfd_args.fd = req->data.args[0];
>> + fd = ptrace_getfd(req->pid, &getfd_args);
>
>and iiuc otherwise you do not need to ptrace the child. So you could
>remove
>ptrace(PTRACE_SEIZE) in main() and just do
>
> ptrace(PTRACE_SEIZE, req->pid);
> fd = ptrace_getfd(req->pid, &getfd_args);
> ptrace(PTRACE_DETACH, req->pid);
>
>here. However, PTRACE_DETACH won't work, it needs the stopped tracee.
>We can
>add PTRACE_DETACH_ASYNC, but this makes me think that PTRACE_GETFD has
>nothing
>to do with ptrace.
>
>May be a new syscall which does ptrace_may_access() + get_task_file()
>will make
>more sense?
>
>Oleg.
Once more since this annoying app uses html by default...
But we can already do this right now and this is just an improvement.
That's a bit rich for a new syscall imho...
Christian
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Oleg Nesterov @ 2019-12-09 20:46 UTC (permalink / raw)
To: Christian Brauner
Cc: Sargun Dhillon, linux-kernel, containers, linux-api,
linux-fsdevel, tycho, jannh, cyphar, luto, viro
In-Reply-To: <BE3E056F-0147-4A00-8FF7-6CC9DE02A30C@ubuntu.com>
On 12/09, Christian Brauner wrote:
>
> >We can
> >add PTRACE_DETACH_ASYNC, but this makes me think that PTRACE_GETFD has
> >nothing
> >to do with ptrace.
> >
> >May be a new syscall which does ptrace_may_access() + get_task_file()
> >will make
> >more sense?
> >
> >Oleg.
>
> Once more since this annoying app uses html by default...
>
> But we can already do this right now and this is just an improvement.
> That's a bit rich for a new syscall imho...
I agree, and I won't really argue...
but the changelog in 2/4 says
The requirement that the tracer has attached to the tracee prior to the
capture of the file descriptor may be lifted at a later point.
so may be we should do this right now?
plus this part
@@ -1265,7 +1295,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
}
ret = ptrace_check_attach(child, request == PTRACE_KILL ||
- request == PTRACE_INTERRUPT);
+ request == PTRACE_INTERRUPT ||
+ request == PTRACE_GETFD);
actually means "we do not need ptrace, but we do not know where else we
can add this fd_install(get_task_file()).
Oleg.
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in mem16_serial_out
From: syzbot @ 2019-12-10 1:38 UTC (permalink / raw)
To: andriy.shevchenko, asierra, corbet, ext-kimmo.rautkoski, gregkh,
jslaby, kai.heng.feng, linux-api, linux-doc, linux-kernel,
linux-serial, mika.westerberg, paulburton, peter, sr,
syzkaller-bugs, yamada.masahiro, yegorslists
In-Reply-To: <00000000000044a65205994a7e13@google.com>
syzbot has bisected this bug to:
commit bd94c4077a0b2ecc35562c294f80f3659ecd8499
Author: Masahiro Yamada <yamada.masahiro@socionext.com>
Date: Wed Oct 28 03:46:05 2015 +0000
serial: support 16-bit register interface for console
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=13723196e00000
start commit: e42617b8 Linux 5.5-rc1
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=10f23196e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=17723196e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=3754e2c78c1adb82
dashboard link: https://syzkaller.appspot.com/bug?extid=92f32d4e21fb246d31a2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=136f7e41e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=112b7c82e00000
Reported-by: syzbot+92f32d4e21fb246d31a2@syzkaller.appspotmail.com
Fixes: bd94c4077a0b ("serial: support 16-bit register interface for
console")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH NOTFORMERGE 0/5] Extend remote madvise API to KSM hints
From: Oleksandr Natalenko @ 2019-12-10 10:49 UTC (permalink / raw)
To: Minchan Kim; +Cc: linux-kernel, linux-mm, linux-api
In-Reply-To: <20190616085835.953-1-oleksandr@redhat.com>
Hello, Minchan.
On Sun, Jun 16, 2019 at 10:58:30AM +0200, Oleksandr Natalenko wrote:
> This is a set of commits based on our discussion on your submission [1].
>
> First 2 implement minor suggestions just for you to not forget to take
> them into account.
>
> uio.h inclusion was needed for me to be able to compile your series
> successfully. Also please note I had to enable "Transparent Hugepage
> Support" as well as "Enable idle page tracking" options, otherwise the
> build failed. I guess this can be addressed by you better since the
> errors are introduced with MADV_COLD introduction.
>
> Last 2 commits are the actual KSM hints enablement. The first one
> implements additional check for the case where the mmap_sem is taken for
> write, and the second one just allows KSM hints to be used by the remote
> interface.
>
> I'm not Cc'ing else anyone except two mailing lists to not distract
> people unnecessarily. If you are fine with this addition, please use it
> for your next iteration of process_madvise(), and then you'll Cc all the
> people needed.
>
> Thanks.
>
> [1] https://lore.kernel.org/lkml/20190531064313.193437-1-minchan@kernel.org/
>
> Oleksandr Natalenko (5):
> mm: rename madvise_core to madvise_common
> mm: revert madvise_inject_error line split
> mm: include uio.h to madvise.c
> mm/madvise: employ mmget_still_valid for write lock
> mm/madvise: allow KSM hints for remote API
>
> mm/madvise.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> --
> 2.22.0
>
This is a gentle ping. Are you still planning to submit process_madvise() solution?
--
Best regards,
Oleksandr Natalenko (post-factum)
Senior Software Maintenance Engineer
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Christian Brauner @ 2019-12-10 11:10 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Sargun Dhillon, linux-kernel, containers, linux-api,
linux-fsdevel, tycho, jannh, cyphar, luto, viro, Jed Davis,
Gian-Carlo Pascutto, Emilio Cobos Álvarez, Florian Weimer
In-Reply-To: <20191209204635.GC10721@redhat.com>
[I'm expanding the Cc to a few Firefox and glibc people since we've been
been talking about replacing SECCOMP_RET_TRAP with
SECCOMP_RET_USER_NOTIF for a bit now because the useage of
SECCOMP_RET_TRAP in the broker blocks desirable core glibc changes.
Even if just for their lurking pleasure. :)]
On Mon, Dec 09, 2019 at 09:46:35PM +0100, Oleg Nesterov wrote:
> On 12/09, Christian Brauner wrote:
> >
> > >We can
> > >add PTRACE_DETACH_ASYNC, but this makes me think that PTRACE_GETFD has
> > >nothing
> > >to do with ptrace.
> > >
> > >May be a new syscall which does ptrace_may_access() + get_task_file()
> > >will make
> > >more sense?
> > >
> > >Oleg.
> >
> > Once more since this annoying app uses html by default...
> >
> > But we can already do this right now and this is just an improvement.
> > That's a bit rich for a new syscall imho...
>
> I agree, and I won't really argue...
>
> but the changelog in 2/4 says
>
> The requirement that the tracer has attached to the tracee prior to the
> capture of the file descriptor may be lifted at a later point.
>
> so may be we should do this right now?
I think so, yes. This doesn't strike me as premature optimization but
rather as a core design questions.
>
> plus this part
>
> @@ -1265,7 +1295,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
> }
>
> ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> - request == PTRACE_INTERRUPT);
> + request == PTRACE_INTERRUPT ||
> + request == PTRACE_GETFD);
>
> actually means "we do not need ptrace, but we do not know where else we
> can add this fd_install(get_task_file()).
Right, I totally get your point and I'm not a fan of this being in
ptrace() either.
The way I see is is that the main use-case for this feature is the
seccomp notifier and I can see this being useful. So the right place to
plumb this into might just be seccomp and specifically on to of the
notifier.
If we don't care about getting and setting fds at random points of
execution it might make sense to add new options to the notify ioctl():
#define SECCOMP_IOCTL_NOTIF_GET_FD SECCOMP_IOWR(3, <sensible struct>)
#define SECCOMP_IOCTL_NOTIF_SET_FD SECCOMP_IOWR(4, <sensible struct>)
which would let you get and set fds while the supervisee is blocked.
Christian
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Tycho Andersen @ 2019-12-10 15:35 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Sargun Dhillon, linux-kernel, containers,
linux-api, linux-fsdevel, jannh, cyphar, luto, viro, Jed Davis,
Gian-Carlo Pascutto, Emilio Cobos Álvarez, Florian Weimer
In-Reply-To: <20191210111051.j5opodgjalqigx6q@wittgenstein>
On Tue, Dec 10, 2019 at 12:10:52PM +0100, Christian Brauner wrote:
>
> #define SECCOMP_IOCTL_NOTIF_SET_FD SECCOMP_IOWR(4, <sensible struct>)
There's even some code already for this one:
https://lore.kernel.org/linux-fsdevel/20180927151119.9989-6-tycho@tycho.ws/
Tycho
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Sargun Dhillon @ 2019-12-10 16:07 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, LKML, Linux Containers, Linux API, linux-fsdevel,
Tycho Andersen, Jann Horn, cyphar, Andy Lutomirski, viro,
Jed Davis, Gian-Carlo Pascutto, Emilio Cobos Álvarez,
Florian Weimer
In-Reply-To: <20191210111051.j5opodgjalqigx6q@wittgenstein>
On Tue, Dec 10, 2019 at 3:10 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> [I'm expanding the Cc to a few Firefox and glibc people since we've been
> been talking about replacing SECCOMP_RET_TRAP with
> SECCOMP_RET_USER_NOTIF for a bit now because the useage of
> SECCOMP_RET_TRAP in the broker blocks desirable core glibc changes.
> Even if just for their lurking pleasure. :)]
>
> On Mon, Dec 09, 2019 at 09:46:35PM +0100, Oleg Nesterov wrote:
> > On 12/09, Christian Brauner wrote
> >
> > I agree, and I won't really argue...
> >
> > but the changelog in 2/4 says
> >
> > The requirement that the tracer has attached to the tracee prior to the
> > capture of the file descriptor may be lifted at a later point.
> >
> > so may be we should do this right now?
>
> I think so, yes. This doesn't strike me as premature optimization but
> rather as a core design questions.
>
> >
> > plus this part
> >
> > @@ -1265,7 +1295,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
> > }
> >
> > ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> > - request == PTRACE_INTERRUPT);
> > + request == PTRACE_INTERRUPT ||
> > + request == PTRACE_GETFD);
> >
> > actually means "we do not need ptrace, but we do not know where else we
> > can add this fd_install(get_task_file()).
>
> Right, I totally get your point and I'm not a fan of this being in
> ptrace() either.
>
> The way I see is is that the main use-case for this feature is the
> seccomp notifier and I can see this being useful. So the right place to
> plumb this into might just be seccomp and specifically on to of the
> notifier.
> If we don't care about getting and setting fds at random points of
> execution it might make sense to add new options to the notify ioctl():
>
> #define SECCOMP_IOCTL_NOTIF_GET_FD SECCOMP_IOWR(3, <sensible struct>)
> #define SECCOMP_IOCTL_NOTIF_SET_FD SECCOMP_IOWR(4, <sensible struct>)
>
> which would let you get and set fds while the supervisee is blocked.
>
> Christian
Doesn't SECCOMP_IOCTL_NOTIF_GET_FD have some ambiguity to it?
Specifically, because
multiple processes can have the same notifier attached to them? If we
choose to go down the
route of introducing an ioctl (which I'm not at all opposed to), I
would rather do it on pidfd. We
can then plumb seccomp notifier to send pidfd instead of raw pid. In
the mean time, folks
can just open up /proc/${PID}, and do the check cookie dance.
Christian,
As the maintainer of pidfd, what do you think?
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Tycho Andersen @ 2019-12-10 16:13 UTC (permalink / raw)
To: Sargun Dhillon
Cc: Christian Brauner, Oleg Nesterov, LKML, Linux Containers,
Linux API, linux-fsdevel, Jann Horn, cyphar, Andy Lutomirski,
viro, Jed Davis, Gian-Carlo Pascutto, Emilio Cobos Álvarez,
Florian Weimer
In-Reply-To: <CAMp4zn84YQHz62x-nxZFBgMEW9AiMt75q_rO83uaGg=YtyKV-w@mail.gmail.com>
On Tue, Dec 10, 2019 at 08:07:45AM -0800, Sargun Dhillon wrote:
> On Tue, Dec 10, 2019 at 3:10 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > [I'm expanding the Cc to a few Firefox and glibc people since we've been
> > been talking about replacing SECCOMP_RET_TRAP with
> > SECCOMP_RET_USER_NOTIF for a bit now because the useage of
> > SECCOMP_RET_TRAP in the broker blocks desirable core glibc changes.
> > Even if just for their lurking pleasure. :)]
> >
> > On Mon, Dec 09, 2019 at 09:46:35PM +0100, Oleg Nesterov wrote:
> > > On 12/09, Christian Brauner wrote
> > >
> > > I agree, and I won't really argue...
> > >
> > > but the changelog in 2/4 says
> > >
> > > The requirement that the tracer has attached to the tracee prior to the
> > > capture of the file descriptor may be lifted at a later point.
> > >
> > > so may be we should do this right now?
> >
> > I think so, yes. This doesn't strike me as premature optimization but
> > rather as a core design questions.
> >
> > >
> > > plus this part
> > >
> > > @@ -1265,7 +1295,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
> > > }
> > >
> > > ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> > > - request == PTRACE_INTERRUPT);
> > > + request == PTRACE_INTERRUPT ||
> > > + request == PTRACE_GETFD);
> > >
> > > actually means "we do not need ptrace, but we do not know where else we
> > > can add this fd_install(get_task_file()).
> >
> > Right, I totally get your point and I'm not a fan of this being in
> > ptrace() either.
> >
> > The way I see is is that the main use-case for this feature is the
> > seccomp notifier and I can see this being useful. So the right place to
> > plumb this into might just be seccomp and specifically on to of the
> > notifier.
> > If we don't care about getting and setting fds at random points of
> > execution it might make sense to add new options to the notify ioctl():
> >
> > #define SECCOMP_IOCTL_NOTIF_GET_FD SECCOMP_IOWR(3, <sensible struct>)
> > #define SECCOMP_IOCTL_NOTIF_SET_FD SECCOMP_IOWR(4, <sensible struct>)
> >
> > which would let you get and set fds while the supervisee is blocked.
> >
> > Christian
> Doesn't SECCOMP_IOCTL_NOTIF_GET_FD have some ambiguity to it?
> Specifically, because
> multiple processes can have the same notifier attached to them?
The id member corresponds to a particular syscall from a particular
pid, which makes it unique.
> If we
> choose to go down the
> route of introducing an ioctl (which I'm not at all opposed to), I
> would rather do it on pidfd. We
> can then plumb seccomp notifier to send pidfd instead of raw pid. In
> the mean time, folks
> can just open up /proc/${PID}, and do the check cookie dance.
This might be more generally useful, the problem is synchronization, I
guess.
Tycho
^ permalink raw reply
* Re: [PATCH v2 4/4] samples: Add example of using PTRACE_GETFD in conjunction with user trap
From: Christian Brauner @ 2019-12-10 16:38 UTC (permalink / raw)
To: Sargun Dhillon
Cc: Oleg Nesterov, LKML, Linux Containers, Linux API, linux-fsdevel,
Tycho Andersen, Jann Horn, cyphar, Andy Lutomirski, viro,
Jed Davis, Gian-Carlo Pascutto, Emilio Cobos Álvarez,
Florian Weimer
In-Reply-To: <CAMp4zn84YQHz62x-nxZFBgMEW9AiMt75q_rO83uaGg=YtyKV-w@mail.gmail.com>
On Tue, Dec 10, 2019 at 08:07:45AM -0800, Sargun Dhillon wrote:
> On Tue, Dec 10, 2019 at 3:10 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > [I'm expanding the Cc to a few Firefox and glibc people since we've been
> > been talking about replacing SECCOMP_RET_TRAP with
> > SECCOMP_RET_USER_NOTIF for a bit now because the useage of
> > SECCOMP_RET_TRAP in the broker blocks desirable core glibc changes.
> > Even if just for their lurking pleasure. :)]
> >
> > On Mon, Dec 09, 2019 at 09:46:35PM +0100, Oleg Nesterov wrote:
> > > On 12/09, Christian Brauner wrote
> > >
> > > I agree, and I won't really argue...
> > >
> > > but the changelog in 2/4 says
> > >
> > > The requirement that the tracer has attached to the tracee prior to the
> > > capture of the file descriptor may be lifted at a later point.
> > >
> > > so may be we should do this right now?
> >
> > I think so, yes. This doesn't strike me as premature optimization but
> > rather as a core design questions.
> >
> > >
> > > plus this part
> > >
> > > @@ -1265,7 +1295,8 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
> > > }
> > >
> > > ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> > > - request == PTRACE_INTERRUPT);
> > > + request == PTRACE_INTERRUPT ||
> > > + request == PTRACE_GETFD);
> > >
> > > actually means "we do not need ptrace, but we do not know where else we
> > > can add this fd_install(get_task_file()).
> >
> > Right, I totally get your point and I'm not a fan of this being in
> > ptrace() either.
> >
> > The way I see is is that the main use-case for this feature is the
> > seccomp notifier and I can see this being useful. So the right place to
> > plumb this into might just be seccomp and specifically on to of the
> > notifier.
> > If we don't care about getting and setting fds at random points of
> > execution it might make sense to add new options to the notify ioctl():
> >
> > #define SECCOMP_IOCTL_NOTIF_GET_FD SECCOMP_IOWR(3, <sensible struct>)
> > #define SECCOMP_IOCTL_NOTIF_SET_FD SECCOMP_IOWR(4, <sensible struct>)
> >
> > which would let you get and set fds while the supervisee is blocked.
> >
> > Christian
> Doesn't SECCOMP_IOCTL_NOTIF_GET_FD have some ambiguity to it?
As Tycho mentioned, this is why we have a the tid of the calling task
but we also have a cookie per request.
The cookie is useful so that you can do
- receive request <chocolate> cookie
- open(/proc/<pid>{/mem})
- verify <chocolate> cookie still exists
- <chocolate> cookie still exists -> file descriptor refers to correct
task
- <chocolate> cookie gone -> task has been recycled
> Specifically, because
> multiple processes can have the same notifier attached to them? If we
> choose to go down the
> route of introducing an ioctl (which I'm not at all opposed to), I
> would rather do it on pidfd. We
> can then plumb seccomp notifier to send pidfd instead of raw pid. In
> the mean time, folks
> can just open up /proc/${PID}, and do the check cookie dance.
>
> Christian,
> As the maintainer of pidfd, what do you think?
Let me quote what I wrote to the Mozilla folks today. :)
"(One thing that always strikes me is that if my pidfd patches would've
been ready back when we did the seccomp notifier we could've added a
pidfd argument to the seccomp notifier kernel struct and if a flag is
set given back a pidfd alongside the notifier fd. This way none of this
revalidting the id stuff would've been necessary and you could also
safely translate from a pidfd into a /proc/<pid> directory to e.g. open
/proc/<pid>/mem. Anyway, that's not out of scope. One could still
write a patch for that to add a pidfd argument under a new flag to the
kernel struct. Should be rather trivial.)"
So yeah, it crossed my mind. ;)
I really would like to have this placed under a flag though...
I very much dislike the idea of receiving any kind of fd
- _especially a pidfd_ - implicitly.
So ideally this would be a flag to the receive ioctl(). Kees just got my
SECCOMP_USER_NOTIF_FLAG_CONTINUE patchset merged for v5.5 which adds the
#define SECCOMP_USER_NOTIF_FLAG_CONTINUE (1UL << 0)
flag which when set in the send case (i.e. supervisor -> kernel) will
cause the syscall to be executed.
When we add a new flag to get a pidfd it might make sense to rename the
CONTINUE flag in master before v5.5 is out to
#define SECCOMP_USER_NOTIF_SEND_FLAG_CONTINUE (1UL << 0)
to indicate that it's only valid for the SEND ioctl().
Then we add
#define SECCOMP_USER_NOTIF_RECV_FLAG_PIDFD (1UL << 0)
for v5.6. This way send and receive flags are named differently for
clarity. (I don't care about the name being long. Other people might
though _shrug_.)
Christian
^ permalink raw reply
* Re: [PATCH NOTFORMERGE 0/5] Extend remote madvise API to KSM hints
From: Minchan Kim @ 2019-12-11 1:35 UTC (permalink / raw)
To: Oleksandr Natalenko; +Cc: linux-kernel, linux-mm, linux-api
In-Reply-To: <20191210104939.jauw5hnv3smhtvtr@butterfly.localdomain>
Hi Oleksandr,
On Tue, Dec 10, 2019 at 11:49:39AM +0100, Oleksandr Natalenko wrote:
> Hello, Minchan.
>
> On Sun, Jun 16, 2019 at 10:58:30AM +0200, Oleksandr Natalenko wrote:
> > This is a set of commits based on our discussion on your submission [1].
> >
> > First 2 implement minor suggestions just for you to not forget to take
> > them into account.
> >
> > uio.h inclusion was needed for me to be able to compile your series
> > successfully. Also please note I had to enable "Transparent Hugepage
> > Support" as well as "Enable idle page tracking" options, otherwise the
> > build failed. I guess this can be addressed by you better since the
> > errors are introduced with MADV_COLD introduction.
> >
> > Last 2 commits are the actual KSM hints enablement. The first one
> > implements additional check for the case where the mmap_sem is taken for
> > write, and the second one just allows KSM hints to be used by the remote
> > interface.
> >
> > I'm not Cc'ing else anyone except two mailing lists to not distract
> > people unnecessarily. If you are fine with this addition, please use it
> > for your next iteration of process_madvise(), and then you'll Cc all the
> > people needed.
> >
> > Thanks.
> >
> > [1] https://lore.kernel.org/lkml/20190531064313.193437-1-minchan@kernel.org/
> >
> > Oleksandr Natalenko (5):
> > mm: rename madvise_core to madvise_common
> > mm: revert madvise_inject_error line split
> > mm: include uio.h to madvise.c
> > mm/madvise: employ mmget_still_valid for write lock
> > mm/madvise: allow KSM hints for remote API
> >
> > mm/madvise.c | 23 ++++++++++++++---------
> > 1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > --
> > 2.22.0
> >
>
> This is a gentle ping. Are you still planning to submit process_madvise() solution?
I'm really sorry for being slow progress.
I am stuck with internal stuff of company.
I will do best effort to send it within one or two weeks.
Thanks.
^ 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