netdev - Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC

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]

Date: Fri, 3 Nov 2023 19:05:34 +0800
From: Yang Sun <sunytt@...gle.com>
To: nicolas.dichtel@...nd.com
Cc: Ido Schimmel <idosch@...sch.org>, davem@...emloft.net, dsahern@...nel.org, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	netdev@...r.kernel.org
Subject: Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC

On Thu, Nov 2, 2023 at 10:19 PM Nicolas Dichtel
<nicolas.dichtel@...nd.com> wrote:
>
> Le 02/11/2023 à 12:48, Yang Sun a écrit :
> >> Is this a regression (doesn't seem that way)? If not, the change should
> >> be targeted at net-next which is closed right now:
> >
> >> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> >
> > I see.
> >
> >>> - if (c->mfc_un.res.ttls[vifi] < 255)
> >>> + if (c->mfc_parent == vifi && c->mfc_un.res.ttls[vifi] < 255)
> >
> >> What happens if the route doesn't have an iif (-1)? It won't match
> >> anymore?
> >
> > Looks like the mfc_parent can't be -1? There is the check:
> >     if (mfc->mf6cc_parent >= MAXMIFS)
> >         return -ENFILE;
> > before setting the parent:
> >     c->_c.mfc_parent = mfc->mf6cc_parent;
> >
> > I wrote this patch thinking (*, G) MFCs could be per iif, similar to the
> > (S, G) MFCs, like we can add the following MFCs to forward packets from
> > any address with group destination ff05::aa from if1 to if2, and forward
> > packets from any address with group destination ff05::aa from if2 to
> > both if1 and if3.
> >
> > (::, ff05::aa)      Iif: if1 Oifs: if1 if2  State: resolved
> > (::, ff05::aa)      Iif: if2 Oifs: if1 if2 if3  State: resolved
> >
> > But reading Nicolas's initial commit message again, it seems to me that
> > (*, G) has to be used together with (*, *) and there should be only one
> > (*, G) entry per group address and include all relevant interfaces in
> > the oifs? Like the following:
> >
> > (::, ::)         Iif: if1 Oifs: if1 if2 if3   State: resolved
> > (::, ff05::aa)   Iif: if1 Oifs: if1 if2 if3   State: resolved
> >
> > Is this how the (*, *|G) MFCs are intended to be used? which means packets
> > to ff05::aa are forwarded from any one of the interfaces to all the other
> > interfaces? If this is the intended way it works then my patch would break
> > things and should be rejected.
> Yes, this was the intend. Only one (*, G) entry was expected (per G).
>
> >
> > Is there a way to achieve the use case I described above? Like having
> > different oifs for different iif?
> Instead of being too strict, maybe you could try to return the 'best' entry.
>
> #1 (::, ff05::aa)      Iif: if1 Oifs: if1 if2  State: resolved
> #2 (::, ff05::aa)      Iif: if2 Oifs: if1 if2 if3  State: resolved
>
> If a packet comes from if2, returns #2, but if a packet comes from if3, returns
> the first matching entry, ie #1 here.
>

Thanks for your reply Nicolas!
Here if it returns the first matching then it depends on which entry
is returned first
by the hash table lookup, the forwarding behavior may be indeterminate
in that case
it seems.

If a packet has no matching (*, G) entry, then it will use the (*, *)
entry to be forwarded
to the upstream interface in (*, *). And with the (*, *) it means we
won't get any nocache upcall
for interfaces included in the static tree, right? So the (S, G) MFC
and the static proxy MFCs
are not meant to be used together?

I wonder how a real use case with (*, G|*) would look like, what
interface could be an
upstream interface. Is there an example?

Thanks,
Yang

>
> Regards,
> Nicolas