* Re: [GIT PULL] pipe: Notification queue preparation
From: Linus Torvalds @ 2019-12-05 17:33 UTC (permalink / raw)
To: David Sterba, David Howells, Linus Torvalds, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, raven, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <20191205172127.GW2734@suse.cz>
On Thu, Dec 5, 2019 at 9:22 AM David Sterba <dsterba@suse.cz> wrote:
>
> I rerun the test again (with a different address where it's stuck), there's
> nothing better I can get from the debug info, it always points to pipe_wait,
> disassembly points to.
Hah. I see another bug.
"pipe_wait()" depends on the fact that all events that wake it up
happen with the pipe lock held.
But we do some of the "do_wakeup()" handling outside the pipe lock now
on the reader side
__pipe_unlock(pipe);
/* Signal writers asynchronously that there is more room. */
if (do_wakeup) {
wake_up_interruptible_poll(&pipe->wait, EPOLLOUT | EPOLLWRNORM);
kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
}
However, that isn't new to this series _either_, so I don't think
that's it. It does wake up things inside the lock _too_ if it ended up
emptying a whole buffer.
So it could be triggered by timing and behavior changes, but I doubt
this pipe_wait() thing is it either. The fact that it bisects to the
thing that changes things to use head/tail pointers makes me think
there's some other incorrect update or comparison somewhere.
That said, "pipe_wait()" is an abomination. It should use a proper
wait condition and use wait_event(), but the code predates all of
that. I suspect pipe_wait() goes back to the dark ages with the BKL
and no actual races between kernel code.
Linus
^ permalink raw reply
* Re: [GIT PULL] pipe: Notification queue preparation
From: David Sterba @ 2019-12-05 18:18 UTC (permalink / raw)
To: David Howells
Cc: torvalds, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
raven, Christian Brauner, keyrings, linux-usb, linux-block,
linux-security-module, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <21493.1575566720@warthog.procyon.org.uk>
On Thu, Dec 05, 2019 at 05:25:20PM +0000, David Howells wrote:
> I've just posted a couple of patches - can you check to see if they fix your
> problem?
>
> https://lore.kernel.org/linux-fsdevel/157556649610.20869.8537079649495343567.stgit@warthog.procyon.org.uk/T/#t
Not fixed, the test still hangs with the same call stack.
^ permalink raw reply
* Re: [RFC PATCH v3 00/12] fs: interface for directly reading/writing compressed data
From: Omar Sandoval @ 2019-12-05 18:58 UTC (permalink / raw)
To: linux-fsdevel, linux-btrfs, Al Viro
Cc: Dave Chinner, Jann Horn, Amir Goldstein, Aleksa Sarai, linux-api,
kernel-team
In-Reply-To: <cover.1574273658.git.osandov@fb.com>
On Wed, Nov 20, 2019 at 10:24:20AM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> Hello,
>
> This series adds an API for reading compressed data on a filesystem
> without decompressing it as well as support for writing compressed data
> directly to the filesystem. As with the previous submissions, I've
> included a man page patch describing the API, and test cases and example
> programs are available [1].
>
> This version reworks the VFS interface to be backward and forward
> compatible and support for writing inline and bookend extents to the
> Btrfs implementation.
>
> Patches 1-3 add the VFS support. Patches 4-7 are Btrfs cleanups
> necessary for the encoded I/O support that can go in independently of
> this series. Patches 8-10 are Btrfs prep patches. Patch 11 adds Btrfs
> encoded read support and patch 12 adds Btrfs encoded write support.
>
> A few TODOs remain:
>
> - Once we've settled on the interface, I'll add RWF_ENCODED support to
> fsstress and friends and send up the xfstests patches in [1].
> - btrfs_encoded_read() still doesn't implement repair.
>
> Changes from v2 [2]:
>
> - Rebase on v5.4-rc8
> - Add patch 1 introducing copy_struct_from_iter() as suggested by Aleksa
> - Rename O_ENCODED to O_ALLOW_ENCODED as suggested by Amir
> - Add arch-specific definitions of O_ALLOW_ENCODED for alpha, parisc,
> and sparc
> - Rework the VFS interface to be backward and forward compatible
> - Document the VFS interface as requested by Dave
> - Use __aligned_u64 for struct encoded_iov as noted by Aleksa
> - Fix len/unencoded_len mixup in mm/filemap.c as noted by Nikolay
> - Add support for writing inline and bookend extents to Btrfs
> - Use ENOBUFS for "buffers not big enough for encoded extent" case and
> E2BIG for "encoded_iov has unsupported fields" case
>
> Please share any comments on the API or implementation. Thanks!
>
> 1: https://github.com/osandov/xfstests/tree/rwf-encoded
> 2: https://lore.kernel.org/linux-btrfs/cover.1571164762.git.osandov@fb.com/
>
> Omar Sandoval (12):
> iov_iter: add copy_struct_from_iter()
> fs: add O_ALLOW_ENCODED open flag
> fs: add RWF_ENCODED for reading/writing compressed data
> btrfs: get rid of trivial __btrfs_lookup_bio_sums() wrappers
> btrfs: don't advance offset for compressed bios in
> btrfs_csum_one_bio()
> btrfs: remove dead snapshot-aware defrag code
> btrfs: make btrfs_ordered_extent naming consistent with
> btrfs_file_extent_item
> btrfs: add ram_bytes and offset to btrfs_ordered_extent
> btrfs: support different disk extent size for delalloc
> btrfs: optionally extend i_size in cow_file_range_inline()
> btrfs: implement RWF_ENCODED reads
> btrfs: implement RWF_ENCODED writes
>
> Documentation/filesystems/encoded_io.rst | 79 +
> Documentation/filesystems/index.rst | 1 +
> arch/alpha/include/uapi/asm/fcntl.h | 1 +
> arch/parisc/include/uapi/asm/fcntl.h | 1 +
> arch/sparc/include/uapi/asm/fcntl.h | 1 +
> fs/btrfs/compression.c | 15 +-
> fs/btrfs/compression.h | 5 +-
> fs/btrfs/ctree.h | 13 +-
> fs/btrfs/delalloc-space.c | 38 +-
> fs/btrfs/delalloc-space.h | 4 +-
> fs/btrfs/file-item.c | 54 +-
> fs/btrfs/file.c | 61 +-
> fs/btrfs/inode.c | 2463 +++++++++++-----------
> fs/btrfs/ordered-data.c | 106 +-
> fs/btrfs/ordered-data.h | 28 +-
> fs/btrfs/relocation.c | 9 +-
> fs/fcntl.c | 10 +-
> fs/namei.c | 4 +
> include/linux/fcntl.h | 2 +-
> include/linux/fs.h | 16 +
> include/linux/uio.h | 2 +
> include/trace/events/btrfs.h | 6 +-
> include/uapi/asm-generic/fcntl.h | 4 +
> include/uapi/linux/fs.h | 33 +-
> lib/iov_iter.c | 82 +
> mm/filemap.c | 165 +-
> 26 files changed, 1807 insertions(+), 1396 deletions(-)
> create mode 100644 Documentation/filesystems/encoded_io.rst
Ping. Al, would you mind taking a look at the generic bits/interface?
^ permalink raw reply
* Re: [GIT PULL] pipe: General notification queue
From: Linus Torvalds @ 2019-12-05 20:26 UTC (permalink / raw)
To: David Howells
Cc: Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <31555.1574810303@warthog.procyon.org.uk>
On Tue, Nov 26, 2019 at 3:18 PM David Howells <dhowells@redhat.com> wrote:
>
> Can you consider pulling my general notification queue patchset after
> you've pulled the preparatory pipework patchset? Or should it be deferred
> to the next window?
So it's perhaps obvious by now, but I had delayed this pull request
because I was waiting to see if there were any reports of issues with
the core pipe changes.
And considering that there clearly _is_ something going on with the
pipe changes, I'm not going to pull this for this merge window.
I'm obviously hoping that we'll figure out what the btrfs-test issue
is asap, but even if we do, it's too late to pull stuff on top of our
current situation right now.
I suspect this is what you expected anyway (considering your own query
about the next merge window), but I thought I'd reply to it explicitly
since I had kept this pull request in my "maybe" queue, but with the
pipe thread from this morning it's dropped from that.
Linus
^ permalink raw reply
* Re: [PATCH] Add prctl support for controlling mem reclaim V4
From: Shakeel Butt @ 2019-12-05 22:43 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-api, idryomov, Michal Hocko, david, Linux MM, LKML,
linux-scsi, linux-fsdevel, linux-block, martin, Damien.LeMoal,
Mike Christie, Michal Hocko, Masato Suzuki
In-Reply-To: <20191112001900.9206-1-mchristi@redhat.com>
On Mon, Nov 11, 2019 at 4:19 PM Mike Christie <mchristi@redhat.com> wrote:
>
> There are several storage drivers like dm-multipath, iscsi, tcmu-runner,
> amd nbd that have userspace components that can run in the IO path. For
> example, iscsi and nbd's userspace deamons may need to recreate a socket
> and/or send IO on it, and dm-multipath's daemon multipathd may need to
> send SG IO or read/write IO to figure out the state of paths and re-set
> them up.
>
> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the
> memalloc_*_save/restore functions to control the allocation behavior,
> but for userspace we would end up hitting an allocation that ended up
> writing data back to the same device we are trying to allocate for.
> The device is then in a state of deadlock, because to execute IO the
> device needs to allocate memory, but to allocate memory the memory
> layers want execute IO to the device.
>
> Here is an example with nbd using a local userspace daemon that performs
> network IO to a remote server. We are using XFS on top of the nbd device,
> but it can happen with any FS or other modules layered on top of the nbd
> device that can write out data to free memory. Here a nbd daemon helper
> thread, msgr-worker-1, is performing a write/sendmsg on a socket to execute
> a request. This kicks off a reclaim operation which results in a WRITE to
> the nbd device and the nbd thread calling back into the mm layer.
>
> [ 1626.609191] msgr-worker-1 D 0 1026 1 0x00004000
> [ 1626.609193] Call Trace:
> [ 1626.609195] ? __schedule+0x29b/0x630
> [ 1626.609197] ? wait_for_completion+0xe0/0x170
> [ 1626.609198] schedule+0x30/0xb0
> [ 1626.609200] schedule_timeout+0x1f6/0x2f0
> [ 1626.609202] ? blk_finish_plug+0x21/0x2e
> [ 1626.609204] ? _xfs_buf_ioapply+0x2e6/0x410
> [ 1626.609206] ? wait_for_completion+0xe0/0x170
> [ 1626.609208] wait_for_completion+0x108/0x170
> [ 1626.609210] ? wake_up_q+0x70/0x70
> [ 1626.609212] ? __xfs_buf_submit+0x12e/0x250
> [ 1626.609214] ? xfs_bwrite+0x25/0x60
> [ 1626.609215] xfs_buf_iowait+0x22/0xf0
> [ 1626.609218] __xfs_buf_submit+0x12e/0x250
> [ 1626.609220] xfs_bwrite+0x25/0x60
> [ 1626.609222] xfs_reclaim_inode+0x2e8/0x310
> [ 1626.609224] xfs_reclaim_inodes_ag+0x1b6/0x300
> [ 1626.609227] xfs_reclaim_inodes_nr+0x31/0x40
> [ 1626.609228] super_cache_scan+0x152/0x1a0
> [ 1626.609231] do_shrink_slab+0x12c/0x2d0
> [ 1626.609233] shrink_slab+0x9c/0x2a0
> [ 1626.609235] shrink_node+0xd7/0x470
> [ 1626.609237] do_try_to_free_pages+0xbf/0x380
> [ 1626.609240] try_to_free_pages+0xd9/0x1f0
> [ 1626.609245] __alloc_pages_slowpath+0x3a4/0xd30
> [ 1626.609251] ? ___slab_alloc+0x238/0x560
> [ 1626.609254] __alloc_pages_nodemask+0x30c/0x350
> [ 1626.609259] skb_page_frag_refill+0x97/0xd0
> [ 1626.609274] sk_page_frag_refill+0x1d/0x80
> [ 1626.609279] tcp_sendmsg_locked+0x2bb/0xdd0
> [ 1626.609304] tcp_sendmsg+0x27/0x40
> [ 1626.609307] sock_sendmsg+0x54/0x60
> [ 1626.609308] ___sys_sendmsg+0x29f/0x320
> [ 1626.609313] ? sock_poll+0x66/0xb0
> [ 1626.609318] ? ep_item_poll.isra.15+0x40/0xc0
> [ 1626.609320] ? ep_send_events_proc+0xe6/0x230
> [ 1626.609322] ? hrtimer_try_to_cancel+0x54/0xf0
> [ 1626.609324] ? ep_read_events_proc+0xc0/0xc0
> [ 1626.609326] ? _raw_write_unlock_irq+0xa/0x20
> [ 1626.609327] ? ep_scan_ready_list.constprop.19+0x218/0x230
> [ 1626.609329] ? __hrtimer_init+0xb0/0xb0
> [ 1626.609331] ? _raw_spin_unlock_irq+0xa/0x20
> [ 1626.609334] ? ep_poll+0x26c/0x4a0
> [ 1626.609337] ? tcp_tsq_write.part.54+0xa0/0xa0
> [ 1626.609339] ? release_sock+0x43/0x90
> [ 1626.609341] ? _raw_spin_unlock_bh+0xa/0x20
> [ 1626.609342] __sys_sendmsg+0x47/0x80
> [ 1626.609347] do_syscall_64+0x5f/0x1c0
> [ 1626.609349] ? prepare_exit_to_usermode+0x75/0xa0
> [ 1626.609351] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> This patch adds a new prctl command that daemons can use after they have
> done their initial setup, and before they start to do allocations that
> are in the IO path. It sets the PF_MEMALLOC_NOIO and PF_LESS_THROTTLE
> flags so both userspace block and FS threads can use it to avoid the
> allocation recursion and try to prevent from being throttled while
> writing out data to free up memory.
>
> Signed-off-by: Mike Christie <mchristi@redhat.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Tested-by: Masato Suzuki <masato.suzuki@wdc.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
I suppose this patch should be routed through MM tree, so, CCing Andrew.
>
> ---
>
> V4:
> - Fix PR_GET_IO_FLUSHER check to match SET.
>
> V3:
> - Drop NOFS, set PF_LESS_THROTTLE and rename prctl flag to reflect it
> is more general and can support both FS and block devices. Both fuse
> and block device daemons, nbd and tcmu-runner, have been tested to
> confirm the more restrictive PF_MEMALLOC_NOIO also works for fuse.
>
> - Use CAP_SYS_RESOURCE instead of admin.
>
> V2:
> - Use prctl instead of procfs.
> - Add support for NOFS for fuse.
> - Check permissions.
>
>
> include/uapi/linux/capability.h | 1 +
> include/uapi/linux/prctl.h | 4 ++++
> kernel/sys.c | 25 +++++++++++++++++++++++++
> 3 files changed, 30 insertions(+)
>
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 240fdb9a60f6..272dc69fa080 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -301,6 +301,7 @@ struct vfs_ns_cap_data {
> /* Allow more than 64hz interrupts from the real-time clock */
> /* Override max number of consoles on console allocation */
> /* Override max number of keymaps */
> +/* Control memory reclaim behavior */
>
> #define CAP_SYS_RESOURCE 24
>
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 7da1b37b27aa..07b4f8131e36 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -234,4 +234,8 @@ struct prctl_mm_map {
> #define PR_GET_TAGGED_ADDR_CTRL 56
> # define PR_TAGGED_ADDR_ENABLE (1UL << 0)
>
> +/* Control reclaim behavior when allocating memory */
> +#define PR_SET_IO_FLUSHER 57
> +#define PR_GET_IO_FLUSHER 58
> +
> #endif /* _LINUX_PRCTL_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index a611d1d58c7d..c1a360370d09 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2259,6 +2259,8 @@ int __weak arch_prctl_spec_ctrl_set(struct task_struct *t, unsigned long which,
> return -EINVAL;
> }
>
> +#define PR_IO_FLUSHER (PF_MEMALLOC_NOIO | PF_LESS_THROTTLE)
> +
> SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> unsigned long, arg4, unsigned long, arg5)
> {
> @@ -2486,6 +2488,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> return -EINVAL;
> error = GET_TAGGED_ADDR_CTRL();
> break;
> + case PR_SET_IO_FLUSHER:
> + if (!capable(CAP_SYS_RESOURCE))
> + return -EPERM;
> +
> + if (arg3 || arg4 || arg5)
> + return -EINVAL;
> +
> + if (arg2 == 1)
> + current->flags |= PR_IO_FLUSHER;
> + else if (!arg2)
> + current->flags &= ~PR_IO_FLUSHER;
> + else
> + return -EINVAL;
> + break;
> + case PR_GET_IO_FLUSHER:
> + if (!capable(CAP_SYS_RESOURCE))
> + return -EPERM;
> +
> + if (arg2 || arg3 || arg4 || arg5)
> + return -EINVAL;
> +
> + error = (current->flags & PR_IO_FLUSHER) == PR_IO_FLUSHER;
> + break;
> default:
> error = -EINVAL;
> break;
> --
> 2.20.1
>
^ permalink raw reply
* [RFC PATCH] ptrace: add PTRACE_GETFD request
From: Sargun Dhillon @ 2019-12-05 23:44 UTC (permalink / raw)
To: linux-kernel, containers, linux-api; +Cc: tycho
PTRACE_GETFD is a generic ptrace API that allows the tracer to
get file descriptors from the traceee.
The primary reason to use this syscall is to allow sandboxers to
take action on an FD on behalf of the tracee. For example, this
can be combined with seccomp's user notification feature to extract
a file descriptor and call privileged syscalls, like binding
a socket to a privileged port.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
include/uapi/linux/ptrace.h | 5 +++++
kernel/ptrace.c | 39 +++++++++++++++++++++++++++++++++++--
2 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index a71b6e3b03eb..2b69f759826a 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -101,6 +101,11 @@ struct ptrace_syscall_info {
};
};
+/* This gets a file descriptor from a running process. It doesn't require the
+ * process to be stopped.
+ */
+#define PTRACE_GETFD 0x420f
+
/*
* 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..a1d7b289fe8e 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,37 @@ 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 fd)
+{
+ struct files_struct *files;
+ struct file *file;
+ int ret = 0;
+
+ files = get_files_struct(child);
+ if (!files)
+ return -ENOENT;
+
+ spin_lock(&files->file_lock);
+ file = fcheck_files(files, fd);
+ if (!file)
+ ret = -EBADF;
+ else
+ get_file(file);
+ spin_unlock(&files->file_lock);
+ put_files_struct(files);
+
+ if (ret)
+ goto out;
+
+ ret = get_unused_fd_flags(0);
+ if (ret >= 0)
+ fd_install(ret, file);
+
+ fput(file);
+out:
+ return ret;
+}
+
int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
{
@@ -1222,7 +1254,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, data);
+ break;
default:
break;
}
@@ -1265,7 +1299,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] move_pages.2: not return ENOENT if the page are already on the target nodes
From: Yang Shi @ 2019-12-06 1:34 UTC (permalink / raw)
To: mtk.manpages, cl, jhubbard, mhocko, cai, akpm
Cc: yang.shi, linux-man, linux-api, linux-mm, linux-kernel
Since commit e78bbfa82624 ("mm: stop returning -ENOENT
from sys_move_pages() if nothing got migrated"), move_pages doesn't
return -ENOENT anymore if the pages are already on the target nodes, but
this change is never reflected in manpage.
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Qian Cai <cai@lca.pw>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
man2/move_pages.2 | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/man2/move_pages.2 b/man2/move_pages.2
index 2d96468..2a2f3cd 100644
--- a/man2/move_pages.2
+++ b/man2/move_pages.2
@@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
One of the target nodes is not online.
.TP
.B ENOENT
-No pages were found that require moving.
-All pages are either already
-on the target node, not present, had an invalid address or could not be
+No pages were found.
+All pages are either not present, had an invalid address or could not be
moved because they were mapped by multiple processes.
.TP
.B EPERM
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: John Hubbard @ 2019-12-06 1:47 UTC (permalink / raw)
To: Yang Shi, mtk.manpages, cl, mhocko, cai, akpm
Cc: linux-man, linux-api, linux-mm, linux-kernel
In-Reply-To: <1575596090-115377-1-git-send-email-yang.shi@linux.alibaba.com>
On 12/5/19 5:34 PM, Yang Shi wrote:
...
>
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468..2a2f3cd 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
> One of the target nodes is not online.
> .TP
> .B ENOENT
> -No pages were found that require moving.
> -All pages are either already
> -on the target node, not present, had an invalid address or could not be
> +No pages were found.
> +All pages are either not present, had an invalid address or could not be
> moved because they were mapped by multiple processes.
How about this wording (ignoring man formatting for the moment):
No pages were moved, because all requested pages fell into one or more of
the following cases:
* Page not present.
* Page has an invalid address.
* Page is mapped by multiple processes.
Reasoning: I don't like the "no pages were found" all by itself, because it
blindly rewords the meaning of ENOENT. ENOENT is merely the closest
symbol we have. So we use ENOENT and that's fine, but the descriptive text
should describe what really happened, which is "no pages were moved". If we had
an ENOPAGESMOVED then we'd use that. :)
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply
* Re: [RFC PATCH] ptrace: add PTRACE_GETFD request
From: Jann Horn @ 2019-12-06 2:38 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: kernel list, Linux Containers, Linux API, Tycho Andersen
In-Reply-To: <20191205234450.GA26369@ircssh-2.c.rugged-nimbus-611.internal>
On Fri, Dec 6, 2019 at 12:44 AM Sargun Dhillon <sargun@sargun.me> wrote:
> PTRACE_GETFD is a generic ptrace API that allows the tracer to
> get file descriptors from the traceee.
typo: tracee
> The primary reason to use this syscall is to allow sandboxers to
> take action on an FD on behalf of the tracee. For example, this
> can be combined with seccomp's user notification feature to extract
> a file descriptor and call privileged syscalls, like binding
> a socket to a privileged port.
[...]
> +/* This gets a file descriptor from a running process. It doesn't require the
> + * process to be stopped.
> + */
> +#define PTRACE_GETFD 0x420f
[...]
> +static int ptrace_getfd(struct task_struct *child, unsigned long fd)
I'd make the "fd" parameter of this function an "unsigned int", given
that that's also the argument type of fcheck_files().
> +{
> + struct files_struct *files;
> + struct file *file;
> + int ret = 0;
> +
> + files = get_files_struct(child);
> + if (!files)
> + return -ENOENT;
> +
> + spin_lock(&files->file_lock);
> + file = fcheck_files(files, fd);
> + if (!file)
> + ret = -EBADF;
> + else
> + get_file(file);
> + spin_unlock(&files->file_lock);
> + put_files_struct(files);
> +
> + if (ret)
> + goto out;
> +
> + ret = get_unused_fd_flags(0);
You're hardcoding the flags for the fd as 0, which means that there is
no way for the caller to enable O_CLOEXEC on the fd in a way that is
race-free against a concurrent execve(). If you can't easily plumb
through an O_CLOEXEC flag from userspace to here, you should probably
hardcode O_CLOEXEC here.
> + if (ret >= 0)
> + fd_install(ret, file);
> +
> + fput(file);
Annoyingly, this isn't how fd_install() works. fd_install() has
slightly weird semantics and consumes the reference passed to it, so
this should be:
if (ret >= 0)
fd_install(ret, file);
else
fput(file);
> +out:
> + return ret;
> +}
^ permalink raw reply
* Re: [RFC PATCH] ptrace: add PTRACE_GETFD request
From: Sargun Dhillon @ 2019-12-06 6:16 UTC (permalink / raw)
To: Jann Horn; +Cc: kernel list, Linux Containers, Linux API, Tycho Andersen
In-Reply-To: <CAG48ez0_CCxO=stFvK=4G4Og=xe9Rtws8PEVy-cSmLqcxfE2Zw@mail.gmail.com>
On Thu, Dec 5, 2019 at 6:38 PM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Dec 6, 2019 at 12:44 AM Sargun Dhillon <sargun@sargun.me> wrote:
> > PTRACE_GETFD is a generic ptrace API that allows the tracer to
> > get file descriptors from the traceee.
>
> typo: tracee
>
> > The primary reason to use this syscall is to allow sandboxers to
> > take action on an FD on behalf of the tracee. For example, this
> > can be combined with seccomp's user notification feature to extract
> > a file descriptor and call privileged syscalls, like binding
> > a socket to a privileged port.
> [...]
> > +/* This gets a file descriptor from a running process. It doesn't require the
> > + * process to be stopped.
> > + */
> > +#define PTRACE_GETFD 0x420f
> [...]
> > +static int ptrace_getfd(struct task_struct *child, unsigned long fd)
>
> I'd make the "fd" parameter of this function an "unsigned int", given
> that that's also the argument type of fcheck_files().
>
> > +{
> > + struct files_struct *files;
> > + struct file *file;
> > + int ret = 0;
> > +
> > + files = get_files_struct(child);
> > + if (!files)
> > + return -ENOENT;
> > +
> > + spin_lock(&files->file_lock);
> > + file = fcheck_files(files, fd);
> > + if (!file)
> > + ret = -EBADF;
> > + else
> > + get_file(file);
> > + spin_unlock(&files->file_lock);
> > + put_files_struct(files);
> > +
> > + if (ret)
> > + goto out;
> > +
> > + ret = get_unused_fd_flags(0);
>
> You're hardcoding the flags for the fd as 0, which means that there is
> no way for the caller to enable O_CLOEXEC on the fd in a way that is
> race-free against a concurrent execve(). If you can't easily plumb
> through an O_CLOEXEC flag from userspace to here, you should probably
> hardcode O_CLOEXEC here.
>
I thought about making addr used for flags. It seems a little weird, given the
name, but it'll do the job. Alternatively, it could be a point to an
options struct.
If we introduce options, one of the nice things we could add is add the ability
to cleanse the FD of certain information, like cgroups.
> > + if (ret >= 0)
> > + fd_install(ret, file);
> > +
> > + fput(file);
>
> Annoyingly, this isn't how fd_install() works. fd_install() has
> slightly weird semantics and consumes the reference passed to it, so
> this should be:
>
> if (ret >= 0)
> fd_install(ret, file);
> else
> fput(file);
>
> > +out:
> > + return ret;
> > +}
^ permalink raw reply
* Re: [RFC PATCH] ptrace: add PTRACE_GETFD request
From: Aleksa Sarai @ 2019-12-06 6:52 UTC (permalink / raw)
To: Sargun Dhillon
Cc: Jann Horn, kernel list, Linux Containers, Linux API,
Tycho Andersen
In-Reply-To: <CAMp4zn85sz_y8EvXUULVY0a0fAmg91pFkYX5zZSXDz6Q-EiUoA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2857 bytes --]
On 2019-12-05, Sargun Dhillon <sargun@sargun.me> wrote:
> On Thu, Dec 5, 2019 at 6:38 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Fri, Dec 6, 2019 at 12:44 AM Sargun Dhillon <sargun@sargun.me> wrote:
> > > PTRACE_GETFD is a generic ptrace API that allows the tracer to
> > > get file descriptors from the traceee.
> >
> > typo: tracee
> >
> > > The primary reason to use this syscall is to allow sandboxers to
> > > take action on an FD on behalf of the tracee. For example, this
> > > can be combined with seccomp's user notification feature to extract
> > > a file descriptor and call privileged syscalls, like binding
> > > a socket to a privileged port.
> > [...]
> > > +/* This gets a file descriptor from a running process. It doesn't require the
> > > + * process to be stopped.
> > > + */
> > > +#define PTRACE_GETFD 0x420f
> > [...]
> > > +static int ptrace_getfd(struct task_struct *child, unsigned long fd)
> >
> > I'd make the "fd" parameter of this function an "unsigned int", given
> > that that's also the argument type of fcheck_files().
> >
> > > +{
> > > + struct files_struct *files;
> > > + struct file *file;
> > > + int ret = 0;
> > > +
> > > + files = get_files_struct(child);
> > > + if (!files)
> > > + return -ENOENT;
> > > +
> > > + spin_lock(&files->file_lock);
> > > + file = fcheck_files(files, fd);
> > > + if (!file)
> > > + ret = -EBADF;
> > > + else
> > > + get_file(file);
> > > + spin_unlock(&files->file_lock);
> > > + put_files_struct(files);
> > > +
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = get_unused_fd_flags(0);
> >
> > You're hardcoding the flags for the fd as 0, which means that there is
> > no way for the caller to enable O_CLOEXEC on the fd in a way that is
> > race-free against a concurrent execve(). If you can't easily plumb
> > through an O_CLOEXEC flag from userspace to here, you should probably
> > hardcode O_CLOEXEC here.
> >
> I thought about making addr used for flags. It seems a little weird,
> given the name, but it'll do the job. Alternatively, it could be a
> point to an options struct. If we introduce options, one of the nice
> things we could add is add the ability to cleanse the FD of certain
> information, like cgroups.
If you do end up opting for an options struct, please make sure you use
copy_struct_from_user() or something similar so that we can painlessly
extend it in the future (if necessary). Since there isn't an additional
argument, you might want to do what perf_event_open() does and embed the
size as the first field of the options struct.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: Michal Hocko @ 2019-12-06 7:47 UTC (permalink / raw)
To: Yang Shi
Cc: mtk.manpages, cl, jhubbard, cai, akpm, linux-man, linux-api,
linux-mm, linux-kernel
In-Reply-To: <1575596090-115377-1-git-send-email-yang.shi@linux.alibaba.com>
On Fri 06-12-19 09:34:50, Yang Shi wrote:
> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
> from sys_move_pages() if nothing got migrated"), move_pages doesn't
> return -ENOENT anymore if the pages are already on the target nodes, but
> this change is never reflected in manpage.
>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Qian Cai <cai@lca.pw>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
> man2/move_pages.2 | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468..2a2f3cd 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
> One of the target nodes is not online.
> .TP
> .B ENOENT
> -No pages were found that require moving.
> -All pages are either already
> -on the target node, not present, had an invalid address or could not be
> +No pages were found.
> +All pages are either not present, had an invalid address or could not be
> moved because they were mapped by multiple processes.
I would rather not put any specifics here because those reasons might
differ in future. I would simply go with "No pages were found that
require or could be moved."
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH] ptrace: add PTRACE_GETFD request
From: Christian Brauner @ 2019-12-06 8:25 UTC (permalink / raw)
To: Sargun Dhillon, oleg; +Cc: linux-kernel, containers, linux-api, tycho
In-Reply-To: <20191205234450.GA26369@ircssh-2.c.rugged-nimbus-611.internal>
[+ Oleg, the maintainer. This needs to see his review before anything
can happen to this series.]
On Thu, Dec 05, 2019 at 11:44:53PM +0000, Sargun Dhillon wrote:
> PTRACE_GETFD is a generic ptrace API that allows the tracer to
> get file descriptors from the traceee.
>
> The primary reason to use this syscall is to allow sandboxers to
> take action on an FD on behalf of the tracee. For example, this
> can be combined with seccomp's user notification feature to extract
> a file descriptor and call privileged syscalls, like binding
> a socket to a privileged port.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> include/uapi/linux/ptrace.h | 5 +++++
> kernel/ptrace.c | 39 +++++++++++++++++++++++++++++++++++--
> 2 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index a71b6e3b03eb..2b69f759826a 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -101,6 +101,11 @@ struct ptrace_syscall_info {
> };
> };
>
> +/* This gets a file descriptor from a running process. It doesn't require the
> + * process to be stopped.
> + */
> +#define PTRACE_GETFD 0x420f
> +
> /*
> * 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..a1d7b289fe8e 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,37 @@ 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 fd)
> +{
> + struct files_struct *files;
> + struct file *file;
> + int ret = 0;
> +
> + files = get_files_struct(child);
> + if (!files)
> + return -ENOENT;
> +
> + spin_lock(&files->file_lock);
> + file = fcheck_files(files, fd);
> + if (!file)
> + ret = -EBADF;
> + else
> + get_file(file);
> + spin_unlock(&files->file_lock);
> + put_files_struct(files);
> +
> + if (ret)
> + goto out;
> +
> + ret = get_unused_fd_flags(0);
> + if (ret >= 0)
> + fd_install(ret, file);
> +
> + fput(file);
> +out:
> + return ret;
> +}
> +
> int ptrace_request(struct task_struct *child, long request,
> unsigned long addr, unsigned long data)
> {
> @@ -1222,7 +1254,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, data);
> + break;
> default:
> break;
> }
> @@ -1265,7 +1299,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
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: John Hubbard @ 2019-12-06 8:25 UTC (permalink / raw)
To: Yang Shi, mtk.manpages, cl, mhocko, cai, akpm
Cc: linux-man, linux-api, linux-mm, linux-kernel
In-Reply-To: <1575596090-115377-1-git-send-email-yang.shi@linux.alibaba.com>
On 12/5/19 5:34 PM, Yang Shi wrote:
> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
> from sys_move_pages() if nothing got migrated"), move_pages doesn't
> return -ENOENT anymore if the pages are already on the target nodes, but
> this change is never reflected in manpage.
>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Qian Cai <cai@lca.pw>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
> man2/move_pages.2 | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468..2a2f3cd 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
> One of the target nodes is not online.
> .TP
> .B ENOENT
> -No pages were found that require moving.
> -All pages are either already
> -on the target node, not present, had an invalid address or could not be
> +No pages were found.
> +All pages are either not present, had an invalid address or could not be
> moved because they were mapped by multiple processes.
> .TP
> .B EPERM
>
whoa, hold on. If I'm reading through the various error paths correctly, then this
code is *never* going to return ENOENT for the whole function. It can fill in that
value per-page, in the status array, but that's all. Did I get that right?
If so, we need to redo this part of the man page.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: Michal Hocko @ 2019-12-06 9:45 UTC (permalink / raw)
To: John Hubbard
Cc: Yang Shi, mtk.manpages, cl, cai, akpm, linux-man, linux-api,
linux-mm, linux-kernel
In-Reply-To: <0dc96e40-5f2b-a2fe-6e5f-b6f3d5e9ebde@nvidia.com>
On Fri 06-12-19 00:25:53, John Hubbard wrote:
> On 12/5/19 5:34 PM, Yang Shi wrote:
> > Since commit e78bbfa82624 ("mm: stop returning -ENOENT
> > from sys_move_pages() if nothing got migrated"), move_pages doesn't
> > return -ENOENT anymore if the pages are already on the target nodes, but
> > this change is never reflected in manpage.
> >
> > Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Qian Cai <cai@lca.pw>
> > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> > ---
> > man2/move_pages.2 | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/man2/move_pages.2 b/man2/move_pages.2
> > index 2d96468..2a2f3cd 100644
> > --- a/man2/move_pages.2
> > +++ b/man2/move_pages.2
> > @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
> > One of the target nodes is not online.
> > .TP
> > .B ENOENT
> > -No pages were found that require moving.
> > -All pages are either already
> > -on the target node, not present, had an invalid address or could not be
> > +No pages were found.
> > +All pages are either not present, had an invalid address or could not be
> > moved because they were mapped by multiple processes.
> > .TP
> > .B EPERM
> >
>
> whoa, hold on. If I'm reading through the various error paths correctly, then this
> code is *never* going to return ENOENT for the whole function. It can fill in that
> value per-page, in the status array, but that's all. Did I get that right?
You are right. Both store_status and do_move_pages_to_node do overwrite
the error code. So you are right that ENOENT return value is not
possible. I haven't checked since when this is the case. This whole
syscall is a disaster from the API and documentation POV.
Btw. Page states error codes could see some refinements as well.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH] ptrace: add PTRACE_GETFD request
From: Oleg Nesterov @ 2019-12-06 12:23 UTC (permalink / raw)
To: Christian Brauner
Cc: Sargun Dhillon, linux-kernel, containers, linux-api, tycho
In-Reply-To: <20191206082539.gmefytwu3ylixj5d@wittgenstein>
> On Thu, Dec 05, 2019 at 11:44:53PM +0000, Sargun Dhillon wrote:
>
> > +static int ptrace_getfd(struct task_struct *child, unsigned long fd)
> > +{
> > + struct files_struct *files;
> > + struct file *file;
> > + int ret = 0;
> > +
> > + files = get_files_struct(child);
> > + if (!files)
> > + return -ENOENT;
> > +
> > + spin_lock(&files->file_lock);
> > + file = fcheck_files(files, fd);
> > + if (!file)
> > + ret = -EBADF;
> > + else
> > + get_file(file);
> > + spin_unlock(&files->file_lock);
> > + put_files_struct(files);
may be someone can finally create a helper for this, it can have more users.
say,
struct file *get_task_file(task, fd)
{
struct file *file = NULL;
task_lock(task);
rcu_read_lock();
if (task->files) {
file = fcheck_files(task->files, fd);
if (file)
get_file(file);
}
rcu_read_unlock();
task_unlock(task);
return file;
}
no need to get/put files_struct, no need to take ->file_lock.
> > +
> > + if (ret)
> > + goto out;
> > +
> > + ret = get_unused_fd_flags(0);
> > + if (ret >= 0)
> > + fd_install(ret, file);
> > +
> > + fput(file);
this looks wrong or I am totally confused...
if (ret >= 0)
fd_install(file);
else
fput(file);
?
> > @@ -1265,7 +1299,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);
Hmm. not sure why do you want this... But OK, we do not need to stop the tracee.
Oleg.
^ permalink raw reply
* Re: [RFC PATCH] ptrace: add PTRACE_GETFD request
From: Tycho Andersen @ 2019-12-06 14:10 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: linux-kernel, containers, linux-api
In-Reply-To: <20191205234450.GA26369@ircssh-2.c.rugged-nimbus-611.internal>
On Thu, Dec 05, 2019 at 11:44:53PM +0000, Sargun Dhillon wrote:
> PTRACE_GETFD is a generic ptrace API that allows the tracer to
> get file descriptors from the traceee.
>
> The primary reason to use this syscall is to allow sandboxers to
I might change this to "one motivation to use this ptrace command",
because I'm sure people will invent other crazy uses soon after it's
added :)
> take action on an FD on behalf of the tracee. For example, this
> can be combined with seccomp's user notification feature to extract
> a file descriptor and call privileged syscalls, like binding
> a socket to a privileged port.
This can already be accomplished via injecting parasite code like CRIU
does; adding a ptrace() command like this makes it much nicer to be
sure, but it is redundant.
Tycho
^ permalink raw reply
* [PATCH v18 00/13] open: introduce openat2(2) syscall
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Kees Cook, Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Rasmus Villemoes, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Christian Brauner, Aleksa Sarai, Linus Torvalds,
dev, containers, bpf, netdev
This patchset is being developed here:
<https://github.com/cyphar/linux/tree/openat2/master>
Patch changelog:
v18:
* Further fixups from Al Viro:
- Don't WARN_ON in complete_walk() check since it can be trivially
triggered by userspace. Also, improve the comment so the purpose of the
check is more clear.
- Avoid duplicate smp_rmb() when in handle_dots() by doing
__read_seqcount_retry().
- Drop vestigial UPGRADE_NO* flag definitions in uapi.
* Update non-zero __padding test to include all bytes of the padding.
v17: <https://lore.kernel.org/lkml/20191117011713.13032-1-cyphar@cyphar.com/>
<https://lore.kernel.org/lkml/20191120050631.12816-1-cyphar@cyphar.com/>
v16: <https://lore.kernel.org/lkml/20191116002802.6663-1-cyphar@cyphar.com/>
v15: <https://lore.kernel.org/lkml/20191105090553.6350-1-cyphar@cyphar.com/>
v14: <https://lore.kernel.org/lkml/20191010054140.8483-1-cyphar@cyphar.com/>
<https://lore.kernel.org/lkml/20191026185700.10708-1-cyphar@cyphar.com>
v13: <https://lore.kernel.org/lkml/20190930183316.10190-1-cyphar@cyphar.com/>
v12: <https://lore.kernel.org/lkml/20190904201933.10736-1-cyphar@cyphar.com/>
v11: <https://lore.kernel.org/lkml/20190820033406.29796-1-cyphar@cyphar.com/>
<https://lore.kernel.org/lkml/20190728010207.9781-1-cyphar@cyphar.com/>
v10: <https://lore.kernel.org/lkml/20190719164225.27083-1-cyphar@cyphar.com/>
v09: <https://lore.kernel.org/lkml/20190706145737.5299-1-cyphar@cyphar.com/>
v08: <https://lore.kernel.org/lkml/20190520133305.11925-1-cyphar@cyphar.com/>
v07: <https://lore.kernel.org/lkml/20190507164317.13562-1-cyphar@cyphar.com/>
v06: <https://lore.kernel.org/lkml/20190506165439.9155-1-cyphar@cyphar.com/>
v05: <https://lore.kernel.org/lkml/20190320143717.2523-1-cyphar@cyphar.com/>
v04: <https://lore.kernel.org/lkml/20181112142654.341-1-cyphar@cyphar.com/>
v03: <https://lore.kernel.org/lkml/20181009070230.12884-1-cyphar@cyphar.com/>
v02: <https://lore.kernel.org/lkml/20181009065300.11053-1-cyphar@cyphar.com/>
v01: <https://lore.kernel.org/lkml/20180929103453.12025-1-cyphar@cyphar.com/>
For a very long time, extending openat(2) with new features has been
incredibly frustrating. This stems from the fact that openat(2) is
possibly the most famous counter-example to the mantra "don't silently
accept garbage from userspace" -- it doesn't check whether unknown flags
are present[1].
This means that (generally) the addition of new flags to openat(2) has
been fraught with backwards-compatibility issues (O_TMPFILE has to be
defined as __O_TMPFILE|O_DIRECTORY|[O_RDWR or O_WRONLY] to ensure old
kernels gave errors, since it's insecure to silently ignore the
flag[2]). All new security-related flags therefore have a tough road to
being added to openat(2).
Furthermore, the need for some sort of control over VFS's path resolution (to
avoid malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a revival
of Al Viro's old AT_NO_JUMPS[3] patchset (which was a variant of David
Drysdale's O_BENEATH patchset[4] which was a spin-off of the Capsicum
project[5]) with a few additions and changes made based on the previous
discussion within [6] as well as others I felt were useful.
In line with the conclusions of the original discussion of AT_NO_JUMPS, the
flag has been split up into separate flags. However, instead of being an
openat(2) flag it is provided through a new syscall openat2(2) which provides
several other improvements to the openat(2) interface (see the patch
description for more details). The following new LOOKUP_* flags are added:
* LOOKUP_NO_XDEV blocks all mountpoint crossings (upwards, downwards,
or through absolute links). Absolute pathnames alone in openat(2) do not
trigger this. Magic-link traversal which implies a vfsmount jump is also
blocked (though magic-link jumps on the same vfsmount are permitted).
* LOOKUP_NO_MAGICLINKS blocks resolution through /proc/$pid/fd-style
links. This is done by blocking the usage of nd_jump_link() during
resolution in a filesystem. The term "magic-links" is used to match
with the only reference to these links in Documentation/, but I'm
happy to change the name.
It should be noted that this is different to the scope of
~LOOKUP_FOLLOW in that it applies to all path components. However,
you can do openat2(NO_FOLLOW|NO_MAGICLINKS) on a magic-link and it
will *not* fail (assuming that no parent component was a
magic-link), and you will have an fd for the magic-link.
In order to correctly detect magic-links, the introduction of a new
LOOKUP_MAGICLINK_JUMPED state flag was required.
* LOOKUP_BENEATH disallows escapes to outside the starting dirfd's
tree, using techniques such as ".." or absolute links. Absolute
paths in openat(2) are also disallowed. Conceptually this flag is to
ensure you "stay below" a certain point in the filesystem tree --
but this requires some additional to protect against various races
that would allow escape using "..".
Currently LOOKUP_BENEATH implies LOOKUP_NO_MAGICLINKS, because it
can trivially beam you around the filesystem (breaking the
protection). In future, there might be similar safety checks done as
in LOOKUP_IN_ROOT, but that requires more discussion.
In addition, two new flags are added that expand on the above ideas:
* LOOKUP_NO_SYMLINKS does what it says on the tin. No symlink
resolution is allowed at all, including magic-links. Just as with
LOOKUP_NO_MAGICLINKS this can still be used with NOFOLLOW to open an
fd for the symlink as long as no parent path had a symlink
component.
* LOOKUP_IN_ROOT is an extension of LOOKUP_BENEATH that, rather than
blocking attempts to move past the root, forces all such movements
to be scoped to the starting point. This provides chroot(2)-like
protection but without the cost of a chroot(2) for each filesystem
operation, as well as being safe against race attacks that chroot(2)
is not.
If a race is detected (as with LOOKUP_BENEATH) then an error is
generated, and similar to LOOKUP_BENEATH it is not permitted to cross
magic-links with LOOKUP_IN_ROOT.
The primary need for this is from container runtimes, which
currently need to do symlink scoping in userspace[7] when opening
paths in a potentially malicious container. There is a long list of
CVEs that could have bene mitigated by having RESOLVE_THIS_ROOT
(such as CVE-2017-1002101, CVE-2017-1002102, CVE-2018-15664, and
CVE-2019-5736, just to name a few).
In order to make all of the above more usable, I'm working on
libpathrs[8] which is a C-friendly library for safe path resolution. It
features a userspace-emulated backend if the kernel doesn't support
openat2(2). Hopefully we can get userspace to switch to using it, and
thus get openat2(2) support for free once it's ready.
Future work would include implementing things like RESOLVE_NO_AUTOMOUNT and
possibly a RESOLVE_NO_REMOTE (to allow programs to be sure they don't hit DoSes
though stale NFS handles).
[1]: https://lwn.net/Articles/588444/
[2]: https://lore.kernel.org/lkml/CA+55aFyyxJL1LyXZeBsf2ypriraj5ut1XkNDsunRBqgVjZU_6Q@mail.gmail.com
[3]: https://lore.kernel.org/lkml/20170429220414.GT29622@ZenIV.linux.org.uk
[4]: https://lore.kernel.org/lkml/1415094884-18349-1-git-send-email-drysdale@google.com
[5]: https://lore.kernel.org/lkml/1404124096-21445-1-git-send-email-drysdale@google.com
[6]: https://lwn.net/Articles/723057/
[7]: https://github.com/cyphar/filepath-securejoin
[8]: https://github.com/openSUSE/libpathrs
The current draft of the openat2(2) man-page is included below.
--8<---------------------------------------------------------------------------
OPENAT2(2) Linux Programmer's Manual OPENAT2(2)
NAME
openat2 - open and possibly create a file (extended)
SYNOPSIS
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
int openat2(int dirfd, const char *pathname, struct open_how *how, size_t size);
Note: There is no glibc wrapper for this system call; see NOTES.
DESCRIPTION
The openat2() system call opens the file specified by pathname. If the specified file does not exist, it may
optionally (if O_CREAT is specified in how.flags) be created by openat2().
As with openat(2), if pathname is relative, then it is interpreted relative to the directory referred to by
the file descriptor dirfd (or the current working directory of the calling process, if dirfd is the special
value AT_FDCWD.) If pathname is absolute, then dirfd is ignored (unless how.resolve contains RESOLVE_IN_ROOT,
in which case pathname is resolved relative to dirfd.)
The openat2() system call is an extension of openat(2) and provides a superset of its functionality. Rather
than taking a single flag argument, an extensible structure (how) is passed instead to allow for future exten-
sions. size must be set to sizeof(struct open_how), to facilitate future extensions (see the "Extensibility"
section of the NOTES for more detail on how extensions are handled.)
The open_how structure
The following structure indicates how pathname should be opened, and acts as a superset of the flag and mode
arguments to openat(2).
struct open_how {
__aligned_u64 flags; /* O_* flags. */
__u16 mode; /* Mode for O_{CREAT,TMPFILE}. */
__u16 __padding[3]; /* Must be zeroed. */
__aligned_u64 resolve; /* RESOLVE_* flags. */
};
Any future extensions to openat2() will be implemented as new fields appended to the above structure (or
through reuse of pre-existing padding space), with the zero value of the new fields acting as though the ex-
tension were not present.
The meaning of each field is as follows:
flags
The file creation and status flags to use for this operation. All of the O_* flags defined for
openat(2) are valid openat2() flag values.
Unlike openat(2), it is an error to provide openat2() unknown or conflicting flags in flags.
mode
File mode for the new file, with identical semantics to the mode argument to openat(2). How-
ever, unlike openat(2), it is an error to provide openat2() with a mode which contains bits
other than 0777.
It is an error to provide openat2() a non-zero mode if flags does not contain O_CREAT or O_TMP-
FILE.
resolve
Change how the components of pathname will be resolved (see path_resolution(7) for background
information.) The primary use case for these flags is to allow trusted programs to restrict how
untrusted paths (or paths inside untrusted directories) are resolved. The full list of resolve
flags is given below.
RESOLVE_NO_XDEV
Disallow traversal of mount points during path resolution (including all bind mounts).
Users of this flag are encouraged to make its use configurable (unless it is used for a
specific security purpose), as bind mounts are very widely used by end-users. Setting
this flag indiscrimnately for all uses of openat2() may result in spurious errors on pre-
viously-functional systems.
RESOLVE_NO_SYMLINKS
Disallow resolution of symbolic links during path resolution. This option implies RE-
SOLVE_NO_MAGICLINKS.
If the trailing component is a symbolic link, and flags contains both O_PATH and O_NOFOL-
LOW, then an O_PATH file descriptor referencing the symbolic link will be returned.
Users of this flag are encouraged to make its use configurable (unless it is used for a
specific security purpose), as symbolic links are very widely used by end-users. Setting
this flag indiscrimnately for all uses of openat2() may result in spurious errors on pre-
viously-functional systems.
RESOLVE_NO_MAGICLINKS
Disallow all magic link resolution during path resolution.
If the trailing component is a magic link, and flags contains both O_PATH and O_NOFOLLOW,
then an O_PATH file descriptor referencing the magic link will be returned.
Magic-links are symbolic link-like objects that are most notably found in proc(5) (exam-
ples include /proc/[pid]/exe and /proc/[pid]/fd/*.) Due to the potential danger of un-
knowingly opening these magic links, it may be preferable for users to disable their res-
olution entirely (see symboliclink(7) for more details.)
RESOLVE_BENEATH
Do not permit the path resolution to succeed if any component of the resolution is not a
descendant of the directory indicated by dirfd. This results in absolute symbolic links
(and absolute values of pathname) to be rejected.
Currently, this flag also disables magic link resolution. However, this may change in
the future. The caller should explicitly specify RESOLVE_NO_MAGICLINKS to ensure that
magic links are not resolved.
RESOLVE_IN_ROOT
Treat dirfd as the root directory while resolving pathname (as though the user called ch-
root(2) with dirfd as the argument.) Absolute symbolic links and ".." path components
will be scoped to dirfd. If pathname is an absolute path, it is also treated relative to
dirfd.
However, unlike chroot(2) (which changes the filesystem root permanently for a process),
RESOLVE_IN_ROOT allows a program to efficiently restrict path resolution for only certain
operations. It also has several hardening features (such detecting escape attempts dur-
ing .. resolution) which chroot(2) does not.
Currently, this flag also disables magic link resolution. However, this may change in
the future. The caller should explicitly specify RESOLVE_NO_MAGICLINKS to ensure that
magic links are not resolved.
It is an error to provide openat2() unknown flags in resolve.
RETURN VALUE
On success, a new file descriptor is returned. On error, -1 is returned, and errno is set appropriately.
ERRORS
The set of errors returned by openat2() includes all of the errors returned by openat(2), as well as the fol-
lowing additional errors:
EINVAL An unknown flag or invalid value was specified in how.
EINVAL mode is non-zero, but flags does not contain O_CREAT or O_TMPFILE.
EINVAL size was smaller than any known version of struct open_how.
E2BIG An extension was specified in how, which the current kernel does not support (see the "Extensibility"
section of the NOTES for more detail on how extensions are handled.)
EAGAIN resolve contains either RESOLVE_IN_ROOT or RESOLVE_BENEATH, and the kernel could not ensure that a ".."
component didn't escape (due to a race condition or potential attack.) Callers may choose to retry the
openat2() call.
EXDEV resolve contains either RESOLVE_IN_ROOT or RESOLVE_BENEATH, and an escape from the root during path
resolution was detected.
EXDEV resolve contains RESOLVE_NO_XDEV, and a path component attempted to cross a mount point.
ELOOP resolve contains RESOLVE_NO_SYMLINKS, and one of the path components was a symbolic link (or magic
link).
ELOOP resolve contains RESOLVE_NO_MAGICLINKS, and one of the path components was a magic link.
VERSIONS
openat2() first appeared in Linux 5.6.
CONFORMING TO
This system call is Linux-specific.
The semantics of RESOLVE_BENEATH were modelled after FreeBSD's O_BENEATH.
NOTES
Glibc does not provide a wrapper for this system call; call it using syscall(2).
Extensibility
In order to allow for struct open_how to be extended in future kernel revisions, openat2() requires userspace
to specify the size of struct open_how structure they are passing. By providing this information, it is pos-
sible for openat2() to provide both forwards- and backwards-compatibility — with size acting as an implicit
version number (because new extension fields will always be appended, the size will always increase.) This
extensibility design is very similar to other system calls such as perf_setattr(2), perf_event_open(2), and
clone(3).
If we let usize be the size of the structure according to userspace and ksize be the size of the structure
which the kernel supports, then there are only three cases to consider:
* If ksize equals usize, then there is no version mismatch and how can be used verbatim.
* If ksize is larger than usize, then there are some extensions the kernel supports which the
userspace program is unaware of. Because all extensions must have their zero values be a no-op, the
kernel treats all of the extension fields not set by userspace to have zero values. This provides
backwards-compatibility.
* If ksize is smaller than usize, then there are some extensions which the userspace program is aware
of but the kernel does not support. Because all extensions must have their zero values be a no-op,
the kernel can safely ignore the unsupported extension fields if they are all-zero. If any unsup-
ported extension fields are non-zero, then -1 is returned and errno is set to E2BIG. This provides
forwards-compatibility.
Therefore, most userspace programs will not need to have any special handling of extensions. However, if a
userspace program wishes to determine what extensions the running kernel supports, they may conduct a binary
search on size (to find the largest value which doesn't produce an error of E2BIG.)
SEE ALSO
openat(2), path_resolution(7), symlink(7)
Linux 2019-11-05 OPENAT2(2)
--8<---------------------------------------------------------------------------
Aleksa Sarai (13):
namei: only return -ECHILD from follow_dotdot_rcu()
nsfs: clean-up ns_get_path() signature to return int
namei: allow nd_jump_link() to produce errors
namei: allow set_root() to produce errors
namei: LOOKUP_NO_SYMLINKS: block symlink resolution
namei: LOOKUP_NO_MAGICLINKS: block magic-link resolution
namei: LOOKUP_NO_XDEV: block mountpoint crossing
namei: LOOKUP_BENEATH: O_BENEATH-like scoped resolution
namei: LOOKUP_IN_ROOT: chroot-like scoped resolution
namei: LOOKUP_{IN_ROOT,BENEATH}: permit limited ".." resolution
open: introduce openat2(2) syscall
selftests: add openat2(2) selftests
Documentation: path-lookup: include new LOOKUP flags
CREDITS | 4 +-
Documentation/filesystems/path-lookup.rst | 68 ++-
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/namei.c | 199 +++++--
fs/nsfs.c | 29 +-
fs/open.c | 149 +++--
fs/proc/base.c | 3 +-
fs/proc/namespaces.c | 20 +-
include/linux/fcntl.h | 12 +-
include/linux/namei.h | 12 +-
include/linux/proc_ns.h | 4 +-
include/linux/syscalls.h | 3 +
include/uapi/asm-generic/unistd.h | 5 +-
include/uapi/linux/fcntl.h | 35 ++
kernel/bpf/offload.c | 12 +-
kernel/events/core.c | 2 +-
security/apparmor/apparmorfs.c | 6 +-
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/openat2/.gitignore | 1 +
tools/testing/selftests/openat2/Makefile | 8 +
tools/testing/selftests/openat2/helpers.c | 109 ++++
tools/testing/selftests/openat2/helpers.h | 107 ++++
.../testing/selftests/openat2/openat2_test.c | 320 +++++++++++
.../selftests/openat2/rename_attack_test.c | 160 ++++++
.../testing/selftests/openat2/resolve_test.c | 523 ++++++++++++++++++
42 files changed, 1697 insertions(+), 115 deletions(-)
create mode 100644 tools/testing/selftests/openat2/.gitignore
create mode 100644 tools/testing/selftests/openat2/Makefile
create mode 100644 tools/testing/selftests/openat2/helpers.c
create mode 100644 tools/testing/selftests/openat2/helpers.h
create mode 100644 tools/testing/selftests/openat2/openat2_test.c
create mode 100644 tools/testing/selftests/openat2/rename_attack_test.c
create mode 100644 tools/testing/selftests/openat2/resolve_test.c
base-commit: 219d54332a09e8d8741c1e1982f5eae56099de85
--
2.24.0
^ permalink raw reply
* [PATCH v18 01/13] namei: only return -ECHILD from follow_dotdot_rcu()
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Eric W. Biederman, Andy Lutomirski, Andrew Morton,
Kees Cook, Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Rasmus Villemoes, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Christian Brauner, Aleksa Sarai, Linus Torvalds,
dev, containers, bpf, netdev
In-Reply-To: <20191206141338.23338-1-cyphar@cyphar.com>
It's over-zealous to return hard errors under RCU-walk here, given that
a REF-walk will be triggered for all other cases handling ".." under
RCU.
The original purpose of this check was to ensure that if a rename occurs
such that a directory is moved outside of the bind-mount which the
resolution started in, it would be detected and blocked to avoid being
able to mess with paths outside of the bind-mount. However, triggering a
new REF-walk is just as effective a solution.
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Fixes: 397d425dc26d ("vfs: Test for and handle paths that are unreachable from their mnt_root")
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 671c3c1a3425..5a47d9c09581 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1359,7 +1359,7 @@ static int follow_dotdot_rcu(struct nameidata *nd)
nd->path.dentry = parent;
nd->seq = seq;
if (unlikely(!path_connected(&nd->path)))
- return -ENOENT;
+ return -ECHILD;
break;
} else {
struct mount *mnt = real_mount(nd->path.mnt);
--
2.24.0
^ permalink raw reply related
* [PATCH v18 02/13] nsfs: clean-up ns_get_path() signature to return int
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Kees Cook, Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Rasmus Villemoes, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Christian Brauner, Aleksa Sarai, Linus Torvalds,
dev, containers, bpf, netdev
In-Reply-To: <20191206141338.23338-1-cyphar@cyphar.com>
ns_get_path() and ns_get_path_cb() only ever return either NULL or an
ERR_PTR. It is far more idiomatic to simply return an integer, and it
makes all of the callers of ns_get_path() more straightforward to read.
Fixes: e149ed2b805f ("take the targets of /proc/*/ns/* symlinks to separate fs")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/nsfs.c | 29 ++++++++++++++---------------
fs/proc/namespaces.c | 6 +++---
include/linux/proc_ns.h | 4 ++--
kernel/bpf/offload.c | 12 ++++++------
kernel/events/core.c | 2 +-
5 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/fs/nsfs.c b/fs/nsfs.c
index a0431642c6b5..f3d2833c5781 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c
@@ -52,7 +52,7 @@ static void nsfs_evict(struct inode *inode)
ns->ops->put(ns);
}
-static void *__ns_get_path(struct path *path, struct ns_common *ns)
+static int __ns_get_path(struct path *path, struct ns_common *ns)
{
struct vfsmount *mnt = nsfs_mnt;
struct dentry *dentry;
@@ -71,13 +71,13 @@ static void *__ns_get_path(struct path *path, struct ns_common *ns)
got_it:
path->mnt = mntget(mnt);
path->dentry = dentry;
- return NULL;
+ return 0;
slow:
rcu_read_unlock();
inode = new_inode_pseudo(mnt->mnt_sb);
if (!inode) {
ns->ops->put(ns);
- return ERR_PTR(-ENOMEM);
+ return -ENOMEM;
}
inode->i_ino = ns->inum;
inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
@@ -89,7 +89,7 @@ static void *__ns_get_path(struct path *path, struct ns_common *ns)
dentry = d_alloc_anon(mnt->mnt_sb);
if (!dentry) {
iput(inode);
- return ERR_PTR(-ENOMEM);
+ return -ENOMEM;
}
d_instantiate(dentry, inode);
dentry->d_fsdata = (void *)ns->ops;
@@ -98,23 +98,22 @@ static void *__ns_get_path(struct path *path, struct ns_common *ns)
d_delete(dentry); /* make sure ->d_prune() does nothing */
dput(dentry);
cpu_relax();
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
}
goto got_it;
}
-void *ns_get_path_cb(struct path *path, ns_get_path_helper_t *ns_get_cb,
+int ns_get_path_cb(struct path *path, ns_get_path_helper_t *ns_get_cb,
void *private_data)
{
- void *ret;
+ int ret;
do {
struct ns_common *ns = ns_get_cb(private_data);
if (!ns)
- return ERR_PTR(-ENOENT);
-
+ return -ENOENT;
ret = __ns_get_path(path, ns);
- } while (ret == ERR_PTR(-EAGAIN));
+ } while (ret == -EAGAIN);
return ret;
}
@@ -131,7 +130,7 @@ static struct ns_common *ns_get_path_task(void *private_data)
return args->ns_ops->get(args->task);
}
-void *ns_get_path(struct path *path, struct task_struct *task,
+int ns_get_path(struct path *path, struct task_struct *task,
const struct proc_ns_operations *ns_ops)
{
struct ns_get_path_task_args args = {
@@ -147,7 +146,7 @@ int open_related_ns(struct ns_common *ns,
{
struct path path = {};
struct file *f;
- void *err;
+ int err;
int fd;
fd = get_unused_fd_flags(O_CLOEXEC);
@@ -164,11 +163,11 @@ int open_related_ns(struct ns_common *ns,
}
err = __ns_get_path(&path, relative);
- } while (err == ERR_PTR(-EAGAIN));
+ } while (err == -EAGAIN);
- if (IS_ERR(err)) {
+ if (err) {
put_unused_fd(fd);
- return PTR_ERR(err);
+ return err;
}
f = dentry_open(&path, O_RDONLY, current_cred());
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index dd2b35f78b09..08dd94df1a66 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -42,14 +42,14 @@ static const char *proc_ns_get_link(struct dentry *dentry,
const struct proc_ns_operations *ns_ops = PROC_I(inode)->ns_ops;
struct task_struct *task;
struct path ns_path;
- void *error = ERR_PTR(-EACCES);
+ int error = -EACCES;
if (!dentry)
return ERR_PTR(-ECHILD);
task = get_proc_task(inode);
if (!task)
- return error;
+ return ERR_PTR(-EACCES);
if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
error = ns_get_path(&ns_path, task, ns_ops);
@@ -57,7 +57,7 @@ static const char *proc_ns_get_link(struct dentry *dentry,
nd_jump_link(&ns_path);
}
put_task_struct(task);
- return error;
+ return ERR_PTR(error);
}
static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int buflen)
diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
index d31cb6215905..aed366b4795c 100644
--- a/include/linux/proc_ns.h
+++ b/include/linux/proc_ns.h
@@ -76,10 +76,10 @@ static inline int ns_alloc_inum(struct ns_common *ns)
extern struct file *proc_ns_fget(int fd);
#define get_proc_ns(inode) ((struct ns_common *)(inode)->i_private)
-extern void *ns_get_path(struct path *path, struct task_struct *task,
+extern int ns_get_path(struct path *path, struct task_struct *task,
const struct proc_ns_operations *ns_ops);
typedef struct ns_common *ns_get_path_helper_t(void *);
-extern void *ns_get_path_cb(struct path *path, ns_get_path_helper_t ns_get_cb,
+extern int ns_get_path_cb(struct path *path, ns_get_path_helper_t ns_get_cb,
void *private_data);
extern int ns_get_name(char *buf, size_t size, struct task_struct *task,
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 5b9da0954a27..2c5dc6541ece 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -302,14 +302,14 @@ int bpf_prog_offload_info_fill(struct bpf_prog_info *info,
struct inode *ns_inode;
struct path ns_path;
char __user *uinsns;
- void *res;
+ int res;
u32 ulen;
res = ns_get_path_cb(&ns_path, bpf_prog_offload_info_fill_ns, &args);
- if (IS_ERR(res)) {
+ if (res) {
if (!info->ifindex)
return -ENODEV;
- return PTR_ERR(res);
+ return res;
}
down_read(&bpf_devs_lock);
@@ -526,13 +526,13 @@ int bpf_map_offload_info_fill(struct bpf_map_info *info, struct bpf_map *map)
};
struct inode *ns_inode;
struct path ns_path;
- void *res;
+ int res;
res = ns_get_path_cb(&ns_path, bpf_map_offload_info_fill_ns, &args);
- if (IS_ERR(res)) {
+ if (res) {
if (!info->ifindex)
return -ENODEV;
- return PTR_ERR(res);
+ return res;
}
ns_inode = ns_path.dentry->d_inode;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 00a014670ed0..3772a04ca802 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7258,7 +7258,7 @@ static void perf_fill_ns_link_info(struct perf_ns_link_info *ns_link_info,
{
struct path ns_path;
struct inode *ns_inode;
- void *error;
+ int error;
error = ns_get_path(&ns_path, task, ns_ops);
if (!error) {
--
2.24.0
^ permalink raw reply related
* [PATCH v18 03/13] namei: allow nd_jump_link() to produce errors
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Kees Cook, Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Rasmus Villemoes, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Christian Brauner, Aleksa Sarai, Linus Torvalds,
dev, containers, bpf, netdev
In-Reply-To: <20191206141338.23338-1-cyphar@cyphar.com>
In preparation for LOOKUP_NO_MAGICLINKS, it's necessary to add the
ability for nd_jump_link() to return an error which the corresponding
get_link() caller must propogate back up to the VFS.
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 3 ++-
fs/proc/base.c | 3 +--
fs/proc/namespaces.c | 14 +++++++++-----
include/linux/namei.h | 2 +-
security/apparmor/apparmorfs.c | 6 ++++--
5 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 5a47d9c09581..1024a641f075 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -859,7 +859,7 @@ static int nd_jump_root(struct nameidata *nd)
* Helper to directly jump to a known parsed path from ->get_link,
* caller must have taken a reference to path beforehand.
*/
-void nd_jump_link(struct path *path)
+int nd_jump_link(struct path *path)
{
struct nameidata *nd = current->nameidata;
path_put(&nd->path);
@@ -867,6 +867,7 @@ void nd_jump_link(struct path *path)
nd->path = *path;
nd->inode = nd->path.dentry->d_inode;
nd->flags |= LOOKUP_JUMPED;
+ return 0;
}
static inline void put_link(struct nameidata *nd)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ebea9501afb8..ee97dd322f3e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1626,8 +1626,7 @@ static const char *proc_pid_get_link(struct dentry *dentry,
if (error)
goto out;
- nd_jump_link(&path);
- return NULL;
+ error = nd_jump_link(&path);
out:
return ERR_PTR(error);
}
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index 08dd94df1a66..a8cca516f1a9 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -51,11 +51,15 @@ static const char *proc_ns_get_link(struct dentry *dentry,
if (!task)
return ERR_PTR(-EACCES);
- if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
- error = ns_get_path(&ns_path, task, ns_ops);
- if (!error)
- nd_jump_link(&ns_path);
- }
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
+ goto out;
+
+ error = ns_get_path(&ns_path, task, ns_ops);
+ if (error)
+ goto out;
+
+ error = nd_jump_link(&ns_path);
+out:
put_task_struct(task);
return ERR_PTR(error);
}
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 397a08ade6a2..758e9b47db6f 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -68,7 +68,7 @@ extern int follow_up(struct path *);
extern struct dentry *lock_rename(struct dentry *, struct dentry *);
extern void unlock_rename(struct dentry *, struct dentry *);
-extern void nd_jump_link(struct path *path);
+extern int __must_check nd_jump_link(struct path *path);
static inline void nd_terminate_link(void *name, size_t len, size_t maxlen)
{
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 45d13b6462aa..0b7d6dce6291 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -2455,16 +2455,18 @@ static const char *policy_get_link(struct dentry *dentry,
{
struct aa_ns *ns;
struct path path;
+ int error;
if (!dentry)
return ERR_PTR(-ECHILD);
+
ns = aa_get_current_ns();
path.mnt = mntget(aafs_mnt);
path.dentry = dget(ns_dir(ns));
- nd_jump_link(&path);
+ error = nd_jump_link(&path);
aa_put_ns(ns);
- return NULL;
+ return ERR_PTR(error);
}
static int policy_readlink(struct dentry *dentry, char __user *buffer,
--
2.24.0
^ permalink raw reply related
* [PATCH v18 04/13] namei: allow set_root() to produce errors
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Kees Cook, Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Rasmus Villemoes, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Christian Brauner, Aleksa Sarai, Linus Torvalds,
dev, containers, bpf, netdev
In-Reply-To: <20191206141338.23338-1-cyphar@cyphar.com>
For LOOKUP_BENEATH and LOOKUP_IN_ROOT it is necessary to ensure that
set_root() is never called, and thus (for hardening purposes) it should
return an error rather than permit a breakout from the root. In
addition, move all of the repetitive set_root() calls to nd_jump_root().
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 1024a641f075..74574a69a614 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -798,7 +798,7 @@ static int complete_walk(struct nameidata *nd)
return status;
}
-static void set_root(struct nameidata *nd)
+static int set_root(struct nameidata *nd)
{
struct fs_struct *fs = current->fs;
@@ -814,6 +814,7 @@ static void set_root(struct nameidata *nd)
get_fs_root(fs, &nd->root);
nd->flags |= LOOKUP_ROOT_GRABBED;
}
+ return 0;
}
static void path_put_conditional(struct path *path, struct nameidata *nd)
@@ -837,6 +838,11 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (!nd->root.mnt) {
+ int error = set_root(nd);
+ if (error)
+ return error;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1080,10 +1086,9 @@ const char *get_link(struct nameidata *nd)
return res;
}
if (*res == '/') {
- if (!nd->root.mnt)
- set_root(nd);
- if (unlikely(nd_jump_root(nd)))
- return ERR_PTR(-ECHILD);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
while (unlikely(*++res == '/'))
;
}
@@ -1698,8 +1703,13 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
- if (!nd->root.mnt)
- set_root(nd);
+ int error = 0;
+
+ if (!nd->root.mnt) {
+ error = set_root(nd);
+ if (error)
+ return error;
+ }
if (nd->flags & LOOKUP_RCU) {
return follow_dotdot_rcu(nd);
} else
@@ -2162,6 +2172,7 @@ static int link_path_walk(const char *name, struct nameidata *nd)
/* must be paired with terminate_walk() */
static const char *path_init(struct nameidata *nd, unsigned flags)
{
+ int error;
const char *s = nd->name->name;
if (!*s)
@@ -2194,11 +2205,13 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.dentry = NULL;
nd->m_seq = read_seqbegin(&mount_lock);
+
+ /* Figure out the starting path and root (if needed). */
if (*s == '/') {
- set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ return s;
} else if (nd->dfd == AT_FDCWD) {
if (flags & LOOKUP_RCU) {
struct fs_struct *fs = current->fs;
--
2.24.0
^ permalink raw reply related
* [PATCH v18 05/13] namei: LOOKUP_NO_SYMLINKS: block symlink resolution
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Christian Brauner, Linus Torvalds, Eric Biederman,
Andy Lutomirski, Andrew Morton, Kees Cook, Jann Horn,
Tycho Andersen, David Drysdale, Chanho Min, Oleg Nesterov,
Rasmus Villemoes, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Christian Brauner, Aleksa Sarai, dev, containers, bpf@
In-Reply-To: <20191206141338.23338-1-cyphar@cyphar.com>
/* Background. */
Userspace cannot easily resolve a path without resolving symlinks, and
would have to manually resolve each path component with O_PATH and
O_NOFOLLOW. This is clearly inefficient, and can be fairly easy to screw
up (resulting in possible security bugs). Linus has mentioned that Git
has a particular need for this kind of flag[1]. It also resolves a
fairly long-standing perceived deficiency in O_NOFOLLOw -- that it only
blocks the opening of trailing symlinks.
This is part of a refresh of Al's AT_NO_JUMPS patchset[2] (which was a
variation on David Drysdale's O_BENEATH patchset[3], which in turn was
based on the Capsicum project[4]).
/* Userspace API. */
LOOKUP_NO_SYMLINKS will be exposed to userspace through openat2(2).
/* Semantics. */
Unlike most other LOOKUP flags (most notably LOOKUP_FOLLOW),
LOOKUP_NO_SYMLINKS applies to all components of the path.
With LOOKUP_NO_SYMLINKS, any symlink path component encountered during
path resolution will yield -ELOOP. If the trailing component is a
symlink (and no other components were symlinks), then O_PATH|O_NOFOLLOW
will not error out and will instead provide a handle to the trailing
symlink -- without resolving it.
/* Testing. */
LOOKUP_NO_SYMLINKS is tested as part of the openat2(2) selftests.
[1]: https://lore.kernel.org/lkml/CA+55aFyOKM7DW7+0sdDFKdZFXgptb5r1id9=Wvhd8AgSP7qjwQ@mail.gmail.com/
[2]: https://lore.kernel.org/lkml/20170429220414.GT29622@ZenIV.linux.org.uk/
[3]: https://lore.kernel.org/lkml/1415094884-18349-1-git-send-email-drysdale@google.com/
[4]: https://lore.kernel.org/lkml/1404124096-21445-1-git-send-email-drysdale@google.com/
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 3 +++
include/linux/namei.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/fs/namei.c b/fs/namei.c
index 74574a69a614..303731935eb2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1052,6 +1052,9 @@ const char *get_link(struct nameidata *nd)
int error;
const char *res;
+ if (unlikely(nd->flags & LOOKUP_NO_SYMLINKS))
+ return ERR_PTR(-ELOOP);
+
if (!(nd->flags & LOOKUP_RCU)) {
touch_atime(&last->link);
cond_resched();
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 758e9b47db6f..0d86e75c04a7 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -39,6 +39,9 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_ROOT 0x2000
#define LOOKUP_ROOT_GRABBED 0x0008
+/* Scoping flags for lookup. */
+#define LOOKUP_NO_SYMLINKS 0x010000 /* No symlink crossing. */
+
extern int path_pts(struct path *path);
extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
--
2.24.0
^ permalink raw reply related
* [PATCH v18 06/13] namei: LOOKUP_NO_MAGICLINKS: block magic-link resolution
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Christian Brauner, David Drysdale, Andy Lutomirski,
Linus Torvalds, Eric Biederman, Andrew Morton, Kees Cook,
Jann Horn, Tycho Andersen, Chanho Min, Oleg Nesterov,
Rasmus Villemoes, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Christian Brauner, Aleksa Sarai, dev, containers, bpf@
In-Reply-To: <20191206141338.23338-1-cyphar@cyphar.com>
/* Background. */
There has always been a special class of symlink-like objects in procfs
(and a few other pseudo-filesystems) which allow for non-lexical
resolution of paths using nd_jump_link(). These "magic-links" do not
follow traditional mount namespace boundaries, and have been used
consistently in container escape attacks because they can be used to
trick unsuspecting privileged processes into resolving unexpected paths.
It is also non-trivial for userspace to unambiguously avoid resolving
magic-links, because they do not have a reliable indication that they
are a magic-link (in order to verify them you'd have to manually open
the path given by readlink(2) and then verify that the two file
descriptors reference the same underlying file, which is plagued with
possible race conditions or supplementary attack scenarios).
It would therefore be very helpful for userspace to be able to avoid
these symlinks easily, thus hopefully removing a tool from attackers'
toolboxes.
This is part of a refresh of Al's AT_NO_JUMPS patchset[1] (which was a
variation on David Drysdale's O_BENEATH patchset[2], which in turn was
based on the Capsicum project[3]).
/* Userspace API. */
LOOKUP_NO_MAGICLINKS will be exposed to userspace through openat2(2).
/* Semantics. */
Unlike most other LOOKUP flags (most notably LOOKUP_FOLLOW),
LOOKUP_NO_MAGICLINKS applies to all components of the path.
With LOOKUP_NO_MAGICLINKS, any magic-link path component encountered
during path resolution will yield -ELOOP. The handling of ~LOOKUP_FOLLOW
for a trailing magic-link is identical to LOOKUP_NO_SYMLINKS.
LOOKUP_NO_SYMLINKS implies LOOKUP_NO_MAGICLINKS.
/* Testing. */
LOOKUP_NO_MAGICLINKS is tested as part of the openat2(2) selftests.
[1]: https://lore.kernel.org/lkml/20170429220414.GT29622@ZenIV.linux.org.uk/
[2]: https://lore.kernel.org/lkml/1415094884-18349-1-git-send-email-drysdale@google.com/
[3]: https://lore.kernel.org/lkml/1404124096-21445-1-git-send-email-drysdale@google.com/
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: David Drysdale <drysdale@google.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 10 +++++++++-
include/linux/namei.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 303731935eb2..415a897729c8 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -867,13 +867,21 @@ static int nd_jump_root(struct nameidata *nd)
*/
int nd_jump_link(struct path *path)
{
+ int error = -ELOOP;
struct nameidata *nd = current->nameidata;
- path_put(&nd->path);
+ if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
+ goto err;
+
+ path_put(&nd->path);
nd->path = *path;
nd->inode = nd->path.dentry->d_inode;
nd->flags |= LOOKUP_JUMPED;
return 0;
+
+err:
+ path_put(path);
+ return error;
}
static inline void put_link(struct nameidata *nd)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 0d86e75c04a7..1573b8493d98 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -41,6 +41,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
/* Scoping flags for lookup. */
#define LOOKUP_NO_SYMLINKS 0x010000 /* No symlink crossing. */
+#define LOOKUP_NO_MAGICLINKS 0x020000 /* No nd_jump_link() crossing. */
extern int path_pts(struct path *path);
--
2.24.0
^ permalink raw reply related
* [PATCH v18 07/13] namei: LOOKUP_NO_XDEV: block mountpoint crossing
From: Aleksa Sarai @ 2019-12-06 14:13 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet
Cc: Aleksa Sarai, Christian Brauner, David Drysdale, Andy Lutomirski,
Linus Torvalds, Eric Biederman, Andrew Morton, Kees Cook,
Jann Horn, Tycho Andersen, Chanho Min, Oleg Nesterov,
Rasmus Villemoes, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Christian Brauner, Aleksa Sarai, dev, containers, bpf@
In-Reply-To: <20191206141338.23338-1-cyphar@cyphar.com>
/* Background. */
The need to contain path operations within a mountpoint has been a
long-standing usecase that userspace has historically implemented
manually with liberal usage of stat(). find, rsync, tar and
many other programs implement these semantics -- but it'd be much
simpler to have a fool-proof way of refusing to open a path if it
crosses a mountpoint.
This is part of a refresh of Al's AT_NO_JUMPS patchset[1] (which was a
variation on David Drysdale's O_BENEATH patchset[2], which in turn was
based on the Capsicum project[3]).
/* Userspace API. */
LOOKUP_NO_XDEV will be exposed to userspace through openat2(2).
/* Semantics. */
Unlike most other LOOKUP flags (most notably LOOKUP_FOLLOW),
LOOKUP_NO_XDEV applies to all components of the path.
With LOOKUP_NO_XDEV, any path component which crosses a mount-point
during path resolution (including "..") will yield an -EXDEV. Absolute
paths, absolute symlinks, and magic-links will only yield an -EXDEV if
the jump involved changing mount-points.
/* Testing. */
LOOKUP_NO_XDEV is tested as part of the openat2(2) selftests.
[1]: https://lore.kernel.org/lkml/20170429220414.GT29622@ZenIV.linux.org.uk/
[2]: https://lore.kernel.org/lkml/1415094884-18349-1-git-send-email-drysdale@google.com/
[3]: https://lore.kernel.org/lkml/1404124096-21445-1-git-send-email-drysdale@google.com/
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: David Drysdale <drysdale@google.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 31 +++++++++++++++++++++++++++----
include/linux/namei.h | 1 +
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 415a897729c8..321c8ad5d6b3 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -838,6 +838,11 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV)) {
+ /* Absolute path arguments to path_init() are allowed. */
+ if (nd->path.mnt != NULL && nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (!nd->root.mnt) {
int error = set_root(nd);
if (error)
@@ -873,6 +878,12 @@ int nd_jump_link(struct path *path)
if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
goto err;
+ error = -EXDEV;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV)) {
+ if (nd->path.mnt != path->mnt)
+ goto err;
+ }
+
path_put(&nd->path);
nd->path = *path;
nd->inode = nd->path.dentry->d_inode;
@@ -1280,12 +1291,16 @@ static int follow_managed(struct path *path, struct nameidata *nd)
break;
}
- if (need_mntput && path->mnt == mnt)
- mntput(path->mnt);
+ if (need_mntput) {
+ if (path->mnt == mnt)
+ mntput(path->mnt);
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ ret = -EXDEV;
+ else
+ nd->flags |= LOOKUP_JUMPED;
+ }
if (ret == -EISDIR || !ret)
ret = 1;
- if (need_mntput)
- nd->flags |= LOOKUP_JUMPED;
if (unlikely(ret < 0))
path_put_conditional(path, nd);
return ret;
@@ -1342,6 +1357,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
mounted = __lookup_mnt(path->mnt, path->dentry);
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1388,6 +1405,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (&mparent->mnt == nd->path.mnt)
break;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ return -ECHILD;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = &mparent->mnt;
@@ -1402,6 +1421,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ return -ECHILD;
nd->path.mnt = &mounted->mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1500,6 +1521,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(&nd->path))
break;
+ if (unlikely(nd->flags & LOOKUP_NO_XDEV))
+ return -EXDEV;
}
follow_mount(&nd->path);
nd->inode = nd->path.dentry->d_inode;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 1573b8493d98..25ee88c4acb1 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -42,6 +42,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
/* Scoping flags for lookup. */
#define LOOKUP_NO_SYMLINKS 0x010000 /* No symlink crossing. */
#define LOOKUP_NO_MAGICLINKS 0x020000 /* No nd_jump_link() crossing. */
+#define LOOKUP_NO_XDEV 0x040000 /* No mountpoint crossing. */
extern int path_pts(struct path *path);
--
2.24.0
^ permalink raw reply related
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