Re: [ovs-dev] [PATCH v1] ofproto-dpif-upcall: fix for segmentation fault

[Justin Pettit]

> On Mar 5, 2018, at 4:09 PM, Ben Pfaff  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 
>> ---
>> 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, , 
>> _in_port);
>> 
>> ofpbuf_clear(odp_actions);
>> +
>> +if (!ofproto) {
>> +goto exit;
>> +}
>> +
>> compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
>>   ofp_in_port, odp_actions,
>>   ofproto->up.slowpath_meter_id, >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


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] ofproto-dpif-upcall: fix for segmentation fault

[Ben Pfaff]

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 
> ---
>  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, , 
> _in_port);
>  
>  ofpbuf_clear(odp_actions);
> +
> +if (!ofproto) {
> +goto exit;
> +}
> +
>  compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
>ofp_in_port, odp_actions,
>ofproto->up.slowpath_meter_id, >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, ,
-   _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, , _in_port);
 
 ofpbuf_clear(odp_actions);
 compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
-  ofp_in_port, odp_actions, smid, cmid,
-  >uuid);
+  ofp_in_port, odp_actions,
+  ofproto->up.slowpath_meter_id, >uuid);
 }
 
 if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, _mask, )
 == ODP_FIT_ERROR) {
 goto exit;

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v1] ofproto-dpif-upcall: fix for segmentation fault

[Ashish Varma]

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 
---
 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, , _in_port);
 
 ofpbuf_clear(odp_actions);
+
+if (!ofproto) {
+goto exit;
+}
+
 compose_slow_path(udpif, xoutp, , ctx.flow.in_port.odp_port,
   ofp_in_port, odp_actions,
   ofproto->up.slowpath_meter_id, >uuid);
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev