| Message ID | 20200623164111.1939144-1-ferruh.yigit@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Ferruh Yigit |
| Headers | show |
| Series | ethdev: verify reserved HW ring | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/iol-broadcom-Performance | success | Performance Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/iol-nxp-Performance | success | Performance Testing PASS |
| ci/travis-robot | success | Travis build: passed |
| ci/Intel-compilation | success | Compilation OK |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/iol-testing | success | Testing PASS |
On 6/23/20 7:41 PM, Ferruh Yigit wrote: > Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based > on name match, but other requested attributes are discarded. > This may cause driver using a memzone with wrong size or alignment. > > Verify size, alignment and socket_id for matched memzone, and do not use > memzone if any one of the attributes are not justified. > > Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> Few minor notes below, other than that: Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com> > --- > Cc: Anatoly Burakov <anatoly.burakov@intel.com> > --- > lib/librte_ethdev/rte_ethdev.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 8e10a6fc3..0fe84aadc 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -4202,8 +4202,18 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, > } > > mz = rte_memzone_lookup(z_name); > - if (mz) > + if (mz) { > + if ((socket_id != SOCKET_ID_ANY && socket_id != mz->socket_id) || > + size > mz->len || > + (uintptr_t)mz->addr & (align - 1)) { IMHO, it would be a bit more readable if the result compared with 0. Up to you. > + RTE_ETHDEV_LOG(ERR, > + "memzone %s does not justify the requested attributes\n", > + mz->name); > + return NULL; Initially I thought it is a bad behaviour and memzone should be freed and reallocated, but it is even worse (implicit free). May be it is better to highlight it in the description. > + } > + > return mz; > + } > > return rte_memzone_reserve_aligned(z_name, size, socket_id, > RTE_MEMZONE_IOVA_CONTIG, align); >
On 6/23/2020 5:58 PM, Andrew Rybchenko wrote: > On 6/23/20 7:41 PM, Ferruh Yigit wrote: >> Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based >> on name match, but other requested attributes are discarded. >> This may cause driver using a memzone with wrong size or alignment. >> >> Verify size, alignment and socket_id for matched memzone, and do not use >> memzone if any one of the attributes are not justified. >> >> Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> > > Few minor notes below, other than that: > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com> > >> --- >> Cc: Anatoly Burakov <anatoly.burakov@intel.com> >> --- >> lib/librte_ethdev/rte_ethdev.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index 8e10a6fc3..0fe84aadc 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -4202,8 +4202,18 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, >> } >> >> mz = rte_memzone_lookup(z_name); >> - if (mz) >> + if (mz) { >> + if ((socket_id != SOCKET_ID_ANY && socket_id != mz->socket_id) || >> + size > mz->len || >> + (uintptr_t)mz->addr & (align - 1)) { > > IMHO, it would be a bit more readable if the result compared > with 0. Up to you. Both OK, I can update. > >> + RTE_ETHDEV_LOG(ERR, >> + "memzone %s does not justify the requested attributes\n", >> + mz->name); >> + return NULL; > > Initially I thought it is a bad behaviour and memzone should > be freed and reallocated, but it is even worse (implicit free). +1 > May be it is better to highlight it in the description. ok > >> + } >> + >> return mz; >> + } >> >> return rte_memzone_reserve_aligned(z_name, size, socket_id, >> RTE_MEMZONE_IOVA_CONTIG, align); >> >
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 8e10a6fc3..0fe84aadc 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -4202,8 +4202,18 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name, } mz = rte_memzone_lookup(z_name); - if (mz) + if (mz) { + if ((socket_id != SOCKET_ID_ANY && socket_id != mz->socket_id) || + size > mz->len || + (uintptr_t)mz->addr & (align - 1)) { + RTE_ETHDEV_LOG(ERR, + "memzone %s does not justify the requested attributes\n", + mz->name); + return NULL; + } + return mz; + } return rte_memzone_reserve_aligned(z_name, size, socket_id, RTE_MEMZONE_IOVA_CONTIG, align);
Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based on name match, but other requested attributes are discarded. This may cause driver using a memzone with wrong size or alignment. Verify size, alignment and socket_id for matched memzone, and do not use memzone if any one of the attributes are not justified. Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> --- Cc: Anatoly Burakov <anatoly.burakov@intel.com> --- lib/librte_ethdev/rte_ethdev.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)