[ovs-dev] incorrect revalidated action for igmp

[Huanle Han]

This patch can slove the problem.

But I think wc->masks.tp_src should not be masked in userspace as igmp type
is not supported in datapath. Otherwise, flow_wildcards_has_extra() in
revalidate_ukey__ always return true for igmp megaflow, and igmp flow is
never kept in datapath.

Thanks,
Huanle

On Wednesday, July 12, 2017, Ben Pfaff <blp at ovn.org> wrote:

> On Thu, Jun 22, 2017 at 01:04:48AM +0800, Huanle Han wrote:
> > In "Normal" action, igmp report packet is expected to processed in slow
> > path.
> > However, the igmp_type(flow->tp_src) is not supported to be masked in
> > datapath.
> > Then ovs-vswitchd revalidate the flow with igmp_type(flow->tp_src) == 0.
> It
> > leads to a "multicast traffic, flooding" action and overwrites the
> > "userspace()" in datapath.
> >
> > This is code segment from function "xlate_normal":
> >
> >         if (is_igmp(flow, wc)) {
> >             memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
> >             if (mcast_snooping_is_membership(flow->tp_src) ||
> >                 mcast_snooping_is_query(flow->tp_src)) { <<<<<<<<<
> > flow->tp_src is 0
> >                 if (ctx->xin->allow_side_effects && ctx->xin->packet) {
> >                     update_mcast_snooping_table(ctx, flow, vlan,
> >                                                 in_xbundle,
> > ctx->xin->packet);
> >                 }
> >                 /*
> >                  * IGMP packets need to take the slow path, in order to
> be
> >                  * processed for mdb updates. That will prevent expires
> >                  * firing off even after hosts have sent reports.
> >                  */
> >                 ctx->xout->slow |= SLOW_ACTION;
> >             }
> > .......
> > }
> >
> > To reproduce this bug, you need to ovs-appctl upcall/disable-megaflows
>
> Thanks for the bug report.
>
> Does the following patch fix the problem?
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 089c7f170d18..5030dafd9f42 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2720,6 +2720,13 @@ xlate_normal(struct xlate_ctx *ctx)
>          struct mcast_group *grp = NULL;
>
>          if (is_igmp(flow, wc)) {
> +            /*
> +             * IGMP packets need to take the slow path, in order to be
> +             * processed for mdb updates. That will prevent expires
> +             * firing off even after hosts have sent reports.
> +             */
> +            ctx->xout->slow |= SLOW_ACTION;
> +
>              memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
>              if (mcast_snooping_is_membership(flow->tp_src) ||
>                  mcast_snooping_is_query(flow->tp_src)) {
> @@ -2727,12 +2734,6 @@ xlate_normal(struct xlate_ctx *ctx)
>                      update_mcast_snooping_table(ctx, flow, vlan,
>                                                  in_xbundle,
> ctx->xin->packet);
>                  }
> -                /*
> -                 * IGMP packets need to take the slow path, in order to be
> -                 * processed for mdb updates. That will prevent expires
> -                 * firing off even after hosts have sent reports.
> -                 */
> -                ctx->xout->slow |= SLOW_ACTION;
>              }
>
>              if (mcast_snooping_is_membership(flow->tp_src)) {
>