[ovs-dev,2/2] dpctl: init CT entry variable.

Message ID 1505306164-6665-2-git-send-email-antonio.fischetti@intel.com
State Changes Requested
Delegated to: Darrell Ball
Headers show
Series
  • [ovs-dev,1/2] dpctl: manage ret value when dumping CT entries.
Related show

Commit Message

Fischetti, Antonio Sept. 13, 2017, 12:36 p.m.
ct_dpif_entry_uninit could potentially be called even if
ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
a pointer to a CT entry - and just checks it is not null -
it's safer to init to zero any instantiated ct_dpif_entry
variable before its usage.

Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/dpctl.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Gregory Rose Sept. 18, 2017, 6:09 p.m. | #1
On 09/13/2017 05:36 AM, antonio.fischetti@intel.com wrote:
> ct_dpif_entry_uninit could potentially be called even if
> ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
> a pointer to a CT entry - and just checks it is not null -
> it's safer to init to zero any instantiated ct_dpif_entry
> variable before its usage.
> 
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
>   lib/dpctl.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 86d0f90..77d4e58 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
>           return error;
>       }
>   
> +    memset(&cte, 0, sizeof(cte));
>       while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>           struct ds s = DS_EMPTY_INITIALIZER;
>   
> @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
>           return error;
>       }
>   
> +    memset(&cte, 0, sizeof(cte));
>       int tot_conn = 0;
>       while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>           ct_dpif_entry_uninit(&cte);
> @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
>            return 0;
>       }
>   
> +    memset(&cte, 0, sizeof(cte));
>       dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
>   
>       int tot_conn = 0;
> 

Not in the hotpath so OK to be extra careful here.

Reviewed-by: Greg Rose <gvrose8192@gmail.com>
Darrell Ball Sept. 22, 2017, 8:26 a.m. | #2
On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com> wrote:

    ct_dpif_entry_uninit could potentially be called even if
    ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
    a pointer to a CT entry - and just checks it is not null -
    it's safer to init to zero any instantiated ct_dpif_entry
    variable before its usage.
    
[Darrell] I took a look and did not see a particular problem.
               Was there an issue that we are trying to address?; if so, this may hide it ?


    Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
    ---
     lib/dpctl.c | 3 +++
     1 file changed, 3 insertions(+)
    
    diff --git a/lib/dpctl.c b/lib/dpctl.c
    index 86d0f90..77d4e58 100644
    --- a/lib/dpctl.c
    +++ b/lib/dpctl.c
    @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
             return error;
         }
     
    +    memset(&cte, 0, sizeof(cte));
         while (!(ret = ct_dpif_dump_next(dump, &cte))) {
             struct ds s = DS_EMPTY_INITIALIZER;
     
    @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
             return error;
         }
     
    +    memset(&cte, 0, sizeof(cte));
         int tot_conn = 0;
         while (!(ret = ct_dpif_dump_next(dump, &cte))) {
             ct_dpif_entry_uninit(&cte);
    @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
              return 0;
         }
     
    +    memset(&cte, 0, sizeof(cte));
         dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
     
         int tot_conn = 0;
    -- 
    2.4.11
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So&s=tU4fSt243XI_2QHkAF4R2h0smvtTC8fDyiOXBI02_t8&e=
Fischetti, Antonio Sept. 25, 2017, 9:51 a.m. | #3
> -----Original Message-----
> From: Darrell Ball [mailto:dball@vmware.com]
> Sent: Friday, September 22, 2017 9:26 AM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
> 
> 
> 
> On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of
> antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of
> antonio.fischetti@intel.com> wrote:
> 
>     ct_dpif_entry_uninit could potentially be called even if
>     ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
>     a pointer to a CT entry - and just checks it is not null -
>     it's safer to init to zero any instantiated ct_dpif_entry
>     variable before its usage.
> 
> [Darrell] I took a look and did not see a particular problem.
>                Was there an issue that we are trying to address?; if so, this
> may hide it ?

[Antonio]
This change is more a matter of keeping safe habits for future
code additions. 
In a new CT function that could be added down the line, one could
potentially call ct_dpif_entry_uninit without checking what was
returned by ct_dpif_dump_next.

As this is not in the hotpath, I added a memset to be extra-careful 
when initializing the local CT entry variable.

Maybe also a comment on top of the fn definition could help on this,
something like?

