Message ID | 20220328020754.1155063-1-jiayu.hu@intel.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Maxime Coquelin |
Headers | show |
Series | vhost: fix null pointer dereference | expand |
Context | Check | Description |
---|---|---|
ci/intel-Testing | success | Testing PASS |
ci/Intel-compilation | success | Compilation OK |
ci/iol-abi-testing | success | Testing PASS |
ci/iol-aarch64-compile-testing | success | Testing PASS |
ci/iol-aarch64-unit-testing | success | Testing PASS |
ci/iol-x86_64-unit-testing | success | Testing PASS |
ci/iol-x86_64-compile-testing | success | Testing PASS |
ci/iol-intel-Performance | success | Performance Testing PASS |
ci/github-robot: build | success | github build: passed |
ci/iol-intel-Functional | success | Functional Testing PASS |
ci/iol-mellanox-Performance | success | Performance Testing PASS |
ci/checkpatch | success | coding style OK |
On Mon, Mar 28, 2022 at 4:08 AM Jiayu Hu <jiayu.hu@intel.com> wrote: > > NULL check for vq->async must be protected by lock. Otherwise, it is > possible that the data plane thread dereferences vq->async with NULL > value, since the control plane thread is freeing vq->async. > > Fixes: ee8024b3d4ad (vhost: move async data in dedicated structure) > Cc: stable@dpdk.org > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com> > --- > lib/vhost/vhost.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c > index bc88148347..7f60c2824f 100644 > --- a/lib/vhost/vhost.c > +++ b/lib/vhost/vhost.c > @@ -1887,9 +1887,6 @@ rte_vhost_async_get_inflight(int vid, uint16_t queue_id) > if (vq == NULL) > return ret; > > - if (!vq->async) > - return ret; > - > if (!rte_spinlock_trylock(&vq->access_lock)) { > VHOST_LOG_CONFIG(DEBUG, > "(%s) failed to check in-flight packets. virtqueue busy.\n", > @@ -1897,6 +1894,9 @@ rte_vhost_async_get_inflight(int vid, uint16_t queue_id) > return ret; > } > > + if (!vq->async) > + return ret; Lock is still taken at this point. FYI, I'll post a series to instrument locks in vhost, soon. > + > ret = vq->async->pkts_inflight_n; > rte_spinlock_unlock(&vq->access_lock); >
HI Jiayu, On 3/28/22 09:04, David Marchand wrote: > On Mon, Mar 28, 2022 at 4:08 AM Jiayu Hu <jiayu.hu@intel.com> wrote: >> >> NULL check for vq->async must be protected by lock. Otherwise, it is >> possible that the data plane thread dereferences vq->async with NULL >> value, since the control plane thread is freeing vq->async. >> >> Fixes: ee8024b3d4ad (vhost: move async data in dedicated structure) >> Cc: stable@dpdk.org >> >> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com> >> --- >> lib/vhost/vhost.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c >> index bc88148347..7f60c2824f 100644 >> --- a/lib/vhost/vhost.c >> +++ b/lib/vhost/vhost.c >> @@ -1887,9 +1887,6 @@ rte_vhost_async_get_inflight(int vid, uint16_t queue_id) >> if (vq == NULL) >> return ret; >> >> - if (!vq->async) >> - return ret; >> - >> if (!rte_spinlock_trylock(&vq->access_lock)) { >> VHOST_LOG_CONFIG(DEBUG, >> "(%s) failed to check in-flight packets. virtqueue busy.\n", >> @@ -1897,6 +1894,9 @@ rte_vhost_async_get_inflight(int vid, uint16_t queue_id) >> return ret; >> } >> >> + if (!vq->async) >> + return ret; > > Lock is still taken at this point. > > FYI, I'll post a series to instrument locks in vhost, soon. Could you please send a v2 which does not return with the lock taken? >> + >> ret = vq->async->pkts_inflight_n; >> rte_spinlock_unlock(&vq->access_lock); >> > > Thanks, Maxime
Hi Maxime, This issue has fixed in https://patches.dpdk.org/project/dpdk/patch/20220411110013.18624-4-david.marchand@redhat.com/. Thanks, Jiayu > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Wednesday, June 1, 2022 3:58 PM > To: Hu, Jiayu <jiayu.hu@intel.com> > Cc: dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org>; David Marchand > <david.marchand@redhat.com> > Subject: Re: [PATCH] vhost: fix null pointer dereference > > HI Jiayu, > > On 3/28/22 09:04, David Marchand wrote: > > On Mon, Mar 28, 2022 at 4:08 AM Jiayu Hu <jiayu.hu@intel.com> wrote: > >> > >> NULL check for vq->async must be protected by lock. Otherwise, it is > >> possible that the data plane thread dereferences vq->async with NULL > >> value, since the control plane thread is freeing vq->async. > >> > >> Fixes: ee8024b3d4ad (vhost: move async data in dedicated structure) > >> Cc: stable@dpdk.org > >> > >> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com> > >> --- > >> lib/vhost/vhost.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index > >> bc88148347..7f60c2824f 100644 > >> --- a/lib/vhost/vhost.c > >> +++ b/lib/vhost/vhost.c > >> @@ -1887,9 +1887,6 @@ rte_vhost_async_get_inflight(int vid, uint16_t > queue_id) > >> if (vq == NULL) > >> return ret; > >> > >> - if (!vq->async) > >> - return ret; > >> - > >> if (!rte_spinlock_trylock(&vq->access_lock)) { > >> VHOST_LOG_CONFIG(DEBUG, > >> "(%s) failed to check in-flight packets. > >> virtqueue busy.\n", @@ -1897,6 +1894,9 @@ > rte_vhost_async_get_inflight(int vid, uint16_t queue_id) > >> return ret; > >> } > >> > >> + if (!vq->async) > >> + return ret; > > > > Lock is still taken at this point. > > > > FYI, I'll post a series to instrument locks in vhost, soon. > > Could you please send a v2 which does not return with the lock taken? > > >> + > >> ret = vq->async->pkts_inflight_n; > >> rte_spinlock_unlock(&vq->access_lock); > >> > > > > > > Thanks, > Maxime
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index bc88148347..7f60c2824f 100644 --- a/lib/vhost/vhost.c +++ b/lib/vhost/vhost.c @@ -1887,9 +1887,6 @@ rte_vhost_async_get_inflight(int vid, uint16_t queue_id) if (vq == NULL) return ret; - if (!vq->async) - return ret; - if (!rte_spinlock_trylock(&vq->access_lock)) { VHOST_LOG_CONFIG(DEBUG, "(%s) failed to check in-flight packets. virtqueue busy.\n", @@ -1897,6 +1894,9 @@ rte_vhost_async_get_inflight(int vid, uint16_t queue_id) return ret; } + if (!vq->async) + return ret; + ret = vq->async->pkts_inflight_n; rte_spinlock_unlock(&vq->access_lock);
NULL check for vq->async must be protected by lock. Otherwise, it is possible that the data plane thread dereferences vq->async with NULL value, since the control plane thread is freeing vq->async. Fixes: ee8024b3d4ad (vhost: move async data in dedicated structure) Cc: stable@dpdk.org Signed-off-by: Jiayu Hu <jiayu.hu@intel.com> --- lib/vhost/vhost.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)