On Tue, Nov 14, 2023 at 5:33 AM Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
> 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. :)
I'm going to drop the WARN_ON_ONCE() since there is always a risk of
eBPF doing something odd and I don't want to have to keep revisiting
this each time to figure out what is at fault.
Thanks for reporting this Artem, I'll put together a patch and run it
overnight, if everything looks good in the morning I'll post it for
review, comment, etc. before sending it up to Linus.
--
paul-moore.com
