* [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC
@ 2023-10-31 1:57 Yang Sun
2023-11-02 9:52 ` Ido Schimmel
0 siblings, 1 reply; 6+ messages in thread
From: Yang Sun @ 2023-10-31 1:57 UTC (permalink / raw)
To: davem, dsahern; +Cc: edumazet, kuba, pabeni, netdev, Yang Sun
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.
Signed-off-by: Yang Sun <sunytt@google.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)
return c;
/* It's ok if the vifi is part of the static tree */
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC
2023-10-31 1:57 [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC Yang Sun
@ 2023-11-02 9:52 ` Ido Schimmel
2023-11-02 11:52 ` Yang Sun
[not found] ` <CAF+qgb4gW8vBb8c2xDHfsXsm1-O2KCwXMCTUcT2mYqED51fHoQ@mail.gmail.com>
0 siblings, 2 replies; 6+ messages in thread
From: Ido Schimmel @ 2023-11-02 9:52 UTC (permalink / raw)
To: Yang Sun; +Cc: davem, dsahern, edumazet, kuba, pabeni, netdev, nicolas.dichtel
+ 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@google.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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC
2023-11-02 9:52 ` Ido Schimmel
@ 2023-11-02 11:52 ` Yang Sun
[not found] ` <CAF+qgb4gW8vBb8c2xDHfsXsm1-O2KCwXMCTUcT2mYqED51fHoQ@mail.gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Yang Sun @ 2023-11-02 11:52 UTC (permalink / raw)
To: Ido Schimmel
Cc: davem, dsahern, edumazet, kuba, pabeni, netdev, nicolas.dichtel
> 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@idosch.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@google.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
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC
[not found] ` <CAF+qgb4gW8vBb8c2xDHfsXsm1-O2KCwXMCTUcT2mYqED51fHoQ@mail.gmail.com>
@ 2023-11-02 14:19 ` Nicolas Dichtel
2023-11-03 11:05 ` Yang Sun
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Dichtel @ 2023-11-02 14:19 UTC (permalink / raw)
To: Yang Sun, Ido Schimmel; +Cc: davem, dsahern, edumazet, kuba, pabeni, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC
2023-11-02 14:19 ` Nicolas Dichtel
@ 2023-11-03 11:05 ` Yang Sun
2023-11-03 14:21 ` Nicolas Dichtel
0 siblings, 1 reply; 6+ messages in thread
From: Yang Sun @ 2023-11-03 11:05 UTC (permalink / raw)
To: nicolas.dichtel
Cc: Ido Schimmel, davem, dsahern, edumazet, kuba, pabeni, netdev
On Thu, Nov 2, 2023 at 10:19 PM Nicolas Dichtel
<nicolas.dichtel@6wind.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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC
2023-11-03 11:05 ` Yang Sun
@ 2023-11-03 14:21 ` Nicolas Dichtel
0 siblings, 0 replies; 6+ messages in thread
From: Nicolas Dichtel @ 2023-11-03 14:21 UTC (permalink / raw)
To: Yang Sun; +Cc: Ido Schimmel, davem, dsahern, edumazet, kuba, pabeni, netdev
Le 03/11/2023 à 12:05, Yang Sun a écrit :
> On Thu, Nov 2, 2023 at 10:19 PM Nicolas Dichtel
> <nicolas.dichtel@6wind.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.
As I said, only one (*,G) entry was expected thus the 'first' one is
indeterminate if there are several entries.
>
> 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?
Not together. With proxy multicast, the multicast tree is static, ie there is no
multicast daemon. Mcast packets received from one interface are sent to the
other interfaces that are part of the tree.
Regards,
Nicolas
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-03 14:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-31 1:57 [PATCH] net: ipmr_base: Check iif when returning a (*, G) MFC Yang Sun
2023-11-02 9:52 ` Ido Schimmel
2023-11-02 11:52 ` Yang Sun
[not found] ` <CAF+qgb4gW8vBb8c2xDHfsXsm1-O2KCwXMCTUcT2mYqED51fHoQ@mail.gmail.com>
2023-11-02 14:19 ` Nicolas Dichtel
2023-11-03 11:05 ` Yang Sun
2023-11-03 14:21 ` Nicolas Dichtel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).