| Message ID | 1470145619-21812-1-git-send-email-maxime.coquelin@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
> This patch sets *typep to an empty string instead of letting it > uninitialized when no QoS configuration is set. > > It fixes the following vswitchd crash when no QoS has been set on vhost- > user interface: > > $> ovs-appctl -t ovs-vswitchd qos/show vhost-user1 > > #0 0x00007efcbadf18d7 in raise () from /lib64/libc.so.6 > #1 0x00007efcbadf353a in abort () from /lib64/libc.so.6 > #2 0x000000000068d5be in ovs_abort_valist at lib/util.c:335 > #3 0x0000000000693d90 in vlog_abort_valist at lib/vlog.c:1204 > #4 0x0000000000693e17 in vlog_abort at lib/vlog.c:1218 > #5 0x000000000068d3ae in ovs_assert_failure at lib/util.c:72 > #6 0x000000000060425c in ds_put_format_valist at lib/dynamic- > string.c:168 > #7 0x00000000006042e7 in ds_put_format at lib/dynamic-string.c:142 > #8 0x00000000005a9e75 in qos_unixctl_show at vswitchd/bridge.c:3185 > #9 0x000000000068cda1 in process_command at lib/unixctl.c:347 > #11 unixctl_server_run at lib/unixctl.c:400 > #12 0x000000000040a3ff in main at vswitchd/ovs-vswitchd.c:113 > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > lib/netdev-dpdk.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > a0d541a..159fe73 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2680,6 +2680,9 @@ netdev_dpdk_get_qos(const struct netdev *netdev, > *typep = dev->qos_conf->ops->qos_name; > error = (dev->qos_conf->ops->qos_get > ? dev->qos_conf->ops->qos_get(netdev, details): 0); > + } else { > + /* No QoS configuration set, return an empty string */ > + *typep = ""; > } > ovs_mutex_unlock(&dev->mutex); > > -- > 2.7.4 Thanks for the Patch Maxime. I tried to recreate the segfault with the steps you've outlined on my own system without the patch but could not. I'm Running Fedora 22 with kernel 4.1.8-200 and gcc 5.3.1. Out of interest what was your test environment? Either way I agree that type should be set to "" when no QoS is not configured. Acked-by: Ian Stokes <ian.stokes@intel.com> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On 08/02/2016 05:19 PM, Stokes, Ian wrote: >> This patch sets *typep to an empty string instead of letting it >> uninitialized when no QoS configuration is set. >> >> It fixes the following vswitchd crash when no QoS has been set on vhost- >> user interface: >> >> $> ovs-appctl -t ovs-vswitchd qos/show vhost-user1 >> >> #0 0x00007efcbadf18d7 in raise () from /lib64/libc.so.6 >> #1 0x00007efcbadf353a in abort () from /lib64/libc.so.6 >> #2 0x000000000068d5be in ovs_abort_valist at lib/util.c:335 >> #3 0x0000000000693d90 in vlog_abort_valist at lib/vlog.c:1204 >> #4 0x0000000000693e17 in vlog_abort at lib/vlog.c:1218 >> #5 0x000000000068d3ae in ovs_assert_failure at lib/util.c:72 >> #6 0x000000000060425c in ds_put_format_valist at lib/dynamic- >> string.c:168 >> #7 0x00000000006042e7 in ds_put_format at lib/dynamic-string.c:142 >> #8 0x00000000005a9e75 in qos_unixctl_show at vswitchd/bridge.c:3185 >> #9 0x000000000068cda1 in process_command at lib/unixctl.c:347 >> #11 unixctl_server_run at lib/unixctl.c:400 >> #12 0x000000000040a3ff in main at vswitchd/ovs-vswitchd.c:113 >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> lib/netdev-dpdk.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >> a0d541a..159fe73 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2680,6 +2680,9 @@ netdev_dpdk_get_qos(const struct netdev *netdev, >> *typep = dev->qos_conf->ops->qos_name; >> error = (dev->qos_conf->ops->qos_get >> ? dev->qos_conf->ops->qos_get(netdev, details): 0); >> + } else { >> + /* No QoS configuration set, return an empty string */ >> + *typep = ""; >> } >> ovs_mutex_unlock(&dev->mutex); >> >> -- >> 2.7.4 > > Thanks for the Patch Maxime. > > I tried to recreate the segfault with the steps you've outlined on my own system without the patch but could not. Maybe you were just lucky? Or actually, not lucky! Indeed, as *typep contains uninitialized value, maybe that in your case, its value was a valid address that pointed to 0? > > I'm Running Fedora 22 with kernel 4.1.8-200 and gcc 5.3.1. Out of interest what was your test environment? Fedora 21, kernel 3.19.3-200 and gcc 4.9.2. > Either way I agree that type should be set to "" when no QoS is not configured. > > Acked-by: Ian Stokes <ian.stokes@intel.com> Thanks! Maxime
Thanks for the fix! I added you name to AUTHORS and applied this to master 2016-08-02 9:52 GMT-07:00 Maxime Coquelin <maxime.coquelin@redhat.com>: > > > On 08/02/2016 05:19 PM, Stokes, Ian wrote: > >> This patch sets *typep to an empty string instead of letting it >>> uninitialized when no QoS configuration is set. >>> >>> It fixes the following vswitchd crash when no QoS has been set on vhost- >>> user interface: >>> >>> $> ovs-appctl -t ovs-vswitchd qos/show vhost-user1 >>> >>> #0 0x00007efcbadf18d7 in raise () from /lib64/libc.so.6 >>> #1 0x00007efcbadf353a in abort () from /lib64/libc.so.6 >>> #2 0x000000000068d5be in ovs_abort_valist at lib/util.c:335 >>> #3 0x0000000000693d90 in vlog_abort_valist at lib/vlog.c:1204 >>> #4 0x0000000000693e17 in vlog_abort at lib/vlog.c:1218 >>> #5 0x000000000068d3ae in ovs_assert_failure at lib/util.c:72 >>> #6 0x000000000060425c in ds_put_format_valist at lib/dynamic- >>> string.c:168 >>> #7 0x00000000006042e7 in ds_put_format at lib/dynamic-string.c:142 >>> #8 0x00000000005a9e75 in qos_unixctl_show at vswitchd/bridge.c:3185 >>> #9 0x000000000068cda1 in process_command at lib/unixctl.c:347 >>> #11 unixctl_server_run at lib/unixctl.c:400 >>> #12 0x000000000040a3ff in main at vswitchd/ovs-vswitchd.c:113 >>> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>> --- >>> lib/netdev-dpdk.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >>> a0d541a..159fe73 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -2680,6 +2680,9 @@ netdev_dpdk_get_qos(const struct netdev *netdev, >>> *typep = dev->qos_conf->ops->qos_name; >>> error = (dev->qos_conf->ops->qos_get >>> ? dev->qos_conf->ops->qos_get(netdev, details): 0); >>> + } else { >>> + /* No QoS configuration set, return an empty string */ >>> + *typep = ""; >>> } >>> ovs_mutex_unlock(&dev->mutex); >>> >>> -- >>> 2.7.4 >>> >> >> Thanks for the Patch Maxime. >> >> I tried to recreate the segfault with the steps you've outlined on my own >> system without the patch but could not. >> > > Maybe you were just lucky? Or actually, not lucky! > Indeed, as *typep contains uninitialized value, maybe that in your case, > its value was a valid address that pointed to 0? > > >> I'm Running Fedora 22 with kernel 4.1.8-200 and gcc 5.3.1. Out of >> interest what was your test environment? >> > > Fedora 21, kernel 3.19.3-200 and gcc 4.9.2. > > Either way I agree that type should be set to "" when no QoS is not >> configured. >> >> Acked-by: Ian Stokes <ian.stokes@intel.com> >> > > Thanks! > Maxime > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index a0d541a..159fe73 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2680,6 +2680,9 @@ netdev_dpdk_get_qos(const struct netdev *netdev, *typep = dev->qos_conf->ops->qos_name; error = (dev->qos_conf->ops->qos_get ? dev->qos_conf->ops->qos_get(netdev, details): 0); + } else { + /* No QoS configuration set, return an empty string */ + *typep = ""; } ovs_mutex_unlock(&dev->mutex);
This patch sets *typep to an empty string instead of letting it uninitialized when no QoS configuration is set. It fixes the following vswitchd crash when no QoS has been set on vhost-user interface: $> ovs-appctl -t ovs-vswitchd qos/show vhost-user1 #0 0x00007efcbadf18d7 in raise () from /lib64/libc.so.6 #1 0x00007efcbadf353a in abort () from /lib64/libc.so.6 #2 0x000000000068d5be in ovs_abort_valist at lib/util.c:335 #3 0x0000000000693d90 in vlog_abort_valist at lib/vlog.c:1204 #4 0x0000000000693e17 in vlog_abort at lib/vlog.c:1218 #5 0x000000000068d3ae in ovs_assert_failure at lib/util.c:72 #6 0x000000000060425c in ds_put_format_valist at lib/dynamic-string.c:168 #7 0x00000000006042e7 in ds_put_format at lib/dynamic-string.c:142 #8 0x00000000005a9e75 in qos_unixctl_show at vswitchd/bridge.c:3185 #9 0x000000000068cda1 in process_command at lib/unixctl.c:347 #11 unixctl_server_run at lib/unixctl.c:400 #12 0x000000000040a3ff in main at vswitchd/ovs-vswitchd.c:113 Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/netdev-dpdk.c | 3 +++ 1 file changed, 3 insertions(+)