[ovs-dev] dpif: Fix leak and usage of uninitialized dp_extra_info.
diff mbox series

Message ID 20200117222702.12950-1-i.maximets@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] dpif: Fix leak and usage of uninitialized dp_extra_info.
Related show

Commit Message

Ilya Maximets Jan. 17, 2020, 10:27 p.m. UTC
'dpif_probe_feature'/'revalidate' doesn't free the 'dp_extra_info'
string.  Also, all the implementations of dpif_flow_get() should
initialize the value to avoid printing/freeing of random memory.

 30 bytes in 1 blocks are definitely lost in loss record 323 of 889
    at 0x483AD19: realloc (vg_replace_malloc.c:836)
    by 0xDDAD89: xrealloc (util.c:149)
    by 0xCE1609: ds_reserve (dynamic-string.c:63)
    by 0xCE1A90: ds_put_format_valist (dynamic-string.c:161)
    by 0xCE19B9: ds_put_format (dynamic-string.c:142)
    by 0xCCCEA9: dp_netdev_flow_to_dpif_flow (dpif-netdev.c:3170)
    by 0xCCD2DD: dpif_netdev_flow_get (dpif-netdev.c:3278)
    by 0xCCEA0A: dpif_netdev_operate (dpif-netdev.c:3868)
    by 0xCDF81B: dpif_operate (dpif.c:1361)
    by 0xCDEE93: dpif_flow_get (dpif.c:1002)
    by 0xCDECF9: dpif_probe_feature (dpif.c:962)
    by 0xC635D2: check_recirc (ofproto-dpif.c:896)
    by 0xC65C02: check_support (ofproto-dpif.c:1567)
    by 0xC63274: open_dpif_backer (ofproto-dpif.c:818)
    by 0xC65E3E: construct (ofproto-dpif.c:1605)
    by 0xC4D436: ofproto_create (ofproto.c:549)
    by 0xC3931A: bridge_reconfigure (bridge.c:877)
    by 0xC3FEAC: bridge_run (bridge.c:3324)
    by 0xC4551D: main (ovs-vswitchd.c:127)

CC: Emma Finn <emma.finn@intel.com>
Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/dpif-netlink.c            | 1 +
 lib/dpif.c                    | 1 +
 lib/dpif.h                    | 7 ++++---
 lib/netdev-offload-dpdk.c     | 1 +
 lib/netdev-offload-tc.c       | 1 +
 ofproto/ofproto-dpif-upcall.c | 3 +++
 6 files changed, 11 insertions(+), 3 deletions(-)

Comments