+/* This function must be called when the returned
+   value from ct_dpif_dump_next is 0. */
void
ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
{
    if (entry) {
        if (entry->helper.name) {



> 
> 
>     Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
>     ---
>      lib/dpctl.c | 3 +++
>      1 file changed, 3 insertions(+)
> 
>     diff --git a/lib/dpctl.c b/lib/dpctl.c
>     index 86d0f90..77d4e58 100644
>     --- a/lib/dpctl.c
>     +++ b/lib/dpctl.c
>     @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
>              return error;
>          }
> 
>     +    memset(&cte, 0, sizeof(cte));
>          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>              struct ds s = DS_EMPTY_INITIALIZER;
> 
>     @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
>              return error;
>          }
> 
>     +    memset(&cte, 0, sizeof(cte));
>          int tot_conn = 0;
>          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>              ct_dpif_entry_uninit(&cte);
>     @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
>               return 0;
>          }
> 
>     +    memset(&cte, 0, sizeof(cte));
>          dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
> 
>          int tot_conn = 0;
>     --
>     2.4.11
> 
>     _______________________________________________
>     dev mailing list
>     dev@openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-
> uZnsw&m=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So&s=tU4fSt243XI_2QHkAF4R2h0sm
> vtTC8fDyiOXBI02_t8&e=
>
Darrell Ball Sept. 25, 2017, 7:34 p.m. | #4
On 9/25/17, 2:51 AM, "Fischetti, Antonio" <antonio.fischetti@intel.com> wrote:

    > -----Original Message-----
    > From: Darrell Ball [mailto:dball@vmware.com]
    > Sent: Friday, September 22, 2017 9:26 AM
    > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
    > Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
    > 
    > 
    > 
    > On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of
    > antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of
    > antonio.fischetti@intel.com> wrote:
    > 
    >     ct_dpif_entry_uninit could potentially be called even if
    >     ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
    >     a pointer to a CT entry - and just checks it is not null -
    >     it's safer to init to zero any instantiated ct_dpif_entry
    >     variable before its usage.
    > 
    > [Darrell] I took a look and did not see a particular problem.
    >                Was there an issue that we are trying to address?; if so, this
    > may hide it ?
    
    [Antonio]
    This change is more a matter of keeping safe habits for future
    code additions. 
    In a new CT function that could be added down the line, one could
    potentially call ct_dpif_entry_uninit without checking what was
    returned by ct_dpif_dump_next.
    
    As this is not in the hotpath, I added a memset to be extra-careful 
    when initializing the local CT entry variable.
    
    Maybe also a comment on top of the fn definition could help on this,
    something like?
    
    +/* This function must be called when the returned
    +   value from ct_dpif_dump_next is 0. */
    void
    ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
    {
        if (entry) {
            if (entry->helper.name) {


[Darrell] It is usually better to wait for the new code that might need this and
               associate those patches as part of the same series.
               Can we do that ?
    
    
    
    > 
    > 
    >     Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
    >     ---
    >      lib/dpctl.c | 3 +++
    >      1 file changed, 3 insertions(+)
    > 
    >     diff --git a/lib/dpctl.c b/lib/dpctl.c
    >     index 86d0f90..77d4e58 100644
    >     --- a/lib/dpctl.c
    >     +++ b/lib/dpctl.c
    >     @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
    >              return error;
    >          }
    > 
    >     +    memset(&cte, 0, sizeof(cte));
    >          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
    >              struct ds s = DS_EMPTY_INITIALIZER;
    > 
    >     @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
    >              return error;
    >          }
    > 
    >     +    memset(&cte, 0, sizeof(cte));
    >          int tot_conn = 0;
    >          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
    >              ct_dpif_entry_uninit(&cte);
    >     @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
    >               return 0;
    >          }
    > 
    >     +    memset(&cte, 0, sizeof(cte));
    >          dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
    > 
    >          int tot_conn = 0;
    >     --
    >     2.4.11
    > 
    >     _______________________________________________
    >     dev mailing list
    >     dev@openvswitch.org
    >     https://urldefense.proofpoint.com/v2/url?u=https-
    > 3A__mail.openvswitch.org_mailman_listinfo_ovs-
    > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-
    > uZnsw&m=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So&s=tU4fSt243XI_2QHkAF4R2h0sm
    > vtTC8fDyiOXBI02_t8&e=
    >
