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
[ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups
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 + * 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 in6_addr *ip6) memcpy(>s6_addr[13], >s6_addr[13], 3); } +bool +in6_addr_is_solicited_node(struct in6_addr *addr) +{ +union ovs_16aligned_in6_addr *taddr = +(union ovs_16aligned_in6_addr *) addr; +return taddr->be16[0] == htons(0xff02) && + taddr->be16[5] == htons(0x1) && + taddr->be16[6] == htons(0xff00); +} + /* * Generates