On Fri, Dec 6, 2024 at 9:58 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Fri, 2024-12-06 at 09:46 -0800, Andrii Nakryiko wrote:
> > On Fri, Dec 6, 2024 at 9:29 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > >
> > > On Fri, 2024-12-06 at 08:08 -0800, Andrii Nakryiko wrote:
> > >
> > > [...]
> > >
> > > > The tags would be that generalizable side effect declaration approach,
> > > > so seems worth it to set a uniform approach.
> > > >
> > > > > Please take a look at the patch, the change for check_cfg() is 32 lines.
> > > >
> > > > I did, actually. And I already explained what I don't like about it:
> > > > eagerness. check_cfg() is not the right place for this, if we want to
> > > > support dead code elimination and BPF CO-RE-based feature gating.
> > > > Which your patches clearly violate, so I don't like them, sorry.
> > > >
> > > > We made this eagerness mistake with global subprogs verification
> > > > previously, and had to switch it to lazy on-demand global subprog
> > > > validation. I think we should preserve this lazy approach going
> > > > forward.
> > >
> > > In this context tags have same detection power as current changes for check_cfg(),
> >
> > You keep ignoring the eagerness issue. I can't decide whether you
> > think *it makes no difference* (I disagree, but whatever), or you *see
> > no difference* (in which case let me know and I can explain with some
> > simple example).
>
> In the context of the packet pointer invalidation I see no difference.
> Tags are as eager as check_cfg() traversal.
Goodness, Eduard...
static __noinline void maybe_trigger_pkt_invalidation(bool do_trigger)
{
if (do_trigger)
bpf_whatever_helper_triggers_pkt_invalidation();
/* presumably do something useful here */
}
__weak /*global*/ int global_no_pkt_invalidation(void)
{
maybe_trigger_pkt_invalidation(false); /* DO NOT trigger */
return 0;
}
__weak /*global*/ __subprog_triggers_pkt_invalidation_and_I_mean_it
int global_make_pkt_invalidation_great(void)
{
maybe_trigger_pkt_invalidation(true); /* DO trigger */
return 0;
}
What does your check_cfg() say about global_no_pkt_invalidation()? Can
it trigger pkt invalidation or not?
>
> > > it is not possible to remove tag using dead code elimination.
> >
> > That's not the point of the tag to be dynamically adjustable. It's the
> > opposite. It's something that the user declares upfront, and this is
> > being enforced by the verifier (to prevent user errors, for example).
> > If the user wants to have a "dynamic tag", they can have two global
> > subprogs, one with and one without the tag, and pick which one should
> > be called through, e.g., .rodata feature flag variable. I.e., make
> > this decision outside of global subprog itself.
> >
> > > So I really don't see any advantages in the context of this particular issue.
> >
> > See also my reply to Alexei, and keep in mind freplace scenario, as
> > one of the things your approach can't support.
>
> Some freplace related mark will have to be present after program verification.
> It might be in a form of a tag, or in a form of an additional bit in
> an auxiliary structure. There would be code to check this with both approaches.
>
tag vs check_cfg() is not about that aspect, in both cases we need to
recod whether subprog can trigger pkt invalidation or not.
It's about whether we derive this (and then where, in check_cfg() or
in proper verification pass), or whether the user declares it and we
enforce that in the verifier.