Fischetti, Antonio Sept. 26, 2017, 8:14 a.m. | #5
> -----Original Message-----
> From: Darrell Ball [mailto:dball@vmware.com]
> Sent: Monday, September 25, 2017 8:35 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
> 
> 
> 
> On 9/25/17, 2:51 AM, "Fischetti, Antonio" <antonio.fischetti@intel.com> wrote:
> 
>     > -----Original Message-----
>     > From: Darrell Ball [mailto:dball@vmware.com]
>     > Sent: Friday, September 22, 2017 9:26 AM
>     > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
>     > Subject: Re: [ovs-dev] [PATCH 2/2] dpctl: init CT entry variable.
>     >
>     >
>     >
>     > On 9/13/17, 5:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of
>     > antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf
> of
>     > antonio.fischetti@intel.com> wrote:
>     >
>     >     ct_dpif_entry_uninit could potentially be called even if
>     >     ct_dpif_dump_next failed. As ct_dpif_entry_uninit receives
>     >     a pointer to a CT entry - and just checks it is not null -
>     >     it's safer to init to zero any instantiated ct_dpif_entry
>     >     variable before its usage.
>     >
>     > [Darrell] I took a look and did not see a particular problem.
>     >                Was there an issue that we are trying to address?; if so,
> this
>     > may hide it ?
> 
>     [Antonio]
>     This change is more a matter of keeping safe habits for future
>     code additions.
>     In a new CT function that could be added down the line, one could
>     potentially call ct_dpif_entry_uninit without checking what was
>     returned by ct_dpif_dump_next.
> 
>     As this is not in the hotpath, I added a memset to be extra-careful
>     when initializing the local CT entry variable.
> 
>     Maybe also a comment on top of the fn definition could help on this,
>     something like?
> 
>     +/* This function must be called when the returned
>     +   value from ct_dpif_dump_next is 0. */
>     void
>     ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
>     {
>         if (entry) {
>             if (entry->helper.name) {
> 
> 
> [Darrell] It is usually better to wait for the new code that might need this
> and
>                associate those patches as part of the same series.
>                Can we do that ?

[Antonio] Yes, makes sense. I'll keep this one into my backlog.


> 
> 
> 
>     >
>     >
>     >     Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
>     >     ---
>     >      lib/dpctl.c | 3 +++
>     >      1 file changed, 3 insertions(+)
>     >
>     >     diff --git a/lib/dpctl.c b/lib/dpctl.c
>     >     index 86d0f90..77d4e58 100644
>     >     --- a/lib/dpctl.c
>     >     +++ b/lib/dpctl.c
>     >     @@ -1287,6 +1287,7 @@ dpctl_dump_conntrack(int argc, const char
> *argv[],
>     >              return error;
>     >          }
>     >
>     >     +    memset(&cte, 0, sizeof(cte));
>     >          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>     >              struct ds s = DS_EMPTY_INITIALIZER;
>     >
>     >     @@ -1392,6 +1393,7 @@ dpctl_ct_stats_show(int argc, const char
> *argv[],
>     >              return error;
>     >          }
>     >
>     >     +    memset(&cte, 0, sizeof(cte));
>     >          int tot_conn = 0;
>     >          while (!(ret = ct_dpif_dump_next(dump, &cte))) {
>     >              ct_dpif_entry_uninit(&cte);
>     >     @@ -1532,6 +1534,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
>     >               return 0;
>     >          }
>     >
>     >     +    memset(&cte, 0, sizeof(cte));
>     >          dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
>     >
>     >          int tot_conn = 0;
>     >     --
>     >     2.4.11
>     >
>     >     _______________________________________________
>     >     dev mailing list
>     >     dev@openvswitch.org
>     >     https://urldefense.proofpoint.com/v2/url?u=https-
>     > 3A__mail.openvswitch.org_mailman_listinfo_ovs-
>     > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-
>     >
> uZnsw&m=3FF1c4sa7rHZb5a1DAZQlnsPZywcY7R_LNFki9WS9So&s=tU4fSt243XI_2QHkAF4R2h0sm
>     > vtTC8fDyiOXBI02_t8&e=
>     >
> 
>

Patch

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 86d0f90..77d4e58 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1287,6 +1287,7 @@  dpctl_dump_conntrack(int argc, const char *argv[],
         return error;
     }
 
+    memset(&cte, 0, sizeof(cte));
     while (!(ret = ct_dpif_dump_next(dump, &cte))) {
         struct ds s = DS_EMPTY_INITIALIZER;
 
@@ -1392,6 +1393,7 @@  dpctl_ct_stats_show(int argc, const char *argv[],
         return error;
     }
 
+    memset(&cte, 0, sizeof(cte));
     int tot_conn = 0;
     while (!(ret = ct_dpif_dump_next(dump, &cte))) {
         ct_dpif_entry_uninit(&cte);
@@ -1532,6 +1534,7 @@  dpctl_ct_bkts(int argc, const char *argv[],
          return 0;
     }
 
+    memset(&cte, 0, sizeof(cte));
     dpctl_print(dpctl_p, "Total Buckets: %d\n", tot_bkts);
 
     int tot_conn = 0;