On 11/14/23, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> 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.
>
SGTM. Hopefully we can put the matter to rest after that. ;)
--
Mateusz Guzik <mjguzik gmail.com>
