diff mbox series

[ovs-dev,v2] netdev-dpdk: Fix race condition in mempool information dump.

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

Checks

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

Commit Message

Mike Pattrick July 24, 2024, 6:07 p.m. UTC
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(-)

Comments

Ilya Maximets Aug. 6, 2024, 1:28 p.m. UTC | #1
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
Mike Pattrick Aug. 6, 2024, 4:55 p.m. UTC | #2
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
>
Ilya Maximets Aug. 6, 2024, 9:20 p.m. UTC | #3
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 mbox series

Patch

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