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-next>] [day] [month] [year] [list]

Date: Thu, 2 Nov 2023 15:19:18 +0100
From: Nicolas Dichtel <nicolas.dichtel@...nd.com>
To: Yang Sun <sunytt@...gle.com>, Ido Schimmel <idosch@...sch.org>
Cc: 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

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.


Regards,
Nicolas