| Message ID | 1520291041-15769-1-git-send-email-ashishvarma.ovs@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v1] ofproto-dpif-upcall: fix for segmentation fault | expand |
On Mon, Mar 05, 2018 at 03:04:01PM -0800, Ashish Varma wrote: > Added check for NULL pointer on return from xlate_lookup_ofproto > function. Access to "ofproto" variable when NULL was causing segmentation > fault. > > VMware-BZ: #2061914 > Signed-off-by: Ashish Varma <ashishvarma.ovs@gmail.com> > --- > ofproto/ofproto-dpif-upcall.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 526be77..0079674 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2129,6 +2129,11 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey, > ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port); > > ofpbuf_clear(odp_actions); > + > + if (!ofproto) { > + goto exit; > + } > + > compose_slow_path(udpif, xoutp, &ctx.flow, ctx.flow.in_port.odp_port, > ofp_in_port, odp_actions, > ofproto->up.slowpath_meter_id, &ofproto->uuid); This bug got introduced in commit d39ec23de384 (ofproto-dpif: Don't slow-path controller actions.), which introduced the following change. The change seems to conscientiously decide that 'ofproto' could no longer be NULL, since it removes null tests. Justin, do you think you just overlooked the possibility of null? Ashish's commit will obviously fix the crash; do you think that it is the correct change? @@ -2040,19 +2102,16 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey, if (xoutp->slow) { struct ofproto_dpif *ofproto; ofp_port_t ofp_in_port; - ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, - &ofp_in_port); - uint32_t smid = ofproto ? ofproto->up.slowpath_meter_id : UINT32_MAX; - uint32_t cmid = ofproto ? ofproto->up.controller_meter_id : UINT32_MAX; + ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port); ofpbuf_clear(odp_actions); compose_slow_path(udpif, xoutp, &ctx.flow, ctx.flow.in_port.odp_port, - ofp_in_port, odp_actions, smid, cmid, - &ofproto->uuid); + ofp_in_port, odp_actions, + ofproto->up.slowpath_meter_id, &ofproto->uuid); } if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, &dp_mask, &ctx.flow) == ODP_FIT_ERROR) { goto exit; Thanks, Ben.
> On Mar 5, 2018, at 4:09 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Mon, Mar 05, 2018 at 03:04:01PM -0800, Ashish Varma wrote: >> Added check for NULL pointer on return from xlate_lookup_ofproto >> function. Access to "ofproto" variable when NULL was causing segmentation >> fault. >> >> VMware-BZ: #2061914 >> Signed-off-by: Ashish Varma <ashishvarma.ovs@gmail.com> >> --- >> ofproto/ofproto-dpif-upcall.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index 526be77..0079674 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -2129,6 +2129,11 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey, >> ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port); >> >> ofpbuf_clear(odp_actions); >> + >> + if (!ofproto) { >> + goto exit; >> + } >> + >> compose_slow_path(udpif, xoutp, &ctx.flow, ctx.flow.in_port.odp_port, >> ofp_in_port, odp_actions, >> ofproto->up.slowpath_meter_id, &ofproto->uuid); > > This bug got introduced in commit d39ec23de384 (ofproto-dpif: Don't > slow-path controller actions.), which introduced the following change. > The change seems to conscientiously decide that 'ofproto' could no > longer be NULL, since it removes null tests. Justin, do you think you > just overlooked the possibility of null? Ashish's commit will obviously > fix the crash; do you think that it is the correct change? I actually think it was an oversight of the reviewer. (J'accuse.) Yes, I agree that was in error. Thanks for catching it, guys. --Justin
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 526be77..0079674 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2129,6 +2129,11 @@ revalidate_ukey__(struct udpif *udpif, const struct udpif_key *ukey, ofproto = xlate_lookup_ofproto(udpif->backer, &ctx.flow, &ofp_in_port); ofpbuf_clear(odp_actions); + + if (!ofproto) { + goto exit; + } + compose_slow_path(udpif, xoutp, &ctx.flow, ctx.flow.in_port.odp_port, ofp_in_port, odp_actions, ofproto->up.slowpath_meter_id, &ofproto->uuid);
Added check for NULL pointer on return from xlate_lookup_ofproto function. Access to "ofproto" variable when NULL was causing segmentation fault. VMware-BZ: #2061914 Signed-off-by: Ashish Varma <ashishvarma.ovs@gmail.com> --- ofproto/ofproto-dpif-upcall.c | 5 +++++ 1 file changed, 5 insertions(+)