| Message ID | 20191001125315.6191-2-ktraynor@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Marchand |
| Headers | show |
| Series | Coverity fixes and other cleanups | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/iol-compilation | success | Compile Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
> -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Tuesday 1 October 2019 13:53 > To: dev@dpdk.org > Cc: Kevin Traynor <ktraynor@redhat.com>; Ferriter, Cian > <cian.ferriter@intel.com>; stable@dpdk.org > Subject: [PATCH 1/9] net/pcap: fix argument checks > > Previously rx/tx_queues were passed into eth_from_pcaps_common() as > ptrs and were checked for being NULL. > > In commit da6ba28f0540 ("net/pcap: use a struct to pass user options") that > changed to pass in a ptr to a pmd_devargs_all which contains the > rx/tx_queues. > > The parameter checking was not updated as part of that commit and coverity > caught that there was still a check if rx/tx_queues were NULL, apparently > after they had been dereferenced. > > Fix that by checking the pmd_devargs_all ptr and removing the NULL checks > on rx/tx_queues. > > 1231 struct pmd_devargs *rx_queues = &devargs_all->rx_queues; > 1232 struct pmd_devargs *tx_queues = &devargs_all->tx_queues; > 1233 const unsigned int nb_rx_queues = rx_queues->num_of_queue; > deref_ptr: Directly dereferencing pointer tx_queues. > 1234 const unsigned int nb_tx_queues = tx_queues->num_of_queue; > 1235 unsigned int i; > 1236 > 1237 /* do some parameter checking */ > CID 345004: Dereference before null check (REVERSE_INULL) > [select issue] > 1238 if (rx_queues == NULL && nb_rx_queues > 0) > 1239 return -1; > CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL) > check_after_deref: Null-checking tx_queues suggests that it may be > null, but it has already been dereferenced on all paths leading to > the check. > 1240 if (tx_queues == NULL && nb_tx_queues > 0) > 1241 return -1; > > Coverity issue: 345029 > Coverity issue: 345044 > Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options") > Cc: cian.ferriter@intel.com > Cc: stable@dpdk.org > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> Acked-by: Cian Ferriter <cian.ferriter@intel.com> > --- > drivers/net/pcap/rte_eth_pcap.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/pcap/rte_eth_pcap.c > b/drivers/net/pcap/rte_eth_pcap.c index 5489010b6..7cf00306e 100644 > --- a/drivers/net/pcap/rte_eth_pcap.c > +++ b/drivers/net/pcap/rte_eth_pcap.c > @@ -1229,16 +1229,19 @@ eth_from_pcaps_common(struct > rte_vdev_device *vdev, { > struct pmd_process_private *pp; > - struct pmd_devargs *rx_queues = &devargs_all->rx_queues; > - struct pmd_devargs *tx_queues = &devargs_all->tx_queues; > - const unsigned int nb_rx_queues = rx_queues->num_of_queue; > - const unsigned int nb_tx_queues = tx_queues->num_of_queue; > + struct pmd_devargs *rx_queues; > + struct pmd_devargs *tx_queues; > + unsigned int nb_rx_queues; > + unsigned int nb_tx_queues; > unsigned int i; > > - /* do some parameter checking */ > - if (rx_queues == NULL && nb_rx_queues > 0) > - return -1; > - if (tx_queues == NULL && nb_tx_queues > 0) > + if (devargs_all == NULL) > return -1; > > + rx_queues = &devargs_all->rx_queues; > + tx_queues = &devargs_all->tx_queues; > + > + nb_rx_queues = rx_queues->num_of_queue; > + nb_tx_queues = tx_queues->num_of_queue; > + > if (pmd_init_internals(vdev, nb_rx_queues, nb_tx_queues, > internals, > eth_dev) < 0) > -- > 2.21.0
On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com> wrote: > > Previously rx/tx_queues were passed into eth_from_pcaps_common() > as ptrs and were checked for being NULL. > > In commit da6ba28f0540 ("net/pcap: use a struct to pass user options") > that changed to pass in a ptr to a pmd_devargs_all which contains > the rx/tx_queues. > > The parameter checking was not updated as part of that commit and > coverity caught that there was still a check if rx/tx_queues were > NULL, apparently after they had been dereferenced. > > Fix that by checking the pmd_devargs_all ptr and removing the NULL > checks on rx/tx_queues. > > 1231 struct pmd_devargs *rx_queues = &devargs_all->rx_queues; > 1232 struct pmd_devargs *tx_queues = &devargs_all->tx_queues; > 1233 const unsigned int nb_rx_queues = rx_queues->num_of_queue; > deref_ptr: Directly dereferencing pointer tx_queues. > 1234 const unsigned int nb_tx_queues = tx_queues->num_of_queue; > 1235 unsigned int i; > 1236 > 1237 /* do some parameter checking */ > CID 345004: Dereference before null check (REVERSE_INULL) > [select issue] > 1238 if (rx_queues == NULL && nb_rx_queues > 0) > 1239 return -1; > CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL) > check_after_deref: Null-checking tx_queues suggests that it may be > null, but it has already been dereferenced on all paths leading to > the check. > 1240 if (tx_queues == NULL && nb_tx_queues > 0) > 1241 return -1; > > Coverity issue: 345029 > Coverity issue: 345044 > Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options") > Cc: cian.ferriter@intel.com > Cc: stable@dpdk.org > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> This patch hides the coverity warning. But can't we just remove those checks? Iiuc, the checks on NULL pointers are unnecessary since https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebef7a3e60b.
On 30/10/2019 07:52, David Marchand wrote: > On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com> wrote: >> >> Previously rx/tx_queues were passed into eth_from_pcaps_common() >> as ptrs and were checked for being NULL. >> >> In commit da6ba28f0540 ("net/pcap: use a struct to pass user options") >> that changed to pass in a ptr to a pmd_devargs_all which contains >> the rx/tx_queues. >> >> The parameter checking was not updated as part of that commit and >> coverity caught that there was still a check if rx/tx_queues were >> NULL, apparently after they had been dereferenced. >> >> Fix that by checking the pmd_devargs_all ptr and removing the NULL >> checks on rx/tx_queues. >> >> 1231 struct pmd_devargs *rx_queues = &devargs_all->rx_queues; >> 1232 struct pmd_devargs *tx_queues = &devargs_all->tx_queues; >> 1233 const unsigned int nb_rx_queues = rx_queues->num_of_queue; >> deref_ptr: Directly dereferencing pointer tx_queues. >> 1234 const unsigned int nb_tx_queues = tx_queues->num_of_queue; >> 1235 unsigned int i; >> 1236 >> 1237 /* do some parameter checking */ >> CID 345004: Dereference before null check (REVERSE_INULL) >> [select issue] >> 1238 if (rx_queues == NULL && nb_rx_queues > 0) >> 1239 return -1; >> CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL) >> check_after_deref: Null-checking tx_queues suggests that it may be >> null, but it has already been dereferenced on all paths leading to >> the check. >> 1240 if (tx_queues == NULL && nb_tx_queues > 0) >> 1241 return -1; >> >> Coverity issue: 345029 >> Coverity issue: 345044 >> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options") >> Cc: cian.ferriter@intel.com >> Cc: stable@dpdk.org >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > > This patch hides the coverity warning. > But can't we just remove those checks? > > Iiuc, the checks on NULL pointers are unnecessary since > https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebef7a3e60b. > > Aside from the Coverity complaint about rxq/txq NULL check after use, the checks didn't seem to make sense anymore as rxq/txq are part of devargs_all struct now, so they were removed. I added the NULL check on devargs_all to avoid a NULL deference while getting them instead. The check isn't doing any harm, but looking at the current paths to this fn. can see devargs_all would not be NULL, so I'm ok to remove it too. Cian/Ferruh, wdyt?
> -----Original Message----- > From: Kevin Traynor <ktraynor@redhat.com> > Sent: Tuesday 5 November 2019 16:41 > To: David Marchand <david.marchand@redhat.com> > Cc: dev <dev@dpdk.org>; Ferriter, Cian <cian.ferriter@intel.com>; dpdk > stable <stable@dpdk.org>; Yigit, Ferruh <ferruh.yigit@intel.com> > Subject: Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks > > On 30/10/2019 07:52, David Marchand wrote: > > On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com> > wrote: > >> > >> Previously rx/tx_queues were passed into eth_from_pcaps_common() as > >> ptrs and were checked for being NULL. > >> > >> In commit da6ba28f0540 ("net/pcap: use a struct to pass user > >> options") that changed to pass in a ptr to a pmd_devargs_all which > >> contains the rx/tx_queues. > >> > >> The parameter checking was not updated as part of that commit and > >> coverity caught that there was still a check if rx/tx_queues were > >> NULL, apparently after they had been dereferenced. > >> > >> Fix that by checking the pmd_devargs_all ptr and removing the NULL > >> checks on rx/tx_queues. > >> > >> 1231 struct pmd_devargs *rx_queues = &devargs_all->rx_queues; > >> 1232 struct pmd_devargs *tx_queues = &devargs_all->tx_queues; > >> 1233 const unsigned int nb_rx_queues = rx_queues->num_of_queue; > >> deref_ptr: Directly dereferencing pointer tx_queues. > >> 1234 const unsigned int nb_tx_queues = tx_queues->num_of_queue; > >> 1235 unsigned int i; > >> 1236 > >> 1237 /* do some parameter checking */ > >> CID 345004: Dereference before null check (REVERSE_INULL) > >> [select issue] > >> 1238 if (rx_queues == NULL && nb_rx_queues > 0) > >> 1239 return -1; > >> CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL) > >> check_after_deref: Null-checking tx_queues suggests that it may be > >> null, but it has already been dereferenced on all paths leading to > >> the check. > >> 1240 if (tx_queues == NULL && nb_tx_queues > 0) > >> 1241 return -1; > >> > >> Coverity issue: 345029 > >> Coverity issue: 345044 > >> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options") > >> Cc: cian.ferriter@intel.com > >> Cc: stable@dpdk.org > >> > >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > > > > This patch hides the coverity warning. > > But can't we just remove those checks? > > > > Iiuc, the checks on NULL pointers are unnecessary since > > > https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebe > f7a3e60b. > > > > > > Aside from the Coverity complaint about rxq/txq NULL check after use, the > checks didn't seem to make sense anymore as rxq/txq are part of devargs_all > struct now, so they were removed. > > I added the NULL check on devargs_all to avoid a NULL deference while > getting them instead. The check isn't doing any harm, but looking at the > current paths to this fn. can see devargs_all would not be NULL, so I'm ok to > remove it too. Cian/Ferruh, wdyt? I'm OK to remove this. Looks like it's no longer necessary. Good find David!
On 05/11/2019 17:10, Ferriter, Cian wrote: > > >> -----Original Message----- >> From: Kevin Traynor <ktraynor@redhat.com> >> Sent: Tuesday 5 November 2019 16:41 >> To: David Marchand <david.marchand@redhat.com> >> Cc: dev <dev@dpdk.org>; Ferriter, Cian <cian.ferriter@intel.com>; dpdk >> stable <stable@dpdk.org>; Yigit, Ferruh <ferruh.yigit@intel.com> >> Subject: Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks >> >> On 30/10/2019 07:52, David Marchand wrote: >>> On Tue, Oct 1, 2019 at 2:54 PM Kevin Traynor <ktraynor@redhat.com> >> wrote: >>>> >>>> Previously rx/tx_queues were passed into eth_from_pcaps_common() as >>>> ptrs and were checked for being NULL. >>>> >>>> In commit da6ba28f0540 ("net/pcap: use a struct to pass user >>>> options") that changed to pass in a ptr to a pmd_devargs_all which >>>> contains the rx/tx_queues. >>>> >>>> The parameter checking was not updated as part of that commit and >>>> coverity caught that there was still a check if rx/tx_queues were >>>> NULL, apparently after they had been dereferenced. >>>> >>>> Fix that by checking the pmd_devargs_all ptr and removing the NULL >>>> checks on rx/tx_queues. >>>> >>>> 1231 struct pmd_devargs *rx_queues = &devargs_all->rx_queues; >>>> 1232 struct pmd_devargs *tx_queues = &devargs_all->tx_queues; >>>> 1233 const unsigned int nb_rx_queues = rx_queues->num_of_queue; >>>> deref_ptr: Directly dereferencing pointer tx_queues. >>>> 1234 const unsigned int nb_tx_queues = tx_queues->num_of_queue; >>>> 1235 unsigned int i; >>>> 1236 >>>> 1237 /* do some parameter checking */ >>>> CID 345004: Dereference before null check (REVERSE_INULL) >>>> [select issue] >>>> 1238 if (rx_queues == NULL && nb_rx_queues > 0) >>>> 1239 return -1; >>>> CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL) >>>> check_after_deref: Null-checking tx_queues suggests that it may be >>>> null, but it has already been dereferenced on all paths leading to >>>> the check. >>>> 1240 if (tx_queues == NULL && nb_tx_queues > 0) >>>> 1241 return -1; >>>> >>>> Coverity issue: 345029 >>>> Coverity issue: 345044 >>>> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options") >>>> Cc: cian.ferriter@intel.com >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >>> >>> This patch hides the coverity warning. >>> But can't we just remove those checks? >>> >>> Iiuc, the checks on NULL pointers are unnecessary since >>> >> https://git.dpdk.org/dpdk/commit/?id=a0fce1193ce13a50c00624cb36605ebe >> f7a3e60b. >>> >>> >> >> Aside from the Coverity complaint about rxq/txq NULL check after use, the >> checks didn't seem to make sense anymore as rxq/txq are part of devargs_all >> struct now, so they were removed. >> >> I added the NULL check on devargs_all to avoid a NULL deference while >> getting them instead. The check isn't doing any harm, but looking at the >> current paths to this fn. can see devargs_all would not be NULL, so I'm ok to >> remove it too. Cian/Ferruh, wdyt? > > I'm OK to remove this. Looks like it's no longer necessary. Good find David! > Thanks Cian, I kept your Ack for v2.
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c index 5489010b6..7cf00306e 100644 --- a/drivers/net/pcap/rte_eth_pcap.c +++ b/drivers/net/pcap/rte_eth_pcap.c @@ -1229,16 +1229,19 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev, { struct pmd_process_private *pp; - struct pmd_devargs *rx_queues = &devargs_all->rx_queues; - struct pmd_devargs *tx_queues = &devargs_all->tx_queues; - const unsigned int nb_rx_queues = rx_queues->num_of_queue; - const unsigned int nb_tx_queues = tx_queues->num_of_queue; + struct pmd_devargs *rx_queues; + struct pmd_devargs *tx_queues; + unsigned int nb_rx_queues; + unsigned int nb_tx_queues; unsigned int i; - /* do some parameter checking */ - if (rx_queues == NULL && nb_rx_queues > 0) - return -1; - if (tx_queues == NULL && nb_tx_queues > 0) + if (devargs_all == NULL) return -1; + rx_queues = &devargs_all->rx_queues; + tx_queues = &devargs_all->tx_queues; + + nb_rx_queues = rx_queues->num_of_queue; + nb_tx_queues = tx_queues->num_of_queue; + if (pmd_init_internals(vdev, nb_rx_queues, nb_tx_queues, internals, eth_dev) < 0)
Previously rx/tx_queues were passed into eth_from_pcaps_common() as ptrs and were checked for being NULL. In commit da6ba28f0540 ("net/pcap: use a struct to pass user options") that changed to pass in a ptr to a pmd_devargs_all which contains the rx/tx_queues. The parameter checking was not updated as part of that commit and coverity caught that there was still a check if rx/tx_queues were NULL, apparently after they had been dereferenced. Fix that by checking the pmd_devargs_all ptr and removing the NULL checks on rx/tx_queues. 1231 struct pmd_devargs *rx_queues = &devargs_all->rx_queues; 1232 struct pmd_devargs *tx_queues = &devargs_all->tx_queues; 1233 const unsigned int nb_rx_queues = rx_queues->num_of_queue; deref_ptr: Directly dereferencing pointer tx_queues. 1234 const unsigned int nb_tx_queues = tx_queues->num_of_queue; 1235 unsigned int i; 1236 1237 /* do some parameter checking */ CID 345004: Dereference before null check (REVERSE_INULL) [select issue] 1238 if (rx_queues == NULL && nb_rx_queues > 0) 1239 return -1; CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL) check_after_deref: Null-checking tx_queues suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 1240 if (tx_queues == NULL && nb_tx_queues > 0) 1241 return -1; Coverity issue: 345029 Coverity issue: 345044 Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options") Cc: cian.ferriter@intel.com Cc: stable@dpdk.org Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- drivers/net/pcap/rte_eth_pcap.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)