* Re: [PATCH 2/6] aio: remove an outdated comment in aio_complete
From: Matthew Wilcox @ 2018-03-28 17:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: viro, Avi Kivity, linux-aio, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <20180328072639.16885-3-hch@lst.de>
On Wed, Mar 28, 2018 at 09:26:35AM +0200, Christoph Hellwig wrote:
> These days we don't treat sync iocbs special in the aio completion code as
> they never use it. Remove the old comment and BUG_ON given that the
> current definition of is_sync_kiocb makes it impossible to hit.
> iocb to the top of the function.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks like you have a stray line of text in the changelog here ...
--
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 v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 17:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <67c5f9b4-b24a-2806-e8d6-8b5241c66d6e@fb.com>
On Wed, 28 Mar 2018 09:43:56 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> >
> > Given that only eBPF needs this parameter count, we can move
> > it to the struct bpf_raw_event_map newly introduced by Steven,
> > right ? This would reduce bloat of struct tracepoint. For instance,
> > we don't need to keep this count around when eBPF is configured
> > out.
>
> Makes sense. That is indeed cleaner. Will respin.
>
> What I don't like though is 'bloat' argument.
> 'u32 num_args' padded to 8-byte takes exactly the same amount
> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
> The number of these structures is the same as well
> and chances that tracepoints are on while bpf is off are slim.
> More so few emails ago you said:
> "I'm perfectly fine with adding the "num_args" stuff. I think it's
> really useful. It's only the for_each_kernel_tracepoint change for
> which I'm trying to understand the rationale."
I don't really care which one it goes in. The padding bloat is the same
for both :-/ But I wonder if we can shrink it by doing a trick that
Josh did in one of his patches. That is, to use a 32bit offset instead
of a direct pointer. Since you are only accessing core kernel
tracepoints.
Thus, we could have
struct bpf_raw_event_map {
u32 tp_offset;
u32 num_args;
void *bpf_func;
};
and have:
u64 tp_offset = (u64)tp - (u64)_sdata;
if (WARN_ON(tp_offset > UINT_MAX)
return -EINVAL;
btp->tp_offset = (u32)tp_offset;
And to get the tp, all you need to do is:
tp = (struct tracepoint *)(btp->tp_offset + (unsigned long)_sdata);
I've been thinking of doing this for other parts of the tracepoints and
ftrace code.
BTW, thanks for changing your code. I really appreciate it.
-- Steve
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 16:43 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <1074829260.1986.1522244964935.JavaMail.zimbra@efficios.com>
On 3/28/18 6:49 AM, Mathieu Desnoyers wrote:
> ----- On Mar 27, 2018, at 10:11 PM, Alexei Starovoitov ast@fb.com wrote:
>
>> From: Alexei Starovoitov <ast@kernel.org>
>>
>> compute number of arguments passed into tracepoint
>> at compile time and store it as part of 'struct tracepoint'.
>> The number is necessary to check safety of bpf program access that
>> is coming in subsequent patch.
>
>
> Hi Alexei,
>
> Given that only eBPF needs this parameter count, we can move
> it to the struct bpf_raw_event_map newly introduced by Steven,
> right ? This would reduce bloat of struct tracepoint. For instance,
> we don't need to keep this count around when eBPF is configured
> out.
Makes sense. That is indeed cleaner. Will respin.
What I don't like though is 'bloat' argument.
'u32 num_args' padded to 8-byte takes exactly the same amount
of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
The number of these structures is the same as well
and chances that tracepoints are on while bpf is off are slim.
More so few emails ago you said:
"I'm perfectly fine with adding the "num_args" stuff. I think it's
really useful. It's only the for_each_kernel_tracepoint change for
which I'm trying to understand the rationale."
I'm guessing now you found out that num_args is not useful to lttng
and it bloats its data structures?
It's ok to change an opinion and I'm completely fine using that as
a real reason.
Will repsin, as I said. No problem.
^ permalink raw reply
* Re: [PATCH 07/30] aio: add delayed cancel support
From: Al Viro @ 2018-03-28 16:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180328072926.17131-8-hch@lst.de>
On Wed, Mar 28, 2018 at 09:29:03AM +0200, Christoph Hellwig wrote:
> static void aio_fsync_work(struct work_struct *work)
> {
> struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
> + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, fsync);
> + struct file *file = req->file;
> int ret;
>
> ret = vfs_fsync(req->file, req->datasync);
> - fput(req->file);
> - aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
> + if (aio_complete(iocb, ret, 0, 0))
> + fput(file);
IDGI.
1) can aio_complete() ever return false here?
2) do we ever have aio_kiocb that would not have an associated
struct file * that needs to be dropped on successful aio_complete()? AFAICS,
rw, fsync and poll variants all have one, and I'm not sure what kind of
async IO *could* be done without an opened file.
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 16:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328122946.GU4043@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 8:29 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
[...]
>> + /* Ensure that abort_ip is not in the critical section. */
>> + if (rseq_cs.abort_ip - rseq_cs.start_ip < rseq_cs.post_commit_offset)
>> + return -EINVAL;
>
> The kernel will not crash if userspace messes that up right? So why do
> we care to check?
That's because the kernel clears the TLS @rseq_cs pointer whenever it restarts
a rseq critical section. Therefore, if the abort_ip points somewhere within
the rseq critical section, the kernel will clear the @rseq_cs pointer, move the
instruction pointer to the abort_ip, and return to user-space. At that stage,
user-space will still be running within a rseq critical section, but the
@rseq_cs pointer is NULL. So if the kernel preempts again before that critical
section completes, it will completely miss it.
So having the kernel segfault userspace when this pattern is encountered
is an extra safety net ensuring that if user-space ever implement such
construct, at least it will segfault quickly, and will therefore be easier
to debug, because easier to reproduce.
>> +
>> + usig = (u32 __user *)(rseq_cs.abort_ip - sizeof(u32));
>> + ret = get_user(sig, usig);
>> + if (ret)
>> + return ret;
>> +
>
>> + if (current->rseq_sig != sig) {
>> + printk_ratelimited(KERN_WARNING
>> + "Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x
>> (pid=%d, addr=%p).\n",
>> + sig, current->rseq_sig, current->pid, usig);
>> + return -EPERM;
>> + }
>
> Is there any text that explains the thread model and possible attack
> that this signature prevents? I failed to find any, which raises the
> question, why is it there..
The threat model is an attacker partly controlling a user-space process, trying
to execute his own code by abusing the rseq restart mechanism to make the kernel
jump to a user-space address of his choice, which contains either an injected shell
code or specific library functions, thus escalating to full control of the process
execution.
Where should I document this ?
>
>> + int ret;
>> +
>> + /* Get thread flags. */
>> + ret = __get_user(flags, &t->rseq->flags);
>> + if (ret)
>> + return ret;
>> +
>> + /* Take critical section flags into account. */
>> + flags |= cs_flags;
>> +
>> + /*
>> + * Restart on signal can only be inhibited when restart on
>> + * preempt and restart on migrate are inhibited too. Otherwise,
>> + * a preempted signal handler could fail to restart the prior
>> + * execution context on sigreturn.
>> + */
>> + if (unlikely(flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL)) {
>> + if ((flags & (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
>> + | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)) !=
>> + (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
>> + | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT))
>> + return -EINVAL;
>
> Please put operators at the end of the previous line, not at the start
> of the new line when you have to break statements.
>
> Also, that's unreadable.
>
> #define RSEQ_CS_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
> RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
> RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)
>
> if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
> (flags & RSEQ_CS_FLAGS) != RSEQ_CS_FLAGS))
> return -EINVAL;
>
Based on your suggestion:
#define RSEQ_CS_PREEMPT_MIGRATE_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE | \
RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)
if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
(flags & RSEQ_CS_PREEMPT_MIGRATE_FLAGS) !=
RSEQ_CS_PREEMPT_MIGRATE_FLAGS))
return -EINVAL;
>> +}
>> +
>> +static int clear_rseq_cs(struct task_struct *t)
>> +{
>> + unsigned long ptr = 0;
>> +
>> + /*
>> + * The rseq_cs field is set to NULL on preemption or signal
>> + * delivery on top of rseq assembly block, as well as on top
>> + * of code outside of the rseq assembly block. This performs
>> + * a lazy clear of the rseq_cs field.
>> + *
>> + * Set rseq_cs to NULL with single-copy atomicity.
>> + */
>> + return __put_user(ptr, &t->rseq->rseq_cs);
>
> __put_user(0UL, &t->rseq->rseq_cs); ?
Yes.
>
>> +}
>> +
>> +static int rseq_ip_fixup(struct pt_regs *regs)
>> +{
>> + unsigned long ip = instruction_pointer(regs), start_ip = 0,
>> + post_commit_offset = 0, abort_ip = 0;
>
> valid C, but yuck. Just have two 'unsigned long' lines.
>
> Also, why the =0, the below call to rseq_get_rseq_cs() will either
> initialize of fail.
rseq_get_rseq_cs() can return 0 (success) if __get_user finds a
NULL pointer in the @rseq_cs TLS field. I'll use a
struct rseq_cs * parameter instead, and memset to 0 only in that
specific success case within rseq_get_rseq_cs() rather than always
initialize to 0 in its caller.
>
>
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Handle potentially not being within a critical section.
>> + * Unsigned comparison will be true when
>> + * ip >= start_ip, and when ip < start_ip + post_commit_offset.
>> + */
>> + if (ip - start_ip < post_commit_offset)
>> + in_rseq_cs = true;
>> +
>> + /*
>> + * If not nested over a rseq critical section, restart is
>> + * useless. Clear the rseq_cs pointer and return.
>> + */
>> + if (!in_rseq_cs)
>> + return clear_rseq_cs(t);
>
>
> That all seems needlessly complicated; isn't:
>
> if (ip - start_ip >= post_commit_offset)
> return clear_rseq_cs();
>
> equivalent? Nothing seems to use that variable after this.
Yep, Boqun already pointed it out. Fixed.
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 2/6] aio: remove an outdated comment in aio_complete
From: Darrick J. Wong @ 2018-03-28 16:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: viro, Avi Kivity, linux-aio, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <20180328072639.16885-3-hch@lst.de>
On Wed, Mar 28, 2018 at 09:26:35AM +0200, Christoph Hellwig wrote:
> These days we don't treat sync iocbs special in the aio completion code as
> they never use it. Remove the old comment and BUG_ON given that the
> current definition of is_sync_kiocb makes it impossible to hit.
> iocb to the top of the function.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Yeah that looks fairly impossible now...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/aio.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 03d59593912d..f536b0f249d4 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1100,15 +1100,6 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2)
> file_end_write(file);
> }
>
> - /*
> - * Special case handling for sync iocbs:
> - * - events go directly into the iocb for fast handling
> - * - the sync task with the iocb in its stack holds the single iocb
> - * ref, no other paths have a way to get another ref
> - * - the sync task helpfully left a reference to itself in the iocb
> - */
> - BUG_ON(is_sync_kiocb(kiocb));
> -
> if (iocb->ki_list.next) {
> unsigned long flags;
>
> --
> 2.14.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 15:37 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328152814.GI4082@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 11:28 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Wed, Mar 28, 2018 at 11:14:05AM -0400, Mathieu Desnoyers wrote:
>
>> > If at all possible I would make it SIGSEGV when issueing SYSCALL()s from
>> > within an RSEQ.
>>
>> What's the goal there ? rseq critical sections can technically do system calls
>> if they wish. Why prevent this ?
>
> This all started as a way to do 'small' _fast_ per-cpu ops, System calls
> do NOT fit in that pattern. If you're willing to do a system calls the
> cost of atomics is not a problem.
I'm not arguing that a typical rseq would do a system call. I'm merely
pointing out that if we start putting arbitrary limitations like "SIGSEGV
when a fork or system call is encountered on top of rseq", this will cause
pain in user-space.
>
>> How would you handle signal handlers that issue system calls while nested
>> on top of a rseq critical section in the userspace thread ? SIGSEGV on
>> SYSCALLs will break this case.
>
> Have the rseq thing aborted prior to delivering the signal ?
Not if the RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL flag is set either in the TLS
or in the rseq_cs structure.
How about we simply add a rseq_migrate() within rseq_fork() (when
forking to a new process), which will allow me to move the rseq_migrate
from __set_task_cpu() to set_task_cpu() as you request. Is that solution
acceptable for you ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-28 15:28 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <265889560.1.1522250045589.JavaMail.zimbra@efficios.com>
On Wed, Mar 28, 2018 at 11:14:05AM -0400, Mathieu Desnoyers wrote:
> > If at all possible I would make it SIGSEGV when issueing SYSCALL()s from
> > within an RSEQ.
>
> What's the goal there ? rseq critical sections can technically do system calls
> if they wish. Why prevent this ?
This all started as a way to do 'small' _fast_ per-cpu ops, System calls
do NOT fit in that pattern. If you're willing to do a system calls the
cost of atomics is not a problem.
> How would you handle signal handlers that issue system calls while nested
> on top of a rseq critical section in the userspace thread ? SIGSEGV on
> SYSCALLs will break this case.
Have the rseq thing aborted prior to delivering the signal ?
^ permalink raw reply
* Re: [RFC PATCH for 4.17 10/21] cpu_opv: Provide cpu_opv system call (v6)
From: Peter Zijlstra @ 2018-03-28 15:22 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon
In-Reply-To: <20180327160542.28457-11-mathieu.desnoyers@efficios.com>
On Tue, Mar 27, 2018 at 12:05:31PM -0400, Mathieu Desnoyers wrote:
> 1) Allow algorithms to perform per-cpu data migration without relying on
> sched_setaffinity()
>
> The use-cases are migrating memory between per-cpu memory free-lists, or
> stealing tasks from other per-cpu work queues: each require that
> accesses to remote per-cpu data structures are performed.
I think that one completely reduces to the per-cpu (spin)lock case,
right? Because, as per the below, your logging case (8) can 'easily' be
done without the cpu_opv monstrosity.
And if you can construct a per-cpu lock, that can be used to construct
aribtrary logic.
And the difficult case for the per-cpu lock is the remote acquire; all
the other cases are (relatively) trivial.
I've not really managed to get anything sensible to work, I've tried
several variations of split lock, but you invariably end up with
barriers in the fast (local) path, which sucks.
But I feel this should be solvable without cpu_opv. As in, I really hate
that thing ;-)
> 8) Allow libraries with multi-part algorithms to work on same per-cpu
> data without affecting the allowed cpu mask
>
> The lttng-ust tracer presents an interesting use-case for per-cpu
> buffers: the algorithm needs to update a "reserve" counter, serialize
> data into the buffer, and then update a "commit" counter _on the same
> per-cpu buffer_. Using rseq for both reserve and commit can bring
> significant performance benefits.
>
> Clearly, if rseq reserve fails, the algorithm can retry on a different
> per-cpu buffer. However, it's not that easy for the commit. It needs to
> be performed on the same per-cpu buffer as the reserve.
>
> The cpu_opv system call solves that problem by receiving the cpu number
> on which the operation needs to be performed as argument. It can push
> the task to the right CPU if needed, and perform the operations there
> with preemption disabled.
>
> Changing the allowed cpu mask for the current thread is not an
> acceptable alternative for a tracing library, because the application
> being traced does not expect that mask to be changed by libraries.
We talked about this use-case, and it can be solved without cpu_opv if
you keep a dual commit counter, one local and one (atomic) remote.
We retain the cpu_id from the first rseq, and the second part will, when
it (unlikely) finds it runs remotely, do an atomic increment on the
remote counter. The consumer of the counter will then have to sum both
the local and remote counter parts.
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 15:14 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328145946.GH4082@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 10:59 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Wed, Mar 28, 2018 at 10:47:54AM -0400, Mathieu Desnoyers wrote:
>> ----- On Mar 28, 2018, at 8:50 AM, Peter Zijlstra peterz@infradead.org wrote:
>>
>> > On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
>> >> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>> >> index fb5fc458547f..66b070444a7e 100644
>> >> --- a/kernel/sched/sched.h
>> >> +++ b/kernel/sched/sched.h
>> >> @@ -1249,6 +1249,7 @@ static inline void __set_task_cpu(struct task_struct *p,
>> >> unsigned int cpu)
>> >> #endif
>> >> p->wake_cpu = cpu;
>> >> #endif
>> >> + rseq_migrate(p);
>> >> }
>> >
>> > I think you want that in set_task_cpu(), right next to nr_migrations++.
>>
>> This would miss the __set_task_cpu() call from sched_fork() and
>> wake_up_new_task().
>
> Correct; but since those are _new_ tasks they _SHOULD_ not have an
> active RSEQ to begin with.
As long as fork() can be issued from a rseq critical section, nothing
actually prevents this. This is a fork(), not an exec(), so the new tasks
may very well be going through a restartable sequence when fork() happens.
>
>> Those cases are not accounted as explicit "migrations", but it does change the
>> CPU
>> of the current task. So if for some weird reason userspace wants to fork() while
>> in
>> a rseq critical section, we want to trigger a rseq restart.
>
> If at all possible I would make it SIGSEGV when issueing SYSCALL()s from
> within an RSEQ.
What's the goal there ? rseq critical sections can technically do system calls
if they wish. Why prevent this ?
How would you handle signal handlers that issue system calls while nested
on top of a rseq critical section in the userspace thread ? SIGSEGV on
SYSCALLs will break this case.
>
>> An alternative to this would be to call rseq_migrate() in rseq_fork().
>>
>> Thoughts ?
>
> Yes, don't try and support that at all. It's _insane_.
Thomas told me those fork corner-cases should be correctly handled
in a previous version of the patchset. I'm following his advice here.
So either we disallow fork() within rseq critical sections completely with
some kind of validation, or we need to provide a non-bogus behavior when this
happens. Given that fork(2) is async-signal-safe, this means a signal handler
can do a fork() while nested on top of a userspace thread's rseq critical section.
So prohibiting fork() from being called over a rseq c.s. does not seem like
something we can do here.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 15:03 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328125216.GF4129@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 8:52 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Wed, Mar 28, 2018 at 02:29:46PM +0200, Peter Zijlstra wrote:
>> > +static int rseq_get_rseq_cs(struct task_struct *t,
>> > + unsigned long *start_ip,
>> > + unsigned long *post_commit_offset,
>> > + unsigned long *abort_ip,
>> > + uint32_t *cs_flags)
>> > +{
>
>>
>> > +
>> > + *cs_flags = rseq_cs.flags;
>> > + *start_ip = rseq_cs.start_ip;
>> > + *post_commit_offset = rseq_cs.post_commit_offset;
>> > + *abort_ip = rseq_cs.abort_ip;
>>
>> Then this becomes a straight struct assignment.
>
> I initially suggested passing a structure instead of many arguments, but
> then recondidered, mostly because it will be inlined (due to having only
> the one caller) anyway. Still, maybe a struct will work better, I dunno.
I find the result of struct pointer argument cleaner indeed. I'll go for that
approach.
I'll memset rseq_cs to 0 in the following case though, because the caller
expects the content of the structure to be set when rseq_get_rseq_cs() succeeds.
static int rseq_get_rseq_cs(struct task_struct *t, struct rseq_cs *rseq_cs)
{
struct rseq_cs __user *urseq_cs;
unsigned long ptr;
u32 __user *usig;
u32 sig;
int ret;
ret = __get_user(ptr, &t->rseq->rseq_cs);
if (ret)
return ret;
if (!ptr) {
memset(rseq_cs, 0, sizeof(*rseq_cs));
return 0;
}
[...]
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-28 14:59 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <1523662633.2105.1522248474778.JavaMail.zimbra@efficios.com>
On Wed, Mar 28, 2018 at 10:47:54AM -0400, Mathieu Desnoyers wrote:
> ----- On Mar 28, 2018, at 8:50 AM, Peter Zijlstra peterz@infradead.org wrote:
>
> > On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
> >> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> >> index fb5fc458547f..66b070444a7e 100644
> >> --- a/kernel/sched/sched.h
> >> +++ b/kernel/sched/sched.h
> >> @@ -1249,6 +1249,7 @@ static inline void __set_task_cpu(struct task_struct *p,
> >> unsigned int cpu)
> >> #endif
> >> p->wake_cpu = cpu;
> >> #endif
> >> + rseq_migrate(p);
> >> }
> >
> > I think you want that in set_task_cpu(), right next to nr_migrations++.
>
> This would miss the __set_task_cpu() call from sched_fork() and wake_up_new_task().
Correct; but since those are _new_ tasks they _SHOULD_ not have an
active RSEQ to begin with.
> Those cases are not accounted as explicit "migrations", but it does change the CPU
> of the current task. So if for some weird reason userspace wants to fork() while in
> a rseq critical section, we want to trigger a rseq restart.
If at all possible I would make it SIGSEGV when issueing SYSCALL()s from
within an RSEQ.
> An alternative to this would be to call rseq_migrate() in rseq_fork().
>
> Thoughts ?
Yes, don't try and support that at all. It's _insane_.
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 14:47 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328125004.GV4043@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 8:50 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
>> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>> index fb5fc458547f..66b070444a7e 100644
>> --- a/kernel/sched/sched.h
>> +++ b/kernel/sched/sched.h
>> @@ -1249,6 +1249,7 @@ static inline void __set_task_cpu(struct task_struct *p,
>> unsigned int cpu)
>> #endif
>> p->wake_cpu = cpu;
>> #endif
>> + rseq_migrate(p);
>> }
>
> I think you want that in set_task_cpu(), right next to nr_migrations++.
This would miss the __set_task_cpu() call from sched_fork() and wake_up_new_task().
Those cases are not accounted as explicit "migrations", but it does change the CPU
of the current task. So if for some weird reason userspace wants to fork() while in
a rseq critical section, we want to trigger a rseq restart.
Note that rseq_fork() implies rseq_preempt(), but userspace can request to
track only migrations for a given rseq critical section (by using the
RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT flag), so the rseq_preempt() in rseq_fork()
is not enough to restart if a migration between CPUs is done across a fork.
An alternative to this would be to call rseq_migrate() in rseq_fork().
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 14:31 UTC (permalink / raw)
To: Boqun Feng
Cc: Peter Zijlstra, Paul E. McKenney, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas
In-Reply-To: <777590253.2016.1522245960189.JavaMail.zimbra@efficios.com>
----- On Mar 28, 2018, at 10:06 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
> ----- On Mar 28, 2018, at 2:47 AM, Boqun Feng boqun.feng@gmail.com wrote:
>
>> On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
>> [...]
>>> Changes since v11:
>>>
>>> - Replace task struct rseq_preempt, rseq_signal, and rseq_migrate
>>> bool by u32 rseq_event_mask.
>> [...]
>>> @@ -979,6 +980,17 @@ struct task_struct {
>>> unsigned long numa_pages_migrated;
>>> #endif /* CONFIG_NUMA_BALANCING */
>>>
>>> +#ifdef CONFIG_RSEQ
>>> + struct rseq __user *rseq;
>>> + u32 rseq_len;
>>> + u32 rseq_sig;
>>> + /*
>>> + * RmW on rseq_event_mask must be performed atomically
>>> + * with respect to preemption.
>>> + */
>>> + unsigned long rseq_event_mask;
>>
>> s/unsigned long/u32
>
> good point, fixed.
>
Actually, by having a u32 instead of unsigned long here, it triggers those
warnings:
In file included from ./include/linux/bitops.h:38:0,
from ./include/linux/kernel.h:11,
from certs/system_keyring.c:13:
./arch/x86/include/asm/bitops.h:73:1: note: expected ‘volatile long unsigned int *’ but argument is of type ‘u32 *’
set_bit(long nr, volatile unsigned long *addr)
^
I suspect that casting the u32 * to a unsigned long * is not a safe approach, because
the code can generate a load/store on unallocated memory (kasan might complain).
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 14:26 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328112225.GT4043@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 7:22 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
>> +/*
>> + * struct rseq_cs is aligned on 4 * 8 bytes to ensure it is always
>> + * contained within a single cache-line. It is usually declared as
>> + * link-time constant data.
>> + */
>> +struct rseq_cs {
>> + /* Version of this structure. */
>> + uint32_t version;
>> + /* enum rseq_cs_flags */
>> + uint32_t flags;
>> + LINUX_FIELD_u32_u64(start_ip);
>> + /* Offset from start_ip. */
>> + LINUX_FIELD_u32_u64(post_commit_offset);
>> + LINUX_FIELD_u32_u64(abort_ip);
>> +} __attribute__((aligned(4 * sizeof(uint64_t))));
>
> What's with the uint32_t ? The normal Linux API type is __u32 afaik.
Will fix. Working on both kernel and user-space code in parallel kind
of does that to the brain. ;-)
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 14:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328111952.GS4043@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 7:19 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
>> +#ifdef CONFIG_RSEQ
>> + struct rseq __user *rseq;
>> + u32 rseq_len;
>> + u32 rseq_sig;
>> + /*
>> + * RmW on rseq_event_mask must be performed atomically
>> + * with respect to preemption.
>> + */
>> + unsigned long rseq_event_mask;
>> +#endif
>
>> +static inline void rseq_signal_deliver(struct pt_regs *regs)
>> +{
>> + set_bit(RSEQ_EVENT_SIGNAL_BIT, ¤t->rseq_event_mask);
>> + rseq_handle_notify_resume(regs);
>> +}
>> +
>> +static inline void rseq_preempt(struct task_struct *t)
>> +{
>> + set_bit(RSEQ_EVENT_PREEMPT_BIT, &t->rseq_event_mask);
>> + rseq_set_notify_resume(t);
>> +}
>> +
>> +static inline void rseq_migrate(struct task_struct *t)
>> +{
>> + set_bit(RSEQ_EVENT_MIGRATE_BIT, &t->rseq_event_mask);
>> + rseq_set_notify_resume(t);
>> +}
>
> Given that comment above, do you really need the full atomic set bit?
> Isn't __set_bit() sufficient?
For each of rseq_signal_deliver, rseq_preempt, and rseq_migrate, we should
confirm that their callers guarantee preemption is disabled before
we can use __set_bit() in each of those functions.
Is that the case ? If so, we should also document the requirement
about preemption for each function.
AFAIU, rseq_migrate is only invoked from __set_task_cpu, which I *think*
always has preemption disabled. rseq_preempt() is called by the scheduler,
so this one is fine. On x86, rseq_signal_deliver is called from setup_rt_frame,
with preemption enabled.
So one approach would be to use __set_bit in both rseq_preempt and rseq_migrate,
but keep the atomic set_bit() in rseq_signal_deliver.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-28 14:06 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Kees Cook
In-Reply-To: <34b9892d-c091-7b83-d8e8-79c0ff813140@fb.com>
On Tue, 27 Mar 2018 17:51:55 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> Turned out it was in init.data section and got poisoned.
> this fixes it:
> @@ -258,6 +258,7 @@
> LIKELY_PROFILE() \
> BRANCH_PROFILE() \
> TRACE_PRINTKS() \
> + BPF_RAW_TP() \
> TRACEPOINT_STR()
>
> /*
> @@ -585,7 +586,6 @@
> *(.init.rodata) \
> FTRACE_EVENTS() \
> TRACE_SYSCALLS() \
> - BPF_RAW_TP() \
> KPROBE_BLACKLIST() \
> ERROR_INJECT_WHITELIST() \
> MEM_DISCARD(init.rodata) \
>
> and it works :)
> I will clean few other nits I found while debugging and respin.
Getting it properly working was an exercise left to the reader ;-)
Sorry about that, I did a bit of copy and paste to get it working, and
copied from code that did things a bit differently, so I massaged it by
hand, and doing it quickly as I had other things to work on.
-- Steve
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 14:06 UTC (permalink / raw)
To: Boqun Feng
Cc: Peter Zijlstra, Paul E. McKenney, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas
In-Reply-To: <20180328064703.56hsuicmikejg7np@tardis>
----- On Mar 28, 2018, at 2:47 AM, Boqun Feng boqun.feng@gmail.com wrote:
> On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
> [...]
>> Changes since v11:
>>
>> - Replace task struct rseq_preempt, rseq_signal, and rseq_migrate
>> bool by u32 rseq_event_mask.
> [...]
>> @@ -979,6 +980,17 @@ struct task_struct {
>> unsigned long numa_pages_migrated;
>> #endif /* CONFIG_NUMA_BALANCING */
>>
>> +#ifdef CONFIG_RSEQ
>> + struct rseq __user *rseq;
>> + u32 rseq_len;
>> + u32 rseq_sig;
>> + /*
>> + * RmW on rseq_event_mask must be performed atomically
>> + * with respect to preemption.
>> + */
>> + unsigned long rseq_event_mask;
>
> s/unsigned long/u32
good point, fixed.
>
>> +#endif
>> +
>> struct tlbflush_unmap_batch tlb_ubc;
>>
>> struct rcu_head rcu;
>> @@ -1688,4 +1700,110 @@ extern long sched_getaffinity(pid_t pid, struct cpumask
>> *mask);
>> #define TASK_SIZE_OF(tsk) TASK_SIZE
>> #endif
>>
>
> [...]
>
>> +
>> +static int rseq_ip_fixup(struct pt_regs *regs)
>> +{
>> + unsigned long ip = instruction_pointer(regs), start_ip = 0,
>> + post_commit_offset = 0, abort_ip = 0;
>> + struct task_struct *t = current;
>> + uint32_t cs_flags = 0;
>> + bool in_rseq_cs = false;
>
> This seems unnecessary? Because..
>
>> + int ret;
>> +
>> + ret = rseq_get_rseq_cs(t, &start_ip, &post_commit_offset, &abort_ip,
>> + &cs_flags);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Handle potentially not being within a critical section.
>> + * Unsigned comparison will be true when
>> + * ip >= start_ip, and when ip < start_ip + post_commit_offset.
>> + */
>> + if (ip - start_ip < post_commit_offset)
>> + in_rseq_cs = true;
>> +
>> + /*
>> + * If not nested over a rseq critical section, restart is
>> + * useless. Clear the rseq_cs pointer and return.
>> + */
>> + if (!in_rseq_cs)
>> + return clear_rseq_cs(t);
>
> we can write
>
> if (ip - start_ip >= post_commit_offset)
> return clear_rseq_cs(t);
Good point. In a previous version, rseq_get_rseq_cs() had to conditionally
update in_rseq_cs, but it's not the case anymore, so your approach
indeed cleans up the code.
Thanks!
Mathieu
>
> Regards,
> Boqun
>
>> + ret = rseq_need_restart(t, cs_flags);
>> + if (ret <= 0)
>> + return ret;
>> + ret = clear_rseq_cs(t);
>> + if (ret)
>> + return ret;
>> + trace_rseq_ip_fixup(ip, start_ip, post_commit_offset, abort_ip);
>> + instruction_pointer_set(regs, (unsigned long)abort_ip);
>> + return 0;
>> +}
>> +
> [...]
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-28 13:49 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <20180328021105.4061744-7-ast@fb.com>
----- On Mar 27, 2018, at 10:11 PM, Alexei Starovoitov ast@fb.com wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> compute number of arguments passed into tracepoint
> at compile time and store it as part of 'struct tracepoint'.
> The number is necessary to check safety of bpf program access that
> is coming in subsequent patch.
Hi Alexei,
Given that only eBPF needs this parameter count, we can move
it to the struct bpf_raw_event_map newly introduced by Steven,
right ? This would reduce bloat of struct tracepoint. For instance,
we don't need to keep this count around when eBPF is configured
out.
Thanks,
Mathieu
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> include/linux/tracepoint-defs.h | 1 +
> include/linux/tracepoint.h | 12 ++++++------
> include/trace/define_trace.h | 14 +++++++-------
> 3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index 64ed7064f1fa..39a283c61c51 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -33,6 +33,7 @@ struct tracepoint {
> int (*regfunc)(void);
> void (*unregfunc)(void);
> struct tracepoint_func __rcu *funcs;
> + u32 num_args;
> };
>
> #endif
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index c94f466d57ef..c92f4adbc0d7 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -230,18 +230,18 @@ extern void syscall_unregfunc(void);
> * structures, so we create an array of pointers that will be used for iteration
> * on the tracepoints.
> */
> -#define DEFINE_TRACE_FN(name, reg, unreg) \
> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args) \
> static const char __tpstrtab_##name[] \
> __attribute__((section("__tracepoints_strings"))) = #name; \
> struct tracepoint __tracepoint_##name \
> __attribute__((section("__tracepoints"))) = \
> - { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\
> + { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
> static struct tracepoint * const __tracepoint_ptr_##name __used \
> __attribute__((section("__tracepoints_ptrs"))) = \
> &__tracepoint_##name;
>
> -#define DEFINE_TRACE(name) \
> - DEFINE_TRACE_FN(name, NULL, NULL);
> +#define DEFINE_TRACE(name, num_args) \
> + DEFINE_TRACE_FN(name, NULL, NULL, num_args);
>
> #define EXPORT_TRACEPOINT_SYMBOL_GPL(name) \
> EXPORT_SYMBOL_GPL(__tracepoint_##name)
> @@ -275,8 +275,8 @@ extern void syscall_unregfunc(void);
> return false; \
> }
>
> -#define DEFINE_TRACE_FN(name, reg, unreg)
> -#define DEFINE_TRACE(name)
> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args)
> +#define DEFINE_TRACE(name, num_args)
> #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
> #define EXPORT_TRACEPOINT_SYMBOL(name)
>
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index d9e3d4aa3f6e..96b22ace9ae7 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -25,7 +25,7 @@
>
> #undef TRACE_EVENT
> #define TRACE_EVENT(name, proto, args, tstruct, assign, print) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef TRACE_EVENT_CONDITION
> #define TRACE_EVENT_CONDITION(name, proto, args, cond, tstruct, assign, print) \
> @@ -39,24 +39,24 @@
> #undef TRACE_EVENT_FN
> #define TRACE_EVENT_FN(name, proto, args, tstruct, \
> assign, print, reg, unreg) \
> - DEFINE_TRACE_FN(name, reg, unreg)
> + DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>
> #undef TRACE_EVENT_FN_COND
> #define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct, \
> assign, print, reg, unreg) \
> - DEFINE_TRACE_FN(name, reg, unreg)
> + DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT
> #define DEFINE_EVENT(template, name, proto, args) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT_FN
> #define DEFINE_EVENT_FN(template, name, proto, args, reg, unreg) \
> - DEFINE_TRACE_FN(name, reg, unreg)
> + DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT_PRINT
> #define DEFINE_EVENT_PRINT(template, name, proto, args, print) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef DEFINE_EVENT_CONDITION
> #define DEFINE_EVENT_CONDITION(template, name, proto, args, cond) \
> @@ -64,7 +64,7 @@
>
> #undef DECLARE_TRACE
> #define DECLARE_TRACE(name, proto, args) \
> - DEFINE_TRACE(name)
> + DEFINE_TRACE(name, COUNT_ARGS(args))
>
> #undef TRACE_INCLUDE
> #undef __TRACE_INCLUDE
> --
> 2.9.5
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-28 12:52 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon
In-Reply-To: <20180328122946.GU4043@hirez.programming.kicks-ass.net>
On Wed, Mar 28, 2018 at 02:29:46PM +0200, Peter Zijlstra wrote:
> > +static int rseq_get_rseq_cs(struct task_struct *t,
> > + unsigned long *start_ip,
> > + unsigned long *post_commit_offset,
> > + unsigned long *abort_ip,
> > + uint32_t *cs_flags)
> > +{
>
> > +
> > + *cs_flags = rseq_cs.flags;
> > + *start_ip = rseq_cs.start_ip;
> > + *post_commit_offset = rseq_cs.post_commit_offset;
> > + *abort_ip = rseq_cs.abort_ip;
>
> Then this becomes a straight struct assignment.
I initially suggested passing a structure instead of many arguments, but
then recondidered, mostly because it will be inlined (due to having only
the one caller) anyway. Still, maybe a struct will work better, I dunno.
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-28 12:50 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon
In-Reply-To: <20180327160542.28457-3-mathieu.desnoyers@efficios.com>
On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index fb5fc458547f..66b070444a7e 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1249,6 +1249,7 @@ static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu)
> #endif
> p->wake_cpu = cpu;
> #endif
> + rseq_migrate(p);
> }
I think you want that in set_task_cpu(), right next to nr_migrations++.
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-28 12:29 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon
In-Reply-To: <20180327160542.28457-3-mathieu.desnoyers@efficios.com>
On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
> +static int rseq_update_cpu_id(struct task_struct *t)
> +{
> + uint32_t cpu_id = raw_smp_processor_id();
u32
> +
> + if (__put_user(cpu_id, &t->rseq->cpu_id_start))
> + return -EFAULT;
> + if (__put_user(cpu_id, &t->rseq->cpu_id))
> + return -EFAULT;
> + trace_rseq_update(t);
> + return 0;
> +}
> +
> +static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> +{
> + uint32_t cpu_id_start = 0, cpu_id = RSEQ_CPU_ID_UNINITIALIZED;
u32
> +
> + /*
> + * Reset cpu_id_start to its initial state (0).
> + */
> + if (__put_user(cpu_id_start, &t->rseq->cpu_id_start))
> + return -EFAULT;
> + /*
> + * Reset cpu_id to RSEQ_CPU_ID_UNINITIALIZED, so any user coming
> + * in after unregistration can figure out that rseq needs to be
> + * registered again.
> + */
> + if (__put_user(cpu_id, &t->rseq->cpu_id))
> + return -EFAULT;
> + return 0;
> +}
> +
> +static int rseq_get_rseq_cs(struct task_struct *t,
> + unsigned long *start_ip,
> + unsigned long *post_commit_offset,
> + unsigned long *abort_ip,
> + uint32_t *cs_flags)
> +{
> + struct rseq_cs __user *urseq_cs;
> + struct rseq_cs rseq_cs;
> + unsigned long ptr;
> + u32 __user *usig;
> + u32 sig;
> + int ret;
> +
> + ret = __get_user(ptr, &t->rseq->rseq_cs);
> + if (ret)
> + return ret;
> + if (!ptr)
> + return 0;
> + urseq_cs = (struct rseq_cs __user *)ptr;
> + if (copy_from_user(&rseq_cs, urseq_cs, sizeof(rseq_cs)))
> + return -EFAULT;
> + if (rseq_cs.version > 0)
> + return -EINVAL;
> +
> + /* Ensure that abort_ip is not in the critical section. */
> + if (rseq_cs.abort_ip - rseq_cs.start_ip < rseq_cs.post_commit_offset)
> + return -EINVAL;
The kernel will not crash if userspace messes that up right? So why do
we care to check?
> +
> + *cs_flags = rseq_cs.flags;
> + *start_ip = rseq_cs.start_ip;
> + *post_commit_offset = rseq_cs.post_commit_offset;
> + *abort_ip = rseq_cs.abort_ip;
Then this becomes a straight struct assignment.
> +
> + usig = (u32 __user *)(rseq_cs.abort_ip - sizeof(u32));
> + ret = get_user(sig, usig);
> + if (ret)
> + return ret;
> +
> + if (current->rseq_sig != sig) {
> + printk_ratelimited(KERN_WARNING
> + "Possible attack attempt. Unexpected rseq signature 0x%x, expecting 0x%x (pid=%d, addr=%p).\n",
> + sig, current->rseq_sig, current->pid, usig);
> + return -EPERM;
> + }
Is there any text that explains the thread model and possible attack
that this signature prevents? I failed to find any, which raises the
question, why is it there..
> + return 0;
> +}
> +
> +static int rseq_need_restart(struct task_struct *t, uint32_t cs_flags)
u32
> +{
> + uint32_t flags, event_mask;
u32
> + int ret;
> +
> + /* Get thread flags. */
> + ret = __get_user(flags, &t->rseq->flags);
> + if (ret)
> + return ret;
> +
> + /* Take critical section flags into account. */
> + flags |= cs_flags;
> +
> + /*
> + * Restart on signal can only be inhibited when restart on
> + * preempt and restart on migrate are inhibited too. Otherwise,
> + * a preempted signal handler could fail to restart the prior
> + * execution context on sigreturn.
> + */
> + if (unlikely(flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL)) {
> + if ((flags & (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
> + | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)) !=
> + (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
> + | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT))
> + return -EINVAL;
Please put operators at the end of the previous line, not at the start
of the new line when you have to break statements.
Also, that's unreadable.
#define RSEQ_CS_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)
if (unlikely((flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) &&
(flags & RSEQ_CS_FLAGS) != RSEQ_CS_FLAGS))
return -EINVAL;
> + }
> +
> + /*
> + * Load and clear event mask atomically with respect to
> + * scheduler preemption.
> + */
> + preempt_disable();
> + event_mask = t->rseq_event_mask;
> + t->rseq_event_mask = 0;
> + preempt_enable();
> +
> + event_mask &= ~flags;
> + if (event_mask)
> + return 1;
> + return 0;
return !!(event_mask & ~flags);
> +}
> +
> +static int clear_rseq_cs(struct task_struct *t)
> +{
> + unsigned long ptr = 0;
> +
> + /*
> + * The rseq_cs field is set to NULL on preemption or signal
> + * delivery on top of rseq assembly block, as well as on top
> + * of code outside of the rseq assembly block. This performs
> + * a lazy clear of the rseq_cs field.
> + *
> + * Set rseq_cs to NULL with single-copy atomicity.
> + */
> + return __put_user(ptr, &t->rseq->rseq_cs);
__put_user(0UL, &t->rseq->rseq_cs); ?
> +}
> +
> +static int rseq_ip_fixup(struct pt_regs *regs)
> +{
> + unsigned long ip = instruction_pointer(regs), start_ip = 0,
> + post_commit_offset = 0, abort_ip = 0;
valid C, but yuck. Just have two 'unsigned long' lines.
Also, why the =0, the below call to rseq_get_rseq_cs() will either
initialize of fail.
> + struct task_struct *t = current;
> + uint32_t cs_flags = 0;
u32
> + bool in_rseq_cs = false;
> + int ret;
> +
> + ret = rseq_get_rseq_cs(t, &start_ip, &post_commit_offset, &abort_ip,
> + &cs_flags);
ret = rseq_get_rseq_cs(t, &start_ip, &post_commit_offset,
&abort_ip, &cs_flags);
> + if (ret)
> + return ret;
> +
> + /*
> + * Handle potentially not being within a critical section.
> + * Unsigned comparison will be true when
> + * ip >= start_ip, and when ip < start_ip + post_commit_offset.
> + */
> + if (ip - start_ip < post_commit_offset)
> + in_rseq_cs = true;
> +
> + /*
> + * If not nested over a rseq critical section, restart is
> + * useless. Clear the rseq_cs pointer and return.
> + */
> + if (!in_rseq_cs)
> + return clear_rseq_cs(t);
That all seems needlessly complicated; isn't:
if (ip - start_ip >= post_commit_offset)
return clear_rseq_cs();
equivalent? Nothing seems to use that variable after this.
> + ret = rseq_need_restart(t, cs_flags);
> + if (ret <= 0)
> + return ret;
> + ret = clear_rseq_cs(t);
> + if (ret)
> + return ret;
> + trace_rseq_ip_fixup(ip, start_ip, post_commit_offset, abort_ip);
> + instruction_pointer_set(regs, (unsigned long)abort_ip);
> + return 0;
> +}
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-28 11:22 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon
In-Reply-To: <20180327160542.28457-3-mathieu.desnoyers@efficios.com>
On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
> +/*
> + * struct rseq_cs is aligned on 4 * 8 bytes to ensure it is always
> + * contained within a single cache-line. It is usually declared as
> + * link-time constant data.
> + */
> +struct rseq_cs {
> + /* Version of this structure. */
> + uint32_t version;
> + /* enum rseq_cs_flags */
> + uint32_t flags;
> + LINUX_FIELD_u32_u64(start_ip);
> + /* Offset from start_ip. */
> + LINUX_FIELD_u32_u64(post_commit_offset);
> + LINUX_FIELD_u32_u64(abort_ip);
> +} __attribute__((aligned(4 * sizeof(uint64_t))));
What's with the uint32_t ? The normal Linux API type is __u32 afaik.
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-28 11:19 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E . McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon
In-Reply-To: <20180327160542.28457-3-mathieu.desnoyers@efficios.com>
On Tue, Mar 27, 2018 at 12:05:23PM -0400, Mathieu Desnoyers wrote:
> +#ifdef CONFIG_RSEQ
> + struct rseq __user *rseq;
> + u32 rseq_len;
> + u32 rseq_sig;
> + /*
> + * RmW on rseq_event_mask must be performed atomically
> + * with respect to preemption.
> + */
> + unsigned long rseq_event_mask;
> +#endif
> +static inline void rseq_signal_deliver(struct pt_regs *regs)
> +{
> + set_bit(RSEQ_EVENT_SIGNAL_BIT, ¤t->rseq_event_mask);
> + rseq_handle_notify_resume(regs);
> +}
> +
> +static inline void rseq_preempt(struct task_struct *t)
> +{
> + set_bit(RSEQ_EVENT_PREEMPT_BIT, &t->rseq_event_mask);
> + rseq_set_notify_resume(t);
> +}
> +
> +static inline void rseq_migrate(struct task_struct *t)
> +{
> + set_bit(RSEQ_EVENT_MIGRATE_BIT, &t->rseq_event_mask);
> + rseq_set_notify_resume(t);
> +}
Given that comment above, do you really need the full atomic set bit?
Isn't __set_bit() sufficient?
^ permalink raw reply
* [PATCH 30/30] random: convert to ->poll_mask
From: Christoph Hellwig @ 2018-03-28 7:29 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180328072926.17131-1-hch@lst.de>
The big change is that random_read_wait and random_write_wait are merged
into a single waitqueue that uses keyed wakeups. Because wait_event_*
doesn't know about that this will lead to occassional spurious wakeups
in _random_read and add_hwgenerator_randomness, but wait_event_* is
designed to handle these and were are not in a a hot path there.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/char/random.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index e5b3d3ba4660..840d80b64431 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -401,8 +401,7 @@ static struct poolinfo {
/*
* Static global variables
*/
-static DECLARE_WAIT_QUEUE_HEAD(random_read_wait);
-static DECLARE_WAIT_QUEUE_HEAD(random_write_wait);
+static DECLARE_WAIT_QUEUE_HEAD(random_wait);
static struct fasync_struct *fasync;
static DEFINE_SPINLOCK(random_ready_list_lock);
@@ -710,7 +709,7 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits)
/* should we wake readers? */
if (entropy_bits >= random_read_wakeup_bits) {
- wake_up_interruptible(&random_read_wait);
+ wake_up_interruptible_poll(&random_wait, POLLIN);
kill_fasync(&fasync, SIGIO, POLL_IN);
}
/* If the input pool is getting full, send some
@@ -1293,7 +1292,7 @@ static size_t account(struct entropy_store *r, size_t nbytes, int min,
trace_debit_entropy(r->name, 8 * ibytes);
if (ibytes &&
(r->entropy_count >> ENTROPY_SHIFT) < random_write_wakeup_bits) {
- wake_up_interruptible(&random_write_wait);
+ wake_up_interruptible_poll(&random_wait, POLLOUT);
kill_fasync(&fasync, SIGIO, POLL_OUT);
}
@@ -1748,7 +1747,7 @@ _random_read(int nonblock, char __user *buf, size_t nbytes)
if (nonblock)
return -EAGAIN;
- wait_event_interruptible(random_read_wait,
+ wait_event_interruptible(random_wait,
ENTROPY_BITS(&input_pool) >=
random_read_wakeup_bits);
if (signal_pending(current))
@@ -1784,14 +1783,17 @@ urandom_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
return ret;
}
+static struct wait_queue_head *
+random_get_poll_head(struct file *file, __poll_t events)
+{
+ return &random_wait;
+}
+
static __poll_t
-random_poll(struct file *file, poll_table * wait)
+random_poll_mask(struct file *file, __poll_t events)
{
- __poll_t mask;
+ __poll_t mask = 0;
- poll_wait(file, &random_read_wait, wait);
- poll_wait(file, &random_write_wait, wait);
- mask = 0;
if (ENTROPY_BITS(&input_pool) >= random_read_wakeup_bits)
mask |= EPOLLIN | EPOLLRDNORM;
if (ENTROPY_BITS(&input_pool) < random_write_wakeup_bits)
@@ -1890,7 +1892,8 @@ static int random_fasync(int fd, struct file *filp, int on)
const struct file_operations random_fops = {
.read = random_read,
.write = random_write,
- .poll = random_poll,
+ .get_poll_head = random_get_poll_head,
+ .poll_mask = random_poll_mask,
.unlocked_ioctl = random_ioctl,
.fasync = random_fasync,
.llseek = noop_llseek,
@@ -2223,7 +2226,7 @@ void add_hwgenerator_randomness(const char *buffer, size_t count,
* We'll be woken up again once below random_write_wakeup_thresh,
* or when the calling thread is about to terminate.
*/
- wait_event_interruptible(random_write_wait, kthread_should_stop() ||
+ wait_event_interruptible(random_wait, kthread_should_stop() ||
ENTROPY_BITS(&input_pool) <= random_write_wakeup_bits);
mix_pool_bytes(poolp, buffer, count);
credit_entropy_bits(poolp, entropy);
--
2.14.2
--
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 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