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

Message-ID: <CAF+qgb4A9ge4JTqeQvQPx1cAkP_MUMMpCOzmgGk1BnDe1dVa6g@mail.gmail.com>
Date: Thu, 2 Nov 2023 19:52:28 +0800
From: Yang Sun <sunytt@...gle.com>
To: 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, 
	nicolas.dichtel@...nd.com
Subject: Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC

> 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.

Is there a way to achieve the use case I described above? Like having
different oifs for different iif?

Thanks!


On Thu, Nov 2, 2023 at 5:52 PM Ido Schimmel <idosch@...sch.org> wrote:
>
> + Nicolas
>
> On Tue, Oct 31, 2023 at 09:57:56AM +0800, Yang Sun wrote:
> > Looking for a (*, G) MFC returns the first match without checking
> > the iif. This can return a MFC not intended for a packet's iif and
> > forwarding the packet with this MFC will not work correctly.
> >
> > When looking up for a (*, G) MFC, check that the MFC's iif is
> > the same as the packet's iif.
>
> 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
>
> >
> > Signed-off-by: Yang Sun <sunytt@...gle.com>
> > ---
> >  net/ipv4/ipmr_base.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
> > index 271dc03fc6db..5cf7c7088dfe 100644
> > --- a/net/ipv4/ipmr_base.c
> > +++ b/net/ipv4/ipmr_base.c
> > @@ -97,7 +97,7 @@ void *mr_mfc_find_any(struct mr_table *mrt, int vifi, void *hasharg)
> >
> >       list = rhltable_lookup(&mrt->mfc_hash, hasharg, *mrt->ops.rht_params);
> >       rhl_for_each_entry_rcu(c, tmp, list, mnode) {
> > -             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?
>
> >                       return c;
> >
> >               /* It's ok if the vifi is part of the static tree */
> > --
> > 2.42.0.820.g83a721a137-goog
> >
> >