| Message ID | 20240724180732.377073-1-mkp@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] netdev-dpdk: Fix race condition in mempool information dump. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 7/24/24 20:07, Mike Pattrick wrote: > Currently it is possible to call netdev-dpdk/get-mempool-info before a > mempool as been created. This can happen because a device is added to > the netdev_shash before a mempool is allocated for it, which results in > a segmentation fault. > > Now we check for a NULL value before attempting to dereference it. > > Fixes: be4817331071 ("netdev-dpdk: Add debug appctl to get mempool information.") > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > v2: Added a test for this issue. > --- > lib/netdev-dpdk.c | 12 ++++++++---- > tests/system-dpdk.at | 5 +++++ > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 02cef6e45..6afa6da88 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -4664,10 +4664,14 @@ netdev_dpdk_get_mempool_info(struct unixctl_conn *conn, > ovs_mutex_lock(&dev->mutex); > ovs_mutex_lock(&dpdk_mp_mutex); > > - rte_mempool_dump(stream, dev->dpdk_mp->mp); > - fprintf(stream, " count: avail (%u), in use (%u)\n", > - rte_mempool_avail_count(dev->dpdk_mp->mp), > - rte_mempool_in_use_count(dev->dpdk_mp->mp)); > + if (dev->dpdk_mp) { > + rte_mempool_dump(stream, dev->dpdk_mp->mp); > + fprintf(stream, " count: avail (%u), in use (%u)\n", > + rte_mempool_avail_count(dev->dpdk_mp->mp), > + rte_mempool_in_use_count(dev->dpdk_mp->mp)); > + } else { > + fprintf(stream, " Not allocated\n"); Hi, Mike. Do we actually need these spaces at the beginning? The other fprintf has them because this is the way rte_mempool_dump() formats mempool statistics and we're trying to blend in, but it seems unnecessary for the no-mempool case. Best regards, Ilya Maixmets. > + } > > ovs_mutex_unlock(&dpdk_mp_mutex); > ovs_mutex_unlock(&dev->mutex); > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at > index 1c97bf777..ecf4c7496 100644 > --- a/tests/system-dpdk.at > +++ b/tests/system-dpdk.at > @@ -88,6 +88,11 @@ ADD_VHOST_USER_CLIENT_PORT([br10], [dpdkvhostuserclient0], [$OVS_RUNDIR/dpdkvhos > AT_CHECK([ovs-vsctl show], [], [stdout]) > sleep 2 > > +dnl Check that no mempool was allocated. > +AT_CHECK([ovs-appctl netdev-dpdk/get-mempool-info dpdkvhostuserclient0], [0], [dnl > + Not allocated > +]) > + > dnl Clean up > AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) > OVS_DPDK_STOP_VSWITCHD(["dnl
On Tue, Aug 6, 2024 at 9:29 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 7/24/24 20:07, Mike Pattrick wrote: > > Currently it is possible to call netdev-dpdk/get-mempool-info before a > > mempool as been created. This can happen because a device is added to > > the netdev_shash before a mempool is allocated for it, which results in > > a segmentation fault. > > > > Now we check for a NULL value before attempting to dereference it. > > > > Fixes: be4817331071 ("netdev-dpdk: Add debug appctl to get mempool information.") > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > --- > > v2: Added a test for this issue. > > --- > > lib/netdev-dpdk.c | 12 ++++++++---- > > tests/system-dpdk.at | 5 +++++ > > 2 files changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index 02cef6e45..6afa6da88 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -4664,10 +4664,14 @@ netdev_dpdk_get_mempool_info(struct unixctl_conn *conn, > > ovs_mutex_lock(&dev->mutex); > > ovs_mutex_lock(&dpdk_mp_mutex); > > > > - rte_mempool_dump(stream, dev->dpdk_mp->mp); > > - fprintf(stream, " count: avail (%u), in use (%u)\n", > > - rte_mempool_avail_count(dev->dpdk_mp->mp), > > - rte_mempool_in_use_count(dev->dpdk_mp->mp)); > > + if (dev->dpdk_mp) { > > + rte_mempool_dump(stream, dev->dpdk_mp->mp); > > + fprintf(stream, " count: avail (%u), in use (%u)\n", > > + rte_mempool_avail_count(dev->dpdk_mp->mp), > > + rte_mempool_in_use_count(dev->dpdk_mp->mp)); > > + } else { > > + fprintf(stream, " Not allocated\n"); > > Hi, Mike. Do we actually need these spaces at the beginning? > The other fprintf has them because this is the way rte_mempool_dump() > formats mempool statistics and we're trying to blend in, but it seems > unnecessary for the no-mempool case. I don't know what is best here. The spaces are similar to the current formatting, but maybe nothing should be printed at all instead? -M > > Best regards, Ilya Maixmets. > > > + } > > > > ovs_mutex_unlock(&dpdk_mp_mutex); > > ovs_mutex_unlock(&dev->mutex); > > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at > > index 1c97bf777..ecf4c7496 100644 > > --- a/tests/system-dpdk.at > > +++ b/tests/system-dpdk.at > > @@ -88,6 +88,11 @@ ADD_VHOST_USER_CLIENT_PORT([br10], [dpdkvhostuserclient0], [$OVS_RUNDIR/dpdkvhos > > AT_CHECK([ovs-vsctl show], [], [stdout]) > > sleep 2 > > > > +dnl Check that no mempool was allocated. > > +AT_CHECK([ovs-appctl netdev-dpdk/get-mempool-info dpdkvhostuserclient0], [0], [dnl > > + Not allocated > > +]) > > + > > dnl Clean up > > AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) > > OVS_DPDK_STOP_VSWITCHD(["dnl >
On 8/6/24 18:55, Mike Pattrick wrote: > On Tue, Aug 6, 2024 at 9:29 AM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 7/24/24 20:07, Mike Pattrick wrote: >>> Currently it is possible to call netdev-dpdk/get-mempool-info before a >>> mempool as been created. This can happen because a device is added to >>> the netdev_shash before a mempool is allocated for it, which results in >>> a segmentation fault. >>> >>> Now we check for a NULL value before attempting to dereference it. >>> >>> Fixes: be4817331071 ("netdev-dpdk: Add debug appctl to get mempool information.") >>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >>> --- >>> v2: Added a test for this issue. >>> --- >>> lib/netdev-dpdk.c | 12 ++++++++---- >>> tests/system-dpdk.at | 5 +++++ >>> 2 files changed, 13 insertions(+), 4 deletions(-) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index 02cef6e45..6afa6da88 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -4664,10 +4664,14 @@ netdev_dpdk_get_mempool_info(struct unixctl_conn *conn, >>> ovs_mutex_lock(&dev->mutex); >>> ovs_mutex_lock(&dpdk_mp_mutex); >>> >>> - rte_mempool_dump(stream, dev->dpdk_mp->mp); >>> - fprintf(stream, " count: avail (%u), in use (%u)\n", >>> - rte_mempool_avail_count(dev->dpdk_mp->mp), >>> - rte_mempool_in_use_count(dev->dpdk_mp->mp)); >>> + if (dev->dpdk_mp) { >>> + rte_mempool_dump(stream, dev->dpdk_mp->mp); >>> + fprintf(stream, " count: avail (%u), in use (%u)\n", >>> + rte_mempool_avail_count(dev->dpdk_mp->mp), >>> + rte_mempool_in_use_count(dev->dpdk_mp->mp)); >>> + } else { >>> + fprintf(stream, " Not allocated\n"); >> >> Hi, Mike. Do we actually need these spaces at the beginning? >> The other fprintf has them because this is the way rte_mempool_dump() >> formats mempool statistics and we're trying to blend in, but it seems >> unnecessary for the no-mempool case. > > I don't know what is best here. The spaces are similar to the current > formatting, The actual mempool dump starts from the beginning of the line without extra spaces. So, I'd not say it's similar. > but maybe nothing should be printed at all instead? One other option might be to return the same message, but as an error, i.e. with unixctl_command_reply_error(). > > -M > >> >> Best regards, Ilya Maixmets. >> >>> + } >>> >>> ovs_mutex_unlock(&dpdk_mp_mutex); >>> ovs_mutex_unlock(&dev->mutex); >>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at >>> index 1c97bf777..ecf4c7496 100644 >>> --- a/tests/system-dpdk.at >>> +++ b/tests/system-dpdk.at >>> @@ -88,6 +88,11 @@ ADD_VHOST_USER_CLIENT_PORT([br10], [dpdkvhostuserclient0], [$OVS_RUNDIR/dpdkvhos >>> AT_CHECK([ovs-vsctl show], [], [stdout]) >>> sleep 2 >>> >>> +dnl Check that no mempool was allocated. >>> +AT_CHECK([ovs-appctl netdev-dpdk/get-mempool-info dpdkvhostuserclient0], [0], [dnl >>> + Not allocated >>> +]) >>> + >>> dnl Clean up >>> AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) >>> OVS_DPDK_STOP_VSWITCHD(["dnl >> >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 02cef6e45..6afa6da88 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -4664,10 +4664,14 @@ netdev_dpdk_get_mempool_info(struct unixctl_conn *conn, ovs_mutex_lock(&dev->mutex); ovs_mutex_lock(&dpdk_mp_mutex); - rte_mempool_dump(stream, dev->dpdk_mp->mp); - fprintf(stream, " count: avail (%u), in use (%u)\n", - rte_mempool_avail_count(dev->dpdk_mp->mp), - rte_mempool_in_use_count(dev->dpdk_mp->mp)); + if (dev->dpdk_mp) { + rte_mempool_dump(stream, dev->dpdk_mp->mp); + fprintf(stream, " count: avail (%u), in use (%u)\n", + rte_mempool_avail_count(dev->dpdk_mp->mp), + rte_mempool_in_use_count(dev->dpdk_mp->mp)); + } else { + fprintf(stream, " Not allocated\n"); + } ovs_mutex_unlock(&dpdk_mp_mutex); ovs_mutex_unlock(&dev->mutex); diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index 1c97bf777..ecf4c7496 100644 --- a/tests/system-dpdk.at +++ b/tests/system-dpdk.at @@ -88,6 +88,11 @@ ADD_VHOST_USER_CLIENT_PORT([br10], [dpdkvhostuserclient0], [$OVS_RUNDIR/dpdkvhos AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2 +dnl Check that no mempool was allocated. +AT_CHECK([ovs-appctl netdev-dpdk/get-mempool-info dpdkvhostuserclient0], [0], [dnl + Not allocated +]) + dnl Clean up AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr]) OVS_DPDK_STOP_VSWITCHD(["dnl
Currently it is possible to call netdev-dpdk/get-mempool-info before a mempool as been created. This can happen because a device is added to the netdev_shash before a mempool is allocated for it, which results in a segmentation fault. Now we check for a NULL value before attempting to dereference it. Fixes: be4817331071 ("netdev-dpdk: Add debug appctl to get mempool information.") Signed-off-by: Mike Pattrick <mkp@redhat.com> --- v2: Added a test for this issue. --- lib/netdev-dpdk.c | 12 ++++++++---- tests/system-dpdk.at | 5 +++++ 2 files changed, 13 insertions(+), 4 deletions(-)