| 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); }
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(-)