netdev - Re: [PATCH ipsec] xfrm: migrate: work around 0 if

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANP3RGcWhTKOgNsCEb8bMNhktgdzXH+00s5zTKU3=iVocT5rqw@mail.gmail.com>
Date: Thu, 17 Oct 2024 13:03:57 +0200
From: Maciej Żenczykowski <maze@...gle.com>
To: Florian Westphal <fw@...len.de>
Cc: netdev@...r.kernel.org, Nathan Harold <nharold@...gle.com>, 
	Steffen Klassert <steffen.klassert@...unet.com>, Yan Yan <evitayan@...gle.com>
Subject: Re: [PATCH ipsec] xfrm: migrate: work around 0 if_id on migrate

On Thu, Oct 17, 2024 at 12:52 PM Florian Westphal <fw@...len.de> wrote:
>
> Maciej Żenczykowski <maze@...gle.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.

I'll trust your judgment.  I thought we could maybe eventually
deprecate and delete this, but it sounds like that isn't going to be
the case...