Re: [PATCH ipsec] xfrm: migrate: work around 0 if_id on migrate

[Florian Westphal <fw@strlen.de>]

Maciej Żenczykowski <maze@google.com> wrote:
> > +found:
> >         /* Stage 2 - find and update state(s) */
> >         for (i = 0, mp = m; i < num_migrate; i++, mp++) {
> >                 if ((x = xfrm_migrate_state_find(mp, net, if_id))) {
> > --
> > 2.45.2
> >
> 
> Q: Considering the performance impact... would it make sense to hide
> this behind a sysctl or a kconfig?

Kconfig?  I don't think so, all distros except Android would turn it on.

> Yan Yan: Also, while I know we found this issue in Android... do we
> actually need the fix?  Wasn't the adjustment to netd sufficient?
> Android <16 doesn't support >6.6 anyway, and Android 16 should already
> have the netd fix...

... seems you already fixed this, so I suspect this slowpath won't ever
run in your case.

Following relevant cases exist:
1. Userspace asks to migrate existing policy, provides if_id > 0.
   -> slowpath is elided.

2. Userspace asks to migrate existing policy, the policy is NOT for
   xfrm_interface, -> slowpath is also elided because first attempt
   finds the if_id 0 policy.

3. Like 1, but userspace does not set the if_id.
   -> slowpath runs, BUT without it migration would not work.

4. Like 2, but the policy doesn't exist.
   -> slowpath runs and slows things down for no reason.

For 1 and 2 even sysctl knob is irrelevant.

For 3, sysctl knob is *technically* irrelevant, either migrate is
broken (sysctl off) or its on and policy migrate will work.
This also hints we'd have to turn such sysctl on by default...

For 4, sysctl could be used to disable/avoid such slowdown.
But I'm not sure this is a relevant scenario in practice, aside
from fuzzers, AND it breaks 3) again if its off.

So I don't see a need to provide a config knob or a sysctl
that would have to be on by default...

If you think a Kconfig knob makes sense for Android sake I can respin
with such a knob, but I think I'd have to make it default-y.