From: Thomas Gleixner <hidden> Date: 2020-02-14 16:45:34
Hi!
This is a follow up to the initial patch series which David posted a while
ago:
https://lore.kernel.org/bpf/20191207.160357.828344895192682546.davem@davemloft.net/
which was (while non-functional on RT) a good starting point for further
investigations.
PREEMPT_RT aims to make the kernel fully preemptible except for a few low
level operations which have to be unpreemptible, like scheduler, low level
entry/exit code, low level interrupt handling, locking internals etc.
This is achieved by the following main transformations (there are some
more minor ones, but they don't matter in this context):
1) All interrupts, which are not explicitely marked IRQF_NO_THREAD, are
forced into threaded interrupt handlers. This applies to almost all
device interrupt handlers.
2) hrtimers which are not explicitely marked to expire in hard interrupt
context are expired in soft interrupt context.
3) Soft interrupts are also forced into thread context. They run either
in the context of a threaded interrupt handler (similar to the !RT
mode of running on return from interrupt), in ksoftirqd or in a
thread context which raised a soft interrupt (same as in !RT).
Soft interrupts (BH) are serialized per CPU so the usual bottom half
protections still work.
4) spinlocks and rwlocks are substituted with 'sleeping spin/rwlocks'.
The internal representation is a priority inheritance aware rtmutex
which has glue code attached to preserve the spin/rwlock semantics
vs. task state. They neither disable preemption nor interrupts, which
is fine as the interrupt handler they want to be protected against is
forced into thread context.
The true hard interrupt handlers are not affecting these sections as
they are completely independent.
The code pathes which need to be atomic are using raw_spinlocks which
disable preemption (and/or interrupts).
On a non RT kernel spinlocks are mapped to raw_spinlocks so everything
works as usual.
As a consequence allocation of memory is not possible from truly atomic
contexts on RT, even with GFP_ATOMIC set. This is required because the slab
allocator can run into a situation even with GPF_ATOMIC where it needs to
call into the page allocator, which in turn takes regular spinlocks. As the
RT substitution of regular spinlocks might sleep, it's obvious that memory
allocations from truly atomic contexts on RT are not permitted. The page
allocator locks cannot be converted to raw locks as the length of the
resulting preempt/interrupt disabled sections are way above the tolerance
level of demanding realtime applications.
So this leads to a conflict with the current BPF implementation. BPF
disables preemption around:
1) The invocation of BPF programs. This is required to guarantee
that the program runs to completion on one CPU
2) The invocation of map operations from sys_bpf(). This is required
to protect the operation against BPF programs which access the
same map from perf, kprobes or tracing context because the syscall
operation has to lock the hashtab bucket lock.
If the perf NMI, kprobe or tracepoint happens inside the bucket lock
held region and the BPF program needs to access the same bucket then
the system would deadlock.
The mechanism used for this is to increment the per CPU recursion
protection which prevents such programs to be executed. The same per
CPU variable is used to prevent recursion of the programs themself,
which might happen when e.g. a kprobe attached BPF program triggers
another kprobe attached BPF program.
In principle this works on RT as well as long as the BPF programs use
preallocated maps, which is required for trace type programs to prevent
deadlocks of all sorts. Other BPF programs which run in softirq context,
e.g. packet filtering, or in thread context, e.g. syscall auditing have no
requirement for preallocated maps. They can allocate memory when required.
But as explained above on RT memory allocation from truly atomic context is
not permitted, which required a few teaks to the BPF code.
The initial misunderstanding on my side was that the preempt disable around
the invocations of BPF programs is not only required to prevent migration
to a different CPU (in case the program is invoked from preemptible
context) but is also required to prevent reentrancy from a preempting
task. In hindsight I should have figured out myself that this is not the
case because the same program can run concurrently on a different CPU or
from a different context (e.g. interrupt). Alexei thankfully enlightened me
recently over a beer that the real intent here is to guarantee that the
program runs to completion on the same CPU where it started. The same is
true for the syscall side as this just has to guarantee the per CPU
recursion protection.
This part of the problem is trivial. RT provides a true migrate_disable()
mechanism which does not disable preemption. So the preempt_disable /
enable() pairs can be replaced with migrate_disable / enable() pairs.
migrate_disable / enable() maps to preempt_disable / enable() on a
not-RT kernel, so there is no functional change when RT is disabled.
But there is another issue which is not as straight forward to solve:
The map operations which deal with elements in hashtab buckets (or other
map mechanisms) have spinlocks to protect them against concurrent access
from other CPUs. These spinlocks are raw_spinlocks, so they disable
preemption and interrupts (_irqsave()). Not a problem per se, but some of
these code pathes invoke memory allocations with a bucket lock held. This
obviously conflicts with the RT semantics.
The easy way out would be to convert these locks to regular spinlocks which
can be substituted by RT. Works like a charm for both RT and !RT for BPF
programs which run in thread context (includes soft interrupt and device
interrupt handler context on RT).
But there are also the BPF programs which run in truly atomic context even
on a RT kernel, i.e. tracing types (perf, perf NMI, kprobes and trace).
For obvious reasons these context cannot take regular spinlocks on RT
because RT substitutes them with sleeping spinlocks. might_sleep() splats
from NMI context are not really desired and on lock contention the
scheduler explodes in colourful ways. Same problem for kprobes and
tracepoints. So these program types need a raw spinlock which brings us
back to square one.
But, there is an important detail to the rescue. The trace type programs
require preallocated maps to prevent deadlocks of all sorts in the memory
allocator. Preallocated maps never call into the memory allocator or other
code pathes which might sleep on a RT kernel. The allocation type is known
when the program and the map is initialized.
This allows to differentiate between lock types for preallocated and
run-time allocated maps. While not pretty, the proposed solution is to have
a lock union in the bucket:
union {
raw_spinlock_t raw_lock;
spinlock_t lock;
};
and have init/lock/unlock helpers which handle the lock type depending on
the allocation mechanism, i.e. for preallocated maps it uses the raw lock
and for dynamic allocations it uses the regular spinlock. The locks in the
percpu_freelist need to stay raw as well as they nest into the bucket lock
held section, which works for both the raw and the regular spinlock
variant.
I'm not proud of that, but I really couldn't come up with anything better
aside of completely splitting the code pathes which would be even worse due
to the resulting code duplication.
The locks in the LPM trie map needs to become a regular spinlock as well as
trie map is based on dynamic allocation, but again not a problem as this
map cannot be used for the critical types anyway.
I kept the LRU and the stack map locks raw for now as they do not use
dynamic allocations, but I haven't done any testing on latency impact
yet. The stack map critical sections are truly short, so they should not
matter at all, but the LRU ones could. That can be revisited once we have
numbers. I assume that LRU is not the right choice for the trace type
programs anyway, but who knows.
The last and trivial to solve (Dave solved that already) issue is the non
owner release of mmap sem, which is forbidden on RT as well. So this just
forces the IP fallback path.
On a non RT kernel this does not change anything. The compiler optimizes
the lock magic out and everything maps to the state before these changes.
This survives the selftests and a bunch of different invocations of
bpftrace on mainline and on a backport to 5.4-rt.
But of course I surely have missed some details here and there, so please
have a close look at this.
The diffstat of hashtab.c is so large because I sat down and documented the
above write up in condensed form in a comment on top of the file as I
wanted to spare others the dubious experience of having to reverse engineer
the inner workings of BPF.
Thanks,
tglx
8<----------------
include/linux/bpf.h | 8 +-
include/linux/filter.h | 33 ++++++--
include/linux/kernel.h | 7 +
include/linux/preempt.h | 30 +++++++
kernel/bpf/hashtab.c | 164 ++++++++++++++++++++++++++++++++-----------
kernel/bpf/lpm_trie.c | 12 +--
kernel/bpf/percpu_freelist.c | 20 ++---
kernel/bpf/stackmap.c | 18 +++-
kernel/bpf/syscall.c | 16 ++--
kernel/bpf/trampoline.c | 9 +-
kernel/events/core.c | 2
kernel/seccomp.c | 4 -
kernel/trace/bpf_trace.c | 6 -
lib/test_bpf.c | 4 -
net/bpf/test_run.c | 8 +-
net/core/flow_dissector.c | 4 -
net/core/skmsg.c | 8 --
net/kcm/kcmsock.c | 4 -
18 files changed, 248 insertions(+), 109 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:22:45
__bpf_trace_run() disables preemption around the BPF_PROG_RUN() invocation.
This is redundant because __bpf_trace_run() is invoked from a trace point
via __DO_TRACE() which already disables preemption _before_ invoking any of
the functions which are attached to a trace point.
Remove it.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/trace/bpf_trace.c | 2 --
1 file changed, 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:22:54
From: David Miller <davem@davemloft.net>
In a RT kernel down_read_trylock() cannot be used from NMI context and
up_read_non_owner() is another problematic issue.
So in such a configuration, simply elide the annotated stackmap and
just report the raw IPs.
In the longer term, it might be possible to provide a atomic friendly
versions of the page cache traversal which will at least provide the info
if the pages are resident and don't need to be paged in.
[ tglx: Use IS_ENABLED() to avoid the #ifdeffery, fixup the irq work
callback and add a comment ]
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/stackmap.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:40:38
The LPM trie map cannot be used in contexts like perf, kprobes and tracing
as this map type dynamically allocates memory.
The memory allocation happens with a raw spinlock held which is a truly
spinning lock on a PREEMPT RT enabled kernel which disables preemption and
interrupts.
As RT does not allow memory allocation from such a section for various
reasons, convert the raw spinlock to a regular spinlock.
On a RT enabled kernel these locks are substituted by 'sleeping' spinlocks
which provide the proper protection but keep the code preemptible.
On a non-RT kernel regular spinlocks map to raw spinlocks, i.e. this does
not cause any functional change.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/lpm_trie.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -34,7 +34,7 @@ struct lpm_trie {size_tn_entries;size_tmax_prefixlen;size_tdata_size;-raw_spinlock_tlock;+spinlock_tlock;};/* This trie implements a longest prefix match algorithm that can be used to
@@ -315,7 +315,7 @@ static int trie_update_elem(struct bpf_mif(key->prefixlen>trie->max_prefixlen)return-EINVAL;-raw_spin_lock_irqsave(&trie->lock,irq_flags);+spin_lock_irqsave(&trie->lock,irq_flags);/* Allocate and fill a new node */
@@ -422,7 +422,7 @@ static int trie_update_elem(struct bpf_mkfree(im_node);}-raw_spin_unlock_irqrestore(&trie->lock,irq_flags);+spin_unlock_irqrestore(&trie->lock,irq_flags);returnret;}
@@ -442,7 +442,7 @@ static int trie_delete_elem(struct bpf_mif(key->prefixlen>trie->max_prefixlen)return-EINVAL;-raw_spin_lock_irqsave(&trie->lock,irq_flags);+spin_lock_irqsave(&trie->lock,irq_flags);/* Walk the tree looking for an exact key/length match and keeping*trackofthepathwetraverse.Wewillneedtoknowthenode
@@ -518,7 +518,7 @@ static int trie_delete_elem(struct bpf_mkfree_rcu(node,rcu);out:-raw_spin_unlock_irqrestore(&trie->lock,irq_flags);+spin_unlock_irqrestore(&trie->lock,irq_flags);returnret;}
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:40:54
From: David Miller <davem@davemloft.net>
Replace the preemption disable/enable with migrate_disable/enable() to
reflect the actual requirement and to allow PREEMPT_RT to substitute it
with an actual migration disable mechanism which does not disable
preemption.
Including the code paths that go via __bpf_prog_run_save_cb().
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/bpf.h | 8 ++++----
include/linux/filter.h | 5 +++--
2 files changed, 7 insertions(+), 6 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:41:02
From: David Miller <davem@davemloft.net>
Replace the preemption disable/enable with migrate_disable/enable() to
reflect the actual requirement and to allow PREEMPT_RT to substitute it
with an actual migration disable mechanism which does not disable
preemption.
[ tglx: Switched it over to migrate disable ]
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
lib/test_bpf.c | 4 ++--
net/bpf/test_run.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
--- a/lib/test_bpf.c+++ b/lib/test_bpf.c
@@ -6660,14 +6660,14 @@ static int __run_one(const struct bpf_pru64start,finish;intret=0,i;-preempt_disable();+migrate_disable();start=ktime_get_ns();for(i=0;i<runs;i++)ret=BPF_PROG_RUN(fp,data);finish=ktime_get_ns();-preempt_enable();+migrate_enable();*duration=finish-start;do_div(*duration,runs);---a/net/bpf/test_run.c+++b/net/bpf/test_run.c
@@ -37,7 +37,7 @@ static int bpf_test_run(struct bpf_progrepeat=1;rcu_read_lock();-preempt_disable();+migrate_disable();time_start=ktime_get_ns();for(i=0;i<repeat;i++){bpf_cgroup_storage_set(storage);
@@ -54,18 +54,18 @@ static int bpf_test_run(struct bpf_progif(need_resched()){time_spent+=ktime_get_ns()-time_start;-preempt_enable();+migrate_enable();rcu_read_unlock();cond_resched();rcu_read_lock();-preempt_disable();+migrate_disable();time_start=ktime_get_ns();}}time_spent+=ktime_get_ns()-time_start;-preempt_enable();+migrate_enable();rcu_read_unlock();do_div(time_spent,repeat);
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:41:11
BPF does not require preemption disable. It only requires to stay on the
same CPU while running a program. Reflect this by replacing
preempt_disable/enable() with migrate_disable/enable() pairs.
On a non-RT kernel this maps to preempt_disable/enable().
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/trace/bpf_trace.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:44:28
Some code pathes rely on preempt_disable() to prevent migration on a non RT
enabled kernel. These preempt_disable/enable() pairs are substituted by
migrate_disable/enable() pairs or other forms of RT specific protection. On
RT these protections prevent migration but not preemption. Obviously a
cant_sleep() check in such a section will trigger on RT because preemption
is not disabled.
Provide a cant_migrate() macro which maps to cant_sleep() on a non RT
kernel and an empty placeholder for RT for now. The placeholder will be
changed to a proper debug check along with the RT specific migration
protection mechanism.
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/kernel.h | 7 +++++++
1 file changed, 7 insertions(+)
@@ -257,6 +257,13 @@ extern void __cant_sleep(const char *fil#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)+#ifndef CONFIG_PREEMPT_RT+# define cant_migrate() cant_sleep()+#else+/* Placeholder for now */+# define cant_migrate() do { } while (0)+#endif+/***abs-returnabsolutevalueofanargument*@x:thevalue.Ifitisunsignedtype,itisconvertedtosignedtypefirst.
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:44:35
From: David Miller <davem@davemloft.net>
All of these cases are strictly of the form:
preempt_disable();
BPF_PROG_RUN(...);
preempt_enable();
Replace this with BPF_PROG_RUN_PIN_ON_CPU() which wraps BPF_PROG_RUN()
with:
migrate_disable();
BPF_PROG_RUN(...);
migrate_enable();
On non RT enabled kernels this maps to preempt_disable/enable() and on RT
enabled kernels this solely prevents migration, which is sufficient as
there is no requirement to prevent reentrancy to any BPF program from a
preempting task. The only requirement is that the program stays on the same
CPU.
Therefore, this is a trivially correct transformation.
[ tglx: Converted to BPF_PROG_RUN_PIN_ON_CPU() ]
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/filter.h | 4 +---
kernel/seccomp.c | 4 +---
net/core/flow_dissector.c | 4 +---
net/core/skmsg.c | 8 ++------
net/kcm/kcmsock.c | 4 +---
5 files changed, 6 insertions(+), 18 deletions(-)
@@ -628,7 +628,6 @@ int sk_psock_msg_verdict(struct sock *skstructbpf_prog*prog;intret;-preempt_disable();rcu_read_lock();prog=READ_ONCE(psock->progs.msg_parser);if(unlikely(!prog)){
@@ -638,7 +637,7 @@ int sk_psock_msg_verdict(struct sock *sksk_msg_compute_data_pointers(msg);msg->sk=sk;-ret=BPF_PROG_RUN(prog,msg);+ret=BPF_PROG_RUN_PIN_ON_CPU(prog,msg);ret=sk_psock_map_verd(ret,msg->sk_redir);psock->apply_bytes=msg->apply_bytes;if(ret==__SK_REDIRECT){
@@ -653,7 +652,6 @@ int sk_psock_msg_verdict(struct sock *sk}out:rcu_read_unlock();-preempt_enable();returnret;}EXPORT_SYMBOL_GPL(sk_psock_msg_verdict);
@@ -665,9 +663,7 @@ static int sk_psock_bpf_run(struct sk_psskb->sk=psock->sk;bpf_compute_data_end_sk_skb(skb);-preempt_disable();-ret=BPF_PROG_RUN(prog,skb);-preempt_enable();+ret=BPF_PROG_RUN_PIN_ON_CPU(prog,skb);/* strparser clones the skb before handing it to a upper layer,*meaningskb_orphanhasbeencalled.WeNULLskonthewayout*toensurewedon'ttriggeraBUG_ON()inskb/skoperations---a/net/kcm/kcmsock.c+++b/net/kcm/kcmsock.c
@@ -380,9 +380,7 @@ static int kcm_parse_func_strparser(strustructbpf_prog*prog=psock->bpf_prog;intres;-preempt_disable();-res=BPF_PROG_RUN(prog,skb);-preempt_enable();+res=BPF_PROG_RUN_PIN_ON_CPU(prog,skb);returnres;}
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:44:44
The required protection is that the caller cannot be migrated to a
different CPU as these places take either a hash bucket lock or might
trigger a kprobe inside the memory allocator. Both scenarios can lead to
deadlocks. The deadlock prevention is per CPU by incrementing a per CPU
variable which temporarily blocks the invocation of BPF programs from perf
and kprobes.
Replace the preempt_disable/enable() pairs with migrate_disable/enable()
pairs to prepare BPF to work on PREEMPT_RT enabled kernels. On a non-RT
kernel this maps to preempt_disable/enable(), i.e. no functional change.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/hashtab.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:44:51
Currently code which solely needs to prevent migration of a task uses
preempt_disable()/enable() pairs. This is the only reliable way to do so as
setting the task affinity to a single CPU can be undone by a setaffinity
operation from a different task/process. It's also significantly faster.
RT provides a seperate migrate_disable/enable() mechanism which does not
disable preemption to achieve the semantic requirements of a (almost) fully
preemptible kernel.
As it is unclear from looking at a given code path whether the intention is
to disable preemption or migration, introduce migrate_disable/enable()
inline functions which can be used to annotate code which merely needs to
disable migration. Map them to preempt_disable/enable() for now. The RT
substitution will be provided later.
Code which is annotated that way documents that it has no requirement to
protect against reentrancy of a preempting task. Either this is not
required at all or the call sites are already serialized by other means.
Signed-off-by: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
---
include/linux/preempt.h | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:45:01
The comment where the bucket lock is acquired says:
/* bpf_map_update_elem() can be called in_irq() */
which is not really helpful and aside of that it does not explain the
subtle details of the hash bucket locks expecially in the context of BPF
and perf, kprobes and tracing.
Add a comment at the top of the file which explains the protection scopes
and the details how potential deadlocks are prevented.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/hashtab.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
@@ -872,7 +892,6 @@ static int htab_map_update_elem(struct b*/}-/* bpf_map_update_elem() can be called in_irq() */raw_spin_lock_irqsave(&b->lock,flags);l_old=lookup_elem_raw(head,hash,key,key_size);
@@ -952,7 +971,6 @@ static int htab_lru_map_update_elem(strureturn-ENOMEM;memcpy(l_new->key+round_up(map->key_size,8),value,map->value_size);-/* bpf_map_update_elem() can be called in_irq() */raw_spin_lock_irqsave(&b->lock,flags);l_old=lookup_elem_raw(head,hash,key,key_size);
@@ -1007,7 +1025,6 @@ static int __htab_percpu_map_update_elemb=__select_bucket(htab,hash);head=&b->head;-/* bpf_map_update_elem() can be called in_irq() */raw_spin_lock_irqsave(&b->lock,flags);l_old=lookup_elem_raw(head,hash,key,key_size);
@@ -1071,7 +1088,6 @@ static int __htab_lru_percpu_map_update_return-ENOMEM;}-/* bpf_map_update_elem() can be called in_irq() */raw_spin_lock_irqsave(&b->lock,flags);l_old=lookup_elem_raw(head,hash,key,key_size);
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:45:05
PREEMPT_RT forbids certain operations like memory allocations (even with
GFP_ATOMIC) from atomic contexts. This is required because even with
GFP_ATOMIC the memory allocator calls into code pathes which acquire locks
with long held lock sections. To ensure the deterministic behaviour these
locks are regular spinlocks, which are converted to 'sleepable' spinlocks
on RT. The only true atomic contexts on an RT kernel are the low level
hardware handling, scheduling, low level interrupt handling, NMIs etc. None
of these contexts should ever do memory allocations.
As regular device interrupt handlers and soft interrupts are forced into
thread context, the existing code which does
spin_lock*(); alloc(GPF_ATOMIC); spin_unlock*();
just works.
In theory the BPF locks could be converted to regular spinlocks as well,
but the bucket locks and percpu_freelist locks can be taken from arbitrary
contexts (perf, kprobes, tracepoints) which are required to be atomic
contexts even on RT. These mechanisms require preallocated maps, so there
is no need to invoke memory allocations within the lock held sections.
BPF maps which need dynamic allocation are only used from (forced) thread
context on RT and can therefore use regular spinlocks which in turn allows
to invoke memory allocations from the lock held section.
To achieve this make the hash bucket lock a union of a raw and a regular
spinlock and initialize and lock/unlock either the raw spinlock for
preallocated maps or the regular variant for maps which require memory
allocations.
On a non RT kernel this distinction is neither possible nor required.
spinlock maps to raw_spinlock and the extra code and conditional is
optimized out by the compiler. No functional change.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/hashtab.c | 65 +++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 9 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:45:24
As a preparation for making the BPF locking RT friendly, factor out the
hash bucket lock operations into inline functions. This allows to do the
necessary RT modification in one place instead of sprinkling it all over
the place. No functional change.
The now unused htab argument of the lock/unlock functions will be used in
the next step which adds PREEMPT_RT support.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/hashtab.c | 69 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 46 insertions(+), 23 deletions(-)
@@ -1395,7 +1418,7 @@ static intdst_val+=value_size;}-raw_spin_unlock_irqrestore(&b->lock,flags);+htab_unlock_bucket(htab,b,flags);/* If we are not copying data, we can go to next bucket and avoid*unlockingthercu.*/
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:45:31
The required protection is that the caller cannot be migrated to a
different CPU as these functions end up in places which take either a hash
bucket lock or might trigger a kprobe inside the memory allocator. Both
scenarios can lead to deadlocks. The deadlock prevention is per CPU by
incrementing a per CPU variable which temporarily blocks the invocation of
BPF programs from perf and kprobes.
Replace the preempt_disable/enable() pairs with migrate_disable/enable()
pairs to prepare BPF to work on PREEMPT_RT enabled kernels. On a non-RT
kernel this maps to preempt_disable/enable(), i.e. no functional change.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/syscall.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:45:51
As already discussed in the previous change which introduced
BPF_RUN_PROG_PIN_ON_CPU() BPF only requires to disable migration to
guarantee per CPUness.
If RT substitutes the preempt disable based migration protection then the
cant_sleep() check will obviously trigger as preemption is not disabled.
Replace it by cant_migrate() which maps to cant_sleep() on a non RT kernel
and will verify that migration is disabled on a full RT kernel.
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/filter.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:45:53
pcpu_freelist_populate() is disabling interrupts and then iterates over the
possible CPUs. The reason why this disables interrupts is to silence
lockdep because the invoked ___pcpu_freelist_push() takes spin locks.
Neither the interrupt disabling nor the locking are required in this
function because it's called during initialization and the resulting map is
not yet visible to anything.
Split out the actual push assignement into an inline, call it from the loop
and remove the interrupt disable.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/percpu_freelist.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
@@ -56,21 +62,16 @@ void pcpu_freelist_populate(struct pcpu_u32nr_elems){structpcpu_freelist_head*head;-unsignedlongflags;inti,cpu,pcpu_entries;pcpu_entries=nr_elems/num_possible_cpus()+1;i=0;-/* disable irq to workaround lockdep false positive-*inbpfusagepcpu_freelist_populate()willneverrace-*withpcpu_freelist_push()-*/-local_irq_save(flags);for_each_possible_cpu(cpu){again:head=per_cpu_ptr(s->freelist,cpu);-___pcpu_freelist_push(head,buf);+/* No locking required as this is not visible yet. */+pcpu_freelist_push_node(head,buf);i++;buf+=elem_size;if(i==nr_elems)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:46:14
BPF programs require to run on one CPU to completion as they use per CPU
storage, but according to Alexei they don't need reentrancy protection as
obviously BPF programs running in thread context can always be 'preempted'
by hard and soft interrupts and instrumentation and the same program can
run concurrently on a different CPU.
The currently used mechanism to ensure CPUness is to wrap the invocation
into a preempt_disable/enable() pair. Disabling preemption is also
disabling migration for a task.
preempt_disable/enable() is used because there is no explicit way to
reliably disable only migration.
Provide a separate macro to invoke a BPF program which can be used in
migrateable task context.
It wraps BPF_PROG_RUN() in a migrate_disable/enable() pair which maps on
non RT enabled kernels to preempt_disable/enable(). On RT enabled kernels
this merely disables migration. Both methods ensure that the invoked BPF
program runs on one CPU to completion.
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/filter.h | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:46:17
The BPF invocation from the perf event overflow handler does not require to
disable preemption because this is called from NMI or at least hard
interrupt context which is already non-preemptible.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/events/core.c | 2 --
1 file changed, 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-14 16:46:29
From: David Miller <davem@davemloft.net>
Instead of preemption disable/enable to reflect the purpose. This allows
PREEMPT_RT to substitute it with an actual migration disable
implementation. On non RT kernels this is still mapped to
preempt_disable/enable().
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/trampoline.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
@@ -367,8 +367,9 @@ void bpf_trampoline_put(struct bpf_trampmutex_unlock(&trampoline_mutex);}-/* The logic is similar to BPF_PROG_RUN, but with explicit rcu and preempt that-*areneededfortrampoline.Themacroissplitinto+/* The logic is similar to BPF_PROG_RUN, but with an explicit+*rcu_read_lock()andmigrate_disable()whicharerequired+*forthetrampoline.Themacroissplitinto*call_bpf_prog_enter*callprog->bpf_func*call__bpf_prog_exit
This looks really good after a cursory review, thanks for doing this week.
I was personally unaware of the pre-allocation rules for MAPs used by
tracing et al. And that definitely shapes how this should be handled.
This looks really good after a cursory review, thanks for doing this week.
I was personally unaware of the pre-allocation rules for MAPs used by
tracing et al. And that definitely shapes how this should be handled.
Hmm. I just noticed that my analysis only holds for PERF events. But
that's broken on mainline already.
Assume the following simplified callchain:
kmalloc() from regular non BPF context
cache empty
freelist empty
lock(zone->lock);
tracepoint or kprobe
BPF()
update_elem()
lock(bucket)
kmalloc()
cache empty
freelist empty
lock(zone->lock); <- DEADLOCK
So really, preallocation _must_ be enforced for all variants of
intrusive instrumentation. There is no if and but, it's simply mandatory
as all intrusive instrumentation has to follow the only sensible
principle: KISS = Keep It Safe and Simple.
The above is a perfectly valid scenario and works with perf and tracing,
so it has to work with BPF in the same safe way.
I might be missing some magic enforcement of that, but I got lost in the
maze.
Thanks,
tglx
Does it really have to be a statement expression with a local variable ?
If so, we should consider renaming "ret" to "__ret" to minimize the
chances of a caller issuing BPF_PROG_RUN_PIN_ON_CPU with "ret" as
prog or ctx argument, which would lead to unexpected results.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On 14-Feb-2020 02:39:31 PM, Thomas Gleixner wrote:
The required protection is that the caller cannot be migrated to a
different CPU as these places take either a hash bucket lock or might
trigger a kprobe inside the memory allocator. Both scenarios can lead to
deadlocks. The deadlock prevention is per CPU by incrementing a per CPU
variable which temporarily blocks the invocation of BPF programs from perf
and kprobes.
Replace the preempt_disable/enable() pairs with migrate_disable/enable()
pairs to prepare BPF to work on PREEMPT_RT enabled kernels. On a non-RT
kernel this maps to preempt_disable/enable(), i.e. no functional change.
Will that _really_ work on RT ?
I'm puzzled about what will happen in the following scenario on RT:
Thread A is preempted within e.g. htab_elem_free_rcu, and Thread B is
scheduled and runs through a bunch of tracepoints. Both are on the
same CPU's runqueue:
CPU 1
Thread A is scheduled
(Thread A) htab_elem_free_rcu()
(Thread A) migrate disable
(Thread A) __this_cpu_inc(bpf_prog_active); -> per-cpu variable for
deadlock prevention.
Thread A is preempted
Thread B is scheduled
(Thread B) Runs through various tracepoints:
trace_call_bpf()
if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
-> will skip any instrumentation that happens to be on
this CPU until...
Thread B is preempted
Thread A is scheduled
(Thread A) __this_cpu_dec(bpf_prog_active);
(Thread A) migrate enable
Having all those events randomly and silently discarded might be quite
unexpected from a user standpoint. This turns the deadlock prevention
mechanism into a random tracepoint-dropping facility, which is
unsettling. One alternative approach we could consider to solve this
is to make this deadlock prevention nesting counter per-thread rather
than per-cpu.
Also, I don't think using __this_cpu_inc() without preempt-disable or
irq off is safe. You'll probably want to move to this_cpu_inc/dec
instead, which can be heavier on some architectures.
Thanks,
Mathieu
Does it really have to be a statement expression with a local variable ?
If so, we should consider renaming "ret" to "__ret" to minimize the
chances of a caller issuing BPF_PROG_RUN_PIN_ON_CPU with "ret" as
prog or ctx argument, which would lead to unexpected results.
Indeed. That really can be an inline.
Thanks,
tglx
From: Thomas Gleixner <hidden> Date: 2020-02-14 19:56:36
Mathieu Desnoyers [off-list ref] writes:
On 14-Feb-2020 02:39:31 PM, Thomas Gleixner wrote:
quoted
Replace the preempt_disable/enable() pairs with migrate_disable/enable()
pairs to prepare BPF to work on PREEMPT_RT enabled kernels. On a non-RT
kernel this maps to preempt_disable/enable(), i.e. no functional change.
...
Having all those events randomly and silently discarded might be quite
unexpected from a user standpoint. This turns the deadlock prevention
mechanism into a random tracepoint-dropping facility, which is
unsettling.
Well, it randomly drops events which might be unrelated to the syscall
target today already, this will just drop some more. Shrug.
One alternative approach we could consider to solve this is to make
this deadlock prevention nesting counter per-thread rather than
per-cpu.
That should work both on !RT and RT.
Also, I don't think using __this_cpu_inc() without preempt-disable or
irq off is safe. You'll probably want to move to this_cpu_inc/dec
instead, which can be heavier on some architectures.
From: Thomas Gleixner <hidden> Date: 2020-02-17 12:59:42
The assumption that only programs attached to perf NMI events can deadlock
on memory allocators is wrong. Assume the following simplified callchain:
kmalloc() from regular non BPF context
cache empty
freelist empty
lock(zone->lock);
tracepoint or kprobe
BPF()
update_elem()
lock(bucket)
kmalloc()
cache empty
freelist empty
lock(zone->lock); <- DEADLOCK
There are also other ways to create wreckage:
kmalloc() from regular non BPF context
local_irq_save();
...
obj = slab_first();
kprobe()
BPF()
update_elem()
lock(bucket)
kmalloc()
local_irq_save();
...
obj = slab_first(); <- Same object as above ...
So preallocation _must_ be enforced for all variants of intrusive
instrumentation.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/bpf/verifier.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
@@ -8144,19 +8144,23 @@ static int check_map_prog_compatibility(structbpf_prog*prog){-/* Make sure that BPF_PROG_TYPE_PERF_EVENT programs only use-*preallocatedhashmaps,sincedoingmemoryallocation-*inoverflow_handlercancrashdependingonwherenmigot-*triggered.+/*+*Makesurethattracetypeprogramsusepreallocatedhashmaps.+*Perfprogramsobviouslycan'tdomemoryallocationinNMI+*contextandallothertypescandeadlockonamemoryallocator+*lockwhenatracepoint/kprobetriggersaBPFprograminsidea+*lockheldregionorcreateinconsistentstatewhentheprobeis+*withinaninterruptsdisabledcriticalregioninthememory+*allocator.*/-if(prog->type==BPF_PROG_TYPE_PERF_EVENT){+if((is_tracing_prog_type(prog->type)){if(!check_map_prealloc(map)){-verbose(env,"perf_event programs can only use preallocated hash map\n");+verbose(env,"tracing programs can only use preallocated hash map\n");return-EINVAL;}if(map->inner_map_meta&&!check_map_prealloc(map->inner_map_meta)){-verbose(env,"perf_event programs can only use preallocated inner hash map\n");+verbose(env,"tracing programs can only use preallocated inner hash map\n");return-EINVAL;}}
On Fri, Feb 14, 2020 at 08:56:12PM +0100, Thomas Gleixner wrote:
quoted
Also, I don't think using __this_cpu_inc() without preempt-disable or
irq off is safe. You'll probably want to move to this_cpu_inc/dec
instead, which can be heavier on some architectures.
Good catch.
Overall looks great.
Thank you for taking time to write commit logs and detailed cover letter.
I think s/__this_cpu_inc/this_cpu_inc/ is the only bit that needs to be
addressed for it to be merged.
There were few other suggestions from Mathieu and Jakub.
Could you address them and resend?
I saw patch 1 landing in tip tree, but it needs to be in bpf-next as well
along with the rest of the series. Does it really need to be in the tip?
I would prefer to take the whole thing and avoid conflicts around
migrate_disable() especially if nothing in tip is going to use it in this
development cycle. So just drop patch 1 from the tip?
Regarding
union {
raw_spinlock_t raw_lock;
spinlock_t lock;
};
yeah. it's not pretty, but I also don't have better ideas.
Regarding migrate_disable()... can you enable it without the rest of RT?
I haven't seen its implementation. I suspect it's scheduler only change?
If I can use migrate_disable() without RT it will help my work on sleepable
BPF programs. I would only have to worry about rcu_read_lock() since
preempt_disable() is nicely addressed.
From: Thomas Gleixner <hidden> Date: 2020-02-19 00:50:53
Alexei,
Alexei Starovoitov [off-list ref] writes:
Overall looks great.
Thank you for taking time to write commit logs and detailed cover letter.
I think s/__this_cpu_inc/this_cpu_inc/ is the only bit that needs to be
addressed for it to be merged.
There were few other suggestions from Mathieu and Jakub.
Could you address them and resend?
I have them fixed up already, but I was waiting for further
comments. I'll send it out tomorrow morning as I'm dead tired by now.
I saw patch 1 landing in tip tree, but it needs to be in bpf-next as well
along with the rest of the series. Does it really need to be in the tip?
I would prefer to take the whole thing and avoid conflicts around
migrate_disable() especially if nothing in tip is going to use it in this
development cycle. So just drop patch 1 from the tip?
I'll add patch 2 to a tip branch as well and I'll give you a tag to pull
into BPF (which has only those two commits). That allows us to further
tweak the relevant files without creating conflicts in next.
Regarding
union {
raw_spinlock_t raw_lock;
spinlock_t lock;
};
yeah. it's not pretty, but I also don't have better ideas.
Yeah. I really tried hard to avoid it, but the alternative solution was
code duplication which was even more horrible.
Regarding migrate_disable()... can you enable it without the rest of RT?
I haven't seen its implementation. I suspect it's scheduler only change?
If I can use migrate_disable() without RT it will help my work on sleepable
BPF programs. I would only have to worry about rcu_read_lock() since
preempt_disable() is nicely addressed.
You have to talk to Peter Zijlstra about this as this is really
scheduler relevant stuff. FYI, he undamentaly hates migrate_disable()
from a schedulabilty POV, but as with the above lock construct the
amount of better solutions is also close to zero.
Thanks,
tglx
On Wed, Feb 19, 2020 at 01:49:57AM +0100, Thomas Gleixner wrote:
Alexei,
Alexei Starovoitov [off-list ref] writes:
quoted
Overall looks great.
Thank you for taking time to write commit logs and detailed cover letter.
I think s/__this_cpu_inc/this_cpu_inc/ is the only bit that needs to be
addressed for it to be merged.
There were few other suggestions from Mathieu and Jakub.
Could you address them and resend?
I have them fixed up already, but I was waiting for further
comments. I'll send it out tomorrow morning as I'm dead tired by now.
quoted
I saw patch 1 landing in tip tree, but it needs to be in bpf-next as well
along with the rest of the series. Does it really need to be in the tip?
I would prefer to take the whole thing and avoid conflicts around
migrate_disable() especially if nothing in tip is going to use it in this
development cycle. So just drop patch 1 from the tip?
I'll add patch 2 to a tip branch as well and I'll give you a tag to pull
into BPF (which has only those two commits).
That works too.
quoted
Regarding
union {
raw_spinlock_t raw_lock;
spinlock_t lock;
};
yeah. it's not pretty, but I also don't have better ideas.
Yeah. I really tried hard to avoid it, but the alternative solution was
code duplication which was even more horrible.
quoted
Regarding migrate_disable()... can you enable it without the rest of RT?
I haven't seen its implementation. I suspect it's scheduler only change?
If I can use migrate_disable() without RT it will help my work on sleepable
BPF programs. I would only have to worry about rcu_read_lock() since
preempt_disable() is nicely addressed.
You have to talk to Peter Zijlstra about this as this is really
scheduler relevant stuff. FYI, he undamentaly hates migrate_disable()
from a schedulabilty POV, but as with the above lock construct the
amount of better solutions is also close to zero.
I would imagine that migrate_disable is like temporary cpu pinning. The
scheduler has to have all the logic to make scheduling decisions quickly in
presence of pinned tasks and additional migrate_disable shouldn't introduce
slowdowns or complexity to critical path. Anyway, we'll discuss further
when migrate_disable patches hit mailing lists.
From: Vinicius Costa Gomes <vinicius.gomes@intel.com> Date: 2020-02-19 01:38:05
Hi,
Thomas Gleixner [off-list ref] writes:
quoted hunk
From: David Miller <davem@davemloft.net>
All of these cases are strictly of the form:
preempt_disable();
BPF_PROG_RUN(...);
preempt_enable();
Replace this with BPF_PROG_RUN_PIN_ON_CPU() which wraps BPF_PROG_RUN()
with:
migrate_disable();
BPF_PROG_RUN(...);
migrate_enable();
On non RT enabled kernels this maps to preempt_disable/enable() and on RT
enabled kernels this solely prevents migration, which is sufficient as
there is no requirement to prevent reentrancy to any BPF program from a
preempting task. The only requirement is that the program stays on the same
CPU.
Therefore, this is a trivially correct transformation.
[ tglx: Converted to BPF_PROG_RUN_PIN_ON_CPU() ]
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/filter.h | 4 +---
kernel/seccomp.c | 4 +---
net/core/flow_dissector.c | 4 +---
net/core/skmsg.c | 8 ++------
net/kcm/kcmsock.c | 4 +---
5 files changed, 6 insertions(+), 18 deletions(-)
More a question really, isn't the behavior changing here? i.e. shouldn't
migrate_disable()/migrate_enable() be moved to outside the loop? Or is
running seccomp filters on different cpus not a problem?
--
Vinicius
From: Thomas Gleixner <hidden> Date: 2020-02-19 09:01:40
Vinicius Costa Gomes [off-list ref] writes:
Cc+: seccomp folks
Thomas Gleixner [off-list ref] writes:
quoted
From: David Miller <davem@davemloft.net>
Leaving content for reference
quoted
All of these cases are strictly of the form:
preempt_disable();
BPF_PROG_RUN(...);
preempt_enable();
Replace this with BPF_PROG_RUN_PIN_ON_CPU() which wraps BPF_PROG_RUN()
with:
migrate_disable();
BPF_PROG_RUN(...);
migrate_enable();
On non RT enabled kernels this maps to preempt_disable/enable() and on RT
enabled kernels this solely prevents migration, which is sufficient as
there is no requirement to prevent reentrancy to any BPF program from a
preempting task. The only requirement is that the program stays on the same
CPU.
Therefore, this is a trivially correct transformation.
[ tglx: Converted to BPF_PROG_RUN_PIN_ON_CPU() ]
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/filter.h | 4 +---
kernel/seccomp.c | 4 +---
net/core/flow_dissector.c | 4 +---
net/core/skmsg.c | 8 ++------
net/kcm/kcmsock.c | 4 +---
5 files changed, 6 insertions(+), 18 deletions(-)
More a question really, isn't the behavior changing here? i.e. shouldn't
migrate_disable()/migrate_enable() be moved to outside the loop? Or is
running seccomp filters on different cpus not a problem?
In my understanding this is a list of filters and they are independent
of each other.
Kees, Will. Andy?
Thanks,
tglx
----- On Feb 18, 2020, at 6:36 PM, Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
[...]
If I can use migrate_disable() without RT it will help my work on sleepable
BPF programs. I would only have to worry about rcu_read_lock() since
preempt_disable() is nicely addressed.
Hi Alexei,
You may want to consider using SRCU rather than RCU if you need to sleep while
holding a RCU read-side lock.
This is the synchronization approach I consider for adding the ability to take page
faults when doing syscall tracing.
Then you'll be able to replace preempt_disable() by combining SRCU and
migrate_disable():
AFAIU eBPF currently uses preempt_disable() for two reasons:
- Ensure the thread is not migrated,
-> can be replaced by migrate_disable() in RT
- Provide RCU existence guarantee through sched-RCU
-> can be replaced by SRCU, which allows sleeping and taking page faults.
I wonder if it would be acceptable to take a page fault while migration is
disabled though ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Wed, Feb 19, 2020 at 1:01 AM Thomas Gleixner [off-list ref] wrote:
Vinicius Costa Gomes [off-list ref] writes:
Cc+: seccomp folks
quoted
Thomas Gleixner [off-list ref] writes:
quoted
From: David Miller <davem@davemloft.net>
Leaving content for reference
quoted
quoted
All of these cases are strictly of the form:
preempt_disable();
BPF_PROG_RUN(...);
preempt_enable();
Replace this with BPF_PROG_RUN_PIN_ON_CPU() which wraps BPF_PROG_RUN()
with:
migrate_disable();
BPF_PROG_RUN(...);
migrate_enable();
On non RT enabled kernels this maps to preempt_disable/enable() and on RT
enabled kernels this solely prevents migration, which is sufficient as
there is no requirement to prevent reentrancy to any BPF program from a
preempting task. The only requirement is that the program stays on the same
CPU.
Therefore, this is a trivially correct transformation.
[ tglx: Converted to BPF_PROG_RUN_PIN_ON_CPU() ]
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/filter.h | 4 +---
kernel/seccomp.c | 4 +---
net/core/flow_dissector.c | 4 +---
net/core/skmsg.c | 8 ++------
net/kcm/kcmsock.c | 4 +---
5 files changed, 6 insertions(+), 18 deletions(-)
More a question really, isn't the behavior changing here? i.e. shouldn't
migrate_disable()/migrate_enable() be moved to outside the loop? Or is
running seccomp filters on different cpus not a problem?
In my understanding this is a list of filters and they are independent
of each other.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2020-02-19 16:54:20
On Fri, 14 Feb 2020 14:39:21 +0100
Thomas Gleixner [off-list ref] wrote:
quoted hunk
__bpf_trace_run() disables preemption around the BPF_PROG_RUN() invocation.
This is redundant because __bpf_trace_run() is invoked from a trace point
via __DO_TRACE() which already disables preemption _before_ invoking any of
the functions which are attached to a trace point.
Remove it.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/trace/bpf_trace.c | 2 --
1 file changed, 2 deletions(-)
From: Thomas Gleixner <hidden> Date: 2020-02-19 17:26:30
Steven Rostedt [off-list ref] writes:
On Fri, 14 Feb 2020 14:39:21 +0100
Thomas Gleixner [off-list ref] wrote:
quoted
__bpf_trace_run() disables preemption around the BPF_PROG_RUN() invocation.
This is redundant because __bpf_trace_run() is invoked from a trace point
via __DO_TRACE() which already disables preemption _before_ invoking any of
the functions which are attached to a trace point.
Remove it.
Signed-off-by: Thomas Gleixner <redacted>
---
kernel/trace/bpf_trace.c | 2 --
1 file changed, 2 deletions(-)
On Wed, Feb 19, 2020 at 10:17:28AM -0500, Mathieu Desnoyers wrote:
----- On Feb 18, 2020, at 6:36 PM, Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
[...]
quoted
If I can use migrate_disable() without RT it will help my work on sleepable
BPF programs. I would only have to worry about rcu_read_lock() since
preempt_disable() is nicely addressed.
Hi Alexei,
You may want to consider using SRCU rather than RCU if you need to sleep while
holding a RCU read-side lock.
This is the synchronization approach I consider for adding the ability to take page
faults when doing syscall tracing.
Then you'll be able to replace preempt_disable() by combining SRCU and
migrate_disable():
AFAIU eBPF currently uses preempt_disable() for two reasons:
- Ensure the thread is not migrated,
-> can be replaced by migrate_disable() in RT
- Provide RCU existence guarantee through sched-RCU
-> can be replaced by SRCU, which allows sleeping and taking page faults.
bpf is using normal rcu to protect map values
and rcu+preempt to protect per-cpu map values.
srcu is certainly under consideration. It hasn't been used due to performance
implications. atomics and barriers are too heavy for certain use cases. So we
have to keep rcu where performance matters, but cannot fork map implementations
to rcu and srcu due to huge code bloat. So far I've been thinking to introduce
explicit helper bpf_rcu_read_lock() and let programs use it directly instead of
implicit rcu_read_lock() that is done outside of bpf prog. The tricky part is
teaching verifier to enforce critical section.
On Wed, Feb 19, 2020 at 10:00:56AM +0100, Thomas Gleixner wrote:
Vinicius Costa Gomes [off-list ref] writes:
Cc+: seccomp folks
quoted
Thomas Gleixner [off-list ref] writes:
quoted
From: David Miller <davem@davemloft.net>
Leaving content for reference
quoted
quoted
All of these cases are strictly of the form:
preempt_disable();
BPF_PROG_RUN(...);
preempt_enable();
Replace this with BPF_PROG_RUN_PIN_ON_CPU() which wraps BPF_PROG_RUN()
with:
migrate_disable();
BPF_PROG_RUN(...);
migrate_enable();
On non RT enabled kernels this maps to preempt_disable/enable() and on RT
enabled kernels this solely prevents migration, which is sufficient as
there is no requirement to prevent reentrancy to any BPF program from a
preempting task. The only requirement is that the program stays on the same
CPU.
Therefore, this is a trivially correct transformation.
[ tglx: Converted to BPF_PROG_RUN_PIN_ON_CPU() ]
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Thomas Gleixner <redacted>
---
include/linux/filter.h | 4 +---
kernel/seccomp.c | 4 +---
net/core/flow_dissector.c | 4 +---
net/core/skmsg.c | 8 ++------
net/kcm/kcmsock.c | 4 +---
5 files changed, 6 insertions(+), 18 deletions(-)
More a question really, isn't the behavior changing here? i.e. shouldn't
migrate_disable()/migrate_enable() be moved to outside the loop? Or is
running seccomp filters on different cpus not a problem?
In my understanding this is a list of filters and they are independent
of each other.
Kees, Will. Andy?
They're technically independent, but they are related to each
other. (i.e. order matters, process hierarchy matters, etc). There's no
reason I can see that we can't switch CPUs between running them, though.
(AIUI, nothing here would suddenly make these run in parallel, right?)
As long as "current" is still "current", and they run in the same order,
we'll get the same final result as far as seccomp is concerned.
--
Kees Cook
From: Thomas Gleixner <hidden> Date: 2020-02-21 14:01:33
Kees Cook [off-list ref] writes:
On Wed, Feb 19, 2020 at 10:00:56AM +0100, Thomas Gleixner wrote:
quoted
Vinicius Costa Gomes [off-list ref] writes:
quoted
More a question really, isn't the behavior changing here? i.e. shouldn't
migrate_disable()/migrate_enable() be moved to outside the loop? Or is
running seccomp filters on different cpus not a problem?
In my understanding this is a list of filters and they are independent
of each other.
Kees, Will. Andy?
They're technically independent, but they are related to each
other. (i.e. order matters, process hierarchy matters, etc). There's no
reason I can see that we can't switch CPUs between running them, though.
(AIUI, nothing here would suddenly make these run in parallel, right?)
Of course not. If we'd run the same thread on multiple CPUs in parallel
the ordering of your BPF programs would be the least of your worries.
As long as "current" is still "current", and they run in the same order,
we'll get the same final result as far as seccomp is concerned.
On Fri, Feb 21, 2020 at 03:00:54PM +0100, Thomas Gleixner wrote:
Kees Cook [off-list ref] writes:
quoted
They're technically independent, but they are related to each
other. (i.e. order matters, process hierarchy matters, etc). There's no
reason I can see that we can't switch CPUs between running them, though.
(AIUI, nothing here would suddenly make these run in parallel, right?)
Of course not. If we'd run the same thread on multiple CPUs in parallel
the ordering of your BPF programs would be the least of your worries.
Right, okay, good. I just wanted to be extra sure. :)
--
Kees Cook