| Message ID | 20200106102342.7945-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] dpif-netlink: Fix dumping uninitialized netdev flow stats. | expand |
On 2020-01-06 12:23 PM, 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) > > CC: Roi Dayan <roid@mellanox.com> > Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > > Version 2: > * Do not pass actual error to log function to keep it always in DBG level. > > 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..3d97c786e 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, 0); > 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, 0); > 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, 0); > break; > } > case DPIF_OP_EXECUTE: > Acked-by: Roi Dayan <roid@mellanox.com>
On Mon, Jan 06, 2020 at 12:33:21PM +0000, Roi Dayan wrote: > > > On 2020-01-06 12:23 PM, 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) > > > > CC: Roi Dayan <roid@mellanox.com> > > Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions") > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> ... > Acked-by: Roi Dayan <roid@mellanox.com> Thanks, applied to master.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index a08c2378a..3d97c786e 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, 0); 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, 0); 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, 0); 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) CC: Roi Dayan <roid@mellanox.com> Fixes: 3cd99886191e ("dpif-netlink: Use dpif logging functions") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- Version 2: * Do not pass actual error to log function to keep it always in DBG level. lib/dpif-netlink.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)