On 11/14/23, Artem Savkov <asavkov@xxxxxxxxxx> wrote:
> On Tue, Oct 24, 2023 at 07:59:18PM +0200, Mateusz Guzik wrote:
>> For the thread to start executing ->mm has to be set.
>>
>> Although I do find it plausible there maybe a corner case during
>> kernel bootstrap and it may be that code can land here with that
>> state, but I can't be arsed to check.
>>
>> Given that stock code has an unintentional property of handling empty
>> mm and this is a bugfix, I am definitely not going to protest adding a
>> check. But I would WARN_ONCE it though.
>
> There is a case when this happens. Below is the trace I get when
> unloading a bpf program of type BPF_PROG_TYPE_SOCKET_FILTER while there
> is an audit exe filter in place. So maybe the WARN shouldn't be there
> after all?
>
> [ 722.833206] ------------[ cut here ]------------
> [ 722.833902] WARNING: CPU: 1 PID: 0 at kernel/audit_watch.c:534
> audit_exe_compare+0x14d/0x1a0
[snip]
> [ 722.836308] Call Trace:
> [ 722.836343] <IRQ>
> [ 722.836375] ? __warn+0xc9/0x350
> [ 722.836426] ? audit_exe_compare+0x14d/0x1a0
> [ 722.836485] ? report_bug+0x326/0x3c0
> [ 722.836547] ? handle_bug+0x3c/0x70
> [ 722.836596] ? exc_invalid_op+0x14/0x50
> [ 722.836649] ? asm_exc_invalid_op+0x16/0x20
> [ 722.836721] ? audit_exe_compare+0x14d/0x1a0
> [ 722.838368] audit_filter+0x4ab/0xa70
> [ 722.839965] ? perf_event_bpf_event+0xf1/0x490
> [ 722.841562] ? __pfx_audit_filter+0x10/0x10
> [ 722.843157] ? __pfx_perf_event_bpf_event+0x10/0x10
> [ 722.844757] ? rcu_do_batch+0x3d7/0xf50
> [ 722.846330] audit_log_start+0x28/0x60
> [ 722.847870] bpf_audit_prog.part.0+0x3c/0x150
> [ 722.849398] bpf_prog_put_deferred+0x8b/0x210
> [ 722.850919] sk_filter_release_rcu+0xd7/0x110
> [ 722.852439] rcu_do_batch+0x3d9/0xf50
So the question is if you can get these calls to __bpf_prog_put with
current->mm != NULL, and the answer is yes.
I slapped this in:
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0ed286b8a0f0..fd4385e815f1 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2150,6 +2150,8 @@ static void __bpf_prog_put(struct bpf_prog *prog)
{
struct bpf_prog_aux *aux = prog->aux;
+ WARN_ON(current->mm);
+
if (atomic64_dec_and_test(&aux->refcnt)) {
if (in_irq() || irqs_disabled()) {
INIT_WORK(&aux->work, bpf_prog_put_deferred);
and ran a one-liner I had handy:
bpftrace -e 'kprobe:prepare_exec_creds { @[kstack(),
curtask->cred->usage] = count(); }'
Traces are close -> __fput -> bpf_prog_release.
I think it is a bug that ebpf can call into audit with current which
is not even bpf-related, and other times with the 'right' one -- what
is the exe filter supposed to do? (and what about other audit
codepaths which perhaps also look at current?)
I have 0 interest in working on it and I don't think it is a high
priority anyway.
With that in mind I concede replacing WARN_ONCE with a mere check
would still maintain the original bugfix, while not spewing the new
trace and it probably should be done for the time being (albeit with a
comment why).
I do maintain this warn uncovered a problem though.
Ultimately it is Paul's call I think. :)
--
Mateusz Guzik <mjguzik gmail.com>
