On Tue, 23 Apr 2024 at 18:17, Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> On Tue, 23 Apr 2024 at 17:02, Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Tue, Apr 23, 2024 at 5:06 AM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > >
> > > On Tue, 23 Apr 2024 at 13:17, Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> > > >
> > > > On Tue, Apr 23, 2024 at 06:19:22AM +0000, Kumar Kartikeya Dwivedi wrote:
> > > > > Add tests for nested cases, nested count preservation upon different
> > > > > subprog calls that disable/enable preemption, and test sleepable helper
> > > > > call in non-preemptible regions.
> > > > >
> > > > > 181/1 preempt_lock/preempt_lock_missing_1:OK
> > > > > 181/2 preempt_lock/preempt_lock_missing_2:OK
> > > > > 181/3 preempt_lock/preempt_lock_missing_3:OK
> > > > > 181/4 preempt_lock/preempt_lock_missing_3_minus_2:OK
> > > > > 181/5 preempt_lock/preempt_lock_missing_1_subprog:OK
> > > > > 181/6 preempt_lock/preempt_lock_missing_2_subprog:OK
> > > > > 181/7 preempt_lock/preempt_lock_missing_2_minus_1_subprog:OK
> > > > > 181/8 preempt_lock/preempt_balance:OK
> > > > > 181/9 preempt_lock/preempt_balance_subprog_test:OK
> > > > > 181/10 preempt_lock/preempt_sleepable_helper:OK
> > > >
> > > > should we also check that the global function call is not allowed?
> > > >
> > >
> > > Good point, that is missing, I'll wait for more reviews and then
> > > respin with a failure test for this.
> >
> > I couldn't find the check in patch 1 that does:
> > "Global functions are disallowed from being called".
>
> See this part：
>
> + /* Only global subprogs cannot be called with preemption disabled. */
> + if (env->cur_state->active_preempt_lock) {
> + verbose(env, "global function calls are not allowed with preemption
> disabled,\n"
> + "use static function instead\n");
> + return -EINVAL;
> + }
>
> >
> > And I agree that we need to allow global funcs in preempt disabled region.
> > Sounds like you're planning that in the follow up.
>
> Yeah, but it's not very simple. I noticed a few more problems with
> global functions before that can be done.
> They need to be marked !sleepable before do_check and only verified
> with !sleepable. Otherwise if that is done later in do_check the
> global function will be seen in the non-preemptible section but may
> have been already verified.
> We need some summarization pass for both preempt and rcu_read_lock kfuncs.
E.g. I also have in my TODO list (from hitting this a while ago):
+ * Global functions may clear packet pointers, but data, data_end
remain unclobbered in the caller.
They may call a helper on ctx that invalidates pkt pointers but the
caller may not see that during verification and continue accessing
them.
This needs to be known for the global function before their callers
are verified.
Similarly, the context they are called from is atomic or not
throughput the program needs to be known before their verification is
done.
Otherwise they are verified as sleepable for sleepable programs, while
they maybe called from non-preemptible region.
I will fix the latter first, but a summarization pass for them is
needed and useful to fix the former as well.
