[ovs-dev] [PATCH] dpif-netlink: Fix dumping uninitialized netdev flow stats.

Roi Dayan roid at mellanox.com
Sun Jan 5 09:42:08 UTC 2020



On 2020-01-04 2:44 AM, Ilya Maximets wrote:
> dpif logging functions expects to be called after the operation.
> log_flow_del_message() dumps flow stats on success which are not
> initialized before the actual call to netdev_flow_del():
> 
>  Conditional jump or move depends on uninitialised value(s)
>     at 0x6090875: _itoa_word (_itoa.c:179)
>     by 0x6093F0D: vfprintf (vfprintf.c:1642)
>     by 0x60C090F: vsnprintf (vsnprintf.c:114)
>     by 0xE5E7EC: ds_put_format_valist (dynamic-string.c:155)
>     by 0xE5E755: ds_put_format (dynamic-string.c:142)
>     by 0xE5A5E6: dpif_flow_stats_format (dpif.c:903)
>     by 0xE5B708: log_flow_message (dpif.c:1763)
>     by 0xE5BCA4: log_flow_del_message (dpif.c:1809)
>     by 0xFA6076: try_send_to_netdev (dpif-netlink.c:2190)
>     by 0xFA0D3C: dpif_netlink_operate (dpif-netlink.c:2248)
>     by 0xE5AFAC: dpif_operate (dpif.c:1376)
>     by 0xDF176E: push_dp_ops (ofproto-dpif-upcall.c:2367)
>     by 0xDF04C8: push_ukey_ops (ofproto-dpif-upcall.c:2447)
>     by 0xDF008F: revalidator_sweep__ (ofproto-dpif-upcall.c:2805)
>     by 0xDF5DC6: revalidator_sweep (ofproto-dpif-upcall.c:2816)
>     by 0xDF1E83: udpif_revalidator (ofproto-dpif-upcall.c:949)
>     by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383)
>     by 0x565F6DA: start_thread (pthread_create.c:463)
>     by 0x615988E: clone (clone.S:95)
>   Uninitialised value was created by a stack allocation
>     at 0xDEFC24: revalidator_sweep__ (ofproto-dpif-upcall.c:2733)
> 
> Also, by calling the log functions after the actual operation we could
> pass the real error code instead of reporting success unconditionally.
> 
> CC: Roi Dayan <roid at mellanox.com>
> Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions")
> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> ---
>  lib/dpif-netlink.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index a08c2378a..25423e86c 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2176,8 +2176,8 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>              break;
>          }
>  
> -        log_flow_put_message(&dpif->dpif, &this_module, put, 0);
>          err = parse_flow_put(dpif, put);
> +        log_flow_put_message(&dpif->dpif, &this_module, put, err);
>          break;
>      }
>      case DPIF_OP_FLOW_DEL: {
> @@ -2187,9 +2187,9 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>              break;
>          }
>  
> -        log_flow_del_message(&dpif->dpif, &this_module, del, 0);
>          err = netdev_ports_flow_del(dpif->dpif.dpif_class, del->ufid,
>                                      del->stats);
> +        log_flow_del_message(&dpif->dpif, &this_module, del, err);
>          break;
>      }
>      case DPIF_OP_FLOW_GET: {
> @@ -2199,8 +2199,8 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>              break;
>          }
>  
> -        log_flow_get_message(&dpif->dpif, &this_module, get, 0);
>          err = parse_flow_get(dpif, get);
> +        log_flow_get_message(&dpif->dpif, &this_module, get, err);
>          break;
>      }
>      case DPIF_OP_EXECUTE:
> 


Hi Ilya,

The original prints were debug prints.
and the original commit (fixes line) was to remove the custom dbg function
and use the dpif functions. but still as debug so we pass err code 0.
So we call the debug before the action because each flow not supported to
be offloaded will return an error to fallback to SW but we dont want to
consider this error there. With this patch I think we will get too many
errors in the log you don't care about when ovs will try to add/del/get
a flow that is not offloaded.

The call to log_flow_get_message() with an error code is from dpif_operate()
which first will try offload to netdev, then callback to netlink, and only if that
fails call to log_flow_get_message() with the last error code.

Thanks,
Roi


More information about the dev mailing list