Message ID | 1455720236-31510-1-git-send-email-cascardo@redhat.com |
---|---|
State | RFC |
Headers | show |
Greetings Thadeu, Thadeu Lima de Souza Cascardo <cascardo@redhat.com> writes: > The revalidator thread may set may_learn and call xlate_actions with no packet > data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying > to access the NULL packet. > > Only process IGMP and MLD flows when there is a packet. This is a similar > behavior than what we have for other special packets. Just a question on the approach, would it make sense to check for the bundle being null as well? I don't know if the case where packet == NULL but bundle != NULL could ever exist so I could be off my rocker. Alternately, maybe it's better to just patch the mcast functions to bail early (maybe with a ratelimited warning) in this case, so that if bundle != NULL but packet == NULL, some of the mcast group LRU cache can be updated? Don't know if that makes sense, either (maybe there's a security reason not to do that?). Thanks, Aaron > Not-Signed-off-yet: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > Reported-by: Yi Ba <yby.developer@yahoo.com> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-January/020023.html > Fixes: 06994f879c9d ("mcast-snooping: Add Multicast Listener Discovery support") > --- > > Hi, Yi Ba. > > Can you test this patch for your mcast_snooping bug? Remember to enable > OVS_ENABLE_SG_FIREWALL_MULTICAST again. In order to verify it's working, you can > run ovs-vsctl get bridge br-ex mcast_snooping_enable. > > Thanks. > Cascardo. > > --- > AUTHORS | 1 + > ofproto/ofproto-dpif-xlate.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/AUTHORS b/AUTHORS > index 936394d..366b72f 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -412,6 +412,7 @@ Vishal Swarankar vishal.swarnkar@gmail.com > Vjekoslav Brajkovic balkan@cs.washington.edu > Voravit T. voravit@kth.se > Yeming Zhao zhaoyeming@gmail.com > +Yi Ba yby.developer@yahoo.com > Ying Chen yingchen@vmware.com > Yongqiang Liu liuyq7809@gmail.com > ZHANG Zhiming zhangzhiming@yunshan.net.cn > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index a6ea067..7195d45 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -2409,7 +2409,7 @@ xlate_normal(struct xlate_ctx *ctx) > if (is_igmp(flow)) { > if (mcast_snooping_is_membership(flow->tp_src) || > mcast_snooping_is_query(flow->tp_src)) { > - if (ctx->xin->may_learn) { > + if (ctx->xin->may_learn && ctx->xin->packet) { > update_mcast_snooping_table(ctx->xbridge, flow, vlan, > in_xbundle, ctx->xin->packet); > } > @@ -2441,7 +2441,7 @@ xlate_normal(struct xlate_ctx *ctx) > return; > } else if (is_mld(flow)) { > ctx->xout->slow |= SLOW_ACTION; > - if (ctx->xin->may_learn) { > + if (ctx->xin->may_learn && ctx->xin->packet) { > update_mcast_snooping_table(ctx->xbridge, flow, vlan, > in_xbundle, ctx->xin->packet); > }
On 17 February 2016 at 07:24, Aaron Conole <aconole@redhat.com> wrote: > Greetings Thadeu, > > Thadeu Lima de Souza Cascardo <cascardo@redhat.com> writes: > >> The revalidator thread may set may_learn and call xlate_actions with no packet >> data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying >> to access the NULL packet. >> >> Only process IGMP and MLD flows when there is a packet. This is a similar >> behavior than what we have for other special packets. > > Just a question on the approach, would it make sense to check for the > bundle being null as well? I don't know if the case where packet == NULL > but bundle != NULL could ever exist so I could be off my rocker. If you're referring to "in_xbundle", one of the first things that xlate_normal() does is to ensure it is non-NULL. > Alternately, maybe it's better to just patch the mcast functions to bail > early (maybe with a ratelimited warning) in this case, so that if bundle > != NULL but packet == NULL, some of the mcast group LRU cache can be > updated? Don't know if that makes sense, either (maybe there's a > security reason not to do that?). This codepath shouldn't assume that it has a packet. It is called from revalidator threads to attribute stats and do side-effects (like learning, cache refresh). That path doesn't have a copy of a packet to use for translation. If 'may_learn' is true, it indicates that packets have hit this flow since the last time stats were attributed, so learning/cache refresh should occur if applicable. In the case of special handling of packets which are slowpathed (as indicated by the "ctx->xout->slow |= SLOW_ACTION" line), it is fine to do nothing when there is no packet. The slow path handling code will deal with side-effects.
On Thu, Feb 18, 2016 at 01:35:20PM -0800, Joe Stringer wrote: > On 17 February 2016 at 07:24, Aaron Conole <aconole@redhat.com> wrote: > > Greetings Thadeu, > > > > Thadeu Lima de Souza Cascardo <cascardo@redhat.com> writes: > > > >> The revalidator thread may set may_learn and call xlate_actions with no packet > >> data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying > >> to access the NULL packet. > >> > >> Only process IGMP and MLD flows when there is a packet. This is a similar > >> behavior than what we have for other special packets. > > > > Just a question on the approach, would it make sense to check for the > > bundle being null as well? I don't know if the case where packet == NULL > > but bundle != NULL could ever exist so I could be off my rocker. > > If you're referring to "in_xbundle", one of the first things that > xlate_normal() does is to ensure it is non-NULL. > > > Alternately, maybe it's better to just patch the mcast functions to bail > > early (maybe with a ratelimited warning) in this case, so that if bundle > > != NULL but packet == NULL, some of the mcast group LRU cache can be > > updated? Don't know if that makes sense, either (maybe there's a > > security reason not to do that?). > > This codepath shouldn't assume that it has a packet. It is called from > revalidator threads to attribute stats and do side-effects (like > learning, cache refresh). That path doesn't have a copy of a packet to > use for translation. If 'may_learn' is true, it indicates that packets > have hit this flow since the last time stats were attributed, so > learning/cache refresh should occur if applicable. In the case of > special handling of packets which are slowpathed (as indicated by the > "ctx->xout->slow |= SLOW_ACTION" line), it is fine to do nothing when > there is no packet. The slow path handling code will deal with > side-effects. Thanks for the explanation, Joe. I guess this means the patch is good as it is, though it would be nice to have Yi Ba Tested-by. Regards. Cascardo.
This patch seems obviously correct to me, so whenever you give me a Signed-off-by, I'll apply it. On Wed, Feb 17, 2016 at 12:43:56PM -0200, Thadeu Lima de Souza Cascardo wrote: > The revalidator thread may set may_learn and call xlate_actions with no packet > data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying > to access the NULL packet. > > Only process IGMP and MLD flows when there is a packet. This is a similar > behavior than what we have for other special packets. > > Not-Signed-off-yet: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > Reported-by: Yi Ba <yby.developer@yahoo.com> > Reported-at: http://openvswitch.org/pipermail/discuss/2016-January/020023.html > Fixes: 06994f879c9d ("mcast-snooping: Add Multicast Listener Discovery support") > --- > > Hi, Yi Ba. > > Can you test this patch for your mcast_snooping bug? Remember to enable > OVS_ENABLE_SG_FIREWALL_MULTICAST again. In order to verify it's working, you can > run ovs-vsctl get bridge br-ex mcast_snooping_enable. > > Thanks. > Cascardo. > > --- > AUTHORS | 1 + > ofproto/ofproto-dpif-xlate.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/AUTHORS b/AUTHORS > index 936394d..366b72f 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -412,6 +412,7 @@ Vishal Swarankar vishal.swarnkar@gmail.com > Vjekoslav Brajkovic balkan@cs.washington.edu > Voravit T. voravit@kth.se > Yeming Zhao zhaoyeming@gmail.com > +Yi Ba yby.developer@yahoo.com > Ying Chen yingchen@vmware.com > Yongqiang Liu liuyq7809@gmail.com > ZHANG Zhiming zhangzhiming@yunshan.net.cn > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index a6ea067..7195d45 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -2409,7 +2409,7 @@ xlate_normal(struct xlate_ctx *ctx) > if (is_igmp(flow)) { > if (mcast_snooping_is_membership(flow->tp_src) || > mcast_snooping_is_query(flow->tp_src)) { > - if (ctx->xin->may_learn) { > + if (ctx->xin->may_learn && ctx->xin->packet) { > update_mcast_snooping_table(ctx->xbridge, flow, vlan, > in_xbundle, ctx->xin->packet); > } > @@ -2441,7 +2441,7 @@ xlate_normal(struct xlate_ctx *ctx) > return; > } else if (is_mld(flow)) { > ctx->xout->slow |= SLOW_ACTION; > - if (ctx->xin->may_learn) { > + if (ctx->xin->may_learn && ctx->xin->packet) { > update_mcast_snooping_table(ctx->xbridge, flow, vlan, > in_xbundle, ctx->xin->packet); > } > -- > 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Mon, Feb 22, 2016 at 04:25:57PM -0800, Ben Pfaff wrote: > This patch seems obviously correct to me, so whenever you give me a > Signed-off-by, I'll apply it. > > On Wed, Feb 17, 2016 at 12:43:56PM -0200, Thadeu Lima de Souza Cascardo wrote: > > The revalidator thread may set may_learn and call xlate_actions with no packet > > data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying > > to access the NULL packet. > > > > Only process IGMP and MLD flows when there is a packet. This is a similar > > behavior than what we have for other special packets. > > > > Not-Signed-off-yet: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > Reported-by: Yi Ba <yby.developer@yahoo.com> > > Reported-at: http://openvswitch.org/pipermail/discuss/2016-January/020023.html > > Fixes: 06994f879c9d ("mcast-snooping: Add Multicast Listener Discovery support") We have given enough time for Yi Ba, and given him credit. And I have tested that it fixes the crash. So, Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > --- > > > > Hi, Yi Ba. > > > > Can you test this patch for your mcast_snooping bug? Remember to enable > > OVS_ENABLE_SG_FIREWALL_MULTICAST again. In order to verify it's working, you can > > run ovs-vsctl get bridge br-ex mcast_snooping_enable. > > > > Thanks. > > Cascardo. > > > > --- > > AUTHORS | 1 + > > ofproto/ofproto-dpif-xlate.c | 4 ++-- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/AUTHORS b/AUTHORS > > index 936394d..366b72f 100644 > > --- a/AUTHORS > > +++ b/AUTHORS > > @@ -412,6 +412,7 @@ Vishal Swarankar vishal.swarnkar@gmail.com > > Vjekoslav Brajkovic balkan@cs.washington.edu > > Voravit T. voravit@kth.se > > Yeming Zhao zhaoyeming@gmail.com > > +Yi Ba yby.developer@yahoo.com > > Ying Chen yingchen@vmware.com > > Yongqiang Liu liuyq7809@gmail.com > > ZHANG Zhiming zhangzhiming@yunshan.net.cn > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index a6ea067..7195d45 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -2409,7 +2409,7 @@ xlate_normal(struct xlate_ctx *ctx) > > if (is_igmp(flow)) { > > if (mcast_snooping_is_membership(flow->tp_src) || > > mcast_snooping_is_query(flow->tp_src)) { > > - if (ctx->xin->may_learn) { > > + if (ctx->xin->may_learn && ctx->xin->packet) { > > update_mcast_snooping_table(ctx->xbridge, flow, vlan, > > in_xbundle, ctx->xin->packet); > > } > > @@ -2441,7 +2441,7 @@ xlate_normal(struct xlate_ctx *ctx) > > return; > > } else if (is_mld(flow)) { > > ctx->xout->slow |= SLOW_ACTION; > > - if (ctx->xin->may_learn) { > > + if (ctx->xin->may_learn && ctx->xin->packet) { > > update_mcast_snooping_table(ctx->xbridge, flow, vlan, > > in_xbundle, ctx->xin->packet); > > } > > -- > > 2.5.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev
On Tue, Feb 23, 2016 at 08:10:50AM -0300, Thadeu Lima de Souza Cascardo wrote: > On Mon, Feb 22, 2016 at 04:25:57PM -0800, Ben Pfaff wrote: > > This patch seems obviously correct to me, so whenever you give me a > > Signed-off-by, I'll apply it. > > > > On Wed, Feb 17, 2016 at 12:43:56PM -0200, Thadeu Lima de Souza Cascardo wrote: > > > The revalidator thread may set may_learn and call xlate_actions with no packet > > > data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying > > > to access the NULL packet. > > > > > > Only process IGMP and MLD flows when there is a packet. This is a similar > > > behavior than what we have for other special packets. > > > > > > Not-Signed-off-yet: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > > Reported-by: Yi Ba <yby.developer@yahoo.com> > > > Reported-at: http://openvswitch.org/pipermail/discuss/2016-January/020023.html > > > Fixes: 06994f879c9d ("mcast-snooping: Add Multicast Listener Discovery support") > > We have given enough time for Yi Ba, and given him credit. And I have tested > that it fixes the crash. So, > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> Thank you. I applied this to master and branch-2.5.
Sorry for the late response, I was out of town. I will test this patch, or simply update my git, the next time I load it, which shouldn't take long.Thanks On Tuesday, 23 February 2016 9:36 AM, Ben Pfaff <blp@ovn.org> wrote: On Tue, Feb 23, 2016 at 08:10:50AM -0300, Thadeu Lima de Souza Cascardo wrote: > On Mon, Feb 22, 2016 at 04:25:57PM -0800, Ben Pfaff wrote: > > This patch seems obviously correct to me, so whenever you give me a > > Signed-off-by, I'll apply it. > > > > On Wed, Feb 17, 2016 at 12:43:56PM -0200, Thadeu Lima de Souza Cascardo wrote: > > > The revalidator thread may set may_learn and call xlate_actions with no packet > > > data. If the revalidated flow is IGMPv3 or MLD, vswitchd will crash when trying > > > to access the NULL packet. > > > > > > Only process IGMP and MLD flows when there is a packet. This is a similar > > > behavior than what we have for other special packets. > > > > > > Not-Signed-off-yet: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > > Reported-by: Yi Ba <yby.developer@yahoo.com> > > > Reported-at: http://openvswitch.org/pipermail/discuss/2016-January/020023.html > > > Fixes: 06994f879c9d ("mcast-snooping: Add Multicast Listener Discovery support") > > We have given enough time for Yi Ba, and given him credit. And I have tested > that it fixes the crash. So, > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> Thank you. I applied this to master and branch-2.5.
diff --git a/AUTHORS b/AUTHORS index 936394d..366b72f 100644 --- a/AUTHORS +++ b/AUTHORS @@ -412,6 +412,7 @@ Vishal Swarankar vishal.swarnkar@gmail.com Vjekoslav Brajkovic balkan@cs.washington.edu Voravit T. voravit@kth.se Yeming Zhao zhaoyeming@gmail.com +Yi Ba yby.developer@yahoo.com Ying Chen yingchen@vmware.com Yongqiang Liu liuyq7809@gmail.com ZHANG Zhiming zhangzhiming@yunshan.net.cn diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index a6ea067..7195d45 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2409,7 +2409,7 @@ xlate_normal(struct xlate_ctx *ctx) if (is_igmp(flow)) { if (mcast_snooping_is_membership(flow->tp_src) || mcast_snooping_is_query(flow->tp_src)) { - if (ctx->xin->may_learn) { + if (ctx->xin->may_learn && ctx->xin->packet) { update_mcast_snooping_table(ctx->xbridge, flow, vlan, in_xbundle, ctx->xin->packet); } @@ -2441,7 +2441,7 @@ xlate_normal(struct xlate_ctx *ctx) return; } else if (is_mld(flow)) { ctx->xout->slow |= SLOW_ACTION; - if (ctx->xin->may_learn) { + if (ctx->xin->may_learn && ctx->xin->packet) { update_mcast_snooping_table(ctx->xbridge, flow, vlan, in_xbundle, ctx->xin->packet); }