| Message ID | 20200104004418.12453-1-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netlink: Fix dumping uninitialized netdev flow stats. | expand |
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@mellanox.com> > Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions") > Signed-off-by: Ilya Maximets <i.maximets@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
On 05.01.2020 10:42, Roi Dayan wrote: > > > 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@mellanox.com> >> Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions") >> Signed-off-by: Ilya Maximets <i.maximets@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. I see your point. I'll send v2 without passing err to log functions. Best regards, Ilya Maximets.
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:
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@mellanox.com> Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/dpif-netlink.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)