* Re: [PATCH 11/19] block: implement bio helper to add iter bvec pages to bio
From: Ming Lei @ 2019-02-28 8:37 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Ming Lei, Eric Biggers, open list:AIO, linux-block,
linux-api, Christoph Hellwig, Jeff Moyer, Avi Kivity, jannh,
Al Viro
In-Reply-To: <20190227194241.GA3860@infradead.org>
On Wed, Feb 27, 2019 at 11:42:41AM -0800, Christoph Hellwig wrote:
> On Tue, Feb 26, 2019 at 09:06:23PM -0700, Jens Axboe wrote:
> > On 2/26/19 9:05 PM, Jens Axboe wrote:
> > > On 2/26/19 8:44 PM, Ming Lei wrote:
> > >> On Tue, Feb 26, 2019 at 08:37:05PM -0700, Jens Axboe wrote:
> > >>> On 2/26/19 8:09 PM, Ming Lei wrote:
> > >>>> On Tue, Feb 26, 2019 at 07:43:32PM -0700, Jens Axboe wrote:
> > >>>>> On 2/26/19 7:37 PM, Ming Lei wrote:
> > >>>>>> On Tue, Feb 26, 2019 at 07:28:54PM -0700, Jens Axboe wrote:
> > >>>>>>> On 2/26/19 7:21 PM, Ming Lei wrote:
> > >>>>>>>> On Tue, Feb 26, 2019 at 06:57:16PM -0700, Jens Axboe wrote:
> > >>>>>>>>> On 2/26/19 6:53 PM, Ming Lei wrote:
> > >>>>>>>>>> On Tue, Feb 26, 2019 at 06:47:54PM -0700, Jens Axboe wrote:
> > >>>>>>>>>>> On 2/26/19 6:21 PM, Ming Lei wrote:
> > >>>>>>>>>>>> On Tue, Feb 26, 2019 at 11:56 PM Jens Axboe <axboe@kernel.dk> wrote:
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> On 2/25/19 9:34 PM, Jens Axboe wrote:
> > >>>>>>>>>>>>>> On 2/25/19 8:46 PM, Eric Biggers wrote:
> > >>>>>>>>>>>>>>> Hi Jens,
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> On Thu, Feb 21, 2019 at 10:45:27AM -0700, Jens Axboe wrote:
> > >>>>>>>>>>>>>>>> On 2/20/19 3:58 PM, Ming Lei wrote:
> > >>>>>>>>>>>>>>>>> On Mon, Feb 11, 2019 at 12:00:41PM -0700, Jens Axboe wrote:
> > >>>>>>>>>>>>>>>>>> For an ITER_BVEC, we can just iterate the iov and add the pages
> > >>>>>>>>>>>>>>>>>> to the bio directly. This requires that the caller doesn't releases
> > >>>>>>>>>>>>>>>>>> the pages on IO completion, we add a BIO_NO_PAGE_REF flag for that.
> > >>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>> The current two callers of bio_iov_iter_get_pages() are updated to
> > >>>>>>>>>>>>>>>>>> check if they need to release pages on completion. This makes them
> > >>>>>>>>>>>>>>>>>> work with bvecs that contain kernel mapped pages already.
> > >>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>> Reviewed-by: Hannes Reinecke <hare@suse.com>
> > >>>>>>>>>>>>>>>>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> > >>>>>>>>>>>>>>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > >>>>>>>>>>>>>>>>>> ---
> > >>>>>>>>>>>>>>>>>> block/bio.c | 59 ++++++++++++++++++++++++++++++++-------
> > >>>>>>>>>>>>>>>>>> fs/block_dev.c | 5 ++--
> > >>>>>>>>>>>>>>>>>> fs/iomap.c | 5 ++--
> > >>>>>>>>>>>>>>>>>> include/linux/blk_types.h | 1 +
> > >>>>>>>>>>>>>>>>>> 4 files changed, 56 insertions(+), 14 deletions(-)
> > >>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>> diff --git a/block/bio.c b/block/bio.c
> > >>>>>>>>>>>>>>>>>> index 4db1008309ed..330df572cfb8 100644
> > >>>>>>>>>>>>>>>>>> --- a/block/bio.c
> > >>>>>>>>>>>>>>>>>> +++ b/block/bio.c
> > >>>>>>>>>>>>>>>>>> @@ -828,6 +828,23 @@ int bio_add_page(struct bio *bio, struct page *page,
> > >>>>>>>>>>>>>>>>>> }
> > >>>>>>>>>>>>>>>>>> EXPORT_SYMBOL(bio_add_page);
> > >>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>> +static int __bio_iov_bvec_add_pages(struct bio *bio, struct iov_iter *iter)
> > >>>>>>>>>>>>>>>>>> +{
> > >>>>>>>>>>>>>>>>>> + const struct bio_vec *bv = iter->bvec;
> > >>>>>>>>>>>>>>>>>> + unsigned int len;
> > >>>>>>>>>>>>>>>>>> + size_t size;
> > >>>>>>>>>>>>>>>>>> +
> > >>>>>>>>>>>>>>>>>> + len = min_t(size_t, bv->bv_len, iter->count);
> > >>>>>>>>>>>>>>>>>> + size = bio_add_page(bio, bv->bv_page, len,
> > >>>>>>>>>>>>>>>>>> + bv->bv_offset + iter->iov_offset);
> > >>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>> iter->iov_offset needs to be subtracted from 'len', looks
> > >>>>>>>>>>>>>>>>> the following delta change[1] is required, otherwise memory corruption
> > >>>>>>>>>>>>>>>>> can be observed when running xfstests over loop/dio.
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> Thanks, I folded this in.
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> --
> > >>>>>>>>>>>>>>>> Jens Axboe
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> syzkaller started hitting a crash on linux-next starting with this commit, and
> > >>>>>>>>>>>>>>> it still occurs even with your latest version that has Ming's fix folded in.
> > >>>>>>>>>>>>>>> Specifically, commit a566653ab5ab80a from your io_uring branch with commit date
> > >>>>>>>>>>>>>>> Sun Feb 24 08:20:53 2019 -0700.
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> Reproducer:
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> #define _GNU_SOURCE
> > >>>>>>>>>>>>>>> #include <fcntl.h>
> > >>>>>>>>>>>>>>> #include <linux/loop.h>
> > >>>>>>>>>>>>>>> #include <sys/ioctl.h>
> > >>>>>>>>>>>>>>> #include <sys/sendfile.h>
> > >>>>>>>>>>>>>>> #include <sys/syscall.h>
> > >>>>>>>>>>>>>>> #include <unistd.h>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> int main(void)
> > >>>>>>>>>>>>>>> {
> > >>>>>>>>>>>>>>> int memfd, loopfd;
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> memfd = syscall(__NR_memfd_create, "foo", 0);
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> pwrite(memfd, "\xa8", 1, 4096);
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> loopfd = open("/dev/loop0", O_RDWR|O_DIRECT);
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> ioctl(loopfd, LOOP_SET_FD, memfd);
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> sendfile(loopfd, loopfd, NULL, 1000000);
> > >>>>>>>>>>>>>>> }
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> Crash:
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> page:ffffea0001a6aab8 count:0 mapcount:0 mapping:0000000000000000 index:0x0
> > >>>>>>>>>>>>>>> flags: 0x100000000000000()
> > >>>>>>>>>>>>>>> raw: 0100000000000000 ffffea0001ad2c50 ffff88807fca49d0 0000000000000000
> > >>>>>>>>>>>>>>> raw: 0000000000000000 0000000000000000 00000000ffffffff
> > >>>>>>>>>>>>>>> page dumped because: VM_BUG_ON_PAGE(page_ref_count(page) == 0)
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> I see what this is, I'll cut a fix for this tomorrow.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Folded in a fix for this, it's in my current io_uring branch and my for-next
> > >>>>>>>>>>>>> branch.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Hi Jens,
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> I saw the following change is added:
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> + if (size == len) {
> > >>>>>>>>>>>> + /*
> > >>>>>>>>>>>> + * For the normal O_DIRECT case, we could skip grabbing this
> > >>>>>>>>>>>> + * reference and then not have to put them again when IO
> > >>>>>>>>>>>> + * completes. But this breaks some in-kernel users, like
> > >>>>>>>>>>>> + * splicing to/from a loop device, where we release the pipe
> > >>>>>>>>>>>> + * pages unconditionally. If we can fix that case, we can
> > >>>>>>>>>>>> + * get rid of the get here and the need to call
> > >>>>>>>>>>>> + * bio_release_pages() at IO completion time.
> > >>>>>>>>>>>> + */
> > >>>>>>>>>>>> + get_page(bv->bv_page);
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Now the 'bv' may point to more than one page, so the following one may be
> > >>>>>>>>>>>> needed:
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> int i;
> > >>>>>>>>>>>> struct bvec_iter_all iter_all;
> > >>>>>>>>>>>> struct bio_vec *tmp;
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> mp_bvec_for_each_segment(tmp, bv, i, iter_all)
> > >>>>>>>>>>>> get_page(tmp->bv_page);
> > >>>>>>>>>>>
> > >>>>>>>>>>> I guess that would be the safest, even if we don't currently have more
> > >>>>>>>>>>> than one page in there. I'll fix it up.
> > >>>>>>>>>>
> > >>>>>>>>>> It is easy to see multipage bvec from loop, :-)
> > >>>>>>>>>
> > >>>>>>>>> Speaking of this, I took a quick look at why we've now regressed a lot
> > >>>>>>>>> on IOPS perf with the multipage work. It looks like it's all related to
> > >>>>>>>>> the (much) fatter setup around iteration, which is related to this very
> > >>>>>>>>> topic too.
> > >>>>>>>>>
> > >>>>>>>>> Basically setup of things like bio_for_each_bvec() and indexing through
> > >>>>>>>>> nth_page() is MUCH slower than before.
> > >>>>>>>>
> > >>>>>>>> But bio_for_each_bvec() needn't nth_page(), and only bio_for_each_segment()
> > >>>>>>>> needs that. However, bio_for_each_segment() isn't called from
> > >>>>>>>> blk_queue_split() and blk_rq_map_sg().
> > >>>>>>>>
> > >>>>>>>> One issue is that bio_for_each_bvec() still advances by page size
> > >>>>>>>> instead of bvec->len, I guess that is the problem, will cook a patch
> > >>>>>>>> for your test.
> > >>>>>>>
> > >>>>>>> Probably won't make a difference for my test case...
> > >>>>>>>
> > >>>>>>>>> We need to do something about this, it's like tossing out months of
> > >>>>>>>>> optimizations.
> > >>>>>>>>
> > >>>>>>>> Some following optimization can be done, such as removing
> > >>>>>>>> biovec_phys_mergeable() from blk_bio_segment_split().
> > >>>>>>>
> > >>>>>>> I think we really need a fast path for <= PAGE_SIZE IOs, to the extent
> > >>>>>>> that it is possible. But iteration startup cost is a problem in a lot of
> > >>>>>>> spots, and a split fast path will only help a bit for that specific
> > >>>>>>> case.
> > >>>>>>>
> > >>>>>>> 5% regressions is HUGE. I know I've mentioned this before, I just want
> > >>>>>>> to really stress how big of a deal that is. It's enough to make me
> > >>>>>>> consider just reverting it again, which sucks, but I don't feel great
> > >>>>>>> shipping something that is known that much slower.
> > >>>>>>>
> > >>>>>>> Suggestions?
> > >>>>>>
> > >>>>>> You mentioned nth_page() costs much in bio_for_each_bvec(), but which
> > >>>>>> shouldn't call into nth_page(). I will look into it first.
> > >>>>>
> > >>>>> I'll check on the test box tomorrow, I lost connectivity before. I'll
> > >>>>> double check in the morning.
> > >>>>>
> > >>>>> I'd focus on the blk_rq_map_sg() path, since that's the biggest cycle
> > >>>>> consumer.
> > >>>>
> > >>>> Hi Jens,
> > >>>>
> > >>>> Could you test the following patch which may improve on the 4k randio
> > >>>> test case?
> > >>>
> > >>> A bit, it's up 1% with this patch. I'm going to try without the
> > >>> get_page/put_page that we had earlier, to see where we are in regards to
> > >>> the old baseline.
> > >>
> > >> OK, today I will test io_uring over null_blk on one real machine and see
> > >> if something can be improved.
> > >
> > > For reference, I'm running the default t/io_uring from fio, which is
> > > QD=128, fixed files/buffers, and polled. Running it on two devices to
> > > max out the CPU core:
> > >
> > > sudo taskset -c 0 t/io_uring /dev/nvme1n1 /dev/nvme5n1
> >
> > Forgot to mention, this is loading nvme with 12 poll queues, which is of
> > course important to get good performance on this test case.
>
> Btw, is your nvme device SGL capable? There is some low hanging fruit
> in that IFF a device has SGL support we can basically dumb down
> blk_mq_map_sg to never split in this case ever because we don't have
> any segment size limits.
Indeed.
In case of SGL, big sg list may not be needed and blk_rq_map_sg() can be
skipped if proper DMA mapping interface is to return the dma address
for each segment. That can be one big improvement.
Thanks,
Ming
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH man-pages] Add rseq manpage
From: Michael Kerrisk (man-pages) @ 2019-02-28 8:42 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: mtk.manpages, linux-kernel, linux-api, Peter Zijlstra,
Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Thomas Gleixner,
Ingo Molnar, H . Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, Steven Rostedt, Josh Triplett, Linus Torvalds,
Catalin Marinas, Will
In-Reply-To: <20181206144228.9656-1-mathieu.desnoyers@efficios.com>
On 12/6/18 3:42 PM, Mathieu Desnoyers wrote:
> [ Michael, rseq(2) was merged into 4.18. Can you have a look at this
> patch which adds rseq documentation to the man-pages project ? ]
Hi Matthieu
Sorry for the long delay. I've merged this page into a private
branch and have done quite a lot of editing. I have many
questions :-).
In the first instance, I think it is probably best to have
a free-form text discussion rather than firing patches
back and forward. Could you take a look at the questions below
and respond?
Thanks,
Michael
RSEQ(2) Linux Programmer's Manual RSEQ(2)
NAME
rseq - Restartable sequences and CPU number cache
SYNOPSIS
#include <linux/rseq.h>
int rseq(struct rseq *rseq, uint32_t rseq_len, int flags, uint32_t sig);
DESCRIPTION
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│Imagine you are someone who is pretty new to this │
│idea... What is notably lacking from this page is │
│an overview explaining: │
│ │
│ * What a restartable sequence actually is. │
│ │
│ * An outline of the steps to perform when using │
│ restartable sequences / rseq(2). │
│ │
│I.e., something along the lines of Jon Corbet's │
│https://lwn.net/Articles/697979/. Can you come up │
│with something? (Part of it might be at the start of │
│this page, and the rest in NOTES; it need not be all │
│in one place.) │
└─────────────────────────────────────────────────────┘
The rseq() ABI accelerates user-space operations on per-CPU data by
defining a shared data structure ABI between each user-space thread and
the kernel.
It allows user-space to perform update operations on per-CPU data with‐
out requiring heavy-weight atomic operations.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the following para: "a hardware execution con‐ │
│text"? What is the contrast being drawn here? It │
│would be good to state it more explicitly. │
└─────────────────────────────────────────────────────┘
The term CPU used in this documentation refers to a hardware execution
context.
Restartable sequences are atomic with respect to preemption (making it
atomic with respect to other threads running on the same CPU), as well
as signal delivery (user-space execution contexts nested over the same
thread). They either complete atomically with respect to preemption on
the current CPU and signal delivery, or they are aborted.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the preceding sentence, we need a definition of │
│"current CPU". │
└─────────────────────────────────────────────────────┘
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the following, does "It is" means "Restartable │
│sequences are"? │
└─────────────────────────────────────────────────────┘
It is suited for update operations on per-CPU data.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the following, does "It is" means "Restartable │
│sequences are"? │
└─────────────────────────────────────────────────────┘
It can be used on data structures shared between threads within a
process, and on data structures shared between threads across different
processes.
Some examples of operations that can be accelerated or improved by this
ABI:
· Memory allocator per-CPU free-lists
· Querying the current CPU number
· Incrementing per-CPU counters
· Modifying data protected by per-CPU spinlocks
· Inserting/removing elements in per-CPU linked-lists
· Writing/reading per-CPU ring buffers content
· Accurately reading performance monitoring unit counters with respect
to thread migration
Restartable sequences must not perform system calls. Doing so may
result in termination of the process by a segmentation fault.
The rseq argument is a pointer to the thread-local rseq structure to be
shared between kernel and user-space. The layout of this structure is
shown below.
The rseq_len argument is the size of the struct rseq to register.
The flags argument is 0 for registration, or RSEQ_FLAG_UNREGISTER for
unregistration.
The sig argument is the 32-bit signature to be expected before the
abort handler code.
The rseq structure
The struct rseq is aligned on a 32-byte boundary. This structure is
extensible. Its size is passed as parameter to the rseq() system call.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│Below, I added the structure definition (in abbrevi‐ │
│ated form). Is there any reason not to do this? │
└─────────────────────────────────────────────────────┘
struct rseq {
__u32 cpu_id_start;
__u32 cpu_id;
union {
__u64 ptr64;
#ifdef __LP64__
__u64 ptr;
#else
....
#endif
} rseq_cs;
__u32 flags;
} __attribute__((aligned(4 * sizeof(__u64))));
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the text below, I think it would be helpful to │
│explicitly note which of these fields are set by the │
│kernel (on return from the reseq() call) and which │
│are set by the caller (before calling rseq()). Is │
│the following correct: │
│ │
│ cpu_id_start - initialized by caller to possible │
│ CPU number (e.g., 0), updated by kernel │
│ on return │
│ │
│ cpu_id - initialized to -1 by caller, │
│ updated by kernel on return │
│ │
│ rseq_cs - initialized by caller, either to NULL │
│ or a pointer to an 'rseq_cs' structure │
│ that is initialized by the caller │
│ │
│ flags - initialized by caller, used by kernel │
└─────────────────────────────────────────────────────┘
The structure fields are as follows:
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the following paragraph, and in later places, I │
│changed "current thread" to "calling thread". Okay? │
└─────────────────────────────────────────────────────┘
cpu_id_start
Optimistic cache of the CPU number on which the calling thread
is running. The value in this field is guaranteed to always be
a possible CPU number, even when rseq is not initialized. The
value it contains should always be confirmed by reading the
cpu_id field.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│What does the last sentence mean? │
└─────────────────────────────────────────────────────┘
This field is an optimistic cache in the sense that it is always
guaranteed to hold a valid CPU number in the range [0..(nr_pos‐
sible_cpus - 1)]. It can therefore be loaded by user-space and
used as an offset in per-CPU data structures without having to
check whether its value is within the valid bounds compared to
the number of possible CPUs in the system.
For user-space applications executed on a kernel without rseq
support, the cpu_id_start field stays initialized at 0, which is
indeed a valid CPU number. It is therefore valid to use it as
an offset in per-CPU data structures, and only validate whether
it's actually the current CPU number by comparing it with the
cpu_id field within the rseq critical section.
If the kernel does not provide rseq support, that cpu_id field
stays initialized at -1, so the comparison always fails, as
intended. It is then up to user-space to use a fall-back mecha‐
nism, considering that rseq is not available.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│The last sentence is rather difficult to grok. Can │
│we say some more here? │
└─────────────────────────────────────────────────────┘
cpu_id Cache of the CPU number on which the calling thread is running.
-1 if uninitialized.
rseq_cs
The rseq_cs field is a pointer to a struct rseq_cs (described
below). It is NULL when no rseq assembly block critical section
is active for the calling thread. Setting it to point to a
critical section descriptor (struct rseq_cs) marks the beginning
of the critical section.
flags Flags indicating the restart behavior for the calling thread.
This is mainly used for debugging purposes. Can be either:
RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│Each of the above values needs an explanation. │
│ │
│Is it correct that only one of the values may be │
│specified in 'flags'? I ask because in the 'rseq_cs' │
│structure below, the 'flags' field is a bit mask │
│where any combination of these flags may be ORed │
│together. │
│ │
└─────────────────────────────────────────────────────┘
The rseq_cs structure
The struct rseq_cs is aligned on a 32-byte boundary and has a fixed
size of 32 bytes.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│Below, I added the structure definition (in abbrevi‐ │
│ated form). Is there any reason not to do this? │
└─────────────────────────────────────────────────────┘
struct rseq_cs {
__u32 version;
__u32 flags;
__u64 start_ip;
__u64 post_commit_offset;
__u64 abort_ip;
} __attribute__((aligned(4 * sizeof(__u64))));
The structure fields are as follows:
version
Version of this structure.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│What does 'version' need to be initialized to? │
└─────────────────────────────────────────────────────┘
flags Flags indicating the restart behavior of this structure. Can be
a combination of:
RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│Each of the above values needs an explanation. │
└─────────────────────────────────────────────────────┘
start_ip
Instruction pointer address of the first instruction of the
sequence of consecutive assembly instructions.
post_commit_offset
Offset (from start_ip address) of the address after the last
instruction of the sequence of consecutive assembly instruc‐
tions.
abort_ip
Instruction pointer address where to move the execution flow in
case of abort of the sequence of consecutive assembly instruc‐
tions.
NOTES
A single library per process should keep the rseq structure in a
thread-local storage variable. The cpu_id field should be initialized
to -1, and the cpu_id_start field should be initialized to a possible
CPU value (typically 0).
Each thread is responsible for registering and unregistering its rseq
structure. No more than one rseq structure address can be registered
per thread at a given time.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the following paragraph, what is the difference │
│between "freed" and "reclaim"? I'm supposing they │
│mean the same thing, but it's not clear. And if they │
│do mean the same thing, then the first two sentences │
│appear to contain contradictory information. │
└─────────────────────────────────────────────────────┘
Memory of a registered rseq object must not be freed before the thread
exits. Reclaim of rseq object's memory must only be done after either
an explicit rseq unregistration is performed or after the thread exits.
Keep in mind that the implementation of the Thread-Local Storage (C
language __thread) lifetime does not guarantee existence of the TLS
area up until the thread exits.
In a typical usage scenario, the thread registering the rseq structure
will be performing loads and stores from/to that structure. It is how‐
ever also allowed to read that structure from other threads. The rseq
field updates performed by the kernel provide relaxed atomicity seman‐
tics, which guarantee that other threads performing relaxed atomic
reads of the CPU number cache will always observe a consistent value.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│In the preceding paragraph, can we reasonably add │
│some words to explain "relaxed atomicity semantics" │
│and "relaxed atomic reads"? │
└─────────────────────────────────────────────────────┘
RETURN VALUE
A return value of 0 indicates success. On error, -1 is returned, and
errno is set appropriately.
ERRORS
EBUSY Restartable sequence is already registered for this thread.
EFAULT rseq is an invalid address.
EINVAL Either flags contains an invalid value, or rseq contains an
address which is not appropriately aligned, or rseq_len contains
a size that does not match the size received on registration.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│The last case "rseq_len contains a size that does │
│not match the size received on registration" can │
│occur only on RSEQ_FLAG_UNREGISTER, tight? │
└─────────────────────────────────────────────────────┘
ENOSYS The rseq() system call is not implemented by this kernel.
EPERM The sig argument on unregistration does not match the signature
received on registration.
VERSIONS
The rseq() system call was added in Linux 4.18.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│What is the current state of library support? │
└─────────────────────────────────────────────────────┘
CONFORMING TO
rseq() is Linux-specific.
┌─────────────────────────────────────────────────────┐
│FIXME │
├─────────────────────────────────────────────────────┤
│Is there any example code that can reasonably be │
│included in this manual page? Or some example code │
│that can be referred to? │
└─────────────────────────────────────────────────────┘
SEE ALSO
sched_getcpu(3), membarrier(2)
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Michael Kerrisk (man-pages) @ 2019-02-28 12:52 UTC (permalink / raw)
To: Tycho Andersen, Serge E. Hallyn
Cc: mtk.manpages, linux-man, Kees Cook, Linux API, lkml,
Andy Lutomirski, Jann Horn, Oleg Nesterov, Christian Brauner,
Eric W. Biederman, Containers, Aleksa Sarai, Tyler Hicks,
Akihiro Suda
In-Reply-To: <20181213001106.15268-3-tycho@tycho.ws>
[widening CC to the same scope as the code patches]
Hello Tycho,
Sorry for the late response on this. Could you amend/rebase your patches
in light of the below please.
And anyone's comment on my "big picture" text at the foot of this
mail would be very welcome.
On 12/13/18 1:11 AM, Tycho Andersen wrote:
> Here's some documentation on how to use the userspace notification. I tried
> to mention the biggest TOCTOU gotcha, but the pointer to the sample is
> really the best thing,
Cough! No, the "best thing" really is a good sample program *and* really
good documentation :-).
> as it has actual code and many comments about what's
> going on.
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: Kees Cook <keescook@chromium.org>
> ---
> man2/seccomp.2 | 85 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/man2/seccomp.2 b/man2/seccomp.2
> index d69187783..fef8914bf 100644
> --- a/man2/seccomp.2
> +++ b/man2/seccomp.2
> @@ -228,6 +228,13 @@ An administrator may override this filter flag by preventing specific
> actions from being logged via the
> .IR /proc/sys/kernel/seccomp/actions_logged
> file.
> +.TP
> +.BR SECCOMP_FILTER_FLAG_NEW_LISTENER " (since Linux 4.21)"
> +With this flag, a new userspace notification fd is returned on success. When
> +this filter returns
> +.BR SECCOMP_RET_USER_NOTIF
> +a notification will be sent to this fd. See "Userspace Notification" below for
s/fd/file descriptor/ throughout please.
> +more details.
I think the description here could be better worded as something like:
SECCOMP_FILTER_FLAG_NEW_LISTENER
Register a new filter, as usual, but on success return a
new file descriptor that provides user-space notifications.
When the filter returns SECCOMP_RET_USER_NOTIF, a notification
will be provided via this file descriptor. The close-on-exec
flag is automatically set on the new file descriptor. ...
> .RE
> .TP
> .BR SECCOMP_GET_ACTION_AVAIL " (since Linux 4.14)"
> @@ -606,6 +613,17 @@ file.
> .TP
> .BR SECCOMP_RET_ALLOW
> This value results in the system call being executed.
> +.TP
> +.BR SECCOMP_RET_USER_NOTIF " (since Linux 4.21)"
Please see the start of this hanging list in the manual page.
Can you confirm that SECCOMP_RET_USER_NOTIF really is the lowest
in the precedence order of all of the filter return values?
> +Forwards the syscall to an attached listener in userspace to allow userspace to
s/syscall/system call throughout please.
> +decide what to do with the syscall. If there is no attached listener (either
> +because the filter was not installed with the
> +.BR SECCOMP_FILTER_FLAG_NEW_LISTENER
> +or because the fd was closed), the filter returns
> +.BR ENOSYS
> +similar to what happens when a filter returns
> +.BR SECCOMP_RET_TRACE
> +and there is no tracer. See "Userspace Notification" below for more details.
> .PP
> If an action value other than one of the above is specified,
> then the filter action is treated as either
> @@ -693,10 +711,75 @@ Otherwise, if kernel auditing is enabled and the process is being audited
> the action is logged.
> .IP *
> Otherwise, the action is not logged.
> +.SS Userspace Notification
> +Interactin userspace notification functionality in seccomp is primarily done
> +via file descriptor.
That sentence is somewhat garbled. Even if I correct the typo,
I still don't really understand it. Could you try again?
(Or, take a look at my "big picture" text at the foot of this mail.)
> This file descriptor can be obtained by passing
> +.BR SECCOMP_FILTER_FLAG_NEW_LISTENER
> +as a filter flag when installing a new filter.
> +.PP
> +Once an fd is obtained, userspace can wait for events using
> +.BR poll ()
and presumably select() and epoll?
What kind of notification event do poll(2)/epoll(7)/select(2) provide?
It looks to be POLLIN/EPOLLIN/readable. Is that correct?
These details should be noted here. More generally, my assumption
is that you can use poll(2)/epoll(7)/select(2) to find out about the
availability of an event, and then use SECCOMP_IOCTL_NOTIF_RECV
to read that event. Correct? The text needs to be more explicit on
this.
> +or
> +.BR ioctl ().
> +The supported
> +.BR ioctl ()
> +operations on a notification fd are:
> +.TP
> +.BR SECCOMP_IOCTL_NOTIF_RECV
> +The argument to this command should be a pointer to a struct seccomp_notif:
> +.IP
> +.in +4n
> +.EX
> +struct seccomp_notif {
> + __u64 id;
> + __u32 pid;
> + __u32 flags;
> + struct seccomp_data data;
> +};
> +.EE
> +.in
> +.IP
> +The id field is a filter-unique id for this syscall, and should be supplied in
> +the response. It can additionally be used in
> +.BR SECCOMP_IOCTL_ID_VALID
> +to test whether or not the request is still alive. The pid here is the pid of
> +the task as visible from the listener's pid namespace. If the pid is not
> +visible, it is 0. Flags is unused right now.
So, is 'flags' explicitly zeroed by the kernel? the manual page should note
this.
> struct seccomp_data is the same
> +data that would be passed to a filter running in the kernel.
What are the semantics if multiple monitoring processes are employing
SECCOMP_IOCTL_NOTIF_RECV? Does only one of them get awoken? (Which one?)
Or do they all get woken up and get a 'struct seccomp_notif'? The semantics
should be detailed here.
> +.TP
> +.BR SECCOMP_IOCTL_NOTIF_SEND
> +The argument to this command should be a pointer to a struct seccomp_notif_resp:
> +.IP
> +.in +4n
> +.EX
> +struct seccomp_notif_resp {
> + __u64 id;
> + __s64 val;
> + __s32 error;
> + __u32 flags;
> +};
> +.EE
> +.in
> +.IP
> +The id should be the id from struct seccomp_notif; if error is non-zero, it is
> +used as the return value, otherwise val is. Flags must be 0 right now.
There really isn't enough detail here to help the reader understand. What
is the purpose of 'error' vs 'val'? In particular, I'm assuming that the
monitoring process has (at least) two choices with respect to the system
call being made by the target process:
(1) Cause the system call to fail with an error.
(2) Allow the system call to proceed.
How are 'error' and 'val' used in these two cases?
Are there more than these two cases? How else is 'val' used?
Also, what happens if the monitoring process does a SECCOMP_IOCTL_NOTIF_RECV,
but does not subsequently do a SECCOMP_IOCTL_NOTIF_SEND? This should be
detailed in the manual page.
What are the semantics if multiple monitoring processes are employing
SECCOMP_IOCTL_NOTIF_SEND? An error for all but the first? The semantics
should be detailed here.
> +.TP
> +.BR SECCOMP_IOCTL_NOTIF_ID_VALID
> +The argument to this command is just the id to be tested. There is a pid reuse
> +issue where a task may make a syscall, die, its pid get re-used, and the
> +listener may operate on the wrong pid. So the process for handling modifying a
> +pid's state in some way would be to open the pid's resources (/proc/pid/mem or
> +similar), do this call to verify that the id is still valid, and then use the
> +resource. See the sample below for more detail.
> +.PP
> +A complete example is available in the kernel tree at
> +.IR samples/seccomp/user-trap.c .
> .SH RETURN VALUE
> On success,
> .BR seccomp ()
> -returns 0.
> +returns 0, unless
> +.BR SECCOMP_FILTER_FLAG_NEW_LISTENER
> +was specified, in which case it returns the fd number for the new listener fd.
> On error, if
> .BR SECCOMP_FILTER_FLAG_TSYNC
> was used,
More generally though, I'm struggling with this patch because "the
big picture" is lacking. Here's my estimate of what I think that
picture is:
[[
Instead of having the seccomp() filter decision being made
within the filter itself, it is possible to pass off the
decision making to a (different) user-space process that
makes the decision. This is done using the following steps.
1. The "target process" (i.e., process that will
establish a seccomp filter) and the process that will make
decisions about system calls ("the monitoring process")
establish a connection using a UNIX domain socket. This
socket will subsequently be used to exchange a file
descriptor.
2. The "target process" establishes a seccomp BPF filter in the
usual manner, but with two notable differences:
* The seccomp() 'flags' argument includes the flag
SECCOMP_FILTER_FLAG_NEW_LISTENER. Consequently, the return
value of a successful seccomp() call is a new "listening"
file descriptor that can be used for monitoring.
* In cases where it is needed, the BPF filter returns the
special action SECCOMP_RET_USER_NOTIF. This return value
will trigger a notification event.
3. The "target process" passes the "listening file descriptor"
to the "monitoring process" via the UNIX domain socket.
4. The target process then performs its workload, which includes
system calls that will be controlled by the BPF filter.
Whenever one of these system calls causes the BPF filter to
return SECCOMP_RET_USER_NOTIF, a notification event is
generated on the listening file descriptor.
5. The monitoring process will receive notification events
on the listening file descriptor. These events are returned
as structures of type 'seccomp_notif'. Because this structure
and its size may evolve over kernel version, the monitoring
process must first determine the size of this structure using
the seccomp() SECCOMP_GET_NOTIF_SIZES operation, which
returns a structure of type 'seccomp_notif_sizes'. The
monitoring process allocates a buffer of size
'seccomp_notif_sizes.seccomp_notif' bytes to receive
monitoring events. In addition,the monitoring process
allocates another buffer of size
'seccomp_notif_sizes.seccomp_notif_resp' bytes for the
response (a 'struct seccomp_notif_resp') that it will
provide to the kernel in order to advise how the system
call being made by the target process shall be treated.
6. The monitoring process can now repeatedly monitor the
listening file descriptor.
When a SECCOMP_RET_USER_NOTIF-triggered event occurs,
the file descriptor will test as readable for
poll(2)/epoll(7)/select(2). The call
ioctl(listenfd, SECCOMP_IOCTL_NOTIF_RECV, reqptr)
can be used to read info on the event; this operation
blocks until an event is available. It populates
the 'struct seccomp_notif' pointed to by the third
argument with information about the system call
that is being attempted by the target process.
7. The monitoring process can use the information in the
'struct seccomp_notif' to make a determination about the
system call being made by the target process. This
structure includes a 'data' field that is the same
'struct seccomp_data' that is passed to a BPF filter.
In addition, the monitoring process may make use of other
information that is available from user space. For example,
it may inspect the memory of the target process (whose PID
is provided in the 'struct seccomp_notif') using
/proc/PID/mem, which includes inspecting the values
pointed to by system call arguments (whose location is
available 'seccomp_notif.data.args). However, when using
the target process PID in this way, one must guard against
PID re-use race conditions using the seccomp()
SECCOMP_IOCTL_NOTIF_ID_VALID operation.
8. Having arrived at a decision about the target process's
system call, the monitoring process can inform the kernel
of its decision using the operation
ioctl(listenfd, SECCOMP_IOCTL_NOTIF_SEND, respptr)
where the third argument is a pointer to a
'struct seccomp_notif_resp'. [Some more details
needed here, but I still don't yet understand fully
the semantics of the 'error' and 'val' fields.]
I'd like to include text along the above lines at the start of
the "Userspace Notification" section. Could you take a look at
the above and let me know any errors, improved terminology, or
other improvements generally. I'll then work that text into
the page.
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Michael Kerrisk (man-pages) @ 2019-02-28 13:25 UTC (permalink / raw)
To: Tycho Andersen, Serge E. Hallyn
Cc: mtk.manpages, linux-man, Kees Cook, Linux API, lkml,
Andy Lutomirski, Jann Horn, Oleg Nesterov, Christian Brauner,
Eric W. Biederman, Containers, Aleksa Sarai, Tyler Hicks,
Akihiro Suda
In-Reply-To: <2cea5fec-e73e-5749-18af-15c35a4bd23c@gmail.com>
> 7. The monitoring process can use the information in the
> 'struct seccomp_notif' to make a determination about the
> system call being made by the target process. This
> structure includes a 'data' field that is the same
> 'struct seccomp_data' that is passed to a BPF filter.
>
> In addition, the monitoring process may make use of other
> information that is available from user space. For example,
> it may inspect the memory of the target process (whose PID
> is provided in the 'struct seccomp_notif') using
> /proc/PID/mem, which includes inspecting the values
> pointed to by system call arguments (whose location is
> available 'seccomp_notif.data.args). However, when using
> the target process PID in this way, one must guard against
> PID re-use race conditions using the seccomp()
> SECCOMP_IOCTL_NOTIF_ID_VALID operation.
>
> 8. Having arrived at a decision about the target process's
> system call, the monitoring process can inform the kernel
> of its decision using the operation
>
> ioctl(listenfd, SECCOMP_IOCTL_NOTIF_SEND, respptr)
>
> where the third argument is a pointer to a
> 'struct seccomp_notif_resp'. [Some more details
> needed here, but I still don't yet understand fully
> the semantics of the 'error' and 'val' fields.]
So clearly, I misunderstood these last two steps.
(7) is something like: discover information in userspace
as required; perform userspace actions if appropriate
(perhaps doing the system call operation "on behalf of" the
target process).
(8) is something like:
set 'error' and 'val' to return info to the target process:
* error != 0 ==> make it look like the syscall failed,
with 'errno' set to that value
* error == 0 ==> make it look like the syscall succeeded
and returned 'val'
Right?
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Lorenz Bauer @ 2019-02-28 15:11 UTC (permalink / raw)
To: Martin Lau
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <20190226053751.flbhnbp4gkocfh7a@kafai-mbp.dhcp.thefacebook.com>
On Tue, 26 Feb 2019 at 05:38, Martin Lau <kafai@fb.com> wrote:
>
> On Mon, Feb 25, 2019 at 06:26:42PM +0000, Lorenz Bauer wrote:
> > On Sat, 23 Feb 2019 at 00:44, Martin Lau <kafai@fb.com> wrote:
> > >
> > > On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> > > > Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> > > > to a known connection. However, there is one corner case: no sockets are
> > > > created if SYN cookies are active. This means that the final ACK in the
> > > > 3WHS is misclassified.
> > > >
> > > > Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> > > > and then check whether a packet is a valid SYN cookie ACK.
> > > >
> > > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > > ---
> > > > include/uapi/linux/bpf.h | 18 ++++++++++-
> > > > net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 85 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index bcdd2474eee7..bc2af87e9621 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -2359,6 +2359,21 @@ union bpf_attr {
> > > > * Return
> > > > * A **struct bpf_tcp_sock** pointer on success, or NULL in
> > > > * case of failure.
> > > > + *
> > > > + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> > > > + * Description
> > > > + * Check whether iph and th contain a valid SYN cookie ACK for
> > > > + * the listening socket in sk.
> > > > + *
> > > > + * iph points to the start of the IPv4 or IPv6 header, while
> > > > + * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> > > > + *
> > > > + * th points to the start of the TCP header, while th_len contains
> > > > + * sizeof(struct tcphdr).
> > > > + *
> > > > + * Return
> > > > + * 0 if iph and th are a valid SYN cookie ACK, or a negative error
> > > > + * otherwise.
> > > > */
> > > > #define __BPF_FUNC_MAPPER(FN) \
> > > > FN(unspec), \
> > > > @@ -2457,7 +2472,8 @@ union bpf_attr {
> > > > FN(spin_lock), \
> > > > FN(spin_unlock), \
> > > > FN(sk_fullsock), \
> > > > - FN(tcp_sock),
> > > > + FN(tcp_sock), \
> > > > + FN(sk_check_syncookie),
> > > >
> > > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > > * function eBPF program intends to call
> > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > index 85749f6ec789..9e68897cc7ed 100644
> > > > --- a/net/core/filter.c
> > > > +++ b/net/core/filter.c
> > > > @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> > > > .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> > > > };
> > > >
> > > > +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> > > s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/>
> > >
> > > > + struct tcphdr *, th, u32, th_len)
> > > > +{
> > > > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> > > nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
> > >
> > > > + u32 cookie;
> > > > + int ret;
> > > > +
> > > > + if (unlikely(th_len < sizeof(*th)))
> > > > + return -EINVAL;
> > > > +
> > > > + /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> > > > + if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> > > From the test program in patch 3, the "sk" here is obtained from
> > > bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
> > > AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
> > > even if there is a request_sock. Does it make sense to check
> > > syncookie if there is already a request_sock?
> >
> > No, that doesn't make a lot of sense. I hadn't realised that
> > sk_lookup_tcp only returns full sockets.
> > This means we need a way to detect that there is a request sock for a
> > given tuple.
> >
> > * adding a reqsk_exists(tuple) helper means we have to pay the lookup cost twice
> > * drop the sk argument and do the necessary lookups in the helper
> > itself, but that also
> > wastes a call to __inet_lookup_listener
> > * skip sk_to_full_sk() in a helper and return RET_PTR_TO_SOCK_COMMON,
> > but that violates a bunch of assumptions (e.g. calling bpf_sk_release on them)
> How about creating a new lookup helper, bpf_sk"c"_lookup_tcp,
> that does not call sk_to_full_sk() before returning.
> Its ".ret_type" will be RET_PTR_TO_SOCK_COMMON_OR_NULL which its
> reference(-counting) state has to be tracked in the verifier also.
> Mainly in check_helper_call(), iirc.
>
> The bpf_prog can then check bpf_sock->state for TCP_LISTEN,
> call bpf_tcp_sock() to get the TCP listener sock and pass to
> the bpf_tcp_check_syncookie()
I've started working on this, and I've hit a snag with the reference
tracking behaviour
of bpf_tcp_sock. From what I can tell, the assumption is that a PTR_TO_TCP_SOCK
doesn't need reference tracking, because its either skb->sk or a TCP listener.
In the former case, the socket is refcounted via the sk_buff, in the
latter we don't need
to worry since the eBPF is called with the RCU read lock held.
However, non-listening sockets returned by bpf_sk_lookup_tcp, can be
freed before the
end of the eBPF program. Doing bpf_sk_lookup_tcp, bpf_tcp_sock,
bpf_sk_release allows
eBPF to gain a (read-only) reference to a freed socket. I've attached
a patch with a testcase
which illustrates this issue.
Is this the intended behaviour? If not, maybe it would be the easiest
to make bpf_tcp_sock
increase the refcount if !SOCK_RCU_FREE and require a corresponding
bpf_sk_release?
That would simplify my work to add RET_PTR_TO_SOCK_COMMON as wel..
>
> >
> > For context: ultimately we want use this to answer the question: does
> > this (encapsulated)
> > packet contain a payload destined to a local socket? Amongst the edge
> > cases we need to
> > handle are ICMP Packet Too Big messages and SYN cookies. A solution
> > would be to hide
> > all this in an "uber" helper that takes pointers to the L3 / L4
> > headers and returns a verdict,
> > but that seems a bit gross.
> Please include this use case in the commit message.
> It is useful.
>
> >
> > >
> > > > + return -EINVAL;
> > > > +
> > > > + if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> > > Should tcp_synq_no_recent_overflow(tp) be checked also?
> > >
> >
> > Yes, not sure how that slipped out.
> >
> > > > + return -EINVAL;
> > > > +
> > > > + if (!th->ack || th->rst)
> > > How about th->syn?
> > >
> >
> > Yes, I missed the fact that the callers in tcp_ipv{4,6}.c check this.
> >
> > > > + return -ENOENT;
> > > > +
> > > > + cookie = ntohl(th->ack_seq) - 1;
> > > > +
> > > > + switch (sk->sk_family) {
> > > > + case AF_INET:
> > > > + if (unlikely(iph_len < sizeof(struct iphdr)))
> > > > + return -EINVAL;
> > > > +
> > > > + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> > > > + break;
> > > > +
> > > > +#if IS_ENABLED(CONFIG_IPV6)
> > > > + case AF_INET6:
> > > > + if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> > > > + return -EINVAL;
> > > > +
> > > > + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> > > > + break;
> > > > +#endif /* CONFIG_IPV6 */
> > > > +
> > > > + default:
> > > > + return -EPROTONOSUPPORT;
> > > > + }
> > > > +
> > > > + if (ret > 0)
> > > > + return 0;
> > > > +
> > > > + return -ENOENT;
> > > > +#else
> > > > + return -ENOTSUP;
> > > > +#endif
> > > > +}
> > > > +
> > > > +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> > > > + .func = bpf_sk_check_syncookie,
> > > > + .gpl_only = true,
> > > > + .pkt_access = true,
> > > > + .ret_type = RET_INTEGER,
> > > > + .arg1_type = ARG_PTR_TO_SOCKET,
> > > I think it should be ARG_PTR_TO_TCP_SOCK
> > >
> > > > + .arg2_type = ARG_PTR_TO_MEM,
> > > > + .arg3_type = ARG_CONST_SIZE,
> > > > + .arg4_type = ARG_PTR_TO_MEM,
> > > > + .arg5_type = ARG_CONST_SIZE,
> > > > +};
> > > > +
> > > > #endif /* CONFIG_INET */
> >
> >
> >
> > --
> > Lorenz Bauer | Systems Engineer
> > 25 Lavington St., London SE1 0NZ
> >
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__www.cloudflare.com&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=xhDwvX3iD-mbqSrx-L8XQNaZiYFZzMWNo_2Y38Z9j34&s=I4Ag3HflabFppFv7UtMp8WnMVSqCDW0W28ziWIvuwDE&e=
---
tools/testing/selftests/bpf/verifier/sock.c | 23 +++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/sock.c
b/tools/testing/selftests/bpf/verifier/sock.c
index 0ddfdf76aba5..3307cca6bdd5 100644
--- a/tools/testing/selftests/bpf/verifier/sock.c
+++ b/tools/testing/selftests/bpf/verifier/sock.c
@@ -382,3 +382,26 @@
.result = REJECT,
.errstr = "type=tcp_sock expected=sock",
},
+{
+ "use bpf_tcp_sock after bpf_sk_release",
+ .insns = {
+ BPF_SK_LOOKUP,
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+ BPF_EMIT_CALL(BPF_FUNC_sk_release),
+ BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct
bpf_tcp_sock, snd_cwnd)),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .result = REJECT,
+ .errstr = "bogus",
+},
^ permalink raw reply related
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Martin Lau @ 2019-02-28 17:37 UTC (permalink / raw)
To: Lorenz Bauer
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <CACAyw9_JXEtqOMQGWwTngX2xfMERrpqQiqiFyYfyrx1-=k2MJQ@mail.gmail.com>
On Thu, Feb 28, 2019 at 03:11:09PM +0000, Lorenz Bauer wrote:
> I've started working on this, and I've hit a snag with the reference
> tracking behaviour
> of bpf_tcp_sock. From what I can tell, the assumption is that a PTR_TO_TCP_SOCK
> doesn't need reference tracking, because its either skb->sk or a TCP listener.
> In the former case, the socket is refcounted via the sk_buff, in the
> latter we don't need
> to worry since the eBPF is called with the RCU read lock held.
>
> However, non-listening sockets returned by bpf_sk_lookup_tcp, can be
> freed before the
> end of the eBPF program. Doing bpf_sk_lookup_tcp, bpf_tcp_sock,
> bpf_sk_release allows
> eBPF to gain a (read-only) reference to a freed socket. I've attached
> a patch with a testcase
> which illustrates this issue.
>
> Is this the intended behaviour? If not, maybe it would be the easiest
> to make bpf_tcp_sock
> increase the refcount if !SOCK_RCU_FREE and require a corresponding
> bpf_sk_release?
Increase the refcount at runtime may be a too big hammer for this.
Let me think if it can be resolved within the verifier.
> That would simplify my work to add RET_PTR_TO_SOCK_COMMON as wel..
>
> ---
> tools/testing/selftests/bpf/verifier/sock.c | 23 +++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/verifier/sock.c
> b/tools/testing/selftests/bpf/verifier/sock.c
> index 0ddfdf76aba5..3307cca6bdd5 100644
> --- a/tools/testing/selftests/bpf/verifier/sock.c
> +++ b/tools/testing/selftests/bpf/verifier/sock.c
> @@ -382,3 +382,26 @@
> .result = REJECT,
> .errstr = "type=tcp_sock expected=sock",
> },
> +{
> + "use bpf_tcp_sock after bpf_sk_release",
> + .insns = {
> + BPF_SK_LOOKUP,
> + BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
> + BPF_EXIT_INSN(),
> + BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
> + BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> + BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
> + BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
> + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> + BPF_EMIT_CALL(BPF_FUNC_sk_release),
> + BPF_EXIT_INSN(),
> + BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
> + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> + BPF_EMIT_CALL(BPF_FUNC_sk_release),
> + BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct
> bpf_tcp_sock, snd_cwnd)),
> + BPF_EXIT_INSN(),
> + },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .result = REJECT,
> + .errstr = "bogus",
> +},
> --
> 2.19.1
^ permalink raw reply
* Re: [PATCH] numa: Change get_mempolicy() to use nr_node_ids instead of MAX_NUMNODES
From: Andrew Morton @ 2019-02-28 19:11 UTC (permalink / raw)
To: Vlastimil Babka
Cc: rcampbell, linux-mm, Waiman Long, Linux API, Alexander Duyck,
Andi Kleen, Florian Weimer, Linus Torvalds,
stable@vger.kernel.org
In-Reply-To: <32575d26-b141-6985-833a-12d48c0dce6a@suse.cz>
On Wed, 27 Feb 2019 19:38:47 +0100 Vlastimil Babka <vbabka@suse.cz> wrote:
> On 2/11/19 8:27 PM, Andrew Morton wrote:
> > On Mon, 11 Feb 2019 10:02:45 -0800 <rcampbell@nvidia.com> wrote:
> >
> >> From: Ralph Campbell <rcampbell@nvidia.com>
> >>
> >> The system call, get_mempolicy() [1], passes an unsigned long *nodemask
> >> pointer and an unsigned long maxnode argument which specifies the
> >> length of the user's nodemask array in bits (which is rounded up).
> >> The manual page says that if the maxnode value is too small,
> >> get_mempolicy will return EINVAL but there is no system call to return
> >> this minimum value. To determine this value, some programs search
> >> /proc/<pid>/status for a line starting with "Mems_allowed:" and use
> >> the number of digits in the mask to determine the minimum value.
> >> A recent change to the way this line is formatted [2] causes these
> >> programs to compute a value less than MAX_NUMNODES so get_mempolicy()
> >> returns EINVAL.
> >>
> >> Change get_mempolicy(), the older compat version of get_mempolicy(), and
> >> the copy_nodes_to_user() function to use nr_node_ids instead of
> >> MAX_NUMNODES, thus preserving the defacto method of computing the
> >> minimum size for the nodemask array and the maxnode argument.
> >>
> >> [1] http://man7.org/linux/man-pages/man2/get_mempolicy.2.html
> >> [2] https://lore.kernel.org/lkml/1545405631-6808-1-git-send-email-longman@redhat.com
>
> Please, the next time include linux-api and people involved in the previous
> thread [1] into the CC list. Likely there should have been a Suggested-by: for
> Alexander as well.
>
> >>
> >
> > Ugh, what a mess.
>
> I'm afraid it's even somewhat worse mess now.
>
> > For a start, that's a crazy interface. I wish that had been brought to
> > our attention so we could have provided a sane way for userspace to
> > determine MAX_NUMNODES.
> >
> > Secondly, 4fb8e5b89bcbbb ("include/linux/nodemask.h: use nr_node_ids
> > (not MAX_NUMNODES) in __nodemask_pr_numnodes()") introduced a
>
> There's no such commit, that sha was probably from linux-next. The patch is
> still in mmotm [1]. Luckily, I would say. Maybe Linus or some automation could
> run some script to check for bogus Fixes tags before accepting patches?
Ah, that's a relief.
How about we just drop "include/linux/nodemask.h: use nr_node_ids (not
MAX_NUMNODES) in __nodemask_pr_numnodes()"
(https://ozlabs.org/~akpm/mmotm/broken-out/include-linux-nodemaskh-use-nr_node_ids-not-max_numnodes-in-__nodemask_pr_numnodes.patch)?
It's just a cosmetic thing, really.
^ permalink raw reply
* Re: [PATCH] numa: Change get_mempolicy() to use nr_node_ids instead of MAX_NUMNODES
From: Vlastimil Babka @ 2019-02-28 20:43 UTC (permalink / raw)
To: Andrew Morton
Cc: rcampbell, linux-mm, Waiman Long, Linux API, Alexander Duyck,
Andi Kleen, Florian Weimer, Linus Torvalds,
stable@vger.kernel.org
In-Reply-To: <20190228111110.564d84f62a1b294ca5b1f9df@linux-foundation.org>
On 2/28/19 8:11 PM, Andrew Morton wrote:
>>> Secondly, 4fb8e5b89bcbbb ("include/linux/nodemask.h: use nr_node_ids
>>> (not MAX_NUMNODES) in __nodemask_pr_numnodes()") introduced a
>>
>> There's no such commit, that sha was probably from linux-next. The patch is
>> still in mmotm [1]. Luckily, I would say. Maybe Linus or some automation could
>> run some script to check for bogus Fixes tags before accepting patches?
>
> Ah, that's a relief.
>
> How about we just drop "include/linux/nodemask.h: use nr_node_ids (not
> MAX_NUMNODES) in __nodemask_pr_numnodes()"
> (https://ozlabs.org/~akpm/mmotm/broken-out/include-linux-nodemaskh-use-nr_node_ids-not-max_numnodes-in-__nodemask_pr_numnodes.patch)?
> It's just a cosmetic thing, really.
Yeah the risk of breaking something is not worth it, IMHO.
^ permalink raw reply
* Re: [PATCH] mm,mremap: Bail out earlier in mremap_to under map pressure
From: Joel Fernandes @ 2019-02-28 20:44 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Oscar Salvador, Andrew Morton, linux-mm, LKML, Linux API,
Hugh Dickins, Kirill A. Shutemov, jglisse, Yang Shi, mgorman
In-Reply-To: <5edcfeb8-4f53-0fe6-1e5b-c1e485f91d0d@suse.cz>
On Thu, Feb 28, 2019 at 12:06 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 2/27/19 10:32 PM, Oscar Salvador wrote:
> > On Tue, Feb 26, 2019 at 02:04:28PM -0800, Andrew Morton wrote:
> >> How is this going to affect existing userspace which is aware of the
> >> current behaviour?
> >
> > Well, current behavior is not really predictable.
> > Our customer was "surprised" that the call to mremap() failed, but the regions
> > got unmapped nevertheless.
> > They found it the hard way when they got a segfault when trying to write to those
> > regions when cleaning up.
> >
> > As I said in the changelog, the possibility for false positives exists, due to
> > the fact that we might get rid of several vma's when unmapping, but I do not
> > expect existing userspace applications to start failing.
> > Should be that the case, we can revert the patch, it is not that it adds a lot
> > of churn.
>
> Hopefully the only program that would start failing would be a LTP test
> testing the current behavior near the limit (if such test exists). And
> that can be adjusted.
>
IMO the original behavior is itself probably not a big issue because
if userspace wanted to mremap over something, it was prepared to lose
the "over something" mapping anyway. So it does seem to be a stretch
to call the behavior a "bug". Still I agree with the patch that mremap
should not leave any side effects after returning error.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Tycho Andersen @ 2019-03-01 14:53 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Serge E. Hallyn, linux-man, Kees Cook, Linux API, lkml,
Andy Lutomirski, Jann Horn, Oleg Nesterov, Christian Brauner,
Eric W. Biederman, Containers, Aleksa Sarai, Tyler Hicks,
Akihiro Suda
In-Reply-To: <2cea5fec-e73e-5749-18af-15c35a4bd23c@gmail.com>
On Thu, Feb 28, 2019 at 01:52:19PM +0100, Michael Kerrisk (man-pages) wrote:
> > +a notification will be sent to this fd. See "Userspace Notification" below for
>
> s/fd/file descriptor/ throughout please.
Will do.
> > +more details.
>
> I think the description here could be better worded as something like:
>
> SECCOMP_FILTER_FLAG_NEW_LISTENER
> Register a new filter, as usual, but on success return a
> new file descriptor that provides user-space notifications.
> When the filter returns SECCOMP_RET_USER_NOTIF, a notification
> will be provided via this file descriptor. The close-on-exec
> flag is automatically set on the new file descriptor. ...
>
> > .RE
> > .TP
> > .BR SECCOMP_GET_ACTION_AVAIL " (since Linux 4.14)"
> > @@ -606,6 +613,17 @@ file.
> > .TP
> > .BR SECCOMP_RET_ALLOW
> > This value results in the system call being executed.
> > +.TP
> > +.BR SECCOMP_RET_USER_NOTIF " (since Linux 4.21)"
>
> Please see the start of this hanging list in the manual page.
> Can you confirm that SECCOMP_RET_USER_NOTIF really is the lowest
> in the precedence order of all of the filter return values?
Oh, no, I didn't realize it was in a particular order. I'll switch it.
> > +Forwards the syscall to an attached listener in userspace to allow userspace to
>
> s/syscall/system call throughout please.
Will do.
> > +decide what to do with the syscall. If there is no attached listener (either
> > +because the filter was not installed with the
> > +.BR SECCOMP_FILTER_FLAG_NEW_LISTENER
> > +or because the fd was closed), the filter returns
> > +.BR ENOSYS
> > +similar to what happens when a filter returns
> > +.BR SECCOMP_RET_TRACE
> > +and there is no tracer. See "Userspace Notification" below for more details.
> > .PP
> > If an action value other than one of the above is specified,
> > then the filter action is treated as either
> > @@ -693,10 +711,75 @@ Otherwise, if kernel auditing is enabled and the process is being audited
> > the action is logged.
> > .IP *
> > Otherwise, the action is not logged.
> > +.SS Userspace Notification
> > +Interactin userspace notification functionality in seccomp is primarily done
> > +via file descriptor.
>
> That sentence is somewhat garbled. Even if I correct the typo,
> I still don't really understand it. Could you try again?
Maybe "Userspace interacts with the notification functionality via
a file descriptor"? Perhaps we can just delete it.
> > This file descriptor can be obtained by passing
> > +.BR SECCOMP_FILTER_FLAG_NEW_LISTENER
> > +as a filter flag when installing a new filter.
> > +.PP
> > +Once an fd is obtained, userspace can wait for events using
> > +.BR poll ()
>
> and presumably select() and epoll?
>
> What kind of notification event do poll(2)/epoll(7)/select(2) provide?
> It looks to be POLLIN/EPOLLIN/readable. Is that correct?
> These details should be noted here. More generally, my assumption
> is that you can use poll(2)/epoll(7)/select(2) to find out about the
> availability of an event, and then use SECCOMP_IOCTL_NOTIF_RECV
> to read that event. Correct? The text needs to be more explicit on
> this.
Yes.
> > +or
> > +.BR ioctl ().
> > +The supported
> > +.BR ioctl ()
> > +operations on a notification fd are:
> > +.TP
> > +.BR SECCOMP_IOCTL_NOTIF_RECV
> > +The argument to this command should be a pointer to a struct seccomp_notif:
> > +.IP
> > +.in +4n
> > +.EX
> > +struct seccomp_notif {
> > + __u64 id;
> > + __u32 pid;
> > + __u32 flags;
> > + struct seccomp_data data;
> > +};
> > +.EE
> > +.in
> > +.IP
> > +The id field is a filter-unique id for this syscall, and should be supplied in
> > +the response. It can additionally be used in
> > +.BR SECCOMP_IOCTL_ID_VALID
> > +to test whether or not the request is still alive. The pid here is the pid of
> > +the task as visible from the listener's pid namespace. If the pid is not
> > +visible, it is 0. Flags is unused right now.
>
> So, is 'flags' explicitly zeroed by the kernel? the manual page should note
> this.
Yes.
> > struct seccomp_data is the same
> > +data that would be passed to a filter running in the kernel.
>
> What are the semantics if multiple monitoring processes are employing
> SECCOMP_IOCTL_NOTIF_RECV? Does only one of them get awoken? (Which one?)
> Or do they all get woken up and get a 'struct seccomp_notif'? The semantics
> should be detailed here.
Ok. (Only one notification is sent.)
> > +.TP
> > +.BR SECCOMP_IOCTL_NOTIF_SEND
> > +The argument to this command should be a pointer to a struct seccomp_notif_resp:
> > +.IP
> > +.in +4n
> > +.EX
> > +struct seccomp_notif_resp {
> > + __u64 id;
> > + __s64 val;
> > + __s32 error;
> > + __u32 flags;
> > +};
> > +.EE
> > +.in
> > +.IP
> > +The id should be the id from struct seccomp_notif; if error is non-zero, it is
> > +used as the return value, otherwise val is. Flags must be 0 right now.
>
> There really isn't enough detail here to help the reader understand. What
> is the purpose of 'error' vs 'val'? In particular, I'm assuming that the
> monitoring process has (at least) two choices with respect to the system
> call being made by the target process:
>
> (1) Cause the system call to fail with an error.
> (2) Allow the system call to proceed.
>
> How are 'error' and 'val' used in these two cases?
> Are there more than these two cases? How else is 'val' used?
This is basically just an artifact that's exposed because some arches
use different registers to return errors vs. successful values. I'll
make a note of that and how the precedence works.
> Also, what happens if the monitoring process does a SECCOMP_IOCTL_NOTIF_RECV,
> but does not subsequently do a SECCOMP_IOCTL_NOTIF_SEND? This should be
> detailed in the manual page.
>
> What are the semantics if multiple monitoring processes are employing
> SECCOMP_IOCTL_NOTIF_SEND? An error for all but the first? The semantics
> should be detailed here.
Ok.
> > +.TP
> > +.BR SECCOMP_IOCTL_NOTIF_ID_VALID
> > +The argument to this command is just the id to be tested. There is a pid reuse
> > +issue where a task may make a syscall, die, its pid get re-used, and the
> > +listener may operate on the wrong pid. So the process for handling modifying a
> > +pid's state in some way would be to open the pid's resources (/proc/pid/mem or
> > +similar), do this call to verify that the id is still valid, and then use the
> > +resource. See the sample below for more detail.
> > +.PP
> > +A complete example is available in the kernel tree at
> > +.IR samples/seccomp/user-trap.c .
> > .SH RETURN VALUE
> > On success,
> > .BR seccomp ()
> > -returns 0.
> > +returns 0, unless
> > +.BR SECCOMP_FILTER_FLAG_NEW_LISTENER
> > +was specified, in which case it returns the fd number for the new listener fd.
> > On error, if
> > .BR SECCOMP_FILTER_FLAG_TSYNC
> > was used,
>
> More generally though, I'm struggling with this patch because "the
> big picture" is lacking. Here's my estimate of what I think that
> picture is:
>
> [[
> Instead of having the seccomp() filter decision being made
> within the filter itself, it is possible to pass off the
> decision making to a (different) user-space process that
> makes the decision. This is done using the following steps.
>
> 1. The "target process" (i.e., process that will
> establish a seccomp filter) and the process that will make
> decisions about system calls ("the monitoring process")
> establish a connection using a UNIX domain socket. This
> socket will subsequently be used to exchange a file
> descriptor.
>
> 2. The "target process" establishes a seccomp BPF filter in the
> usual manner, but with two notable differences:
>
> * The seccomp() 'flags' argument includes the flag
> SECCOMP_FILTER_FLAG_NEW_LISTENER. Consequently, the return
> value of a successful seccomp() call is a new "listening"
> file descriptor that can be used for monitoring.
> * In cases where it is needed, the BPF filter returns the
> special action SECCOMP_RET_USER_NOTIF. This return value
> will trigger a notification event.
>
> 3. The "target process" passes the "listening file descriptor"
> to the "monitoring process" via the UNIX domain socket.
or some other means, it doesn't have to be with SCM_RIGHTS.
> 4. The target process then performs its workload, which includes
> system calls that will be controlled by the BPF filter.
> Whenever one of these system calls causes the BPF filter to
> return SECCOMP_RET_USER_NOTIF, a notification event is
> generated on the listening file descriptor.
>
> 5. The monitoring process will receive notification events
> on the listening file descriptor. These events are returned
> as structures of type 'seccomp_notif'. Because this structure
> and its size may evolve over kernel version, the monitoring
> process must first determine the size of this structure using
> the seccomp() SECCOMP_GET_NOTIF_SIZES operation, which
> returns a structure of type 'seccomp_notif_sizes'. The
> monitoring process allocates a buffer of size
> 'seccomp_notif_sizes.seccomp_notif' bytes to receive
> monitoring events. In addition,the monitoring process
> allocates another buffer of size
> 'seccomp_notif_sizes.seccomp_notif_resp' bytes for the
> response (a 'struct seccomp_notif_resp') that it will
> provide to the kernel in order to advise how the system
> call being made by the target process shall be treated.
>
> 6. The monitoring process can now repeatedly monitor the
> listening file descriptor.
>
> When a SECCOMP_RET_USER_NOTIF-triggered event occurs,
> the file descriptor will test as readable for
> poll(2)/epoll(7)/select(2). The call
>
> ioctl(listenfd, SECCOMP_IOCTL_NOTIF_RECV, reqptr)
>
> can be used to read info on the event; this operation
> blocks until an event is available. It populates
> the 'struct seccomp_notif' pointed to by the third
> argument with information about the system call
> that is being attempted by the target process.
>
> 7. The monitoring process can use the information in the
> 'struct seccomp_notif' to make a determination about the
> system call being made by the target process. This
> structure includes a 'data' field that is the same
> 'struct seccomp_data' that is passed to a BPF filter.
>
> In addition, the monitoring process may make use of other
> information that is available from user space. For example,
> it may inspect the memory of the target process (whose PID
> is provided in the 'struct seccomp_notif') using
> /proc/PID/mem, which includes inspecting the values
Again, could be ptrace() or /proc/self/map_files/* or whatever,
/proc/PID/mem is just the most convenient.
> pointed to by system call arguments (whose location is
> available 'seccomp_notif.data.args). However, when using
> the target process PID in this way, one must guard against
> PID re-use race conditions using the seccomp()
> SECCOMP_IOCTL_NOTIF_ID_VALID operation.
>
> 8. Having arrived at a decision about the target process's
> system call, the monitoring process can inform the kernel
> of its decision using the operation
>
> ioctl(listenfd, SECCOMP_IOCTL_NOTIF_SEND, respptr)
>
> where the third argument is a pointer to a
> 'struct seccomp_notif_resp'. [Some more details
> needed here, but I still don't yet understand fully
> the semantics of the 'error' and 'val' fields.]
>
> I'd like to include text along the above lines at the start of
> the "Userspace Notification" section. Could you take a look at
> the above and let me know any errors, improved terminology, or
> other improvements generally. I'll then work that text into
> the page.
Yep, looks good to me, thanks.
Tycho
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Tycho Andersen @ 2019-03-01 14:53 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Serge E. Hallyn, linux-man, Kees Cook, Linux API, lkml,
Andy Lutomirski, Jann Horn, Oleg Nesterov, Christian Brauner,
Eric W. Biederman, Containers, Aleksa Sarai, Tyler Hicks,
Akihiro Suda
In-Reply-To: <e7a5abd9-f7b7-e077-221a-4c3ef7b818df@gmail.com>
On Thu, Feb 28, 2019 at 02:25:55PM +0100, Michael Kerrisk (man-pages) wrote:
> > 7. The monitoring process can use the information in the
> > 'struct seccomp_notif' to make a determination about the
> > system call being made by the target process. This
> > structure includes a 'data' field that is the same
> > 'struct seccomp_data' that is passed to a BPF filter.
> >
> > In addition, the monitoring process may make use of other
> > information that is available from user space. For example,
> > it may inspect the memory of the target process (whose PID
> > is provided in the 'struct seccomp_notif') using
> > /proc/PID/mem, which includes inspecting the values
> > pointed to by system call arguments (whose location is
> > available 'seccomp_notif.data.args). However, when using
> > the target process PID in this way, one must guard against
> > PID re-use race conditions using the seccomp()
> > SECCOMP_IOCTL_NOTIF_ID_VALID operation.
> >
> > 8. Having arrived at a decision about the target process's
> > system call, the monitoring process can inform the kernel
> > of its decision using the operation
> >
> > ioctl(listenfd, SECCOMP_IOCTL_NOTIF_SEND, respptr)
> >
> > where the third argument is a pointer to a
> > 'struct seccomp_notif_resp'. [Some more details
> > needed here, but I still don't yet understand fully
> > the semantics of the 'error' and 'val' fields.]
>
> So clearly, I misunderstood these last two steps.
>
> (7) is something like: discover information in userspace
> as required; perform userspace actions if appropriate
> (perhaps doing the system call operation "on behalf of" the
> target process).
>
>
> (8) is something like:
> set 'error' and 'val' to return info to the target process:
> * error != 0 ==> make it look like the syscall failed,
> with 'errno' set to that value
> * error == 0 ==> make it look like the syscall succeeded
> and returned 'val'
>
> Right?
Yep, exactly.
Tycho
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Michael Kerrisk (man-pages) @ 2019-03-01 15:13 UTC (permalink / raw)
To: Tycho Andersen
Cc: mtk.manpages, Serge E. Hallyn, linux-man, Kees Cook, Linux API,
lkml, Andy Lutomirski, Jann Horn, Oleg Nesterov,
Christian Brauner, Eric W. Biederman, Containers, Aleksa Sarai,
Tyler Hicks, Akihiro Suda
In-Reply-To: <20190301145341.GD7413@cisco>
On 3/1/19 3:53 PM, Tycho Andersen wrote:
> On Thu, Feb 28, 2019 at 02:25:55PM +0100, Michael Kerrisk (man-pages) wrote:
>>> 7. The monitoring process can use the information in the
>>> 'struct seccomp_notif' to make a determination about the
>>> system call being made by the target process. This
>>> structure includes a 'data' field that is the same
>>> 'struct seccomp_data' that is passed to a BPF filter.
>>>
>>> In addition, the monitoring process may make use of other
>>> information that is available from user space. For example,
>>> it may inspect the memory of the target process (whose PID
>>> is provided in the 'struct seccomp_notif') using
>>> /proc/PID/mem, which includes inspecting the values
>>> pointed to by system call arguments (whose location is
>>> available 'seccomp_notif.data.args). However, when using
>>> the target process PID in this way, one must guard against
>>> PID re-use race conditions using the seccomp()
>>> SECCOMP_IOCTL_NOTIF_ID_VALID operation.
>>>
>>> 8. Having arrived at a decision about the target process's
>>> system call, the monitoring process can inform the kernel
>>> of its decision using the operation
>>>
>>> ioctl(listenfd, SECCOMP_IOCTL_NOTIF_SEND, respptr)
>>>
>>> where the third argument is a pointer to a
>>> 'struct seccomp_notif_resp'. [Some more details
>>> needed here, but I still don't yet understand fully
>>> the semantics of the 'error' and 'val' fields.]
>>
>> So clearly, I misunderstood these last two steps.
>>
>> (7) is something like: discover information in userspace
>> as required; perform userspace actions if appropriate
>> (perhaps doing the system call operation "on behalf of" the
>> target process).
>>
>>
>> (8) is something like:
>> set 'error' and 'val' to return info to the target process:
>> * error != 0 ==> make it look like the syscall failed,
>> with 'errno' set to that value
That piece should be amended:
error < 0 ==> make it look like syscall failed.
error > 0 ==> make it look like the syscall succeeded
and returned 'error'
Is that really supposed to happen?
>> * error == 0 ==> make it look like the syscall succeeded
>> and returned 'val'
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Michael Kerrisk (man-pages) @ 2019-03-01 15:16 UTC (permalink / raw)
To: Tycho Andersen
Cc: mtk.manpages, Serge E. Hallyn, linux-man, Kees Cook, Linux API,
lkml, Andy Lutomirski, Jann Horn, Oleg Nesterov,
Christian Brauner, Eric W. Biederman, Containers, Aleksa Sarai,
Tyler Hicks, Akihiro Suda
In-Reply-To: <20190301145310.GC7413@cisco>
Hello Tycho,
On 3/1/19 3:53 PM, Tycho Andersen wrote:
> On Thu, Feb 28, 2019 at 01:52:19PM +0100, Michael Kerrisk (man-pages) wrote:
>>> +a notification will be sent to this fd. See "Userspace Notification" below for
>>
>> s/fd/file descriptor/ throughout please.
>
> Will do.
>
>>> +more details.
>>
>> I think the description here could be better worded as something like:
>>
>> SECCOMP_FILTER_FLAG_NEW_LISTENER
>> Register a new filter, as usual, but on success return a
>> new file descriptor that provides user-space notifications.
>> When the filter returns SECCOMP_RET_USER_NOTIF, a notification
>> will be provided via this file descriptor. The close-on-exec
>> flag is automatically set on the new file descriptor. ...
>>
>>> .RE
>>> .TP
>>> .BR SECCOMP_GET_ACTION_AVAIL " (since Linux 4.14)"
>>> @@ -606,6 +613,17 @@ file.
>>> .TP
>>> .BR SECCOMP_RET_ALLOW
>>> This value results in the system call being executed.
>>> +.TP
>>> +.BR SECCOMP_RET_USER_NOTIF " (since Linux 4.21)"
>>
>> Please see the start of this hanging list in the manual page.
>> Can you confirm that SECCOMP_RET_USER_NOTIF really is the lowest
>> in the precedence order of all of the filter return values?
>
> Oh, no, I didn't realize it was in a particular order. I'll switch it.
Just for my immediate education (I'm experimenting right now),
where/how does it fit in the precedence order?
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Tycho Andersen @ 2019-03-01 15:19 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Serge E. Hallyn, linux-man, Kees Cook, Linux API, lkml,
Andy Lutomirski, Jann Horn, Oleg Nesterov, Christian Brauner,
Eric W. Biederman, Containers, Aleksa Sarai, Tyler Hicks,
Akihiro Suda
In-Reply-To: <052d73e2-c786-a760-f03a-a07b5772de5a@gmail.com>
On Fri, Mar 01, 2019 at 04:16:27PM +0100, Michael Kerrisk (man-pages) wrote:
> Hello Tycho,
>
> On 3/1/19 3:53 PM, Tycho Andersen wrote:
> > On Thu, Feb 28, 2019 at 01:52:19PM +0100, Michael Kerrisk (man-pages) wrote:
> >>> +a notification will be sent to this fd. See "Userspace Notification" below for
> >>
> >> s/fd/file descriptor/ throughout please.
> >
> > Will do.
> >
> >>> +more details.
> >>
> >> I think the description here could be better worded as something like:
> >>
> >> SECCOMP_FILTER_FLAG_NEW_LISTENER
> >> Register a new filter, as usual, but on success return a
> >> new file descriptor that provides user-space notifications.
> >> When the filter returns SECCOMP_RET_USER_NOTIF, a notification
> >> will be provided via this file descriptor. The close-on-exec
> >> flag is automatically set on the new file descriptor. ...
> >>
> >>> .RE
> >>> .TP
> >>> .BR SECCOMP_GET_ACTION_AVAIL " (since Linux 4.14)"
> >>> @@ -606,6 +613,17 @@ file.
> >>> .TP
> >>> .BR SECCOMP_RET_ALLOW
> >>> This value results in the system call being executed.
> >>> +.TP
> >>> +.BR SECCOMP_RET_USER_NOTIF " (since Linux 4.21)"
> >>
> >> Please see the start of this hanging list in the manual page.
> >> Can you confirm that SECCOMP_RET_USER_NOTIF really is the lowest
> >> in the precedence order of all of the filter return values?
> >
> > Oh, no, I didn't realize it was in a particular order. I'll switch it.
>
> Just for my immediate education (I'm experimenting right now),
> where/how does it fit in the precedence order?
In between RET_ERRNO and RET_TRACE; see include/uapi/linux/seccomp.h
for details.
Tycho
^ permalink raw reply
* Re: [PATCH] mm,mremap: Bail out earlier in mremap_to under map pressure
From: Cyril Hrubis @ 2019-03-01 15:25 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Oscar Salvador, Andrew Morton, linux-mm, linux-kernel, linux-api,
hughd, kirill, joel, jglisse, yang.shi, mgorman
In-Reply-To: <5edcfeb8-4f53-0fe6-1e5b-c1e485f91d0d@suse.cz>
Hi!
> Hopefully the only program that would start failing would be a LTP test
> testing the current behavior near the limit (if such test exists). And
> that can be adjusted.
There does not seem to be a mremap() test that would do such a thing, so
we should be safe :-).
BTW there was a similar fix for mmap() with MAP_FIXED that caused a LTP
test to fail and was fixed in:
commit e8420a8ece80b3fe810415ecf061d54ca7fab266
Author: Cyril Hrubis <chrubis@suse.cz>
Date: Mon Apr 29 15:08:33 2013 -0700
mm/mmap: check for RLIMIT_AS before unmapping
And I haven't heard of any breakages so far so I guess that this very
similar situation and that the possibility of breaking real world
applications here is really low.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply
* Re: [PATCH 2/2] seccomp.2: document userspace notification
From: Michael Kerrisk (man-pages) @ 2019-03-01 16:02 UTC (permalink / raw)
To: Tycho Andersen
Cc: mtk.manpages, Serge E. Hallyn, linux-man, Kees Cook, Linux API,
lkml, Andy Lutomirski, Jann Horn, Oleg Nesterov,
Christian Brauner, Eric W. Biederman, Containers, Aleksa Sarai,
Tyler Hicks, Akihiro Suda
In-Reply-To: <20190301151937.GE7413@cisco>
On 3/1/19 4:19 PM, Tycho Andersen wrote:
> On Fri, Mar 01, 2019 at 04:16:27PM +0100, Michael Kerrisk (man-pages) wrote:
>> Hello Tycho,
>>
>> On 3/1/19 3:53 PM, Tycho Andersen wrote:
>>> On Thu, Feb 28, 2019 at 01:52:19PM +0100, Michael Kerrisk (man-pages) wrote:
>>>>> +a notification will be sent to this fd. See "Userspace Notification" below for
>>>>
>>>> s/fd/file descriptor/ throughout please.
>>>
>>> Will do.
>>>
>>>>> +more details.
>>>>
>>>> I think the description here could be better worded as something like:
>>>>
>>>> SECCOMP_FILTER_FLAG_NEW_LISTENER
>>>> Register a new filter, as usual, but on success return a
>>>> new file descriptor that provides user-space notifications.
>>>> When the filter returns SECCOMP_RET_USER_NOTIF, a notification
>>>> will be provided via this file descriptor. The close-on-exec
>>>> flag is automatically set on the new file descriptor. ...
>>>>
>>>>> .RE
>>>>> .TP
>>>>> .BR SECCOMP_GET_ACTION_AVAIL " (since Linux 4.14)"
>>>>> @@ -606,6 +613,17 @@ file.
>>>>> .TP
>>>>> .BR SECCOMP_RET_ALLOW
>>>>> This value results in the system call being executed.
>>>>> +.TP
>>>>> +.BR SECCOMP_RET_USER_NOTIF " (since Linux 4.21)"
>>>>
>>>> Please see the start of this hanging list in the manual page.
>>>> Can you confirm that SECCOMP_RET_USER_NOTIF really is the lowest
>>>> in the precedence order of all of the filter return values?
>>>
>>> Oh, no, I didn't realize it was in a particular order. I'll switch it.
>>
>> Just for my immediate education (I'm experimenting right now),
>> where/how does it fit in the precedence order?
>
> In between RET_ERRNO and RET_TRACE; see include/uapi/linux/seccomp.h
> for details.
Confirmed by experiment :-).
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* a.out coredumping: fix or delete?
From: Jann Horn @ 2019-03-01 23:57 UTC (permalink / raw)
To: Al Viro
Cc: Thomas Gleixner, kernel list, linux-fsdevel,
the arch/x86 maintainers, Linux API
In theory, Linux can dump cores for a.out binaries. In practice, that
code is pretty bitrotten and buggy. Does anyone want that code so much
that they'd like to fix it, or can we just delete it?
Here's a shell script that will give you a minimal a.out binary that
Linux will execute (and that then segfaults immediately because it has
no executable pages mapped):
==============
#!/bin/bash
(
# a_info: magic=OMAGIC
printf '\x07\x01'
# a_info: machtype=M_386
printf '\x64'
# a_info: flags=0
printf '\x00'
# a_text, a_data, a_bss, a_syms: 0
printf '\x00\x00\x00\x00'
printf '\x00\x00\x00\x00'
printf '\x00\x00\x00\x00'
printf '\x00\x00\x00\x00'
# a_entry: 0x42424242
printf '\x42\x42\x42\x42'
# a_trsize, a_drsize: 0
printf '\x00\x00\x00\x00'
printf '\x00\x00\x00\x00'
) > aout_binary
chmod +x aout_binary
==============
You need a kernel with CONFIG_IA32_AOUT enabled (for x86-64) or with
CONFIG_BINFMT_AOUT enabled (for 32-bit x86). If aout is built as a
module, you have to manually load it with "modprobe binfmt_aout",
because even though there is binfmt autoloading code in the kernel, no
aliases are set up for any binfmts.
On a Debian 9 system with a 4.9 stable kernel, if you try to run this
a.out program with core dumps enabled ("ulimit -c unlimited") a few
times, the kernel oopses:
==============
[ 2659.912016] aout_binary[978]: segfault at 42424242 ip 42424242 sp
bfffe4e0 error 14
[ 2659.912318] BUG: unable to handle kernel paging request at bffff000
[ 2659.912336] IP: [<d030bd14>] memcpy+0x14/0x30
[ 2659.912364] *pdpt = 00000000367f7001 *pde = 000000007d0d1067
[ 2659.912368] Oops: 0000 [#1] SMP
[ 2659.912377] Modules linked in: binfmt_aout [...]
[ 2659.912421] CPU: 0 PID: 978 Comm: aout_binary Not tainted
4.9.0-8-686-pae #1 Debian 4.9.144-3.1
[ 2659.912422] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.10.2-1 04/01/2014
[ 2659.912424] task: f30e2000 task.stack: f470a000
[ 2659.912428] EIP: 0060:[<d030bd14>] EFLAGS: 00010206 CPU: 0
[ 2659.912430] EIP is at memcpy+0x14/0x30
[ 2659.912431] EAX: fffba000 EBX: 00001000 ECX: 00000400 EDX: bffff000
[ 2659.912433] ESI: bffff000 EDI: fffba000 EBP: f470bab0 ESP: f470baa4
[ 2659.912434] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[ 2659.912436] CR0: 80050033 CR2: bffff000 CR3: 346ad4e0 CR4: 001406f0
[ 2659.912442] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 2659.912444] DR6: fffe0ff0 DR7: 00000400
[ 2659.912445] Stack:
[ 2659.912446] f470bbf0 bffff000 00001000 00001000 d03111a2 f470bb28
00003000 00000000
[ 2659.912449] fffbb000 f470bc10 fffba000 6721debb 00001000 00002000
00000000 f470bb40
[ 2659.912452] d016cd10 00001000 00000000 00001000 00000001 f470bb28
f470bb2c 00001000
[ 2659.912456] Call Trace:
[ 2659.912475] [<d03111a2>] ? iov_iter_copy_from_user_atomic+0x1a2/0x230
[ 2659.912488] [<d016cd10>] ? generic_perform_write+0xe0/0x1d0
[ 2659.912492] [<d016ef52>] ? __generic_file_write_iter+0x192/0x1f0
[ 2659.912501] [<d0217c67>] ? __find_get_block+0xc7/0x250
[ 2659.912512] [<f8680496>] ? ext4_file_write_iter+0x86/0x460 [ext4]
[ 2659.912514] [<f85c7050>] ? crc32c_intel_init+0x20/0x20 [crc32c_intel]
[ 2659.912517] [<d021816c>] ? __getblk_gfp+0x2c/0x310
[ 2659.912523] [<d01e12bc>] ? generic_file_llseek_size+0x13c/0x1e0
[ 2659.912525] [<d01e1eac>] ? new_sync_write+0xcc/0x130
[ 2659.912527] [<d01e1faf>] ? __kernel_write+0x4f/0x100
[ 2659.912537] [<d023b382>] ? dump_emit+0x92/0xe0
[ 2659.912539] [<f86fead5>] ? aout_core_dump+0x2a5/0x2f1 [binfmt_aout]
[ 2659.912542] [<d023bb43>] ? do_coredump+0x4d3/0xde0
[...]
[ 2659.912618] Code: 58 2b 43 50 88 43 4e 5b 5d c3 90 8d 74 26 00 e8
43 fb ff ff eb e8 90 55 89 e5 57 56 53 3e 8d 74 26 00 89 cb 89 c7 c1
e9 02 89 d6 <f3> a5 89 d9 83 e1 03 74 02 f3 a4 5b 5e 5f 5d c3 8d b6 00
00 00
[ 2659.912639] EIP: [<d030bd14>]
[ 2659.912641] memcpy+0x14/0x30
[ 2659.912642] SS:ESP 0068:f470baa4
[ 2659.912643] CR2: 00000000bffff000
[ 2659.912645] ---[ end trace 6413c918c629c657 ]---
==============
The problem is that since 43a5d548eb594, aout_core_dump() essentially
calls __kernel_write() on a userspace address, which then causes
iov_iter_init() to decide based on uaccess_kernel() that it should use
ITER_KVEC and access the userspace memory with memcpy().
If you try to reproduce this on a 64-bit system with a master branch
kernel, it doesn't work. But that's because that code is even more
broken: The userspace stack pointer is something like 0xffffc4c8, but
fill_dump() for some reason assumes that top-of-stack is at
0xc0000000, causing it to not even attempt to dump the stack:
if (dump->start_stack < 0xc0000000) {
unsigned long tmp;
tmp = (unsigned long) (0xc0000000 - dump->start_stack);
dump->u_ssize = tmp >> PAGE_SHIFT;
}
You can reproduce the oops if you use gdb to move the stack pointer
down below 0xc0000000:
==============
user@debian:~/aout$ ulimit -c unlimited
user@debian:~/aout$ gdb ./aout_binary
[...]
(gdb) break *0x42424242
Breakpoint 1 at 0x42424242
(gdb) run
Starting program: /home/user/aout/aout_binary
[...]
(gdb) p/x $sp
$1 = 0xffffcdcc
(gdb) set $sp=0x80000000
(gdb) detach
Detaching from p[ 94.987218] aout_binary[1079]: segfault at 42424242
ip 0000000042424242 sp 0000000080000000 error 14
rogram: /home/us[ 94.989368] Code: Bad RIP value.
er/aout/aout_binary, process 1079
(gdb) [ 94.991341]
==================================================================
[ 94.993463] BUG: KASAN: user-memory-access in
iov_iter_copy_from_user_atomic+0x23d/0x530
[ 94.995465] Read of size 4096 at addr 0000000080000000 by task
aout_binary/1079
[ 94.997069]
[ 94.997417] CPU: 4 PID: 1079 Comm: aout_binary Not tainted 5.0.0-rc8 #292
[ 94.998942] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.10.2-1 04/01/2014
[ 95.000809] Call Trace:
[ 95.001412] dump_stack+0x71/0xab
[...]
[ 95.004628] kasan_report+0x176/0x192
[...]
[ 95.006746] memcpy+0x1f/0x50
[ 95.007433] iov_iter_copy_from_user_atomic+0x23d/0x530
[...]
[ 95.009459] generic_perform_write+0x1a1/0x2d0
[...]
[ 95.013166] __generic_file_write_iter+0x264/0x2a0
[ 95.014242] ext4_file_write_iter+0x3a4/0x680
[...]
[ 95.027234] __vfs_write+0x294/0x3b0
[...]
[ 95.032673] __kernel_write+0x91/0x190
[ 95.033540] dump_emit+0x131/0x1d0
[...]
[ 95.076087] Disabling lock debugging due to kernel taint
[ 95.077287] BUG: unable to handle kernel paging request at 0000000080000000
[ 95.078812] #PF error: [normal kernel read fault]
[ 95.079845] PGD 1e0629067 P4D 1e0629067 PUD 0
[ 95.080831] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN
[...]
==============
Also, the non-compat version of the coredump code looks like it leaks
some kernel memory into the coredump through "struct user". I don't
think anyone's going to care much, given that it looks like on distro
kernels, you won't usually be able to load a.out binaries...
The rest of a.out is also kind of weird; for example, there is support
for loading text at an unaligned offset (by copying code into an
anonymous mapping), but from a glance, it looks like the resulting
text mapping wouldn't actually be executable? And there is support for
loading files without mmap handler, except that an earlier security
check prevents the use of files without mmap handler, unless you're on
x86-64, where the copied code in ia32_aout.c is used that doesn't have
that security check.
^ permalink raw reply
* Re: a.out coredumping: fix or delete?
From: Andy Lutomirski @ 2019-03-03 1:09 UTC (permalink / raw)
To: Jann Horn
Cc: Al Viro, Thomas Gleixner, kernel list, Linux FS Devel,
the arch/x86 maintainers, Linux API
In-Reply-To: <CAG48ez1RVd5mQ_Pb6eygQESaZhpQz765OAZYSoPE0kPqfZEXQg@mail.gmail.com>
On Fri, Mar 1, 2019 at 3:57 PM Jann Horn <jannh@google.com> wrote:
>
> In theory, Linux can dump cores for a.out binaries. In practice, that
> code is pretty bitrotten and buggy. Does anyone want that code so much
> that they'd like to fix it, or can we just delete it?
I would propose just deleting it.
^ permalink raw reply
* Re: [PATCH man-pages] Add rseq manpage
From: Mathieu Desnoyers @ 2019-03-04 18:02 UTC (permalink / raw)
To: Michael Kerrisk
Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will
In-Reply-To: <f6512944-26e1-6231-87b3-b30a2017b4b8@gmail.com>
----- On Feb 28, 2019, at 3:42 AM, Michael Kerrisk mtk.manpages@gmail.com wrote:
> On 12/6/18 3:42 PM, Mathieu Desnoyers wrote:
>> [ Michael, rseq(2) was merged into 4.18. Can you have a look at this
>> patch which adds rseq documentation to the man-pages project ? ]
> Hi Matthieu
>
> Sorry for the long delay. I've merged this page into a private
> branch and have done quite a lot of editing. I have many
> questions :-).
No worries, thanks for looking into it!
>
> In the first instance, I think it is probably best to have
> a free-form text discussion rather than firing patches
> back and forward. Could you take a look at the questions below
> and respond?
Sure,
>
> Thanks,
>
> Michael
>
>
> RSEQ(2) Linux Programmer's Manual RSEQ(2)
>
> NAME
> rseq - Restartable sequences and CPU number cache
>
> SYNOPSIS
> #include <linux/rseq.h>
>
> int rseq(struct rseq *rseq, uint32_t rseq_len, int flags, uint32_t sig);
>
> DESCRIPTION
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │Imagine you are someone who is pretty new to this │
> │idea... What is notably lacking from this page is │
> │an overview explaining: │
> │ │
> │ * What a restartable sequence actually is. │
> │ │
> │ * An outline of the steps to perform when using │
> │ restartable sequences / rseq(2). │
> │ │
> │I.e., something along the lines of Jon Corbet's │
> │https://lwn.net/Articles/697979/. Can you come up │
> │with something? (Part of it might be at the start of │
> │this page, and the rest in NOTES; it need not be all │
> │in one place.) │
> └─────────────────────────────────────────────────────┘
We recently published a blog post about rseq, which might contain just the
right level of information we are looking for here:
https://www.efficios.com/blog/2019/02/08/linux-restartable-sequences/
Could something along the following lines work ?
"A restartable sequence is a sequence of instructions guaranteed to be
executed atomically with respect to other threads and signal handlers on the
current CPU. If its execution does not complete atomically, the kernel changes
the execution flow by jumping to an abort handler defined by user-space for
that restartable sequence.
Using restartable sequences requires to register a __rseq_abi thread-local storage
data structure (struct rseq) through the rseq(2) system call. Only one __rseq_abi
can be registered per thread, so user-space libraries and applications must follow
a user-space ABI defining how to share this resource. The ABI defining how to share
this resource between applications and libraries is defined by the C library.
The __rseq_abi contains a rseq_cs field which points to the currently executing
critical section. For each thread, a single rseq critical section can run at any
given point. Each critical section need to be implemented in assembly."
> The rseq() ABI accelerates user-space operations on per-CPU data by
> defining a shared data structure ABI between each user-space thread and
> the kernel.
>
> It allows user-space to perform update operations on per-CPU data with‐
> out requiring heavy-weight atomic operations.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the following para: "a hardware execution con‐ │
> │text"? What is the contrast being drawn here? It │
> │would be good to state it more explicitly. │
> └─────────────────────────────────────────────────────┘
Here I'm trying to clarify what we mean by "CPU" in this document. We define
a CPU as having its own number returned by sched_getcpu(), which I think is
sometimes referred to as "logical cpu". This is the current hyperthread on
the current core, on the current "physical CPU", in the current socket.
> The term CPU used in this documentation refers to a hardware execution
> context.
>
> Restartable sequences are atomic with respect to preemption (making it
> atomic with respect to other threads running on the same CPU), as well
> as signal delivery (user-space execution contexts nested over the same
> thread). They either complete atomically with respect to preemption on
> the current CPU and signal delivery, or they are aborted.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the preceding sentence, we need a definition of │
> │"current CPU". │
> └─────────────────────────────────────────────────────┘
Not sure how to word it. If a thread or signal handler execution context can
possibly run and issue, for instance, "sched_getcpu()" between the beginning
and the end of the critical section and get the same logical CPU number as the
current thread, then we are guaranteed to abort. Of course, sched_getcpu() is
just one way to get the CPU number, considering that we can also read it
from the __rseq_abi cpu_id and cpu_id_start fields.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the following, does "It is" means "Restartable │
> │sequences are"? │
> └─────────────────────────────────────────────────────┘
> It is suited for update operations on per-CPU data.
Yes.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the following, does "It is" means "Restartable │
> │sequences are"? │
> └─────────────────────────────────────────────────────┘
"Restartable sequences can be..."
> It can be used on data structures shared between threads within a
> process, and on data structures shared between threads across different
> processes.
>
> Some examples of operations that can be accelerated or improved by this
> ABI:
>
> · Memory allocator per-CPU free-lists
>
> · Querying the current CPU number
>
> · Incrementing per-CPU counters
>
> · Modifying data protected by per-CPU spinlocks
>
> · Inserting/removing elements in per-CPU linked-lists
>
> · Writing/reading per-CPU ring buffers content
>
> · Accurately reading performance monitoring unit counters with respect
> to thread migration
>
> Restartable sequences must not perform system calls. Doing so may
> result in termination of the process by a segmentation fault.
>
> The rseq argument is a pointer to the thread-local rseq structure to be
> shared between kernel and user-space. The layout of this structure is
> shown below.
>
> The rseq_len argument is the size of the struct rseq to register.
>
> The flags argument is 0 for registration, or RSEQ_FLAG_UNREGISTER for
> unregistration.
>
> The sig argument is the 32-bit signature to be expected before the
> abort handler code.
>
> The rseq structure
> The struct rseq is aligned on a 32-byte boundary. This structure is
> extensible. Its size is passed as parameter to the rseq() system call.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │Below, I added the structure definition (in abbrevi‐ │
> │ated form). Is there any reason not to do this? │
> └─────────────────────────────────────────────────────┘
It seems appropriate.
>
> struct rseq {
> __u32 cpu_id_start;
> __u32 cpu_id;
> union {
> __u64 ptr64;
> #ifdef __LP64__
> __u64 ptr;
> #else
> ....
> #endif
> } rseq_cs;
> __u32 flags;
> } __attribute__((aligned(4 * sizeof(__u64))));
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the text below, I think it would be helpful to │
> │explicitly note which of these fields are set by the │
> │kernel (on return from the reseq() call) and which │
> │are set by the caller (before calling rseq()). Is │
> │the following correct: │
> │ │
> │ cpu_id_start - initialized by caller to possible │
> │ CPU number (e.g., 0), updated by kernel │
> │ on return │
"initialized by caller to possible CPU number (e.g., 0), updated
by the kernel on return, and updated by the kernel on return after
thread migration to a different CPU"
> │ │
> │ cpu_id - initialized to -1 by caller, │
> │ updated by kernel on return │
"initialized to -1 by caller, updated by the kernel on return, and
updated by the kernel on return after thread migration to a different
CPU"
> │ │
> │ rseq_cs - initialized by caller, either to NULL │
> │ or a pointer to an 'rseq_cs' structure │
> │ that is initialized by the caller │
"initialized by caller to NULL, then, after returning from successful
registration, updated to a pointer to an "rseq_cs" structure by user-space.
Set to NULL by the kernel when it restarts a rseq critical section,
when it preempts or deliver a signal outside of the range targeted by the
rseq_cs. Set to NULL by user-space before reclaiming memory that
contains the targeted struct rseq_cs."
> │ │
> │ flags - initialized by caller, used by kernel │
> └─────────────────────────────────────────────────────┘
>
> The structure fields are as follows:
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the following paragraph, and in later places, I │
> │changed "current thread" to "calling thread". Okay? │
> └─────────────────────────────────────────────────────┘
Yes.
>
> cpu_id_start
> Optimistic cache of the CPU number on which the calling thread
> is running. The value in this field is guaranteed to always be
> a possible CPU number, even when rseq is not initialized. The
> value it contains should always be confirmed by reading the
> cpu_id field.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │What does the last sentence mean? │
> └─────────────────────────────────────────────────────┘
It means the caller thread can always use __rseq_abi.cpu_id_start to index an
array of per-cpu data and this won't cause an out-of-bound access on load, but it
does not mean it really contains the current CPU number. For instance, if rseq
registration failed, it will contain "0".
Therefore, it's fine to use cpu_is_start to fetch per-cpu data, but the cpu_id
field should be used to compare the cpu_is_start value, so the case where rseq
is not registered is caught. In that case, cpu_id_start=0, but cpu_id=-1 or -2,
which differ, and therefore the critical section needs to jump to the abort
handler.
>
> This field is an optimistic cache in the sense that it is always
> guaranteed to hold a valid CPU number in the range [0..(nr_pos‐
> sible_cpus - 1)]. It can therefore be loaded by user-space and
> used as an offset in per-CPU data structures without having to
> check whether its value is within the valid bounds compared to
> the number of possible CPUs in the system.
>
> For user-space applications executed on a kernel without rseq
> support, the cpu_id_start field stays initialized at 0, which is
> indeed a valid CPU number. It is therefore valid to use it as
> an offset in per-CPU data structures, and only validate whether
> it's actually the current CPU number by comparing it with the
> cpu_id field within the rseq critical section.
>
> If the kernel does not provide rseq support, that cpu_id field
> stays initialized at -1, so the comparison always fails, as
> intended. It is then up to user-space to use a fall-back mecha‐
> nism, considering that rseq is not available.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │The last sentence is rather difficult to grok. Can │
> │we say some more here? │
> └─────────────────────────────────────────────────────┘
Perhaps we could use the explanation I've written above in my reply ?
>
> cpu_id Cache of the CPU number on which the calling thread is running.
> -1 if uninitialized.
>
> rseq_cs
> The rseq_cs field is a pointer to a struct rseq_cs (described
> below). It is NULL when no rseq assembly block critical section
> is active for the calling thread. Setting it to point to a
> critical section descriptor (struct rseq_cs) marks the beginning
> of the critical section.
>
> flags Flags indicating the restart behavior for the calling thread.
> This is mainly used for debugging purposes. Can be either:
>
> RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
Inhibit instruction sequence block restart on preemption for this thread.
>
> RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
Inhibit instruction sequence block restart on signal delivery for this thread.
Restart on signal can only be inhibited when restart on preemption and restart
on migration are inhibited too, else it will terminate the offending process with
a segmentation fault.
>
> RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
Inhibit instruction sequence block restart on migration for this thread.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │Each of the above values needs an explanation. │
> │ │
> │Is it correct that only one of the values may be │
> │specified in 'flags'? I ask because in the 'rseq_cs' │
> │structure below, the 'flags' field is a bit mask │
> │where any combination of these flags may be ORed │
> │together. │
> │ │
> └─────────────────────────────────────────────────────┘
Those are also masks and can be ORed.
>
> The rseq_cs structure
> The struct rseq_cs is aligned on a 32-byte boundary and has a fixed
> size of 32 bytes.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │Below, I added the structure definition (in abbrevi‐ │
> │ated form). Is there any reason not to do this? │
> └─────────────────────────────────────────────────────┘
It's fine.
>
> struct rseq_cs {
> __u32 version;
> __u32 flags;
> __u64 start_ip;
> __u64 post_commit_offset;
> __u64 abort_ip;
> } __attribute__((aligned(4 * sizeof(__u64))));
>
> The structure fields are as follows:
>
> version
> Version of this structure.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │What does 'version' need to be initialized to? │
> └─────────────────────────────────────────────────────┘
Currently version needs to be 0. Eventually, if we implement support for new flags to rseq(),
we could add feature flags which register support for newer versions of struct rseq_cs.
>
> flags Flags indicating the restart behavior of this structure. Can be
> a combination of:
>
> RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
Inhibit instruction sequence block restart on preemption for this thread.
>
> RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
Inhibit instruction sequence block restart on signal delivery for this thread.
Restart on signal can only be inhibited when restart on preemption and restart
on migration are inhibited too, else it will terminate the offending process with
a segmentation fault.
>
> RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
Inhibit instruction sequence block restart on migration for this thread.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │Each of the above values needs an explanation. │
> └─────────────────────────────────────────────────────┘
>
> start_ip
> Instruction pointer address of the first instruction of the
> sequence of consecutive assembly instructions.
>
> post_commit_offset
> Offset (from start_ip address) of the address after the last
> instruction of the sequence of consecutive assembly instruc‐
> tions.
>
> abort_ip
> Instruction pointer address where to move the execution flow in
> case of abort of the sequence of consecutive assembly instruc‐
> tions.
>
> NOTES
> A single library per process should keep the rseq structure in a
> thread-local storage variable. The cpu_id field should be initialized
> to -1, and the cpu_id_start field should be initialized to a possible
> CPU value (typically 0).
The part above is not quite right. All applications/libraries wishing to register
rseq must follow the ABI specified by the C library. It can be defined within more
that a single application/library, but in the end only one symbol will be chosen
for the process's global symbol table.
>
> Each thread is responsible for registering and unregistering its rseq
> structure. No more than one rseq structure address can be registered
> per thread at a given time.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the following paragraph, what is the difference │
> │between "freed" and "reclaim"? I'm supposing they │
> │mean the same thing, but it's not clear. And if they │
> │do mean the same thing, then the first two sentences │
> │appear to contain contradictory information. │
> └─────────────────────────────────────────────────────┘
The mean the same thing, and they are subtly not contradictory.
The first states that memory of a _registered_ rseq object must not
be freed before the thread exits.
The second states that memory of a rseq object must not be freed before
it is unregistered or the thread exits.
Do you have an alternative wording in mind to make this clearer ?
>
> Memory of a registered rseq object must not be freed before the thread
> exits. Reclaim of rseq object's memory must only be done after either
> an explicit rseq unregistration is performed or after the thread exits.
> Keep in mind that the implementation of the Thread-Local Storage (C
> language __thread) lifetime does not guarantee existence of the TLS
> area up until the thread exits.
>
> In a typical usage scenario, the thread registering the rseq structure
> will be performing loads and stores from/to that structure. It is how‐
> ever also allowed to read that structure from other threads. The rseq
> field updates performed by the kernel provide relaxed atomicity seman‐
> tics, which guarantee that other threads performing relaxed atomic
> reads of the CPU number cache will always observe a consistent value.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │In the preceding paragraph, can we reasonably add │
> │some words to explain "relaxed atomicity semantics" │
> │and "relaxed atomic reads"? │
> └─────────────────────────────────────────────────────┘
Not sure how to word this exactly, but here it means the stores and loads need
to be done atomically, but don't require nor provide any ordering guarantees
with respect to other loads/stores (no memory barriers).
>
> RETURN VALUE
> A return value of 0 indicates success. On error, -1 is returned, and
> errno is set appropriately.
>
> ERRORS
> EBUSY Restartable sequence is already registered for this thread.
>
> EFAULT rseq is an invalid address.
>
> EINVAL Either flags contains an invalid value, or rseq contains an
> address which is not appropriately aligned, or rseq_len contains
> a size that does not match the size received on registration.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │The last case "rseq_len contains a size that does │
> │not match the size received on registration" can │
> │occur only on RSEQ_FLAG_UNREGISTER, tight? │
> └─────────────────────────────────────────────────────┘
>
> ENOSYS The rseq() system call is not implemented by this kernel.
>
> EPERM The sig argument on unregistration does not match the signature
> received on registration.
>
> VERSIONS
> The rseq() system call was added in Linux 4.18.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │What is the current state of library support? │
> └─────────────────────────────────────────────────────┘
After going through a few RFC rounds, it's been posted as non-rfc a
few weeks ago. It is pending review from glibc maintainers. I currently
aim for inclusion of the rseq TLS registration by glibc for glibc 2.30:
https://sourceware.org/ml/libc-alpha/2019-02/msg00317.html
https://sourceware.org/ml/libc-alpha/2019-02/msg00320.html
https://sourceware.org/ml/libc-alpha/2019-02/msg00319.html
https://sourceware.org/ml/libc-alpha/2019-02/msg00318.html
https://sourceware.org/ml/libc-alpha/2019-02/msg00321.html
Note that the C library will define a user-space ABI which states how
applications/libraries wishing to register the rseq TLS need to behave so they
are compatible with the C library when it gets updated to a new version providing
rseq registration support. It seems like an important point to document,
perhaps even here in the rseq(2) man page.
>
> CONFORMING TO
> rseq() is Linux-specific.
>
> ┌─────────────────────────────────────────────────────┐
> │FIXME │
> ├─────────────────────────────────────────────────────┤
> │Is there any example code that can reasonably be │
> │included in this manual page? Or some example code │
> │that can be referred to? │
> └─────────────────────────────────────────────────────┘
>
The per-cpu counter example we have here seems compact enough:
https://www.efficios.com/blog/2019/02/08/linux-restartable-sequences/
Thanks,
Mathieu
> SEE ALSO
> sched_getcpu(3), membarrier(2)
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: a.out coredumping: fix or delete?
From: Borislav Petkov @ 2019-03-05 9:19 UTC (permalink / raw)
To: Jann Horn
Cc: Al Viro, Thomas Gleixner, kernel list, linux-fsdevel,
the arch/x86 maintainers, Linux API
In-Reply-To: <CAG48ez1RVd5mQ_Pb6eygQESaZhpQz765OAZYSoPE0kPqfZEXQg@mail.gmail.com>
On Sat, Mar 02, 2019 at 12:57:30AM +0100, Jann Horn wrote:
> In theory, Linux can dump cores for a.out binaries. In practice, that
> code is pretty bitrotten and buggy. Does anyone want that code so much
> that they'd like to fix it, or can we just delete it?
I was asking myself the same question while doing
10970e1b4be9 ("x86/a.out: Clear the dump structure initially")
especially since it is kinda hard to get the toolchain to even create an
a.out executable - your script to create one case-in-point.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: a.out coredumping: fix or delete?
From: Matthew Wilcox @ 2019-03-05 12:22 UTC (permalink / raw)
To: Borislav Petkov
Cc: Jann Horn, Al Viro, Thomas Gleixner, kernel list, linux-fsdevel,
the arch/x86 maintainers, Linux API
In-Reply-To: <20190305091904.GB8256@zn.tnic>
On Tue, Mar 05, 2019 at 10:19:04AM +0100, Borislav Petkov wrote:
> On Sat, Mar 02, 2019 at 12:57:30AM +0100, Jann Horn wrote:
> > In theory, Linux can dump cores for a.out binaries. In practice, that
> > code is pretty bitrotten and buggy. Does anyone want that code so much
> > that they'd like to fix it, or can we just delete it?
>
> I was asking myself the same question while doing
>
> 10970e1b4be9 ("x86/a.out: Clear the dump structure initially")
>
> especially since it is kinda hard to get the toolchain to even create an
> a.out executable - your script to create one case-in-point.
It's been 25 years since Linux added support for ELF. Can we just
delete the a.out support entirely now? According to the Linux-ELF HOWTO,
support was added in 1.1.52 (August 1994). It's pretty much necromancy
at this point.
^ permalink raw reply
* Re: a.out coredumping: fix or delete?
From: Alan Cox @ 2019-03-05 13:43 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Borislav Petkov, Jann Horn, Al Viro, Thomas Gleixner, kernel list,
linux-fsdevel, the arch/x86 maintainers, Linux API
In-Reply-To: <20190305122218.GD13380@bombadil.infradead.org>
> It's been 25 years since Linux added support for ELF. Can we just
> delete the a.out support entirely now? According to the Linux-ELF HOWTO,
> support was added in 1.1.52 (August 1994). It's pretty much necromancy
> at this point.
In the unlikely event that someone actually has an a.out binary they
can't live with they can also just write an a.out loader as an ELF
program entirely in userspace.
I'd vote for giving it the boot unless there are any architectures that
kept using a.out far longer due to tool chain issues ?
Alan
^ permalink raw reply
* [PATCH] x86: Deprecate a.out support
From: Borislav Petkov @ 2019-03-05 14:59 UTC (permalink / raw)
To: Alan Cox
Cc: Matthew Wilcox, Jann Horn, Al Viro, Thomas Gleixner, kernel list,
linux-fsdevel, the arch/x86 maintainers, Linux API, Andrew Morton,
Linus Torvalds, Richard Weinberger, Anton Ivanov
In-Reply-To: <20190305134347.4be2449c@alans-desktop>
+ akpm and Linus.
On Tue, Mar 05, 2019 at 01:43:47PM +0000, Alan Cox wrote:
> > It's been 25 years since Linux added support for ELF. Can we just
> > delete the a.out support entirely now? According to the Linux-ELF HOWTO,
> > support was added in 1.1.52 (August 1994). It's pretty much necromancy
> > at this point.
>
> In the unlikely event that someone actually has an a.out binary they
> can't live with they can also just write an a.out loader as an ELF
> program entirely in userspace.
>
> I'd vote for giving it the boot unless there are any architectures that
> kept using a.out far longer due to tool chain issues ?
We can at least deprecate it on x86...
---
From: Borislav Petkov <bp@suse.de>
Date: Tue, 5 Mar 2019 15:47:51 +0100
Subject: [PATCH] x86: Deprecate a.out support
Linux supports ELF binaries for ~25 years now. a.out coredumping has
bitrotten quite significantly and would need some fixing to get it into
shape again but considering how even the toolchains cannot create a.out
executables in its default configuration, let's deprecate a.out support
and remove it a couple of releases later, instead.
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Jann Horn <jannh@google.com>
Cc: <linux-api@vger.kernel.org>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: lkml <linux-kernel@vger.kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Richard Weinberger <richard@nod.at>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: <x86@kernel.org>
---
arch/x86/Kconfig | 2 +-
arch/x86/um/Kconfig | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 68261430fe6e..ade12ec4224b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -14,7 +14,6 @@ config X86_32
select ARCH_WANT_IPC_PARSE_VERSION
select CLKSRC_I8253
select CLONE_BACKWARDS
- select HAVE_AOUT
select HAVE_GENERIC_DMA_COHERENT
select MODULES_USE_ELF_REL
select OLD_SIGACTION
@@ -2843,6 +2842,7 @@ config IA32_EMULATION
config IA32_AOUT
tristate "IA32 a.out support"
depends on IA32_EMULATION
+ depends on BROKEN
---help---
Support old a.out binaries in the 32bit emulation.
diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
index f518b4744ff8..494eeb51e4e1 100644
--- a/arch/x86/um/Kconfig
+++ b/arch/x86/um/Kconfig
@@ -16,7 +16,6 @@ config 64BIT
config X86_32
def_bool !64BIT
- select HAVE_AOUT
select ARCH_WANT_IPC_PARSE_VERSION
select MODULES_USE_ELF_REL
select CLONE_BACKWARDS
--
2.21.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply related
* Re: [PATCH] x86: Deprecate a.out support
From: Richard Weinberger @ 2019-03-05 15:17 UTC (permalink / raw)
To: Borislav Petkov
Cc: Alan Cox, Matthew Wilcox, Jann Horn, Al Viro, Thomas Gleixner,
kernel list, linux-fsdevel, the arch/x86 maintainers, Linux API,
Andrew Morton, Linus Torvalds, Anton Ivanov
In-Reply-To: <20190305145717.GD8256@zn.tnic>
Am Dienstag, 5. März 2019, 15:59:52 CET schrieb Borislav Petkov:
> + akpm and Linus.
>
> On Tue, Mar 05, 2019 at 01:43:47PM +0000, Alan Cox wrote:
> > > It's been 25 years since Linux added support for ELF. Can we just
> > > delete the a.out support entirely now? According to the Linux-ELF HOWTO,
> > > support was added in 1.1.52 (August 1994). It's pretty much necromancy
> > > at this point.
> >
> > In the unlikely event that someone actually has an a.out binary they
> > can't live with they can also just write an a.out loader as an ELF
> > program entirely in userspace.
> >
> > I'd vote for giving it the boot unless there are any architectures that
> > kept using a.out far longer due to tool chain issues ?
>
> We can at least deprecate it on x86...
>
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Tue, 5 Mar 2019 15:47:51 +0100
> Subject: [PATCH] x86: Deprecate a.out support
>
> Linux supports ELF binaries for ~25 years now. a.out coredumping has
> bitrotten quite significantly and would need some fixing to get it into
> shape again but considering how even the toolchains cannot create a.out
> executables in its default configuration, let's deprecate a.out support
> and remove it a couple of releases later, instead.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: <linux-api@vger.kernel.org>
> Cc: <linux-fsdevel@vger.kernel.org>
> Cc: lkml <linux-kernel@vger.kernel.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: <x86@kernel.org>
> ---
> arch/x86/Kconfig | 2 +-
> arch/x86/um/Kconfig | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 68261430fe6e..ade12ec4224b 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -14,7 +14,6 @@ config X86_32
> select ARCH_WANT_IPC_PARSE_VERSION
> select CLKSRC_I8253
> select CLONE_BACKWARDS
> - select HAVE_AOUT
> select HAVE_GENERIC_DMA_COHERENT
> select MODULES_USE_ELF_REL
> select OLD_SIGACTION
> @@ -2843,6 +2842,7 @@ config IA32_EMULATION
> config IA32_AOUT
> tristate "IA32 a.out support"
> depends on IA32_EMULATION
> + depends on BROKEN
> ---help---
> Support old a.out binaries in the 32bit emulation.
>
> diff --git a/arch/x86/um/Kconfig b/arch/x86/um/Kconfig
> index f518b4744ff8..494eeb51e4e1 100644
> --- a/arch/x86/um/Kconfig
> +++ b/arch/x86/um/Kconfig
> @@ -16,7 +16,6 @@ config 64BIT
>
> config X86_32
> def_bool !64BIT
> - select HAVE_AOUT
> select ARCH_WANT_IPC_PARSE_VERSION
> select MODULES_USE_ELF_REL
> select CLONE_BACKWARDS
>
For UML,
Acked-by: Richard Weinberger <richard@nod.at>
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH] x86: Deprecate a.out support
From: Linus Torvalds @ 2019-03-05 16:22 UTC (permalink / raw)
To: Borislav Petkov
Cc: Alan Cox, Matthew Wilcox, Jann Horn, Al Viro, Thomas Gleixner,
kernel list, linux-fsdevel, the arch/x86 maintainers, Linux API,
Andrew Morton, Richard Weinberger, Anton Ivanov
In-Reply-To: <20190305145717.GD8256@zn.tnic>
[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]
On Tue, Mar 5, 2019 at 6:59 AM Borislav Petkov <bp@alien8.de> wrote:
>
> We can at least deprecate it on x86...
I'd prefer to try to deprecate a.out core dumping first.. That's the
part that is actually broken, no?
In fact, I'd be happy to deprecate a.out entirely, but if somebody
_does_ complain, I'd like to be able to bring it back without the core
dumping.
Because I think the likeliihood that anybody cares about a.out core
dumps is basically zero. While the likelihood that we have some odd
old binary that is still a.out is slightly above zero.
So I'd be much happier with this if it was a two-stage thing where we
just delete a.out core dumping entirely first, and then deprecate even
running a.out binaries separately.
Because I think all the known *bugs* we had were with the core dumping
code, weren't they?
Removing it looks trivial. Untested patch attached.
Then I'd be much happier with your "let's deprecate a.out entirely" as
a second patch, because I think it's a unrelated issue and much more
likely to have somebody pipe up and say "hey, I have this sequence
that generates executables dynamically, and I use a.out because it's
much simpler than ELF, and now it's broken". Or something.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 8506 bytes --]
arch/x86/ia32/ia32_aout.c | 159 ----------------------------------------------
fs/binfmt_aout.c | 82 ------------------------
2 files changed, 241 deletions(-)
diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index 7dbbe9ffda17..3c135084e1eb 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -39,82 +39,10 @@
static int load_aout_binary(struct linux_binprm *);
static int load_aout_library(struct file *);
-#ifdef CONFIG_COREDUMP
-static int aout_core_dump(struct coredump_params *);
-
-static unsigned long get_dr(int n)
-{
- struct perf_event *bp = current->thread.ptrace_bps[n];
- return bp ? bp->hw.info.address : 0;
-}
-
-/*
- * fill in the user structure for a core dump..
- */
-static void fill_dump(struct pt_regs *regs, struct user32 *dump)
-{
- u32 fs, gs;
- memset(dump, 0, sizeof(*dump));
-
-/* changed the size calculations - should hopefully work better. lbt */
- dump->magic = CMAGIC;
- dump->start_code = 0;
- dump->start_stack = regs->sp & ~(PAGE_SIZE - 1);
- dump->u_tsize = ((unsigned long) current->mm->end_code) >> PAGE_SHIFT;
- dump->u_dsize = ((unsigned long)
- (current->mm->brk + (PAGE_SIZE-1))) >> PAGE_SHIFT;
- dump->u_dsize -= dump->u_tsize;
- dump->u_debugreg[0] = get_dr(0);
- dump->u_debugreg[1] = get_dr(1);
- dump->u_debugreg[2] = get_dr(2);
- dump->u_debugreg[3] = get_dr(3);
- dump->u_debugreg[6] = current->thread.debugreg6;
- dump->u_debugreg[7] = current->thread.ptrace_dr7;
-
- if (dump->start_stack < 0xc0000000) {
- unsigned long tmp;
-
- tmp = (unsigned long) (0xc0000000 - dump->start_stack);
- dump->u_ssize = tmp >> PAGE_SHIFT;
- }
-
- dump->regs.ebx = regs->bx;
- dump->regs.ecx = regs->cx;
- dump->regs.edx = regs->dx;
- dump->regs.esi = regs->si;
- dump->regs.edi = regs->di;
- dump->regs.ebp = regs->bp;
- dump->regs.eax = regs->ax;
- dump->regs.ds = current->thread.ds;
- dump->regs.es = current->thread.es;
- savesegment(fs, fs);
- dump->regs.fs = fs;
- savesegment(gs, gs);
- dump->regs.gs = gs;
- dump->regs.orig_eax = regs->orig_ax;
- dump->regs.eip = regs->ip;
- dump->regs.cs = regs->cs;
- dump->regs.eflags = regs->flags;
- dump->regs.esp = regs->sp;
- dump->regs.ss = regs->ss;
-
-#if 1 /* FIXME */
- dump->u_fpvalid = 0;
-#else
- dump->u_fpvalid = dump_fpu(regs, &dump->i387);
-#endif
-}
-
-#endif
-
static struct linux_binfmt aout_format = {
.module = THIS_MODULE,
.load_binary = load_aout_binary,
.load_shlib = load_aout_library,
-#ifdef CONFIG_COREDUMP
- .core_dump = aout_core_dump,
-#endif
- .min_coredump = PAGE_SIZE
};
static int set_brk(unsigned long start, unsigned long end)
@@ -126,93 +54,6 @@ static int set_brk(unsigned long start, unsigned long end)
return vm_brk(start, end - start);
}
-#ifdef CONFIG_COREDUMP
-/*
- * These are the only things you should do on a core-file: use only these
- * macros to write out all the necessary info.
- */
-
-#include <linux/coredump.h>
-
-#define START_DATA(u) (u.u_tsize << PAGE_SHIFT)
-#define START_STACK(u) (u.start_stack)
-
-/*
- * Routine writes a core dump image in the current directory.
- * Currently only a stub-function.
- *
- * Note that setuid/setgid files won't make a core-dump if the uid/gid
- * changed due to the set[u|g]id. It's enforced by the "current->mm->dumpable"
- * field, which also makes sure the core-dumps won't be recursive if the
- * dumping of the process results in another error..
- */
-
-static int aout_core_dump(struct coredump_params *cprm)
-{
- mm_segment_t fs;
- int has_dumped = 0;
- unsigned long dump_start, dump_size;
- struct user32 dump;
-
- fs = get_fs();
- set_fs(KERNEL_DS);
- has_dumped = 1;
-
- fill_dump(cprm->regs, &dump);
-
- strncpy(dump.u_comm, current->comm, sizeof(current->comm));
- dump.u_ar0 = offsetof(struct user32, regs);
- dump.signal = cprm->siginfo->si_signo;
-
- /*
- * If the size of the dump file exceeds the rlimit, then see
- * what would happen if we wrote the stack, but not the data
- * area.
- */
- if ((dump.u_dsize + dump.u_ssize + 1) * PAGE_SIZE > cprm->limit)
- dump.u_dsize = 0;
-
- /* Make sure we have enough room to write the stack and data areas. */
- if ((dump.u_ssize + 1) * PAGE_SIZE > cprm->limit)
- dump.u_ssize = 0;
-
- /* make sure we actually have a data and stack area to dump */
- set_fs(USER_DS);
- if (!access_ok((void *) (unsigned long)START_DATA(dump),
- dump.u_dsize << PAGE_SHIFT))
- dump.u_dsize = 0;
- if (!access_ok((void *) (unsigned long)START_STACK(dump),
- dump.u_ssize << PAGE_SHIFT))
- dump.u_ssize = 0;
-
- set_fs(KERNEL_DS);
- /* struct user */
- if (!dump_emit(cprm, &dump, sizeof(dump)))
- goto end_coredump;
- /* Now dump all of the user data. Include malloced stuff as well */
- if (!dump_skip(cprm, PAGE_SIZE - sizeof(dump)))
- goto end_coredump;
- /* now we start writing out the user space info */
- set_fs(USER_DS);
- /* Dump the data area */
- if (dump.u_dsize != 0) {
- dump_start = START_DATA(dump);
- dump_size = dump.u_dsize << PAGE_SHIFT;
- if (!dump_emit(cprm, (void *)dump_start, dump_size))
- goto end_coredump;
- }
- /* Now prepare to dump the stack area */
- if (dump.u_ssize != 0) {
- dump_start = START_STACK(dump);
- dump_size = dump.u_ssize << PAGE_SHIFT;
- if (!dump_emit(cprm, (void *)dump_start, dump_size))
- goto end_coredump;
- }
-end_coredump:
- set_fs(fs);
- return has_dumped;
-}
-#endif
/*
* create_aout_tables() parses the env- and arg-strings in new user
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index ca9725f18e00..aa0f1e53113f 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -34,92 +34,10 @@
static int load_aout_binary(struct linux_binprm *);
static int load_aout_library(struct file*);
-#ifdef CONFIG_COREDUMP
-/*
- * Routine writes a core dump image in the current directory.
- * Currently only a stub-function.
- *
- * Note that setuid/setgid files won't make a core-dump if the uid/gid
- * changed due to the set[u|g]id. It's enforced by the "current->mm->dumpable"
- * field, which also makes sure the core-dumps won't be recursive if the
- * dumping of the process results in another error..
- */
-static int aout_core_dump(struct coredump_params *cprm)
-{
- mm_segment_t fs;
- int has_dumped = 0;
- void __user *dump_start;
- int dump_size;
- struct user dump;
-#ifdef __alpha__
-# define START_DATA(u) ((void __user *)u.start_data)
-#else
-# define START_DATA(u) ((void __user *)((u.u_tsize << PAGE_SHIFT) + \
- u.start_code))
-#endif
-# define START_STACK(u) ((void __user *)u.start_stack)
-
- fs = get_fs();
- set_fs(KERNEL_DS);
- has_dumped = 1;
- strncpy(dump.u_comm, current->comm, sizeof(dump.u_comm));
- dump.u_ar0 = offsetof(struct user, regs);
- dump.signal = cprm->siginfo->si_signo;
- aout_dump_thread(cprm->regs, &dump);
-
-/* If the size of the dump file exceeds the rlimit, then see what would happen
- if we wrote the stack, but not the data area. */
- if ((dump.u_dsize + dump.u_ssize+1) * PAGE_SIZE > cprm->limit)
- dump.u_dsize = 0;
-
-/* Make sure we have enough room to write the stack and data areas. */
- if ((dump.u_ssize + 1) * PAGE_SIZE > cprm->limit)
- dump.u_ssize = 0;
-
-/* make sure we actually have a data and stack area to dump */
- set_fs(USER_DS);
- if (!access_ok(START_DATA(dump), dump.u_dsize << PAGE_SHIFT))
- dump.u_dsize = 0;
- if (!access_ok(START_STACK(dump), dump.u_ssize << PAGE_SHIFT))
- dump.u_ssize = 0;
-
- set_fs(KERNEL_DS);
-/* struct user */
- if (!dump_emit(cprm, &dump, sizeof(dump)))
- goto end_coredump;
-/* Now dump all of the user data. Include malloced stuff as well */
- if (!dump_skip(cprm, PAGE_SIZE - sizeof(dump)))
- goto end_coredump;
-/* now we start writing out the user space info */
- set_fs(USER_DS);
-/* Dump the data area */
- if (dump.u_dsize != 0) {
- dump_start = START_DATA(dump);
- dump_size = dump.u_dsize << PAGE_SHIFT;
- if (!dump_emit(cprm, dump_start, dump_size))
- goto end_coredump;
- }
-/* Now prepare to dump the stack area */
- if (dump.u_ssize != 0) {
- dump_start = START_STACK(dump);
- dump_size = dump.u_ssize << PAGE_SHIFT;
- if (!dump_emit(cprm, dump_start, dump_size))
- goto end_coredump;
- }
-end_coredump:
- set_fs(fs);
- return has_dumped;
-}
-#else
-#define aout_core_dump NULL
-#endif
-
static struct linux_binfmt aout_format = {
.module = THIS_MODULE,
.load_binary = load_aout_binary,
.load_shlib = load_aout_library,
- .core_dump = aout_core_dump,
- .min_coredump = PAGE_SIZE
};
#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
^ 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