Roi Dayan Jan. 20, 2020, 4:38 p.m. UTC | #1
On 2020-01-18 12:27 AM, Ilya Maximets wrote:
> 'dpif_probe_feature'/'revalidate' doesn't free the 'dp_extra_info'
> string.  Also, all the implementations of dpif_flow_get() should
> initialize the value to avoid printing/freeing of random memory.
> 
>  30 bytes in 1 blocks are definitely lost in loss record 323 of 889
>     at 0x483AD19: realloc (vg_replace_malloc.c:836)
>     by 0xDDAD89: xrealloc (util.c:149)
>     by 0xCE1609: ds_reserve (dynamic-string.c:63)
>     by 0xCE1A90: ds_put_format_valist (dynamic-string.c:161)
>     by 0xCE19B9: ds_put_format (dynamic-string.c:142)
>     by 0xCCCEA9: dp_netdev_flow_to_dpif_flow (dpif-netdev.c:3170)
>     by 0xCCD2DD: dpif_netdev_flow_get (dpif-netdev.c:3278)
>     by 0xCCEA0A: dpif_netdev_operate (dpif-netdev.c:3868)
>     by 0xCDF81B: dpif_operate (dpif.c:1361)
>     by 0xCDEE93: dpif_flow_get (dpif.c:1002)
>     by 0xCDECF9: dpif_probe_feature (dpif.c:962)
>     by 0xC635D2: check_recirc (ofproto-dpif.c:896)
>     by 0xC65C02: check_support (ofproto-dpif.c:1567)
>     by 0xC63274: open_dpif_backer (ofproto-dpif.c:818)
>     by 0xC65E3E: construct (ofproto-dpif.c:1605)
>     by 0xC4D436: ofproto_create (ofproto.c:549)
>     by 0xC3931A: bridge_reconfigure (bridge.c:877)
>     by 0xC3FEAC: bridge_run (bridge.c:3324)
>     by 0xC4551D: main (ovs-vswitchd.c:127)
> 
> CC: Emma Finn <emma.finn@intel.com>
> Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  lib/dpif-netlink.c            | 1 +
>  lib/dpif.c                    | 1 +
>  lib/dpif.h                    | 7 ++++---
>  lib/netdev-offload-dpdk.c     | 1 +
>  lib/netdev-offload-tc.c       | 1 +
>  ofproto/ofproto-dpif-upcall.c | 3 +++
>  6 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index d865c0bdb..5b5c96d72 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1590,6 +1590,7 @@ dpif_netlink_flow_to_dpif_flow(struct dpif_flow *dpif_flow,
>      dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats);
>      dpif_flow->attrs.offloaded = false;
>      dpif_flow->attrs.dp_layer = "ovs";
> +    dpif_flow->attrs.dp_extra_info = NULL;
>  }
>  
>  /* The design is such that all threads are working together on the first dump
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 9d9c716c1..6cbcdfb2e 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -966,6 +966,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name,
>                        && ovs_u128_equals(*ufid, flow.ufid)))) {
>          enable_feature = true;
>      }
> +    free(flow.attrs.dp_extra_info);
>  
>      error = dpif_flow_del(dpif, key->data, key->size, ufid,
>                            NON_PMD_CORE_ID, NULL);
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 59d82dce3..286a0e2d5 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -741,11 +741,12 @@ struct dpif_execute {
>   * 'buffer' must point to an initialized buffer, with a recommended size of
>   * DPIF_FLOW_BUFSIZE bytes.
>   *
> - * On success, 'flow' will be populated with the mask, actions and stats for
> - * the datapath flow corresponding to 'key'. The mask and actions may point
> + * On success, 'flow' will be populated with the mask, actions, stats and attrs
> + * for the datapath flow corresponding to 'key'. The mask and actions may point
>   * within '*buffer', or may point at RCU-protected data. Therefore, callers
>   * that wish to hold these over quiescent periods must make a copy of these
> - * fields before quiescing.
> + * fields before quiescing.  'attrs.dp_extra_info' is a dynamically allocated
> + * string that should be freed if provided by the datapath.
>   *
>   * Callers should always provide 'key' to improve dpif logging in the event of
>   * errors or unexpected behaviour.
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 1ae8230fa..f8c46bbaa 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -1272,6 +1272,7 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev,
>      }
>      memcpy(stats, &rte_flow_data->stats, sizeof *stats);
>  out:
> +    attrs->dp_extra_info = NULL;
>      return ret;
>  }
>  
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 4988dadc3..723ec376d 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -873,6 +873,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>      attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW)
>                         || (flower->offloaded_state == TC_OFFLOADED_STATE_UNDEFINED);
>      attrs->dp_layer = "tc";
> +    attrs->dp_extra_info = NULL;
>  
>      return 0;
>  }
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 409286ab1..3aef9a6c3 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2646,6 +2646,9 @@ revalidate(struct revalidator *revalidator)
>              bool already_dumped;
>              int error;
>  
> +            /* We don't need an extra information. */
> +            free(f->attrs.dp_extra_info);
> +
>              if (ukey_acquire(udpif, f, &ukey, &error)) {
>                  if (error == EBUSY) {
>                      /* Another thread is processing this flow, so don't bother
> 

tested and fixes the issue. thanks!

Acked-by: Roi Dayan <roid@mellanox.com>
Ilya Maximets Jan. 20, 2020, 5:07 p.m. UTC | #2
On 20.01.2020 17:38, Roi Dayan wrote:
> 
> 
> On 2020-01-18 12:27 AM, Ilya Maximets wrote:
>> 'dpif_probe_feature'/'revalidate' doesn't free the 'dp_extra_info'
>> string.  Also, all the implementations of dpif_flow_get() should
>> initialize the value to avoid printing/freeing of random memory.
>>
>>  30 bytes in 1 blocks are definitely lost in loss record 323 of 889
>>     at 0x483AD19: realloc (vg_replace_malloc.c:836)
>>     by 0xDDAD89: xrealloc (util.c:149)
>>     by 0xCE1609: ds_reserve (dynamic-string.c:63)
>>     by 0xCE1A90: ds_put_format_valist (dynamic-string.c:161)
>>     by 0xCE19B9: ds_put_format (dynamic-string.c:142)
>>     by 0xCCCEA9: dp_netdev_flow_to_dpif_flow (dpif-netdev.c:3170)
>>     by 0xCCD2DD: dpif_netdev_flow_get (dpif-netdev.c:3278)
>>     by 0xCCEA0A: dpif_netdev_operate (dpif-netdev.c:3868)
>>     by 0xCDF81B: dpif_operate (dpif.c:1361)
>>     by 0xCDEE93: dpif_flow_get (dpif.c:1002)
>>     by 0xCDECF9: dpif_probe_feature (dpif.c:962)
>>     by 0xC635D2: check_recirc (ofproto-dpif.c:896)
>>     by 0xC65C02: check_support (ofproto-dpif.c:1567)
>>     by 0xC63274: open_dpif_backer (ofproto-dpif.c:818)
>>     by 0xC65E3E: construct (ofproto-dpif.c:1605)
>>     by 0xC4D436: ofproto_create (ofproto.c:549)
>>     by 0xC3931A: bridge_reconfigure (bridge.c:877)
>>     by 0xC3FEAC: bridge_run (bridge.c:3324)
>>     by 0xC4551D: main (ovs-vswitchd.c:127)
>>
>> CC: Emma Finn <emma.finn@intel.com>
>> Fixes: 0e8f5c6a38d0 ("dpif-netdev: Modified ovs-appctl dpctl/dump-flows command")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>> ---
>>  lib/dpif-netlink.c            | 1 +
>>  lib/dpif.c                    | 1 +
>>  lib/dpif.h                    | 7 ++++---
>>  lib/netdev-offload-dpdk.c     | 1 +
>>  lib/netdev-offload-tc.c       | 1 +
>>  ofproto/ofproto-dpif-upcall.c | 3 +++
>>  6 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index d865c0bdb..5b5c96d72 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -1590,6 +1590,7 @@ dpif_netlink_flow_to_dpif_flow(struct dpif_flow *dpif_flow,
>>      dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats);
>>      dpif_flow->attrs.offloaded = false;
>>      dpif_flow->attrs.dp_layer = "ovs";
>> +    dpif_flow->attrs.dp_extra_info = NULL;
>>  }
>>  
>>  /* The design is such that all threads are working together on the first dump
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 9d9c716c1..6cbcdfb2e 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -966,6 +966,7 @@ dpif_probe_feature(struct dpif *dpif, const char *name,
>>                        && ovs_u128_equals(*ufid, flow.ufid)))) {
>>          enable_feature = true;
>>      }
>> +    free(flow.attrs.dp_extra_info);
>>  
>>      error = dpif_flow_del(dpif, key->data, key->size, ufid,
>>                            NON_PMD_CORE_ID, NULL);
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index 59d82dce3..286a0e2d5 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -741,11 +741,12 @@ struct dpif_execute {
>>   * 'buffer' must point to an initialized buffer, with a recommended size of
>>   * DPIF_FLOW_BUFSIZE bytes.
>>   *
>> - * On success, 'flow' will be populated with the mask, actions and stats for
>> - * the datapath flow corresponding to 'key'. The mask and actions may point
>> + * On success, 'flow' will be populated with the mask, actions, stats and attrs
>> + * for the datapath flow corresponding to 'key'. The mask and actions may point
>>   * within '*buffer', or may point at RCU-protected data. Therefore, callers
>>   * that wish to hold these over quiescent periods must make a copy of these
>> - * fields before quiescing.
>> + * fields before quiescing.  'attrs.dp_extra_info' is a dynamically allocated
>> + * string that should be freed if provided by the datapath.
>>   *
>>   * Callers should always provide 'key' to improve dpif logging in the event of
>>   * errors or unexpected behaviour.
>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>> index 1ae8230fa..f8c46bbaa 100644
>> --- a/lib/netdev-offload-dpdk.c
>> +++ b/lib/netdev-offload-dpdk.c
>> @@ -1272,6 +1272,7 @@ netdev_offload_dpdk_flow_get(struct netdev *netdev,
>>      }
>>      memcpy(stats, &rte_flow_data->stats, sizeof *stats);
>>  out:
>> +    attrs->dp_extra_info = NULL;
>>      return ret;
>>  }
>>  
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index 4988dadc3..723ec376d 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -873,6 +873,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>>      attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW)
>>                         || (flower->offloaded_state == TC_OFFLOADED_STATE_UNDEFINED);
>>      attrs->dp_layer = "tc";
>> +    attrs->dp_extra_info = NULL;
>>  
>>      return 0;
>>  }
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 409286ab1..3aef9a6c3 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -2646,6 +2646,9 @@ revalidate(struct revalidator *revalidator)
>>              bool already_dumped;
>>              int error;
>>  
>> +            /* We don't need an extra information. */
>> +            free(f->attrs.dp_extra_info);
>> +
>>              if (ukey_acquire(udpif, f, &ukey, &error)) {
>>                  if (error == EBUSY) {
>>                      /* Another thread is processing this flow, so don't bother
>>
> 
> tested and fixes the issue. thanks!
> 
> Acked-by: Roi Dayan <roid@mellanox.com>
> 

Thanks!  Applied to master.

Best regards, Ilya Maximets.

Patch
diff mbox series

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index d865c0bdb..5b5c96d72 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -1590,6 +1590,7 @@  dpif_netlink_flow_to_dpif_flow(struct dpif_flow *dpif_flow,
     dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats);
     dpif_flow->attrs.offloaded = false;
     dpif_flow->attrs.dp_layer = "ovs";
+    dpif_flow->attrs.dp_extra_info = NULL;
 }
 
 /* The design is such that all threads are working together on the first dump
diff --git a/lib/dpif.c b/lib/dpif.c
index 9d9c716c1..6cbcdfb2e 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -966,6 +966,7 @@  dpif_probe_feature(struct dpif *dpif, const char *name,
                       && ovs_u128_equals(*ufid, flow.ufid)))) {
         enable_feature = true;
     }
+    free(flow.attrs.dp_extra_info);
 
     error = dpif_flow_del(dpif, key->data, key->size, ufid,
                           NON_PMD_CORE_ID, NULL);
diff --git a/lib/dpif.h b/lib/dpif.h
index 59d82dce3..286a0e2d5 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -741,11 +741,12 @@  struct dpif_execute {
  * 'buffer' must point to an initialized buffer, with a recommended size of
  * DPIF_FLOW_BUFSIZE bytes.
  *
- * On success, 'flow' will be populated with the mask, actions and stats for
- * the datapath flow corresponding to 'key'. The mask and actions may point
+ * On success, 'flow' will be populated with the mask, actions, stats and attrs
+ * for the datapath flow corresponding to 'key'. The mask and actions may point
  * within '*buffer', or may point at RCU-protected data. Therefore, callers
  * that wish to hold these over quiescent periods must make a copy of these
- * fields before quiescing.
+ * fields before quiescing.  'attrs.dp_extra_info' is a dynamically allocated
+ * string that should be freed if provided by the datapath.
  *
  * Callers should always provide 'key' to improve dpif logging in the event of
  * errors or unexpected behaviour.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 1ae8230fa..f8c46bbaa 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -1272,6 +1272,7 @@  netdev_offload_dpdk_flow_get(struct netdev *netdev,
     }
     memcpy(stats, &rte_flow_data->stats, sizeof *stats);
 out:
+    attrs->dp_extra_info = NULL;
     return ret;
 }
 
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 4988dadc3..723ec376d 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -873,6 +873,7 @@  parse_tc_flower_to_match(struct tc_flower *flower,
     attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW)
                        || (flower->offloaded_state == TC_OFFLOADED_STATE_UNDEFINED);
     attrs->dp_layer = "tc";
+    attrs->dp_extra_info = NULL;
 
     return 0;
 }
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 409286ab1..3aef9a6c3 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2646,6 +2646,9 @@  revalidate(struct revalidator *revalidator)
             bool already_dumped;
             int error;
 
+            /* We don't need an extra information. */
+            free(f->attrs.dp_extra_info);
+
             if (ukey_acquire(udpif, f, &ukey, &error)) {
                 if (error == EBUSY) {
                     /* Another thread is processing this flow, so don't bother