diff mbox series

[1/9] net/pcap: fix argument checks

Message ID 20191001125315.6191-2-ktraynor@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series Coverity fixes and other cleanups |

Checks

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

Commit Message

Kevin Traynor Oct. 1, 2019, 12:53 p.m. UTC 
  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(-)

Comments

Cian Ferriter Oct. 4, 2019, 10:57 a.m. UTC | #1 
> -----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
David Marchand Oct. 30, 2019, 7:52 a.m. UTC | #2 
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.
Kevin Traynor Nov. 5, 2019, 4:40 p.m. UTC | #3 
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?
Cian Ferriter Nov. 5, 2019, 5:10 p.m. UTC | #4 
> -----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!
Kevin Traynor Nov. 6, 2019, 7:03 p.m. UTC | #5 
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 mbox series

Patch

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)