Re: [ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups
> On 1/23/23 16:36, Lorenzo Bianconi wrote:
> >> On Mon, 2023-01-23 at 10:01 +0100, Lorenzo Bianconi wrote:
> >>> Avoid learning Link-Local reserved multicast addresses if advertised in
> >>> a MLD reports since this interferes with Slaac IPv6 address resolution
> >>> implemented in OVN.
> >>>
> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2154930
> >>> Tested-by: Eduardo Olivares
> >>> Signed-off-by: Lorenzo Bianconi
> >>> ---
> >>> lib/mcast-snooping.c | 69
> >>> lib/packets.c | 11 +++
> >>> lib/packets.h | 13 +
> >>> 3 files changed, 74 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> >>> index 029ca2855..a9d2e7900 100644
> >>> --- a/lib/mcast-snooping.c
> >>> +++ b/lib/mcast-snooping.c
> >>> @@ -38,6 +38,8 @@
> >>> COVERAGE_DEFINE(mcast_snooping_learned);
> >>> COVERAGE_DEFINE(mcast_snooping_expired);
> >>>
> >>> +VLOG_DEFINE_THIS_MODULE(mcast_snooping);
> >>> +
> >>> static struct mcast_port_bundle *
> >>> mcast_snooping_port_lookup(struct ovs_list *list, void *port);
> >>> static struct mcast_mrouter_bundle *
> >>> @@ -489,6 +491,28 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
> >>> return count;
> >>> }
> >>>
> >>> +static bool
> >>> +mcast_snooping_should_learn_mld_group(struct in6_addr *addr)
> >>> +{
> >>> + /* we should learn multicast group from the following IPv6 multicast
> >>
> >> Is this supposed to be "should *not* learn multicast group"?
> >
> > yes, sorry :)
>
> Hi, Lorenzo,
Hi Dumitru,
>
> Even in this form, I'm not sure this approach is correct. I thought
> some more about it and (even if these are reserved groups) an MLD
> snooping switch should probably learn them and only flood packets to
> hosts that registered.
>
> Moreover, with your patch, if we don't learn the groups we should at
> least make sure all traffic towards the group IPs is flooded and we're
> not actually doing that.
>
> But this brings me to the original OVN problem. I think the real issue
> is that in the bug report the user is expecting the logical router port
> to reply with "router advertisement" for a "router solicitation".
correct
>
> The router solicitation packet is sent to ff02::2 but the OVN logical
> router port never sends an MLD report to join that group. That means
> that the OVN logical switch with MLD snoop enabled doesn't learn that it
> should forward traffic with dest IP ff02::2 towards the router port too.
>
> I think the best way to address this for OVN is to just skip generating
> logical flows in OVN for groups that correspond to IPv6 reserved
> multicast addresses. We currently only skip ff02::1 [0].
>
> We probably need to change that to include
> all-routers/all-site-router/solicited-node too.
>
> What do you think?
sure, I am fine with this approach. I will work on a v2.
Regards,
Lorenzo
>
> Regards,
> Dumitru
>
> [0] https://github.com/ovn-org/ovn/blob/main/northd/northd.c#L9022
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups
On 1/23/23 16:36, Lorenzo Bianconi wrote:
>> On Mon, 2023-01-23 at 10:01 +0100, Lorenzo Bianconi wrote:
>>> Avoid learning Link-Local reserved multicast addresses if advertised in
>>> a MLD reports since this interferes with Slaac IPv6 address resolution
>>> implemented in OVN.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2154930
>>> Tested-by: Eduardo Olivares
>>> Signed-off-by: Lorenzo Bianconi
>>> ---
>>> lib/mcast-snooping.c | 69
>>> lib/packets.c | 11 +++
>>> lib/packets.h | 13 +
>>> 3 files changed, 74 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
>>> index 029ca2855..a9d2e7900 100644
>>> --- a/lib/mcast-snooping.c
>>> +++ b/lib/mcast-snooping.c
>>> @@ -38,6 +38,8 @@
>>> COVERAGE_DEFINE(mcast_snooping_learned);
>>> COVERAGE_DEFINE(mcast_snooping_expired);
>>>
>>> +VLOG_DEFINE_THIS_MODULE(mcast_snooping);
>>> +
>>> static struct mcast_port_bundle *
>>> mcast_snooping_port_lookup(struct ovs_list *list, void *port);
>>> static struct mcast_mrouter_bundle *
>>> @@ -489,6 +491,28 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
>>> return count;
>>> }
>>>
>>> +static bool
>>> +mcast_snooping_should_learn_mld_group(struct in6_addr *addr)
>>> +{
>>> + /* we should learn multicast group from the following IPv6 multicast
>>
>> Is this supposed to be "should *not* learn multicast group"?
>
> yes, sorry :)
Hi, Lorenzo,
Even in this form, I'm not sure this approach is correct. I thought
some more about it and (even if these are reserved groups) an MLD
snooping switch should probably learn them and only flood packets to
hosts that registered.
Moreover, with your patch, if we don't learn the groups we should at
least make sure all traffic towards the group IPs is flooded and we're
not actually doing that.
But this brings me to the original OVN problem. I think the real issue
is that in the bug report the user is expecting the logical router port
to reply with "router advertisement" for a "router solicitation".
The router solicitation packet is sent to ff02::2 but the OVN logical
router port never sends an MLD report to join that group. That means
that the OVN logical switch with MLD snoop enabled doesn't learn that it
should forward traffic with dest IP ff02::2 towards the router port too.
I think the best way to address this for OVN is to just skip generating
logical flows in OVN for groups that correspond to IPv6 reserved
multicast addresses. We currently only skip ff02::1 [0].
We probably need to change that to include
all-routers/all-site-router/solicited-node too.
What do you think?
Regards,
Dumitru
[0] https://github.com/ovn-org/ovn/blob/main/northd/northd.c#L9022
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups
> On Mon, 2023-01-23 at 10:01 +0100, Lorenzo Bianconi wrote:
> > Avoid learning Link-Local reserved multicast addresses if advertised in
> > a MLD reports since this interferes with Slaac IPv6 address resolution
> > implemented in OVN.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2154930
> > Tested-by: Eduardo Olivares
> > Signed-off-by: Lorenzo Bianconi
> > ---
> > lib/mcast-snooping.c | 69
> > lib/packets.c | 11 +++
> > lib/packets.h | 13 +
> > 3 files changed, 74 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> > index 029ca2855..a9d2e7900 100644
> > --- a/lib/mcast-snooping.c
> > +++ b/lib/mcast-snooping.c
> > @@ -38,6 +38,8 @@
> > COVERAGE_DEFINE(mcast_snooping_learned);
> > COVERAGE_DEFINE(mcast_snooping_expired);
> >
> > +VLOG_DEFINE_THIS_MODULE(mcast_snooping);
> > +
> > static struct mcast_port_bundle *
> > mcast_snooping_port_lookup(struct ovs_list *list, void *port);
> > static struct mcast_mrouter_bundle *
> > @@ -489,6 +491,28 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
> > return count;
> > }
> >
> > +static bool
> > +mcast_snooping_should_learn_mld_group(struct in6_addr *addr)
> > +{
> > + /* we should learn multicast group from the following IPv6 multicast
>
> Is this supposed to be "should *not* learn multicast group"?
yes, sorry :)
Regards,
Lorenzo
>
> Dan
>
> > + * groups:
> > + * - All Nodes Address (ff02::1)
> > + * - All Router Address (ff02::2)
> > + * - All Site Router Address (ff05::2)
> > + * - Solicited-Node Address (ff02::1:ff00:/104)
> > + */
> > + if (in6_addr_is_solicited_node(addr) || ipv6_is_all_hosts(addr) ||
> > + ipv6_is_all_router(addr) || ipv6_is_all_site_router(addr)) {
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > + char ip_str[INET6_ADDRSTRLEN + 1];
> > +
> > + ipv6_string_mapped(ip_str, addr);
> > + VLOG_DBG_RL(, "Invalid Multicast group %s", ip_str);
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > int
> > mcast_snooping_add_mld(struct mcast_snooping *ms,
> > const struct dp_packet *p,
> > @@ -531,26 +555,33 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
> > break;
> > }
> > /* Only consider known record types. */
> > - if (record->type >= IGMPV3_MODE_IS_INCLUDE
> > - && record->type <= IGMPV3_BLOCK_OLD_SOURCES) {
> > - struct in6_addr maddr;
> > - memcpy(maddr.s6_addr, record->maddr.be16, 16);
> > - addr =
> > - /*
> > - * If record is INCLUDE MODE and there are no sources, it's
> > - * equivalent to a LEAVE.
> > - */
> > - if (record->nsrcs == htons(0)
> > - && (record->type == IGMPV3_MODE_IS_INCLUDE
> > - || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE))
> > {
> > - ret = mcast_snooping_leave_group(ms, addr, vlan, port);
> > - } else {
> > - ret = mcast_snooping_add_group(ms, addr, vlan, port);
> > - }
> > - if (ret) {
> > - count++;
> > - }
> > + if (record->type < IGMPV3_MODE_IS_INCLUDE ||
> > + record->type > IGMPV3_BLOCK_OLD_SOURCES) {
> > + goto next;
> > + }
> > +
> > + struct in6_addr maddr;
> > + memcpy(maddr.s6_addr, record->maddr.be16, 16);
> > + if (!mcast_snooping_should_learn_mld_group()) {
> > + goto next;
> > + }
> > +
> > + /*
> > + * If record is INCLUDE MODE and there are no sources, it's
> > + * equivalent to a LEAVE.
> > + */
> > + addr =
> > + if (record->nsrcs == htons(0)
> > + && (record->type == IGMPV3_MODE_IS_INCLUDE
> > + || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
> > + ret = mcast_snooping_leave_group(ms, addr, vlan, port);
> > + } else {
> > + ret = mcast_snooping_add_group(ms, addr, vlan, port);
> > + }
> > + if (ret) {
> > + count++;
> > }
> > +next:
> > offset += sizeof(*record)
> > + ntohs(record->nsrcs) * sizeof(struct in6_addr)
> > + record->aux_len;
> > diff --git a/lib/packets.c b/lib/packets.c
> > index 06f516cb1..b43f78bc1 100644
> > --- a/lib/packets.c
> > +++ b/lib/packets.c
> > @@ -38,6 +38,7 @@
> > const struct in6_addr in6addr_exact = IN6ADDR_EXACT_INIT;
> > const struct in6_addr in6addr_all_hosts = IN6ADDR_ALL_HOSTS_INIT;
> > const struct in6_addr
Re: [ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups
On Mon, 2023-01-23 at 10:01 +0100, Lorenzo Bianconi wrote:
> Avoid learning Link-Local reserved multicast addresses if advertised in
> a MLD reports since this interferes with Slaac IPv6 address resolution
> implemented in OVN.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2154930
> Tested-by: Eduardo Olivares
> Signed-off-by: Lorenzo Bianconi
> ---
> lib/mcast-snooping.c | 69
> lib/packets.c | 11 +++
> lib/packets.h | 13 +
> 3 files changed, 74 insertions(+), 19 deletions(-)
>
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index 029ca2855..a9d2e7900 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -38,6 +38,8 @@
> COVERAGE_DEFINE(mcast_snooping_learned);
> COVERAGE_DEFINE(mcast_snooping_expired);
>
> +VLOG_DEFINE_THIS_MODULE(mcast_snooping);
> +
> static struct mcast_port_bundle *
> mcast_snooping_port_lookup(struct ovs_list *list, void *port);
> static struct mcast_mrouter_bundle *
> @@ -489,6 +491,28 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
> return count;
> }
>
> +static bool
> +mcast_snooping_should_learn_mld_group(struct in6_addr *addr)
> +{
> + /* we should learn multicast group from the following IPv6 multicast
Is this supposed to be "should *not* learn multicast group"?
Dan
> + * groups:
> + * - All Nodes Address (ff02::1)
> + * - All Router Address (ff02::2)
> + * - All Site Router Address (ff05::2)
> + * - Solicited-Node Address (ff02::1:ff00:/104)
> + */
> + if (in6_addr_is_solicited_node(addr) || ipv6_is_all_hosts(addr) ||
> + ipv6_is_all_router(addr) || ipv6_is_all_site_router(addr)) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> + char ip_str[INET6_ADDRSTRLEN + 1];
> +
> + ipv6_string_mapped(ip_str, addr);
> + VLOG_DBG_RL(, "Invalid Multicast group %s", ip_str);
> + return false;
> + }
> + return true;
> +}
> +
> int
> mcast_snooping_add_mld(struct mcast_snooping *ms,
> const struct dp_packet *p,
> @@ -531,26 +555,33 @@ mcast_snooping_add_mld(struct mcast_snooping *ms,
> break;
> }
> /* Only consider known record types. */
> - if (record->type >= IGMPV3_MODE_IS_INCLUDE
> - && record->type <= IGMPV3_BLOCK_OLD_SOURCES) {
> - struct in6_addr maddr;
> - memcpy(maddr.s6_addr, record->maddr.be16, 16);
> - addr =
> - /*
> - * If record is INCLUDE MODE and there are no sources, it's
> - * equivalent to a LEAVE.
> - */
> - if (record->nsrcs == htons(0)
> - && (record->type == IGMPV3_MODE_IS_INCLUDE
> - || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
> - ret = mcast_snooping_leave_group(ms, addr, vlan, port);
> - } else {
> - ret = mcast_snooping_add_group(ms, addr, vlan, port);
> - }
> - if (ret) {
> - count++;
> - }
> + if (record->type < IGMPV3_MODE_IS_INCLUDE ||
> + record->type > IGMPV3_BLOCK_OLD_SOURCES) {
> + goto next;
> + }
> +
> + struct in6_addr maddr;
> + memcpy(maddr.s6_addr, record->maddr.be16, 16);
> + if (!mcast_snooping_should_learn_mld_group()) {
> + goto next;
> + }
> +
> + /*
> + * If record is INCLUDE MODE and there are no sources, it's
> + * equivalent to a LEAVE.
> + */
> + addr =
> + if (record->nsrcs == htons(0)
> + && (record->type == IGMPV3_MODE_IS_INCLUDE
> + || record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
> + ret = mcast_snooping_leave_group(ms, addr, vlan, port);
> + } else {
> + ret = mcast_snooping_add_group(ms, addr, vlan, port);
> + }
> + if (ret) {
> + count++;
> }
> +next:
> offset += sizeof(*record)
> + ntohs(record->nsrcs) * sizeof(struct in6_addr)
> + record->aux_len;
> diff --git a/lib/packets.c b/lib/packets.c
> index 06f516cb1..b43f78bc1 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -38,6 +38,7 @@
> const struct in6_addr in6addr_exact = IN6ADDR_EXACT_INIT;
> const struct in6_addr in6addr_all_hosts = IN6ADDR_ALL_HOSTS_INIT;
> const struct in6_addr in6addr_all_routers = IN6ADDR_ALL_ROUTERS_INIT;
> +const struct in6_addr in6addr_all_site_routers =
> IN6ADDR_ALL_SITE_ROUTERS_INIT;
>
> struct in6_addr
> flow_tnl_dst(const struct flow_tnl *tnl)
> @@ -605,6 +606,16 @@ in6_addr_solicited_node(struct in6_addr *addr, const
> struct
Re: [ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups
Bleep bloop. Greetings Lorenzo Bianconi, I am a robot and I have tried out
your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 86 characters long (recommended limit is 79)
#152 FILE: lib/packets.h:1150:
#define IN6ADDR_ALL_SITE_ROUTERS_INIT { { {
0xff,0x05,0x00,0x00,0x00,0x00,0x00,0x00, \
WARNING: Line is 89 characters long (recommended limit is 79)
#153 FILE: lib/packets.h:1151:
0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x02 } } }
Lines checked: 184, Warnings: 2, Errors: 0
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
