[PATCH v3 00/10] codeql fixes for various subsystems

[PATCH v3 00/10] codeql fixes for various subsystems

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Following up the codeql reported problems first submitted
by Stephen Hemminger here:

https://lore.kernel.org/all/20220527161210.77212d0b@hermes.local/t/

Posting a series of fixes about potential null pointer accesses.

Changes from v3:
- Dropped net/tap: check if name is null
- Moved the comment to the appropriate position in ethdev: check return result of rte_eth_dev_info_get

Changes from v2:
- Remove braces around single line statements
- use NULL comparisons


Sinan Kaya (10):
  ethdev: check return result of rte_eth_dev_info_get
  memzone: check result of rte_fbarray_get
  memzone: check result of malloc_elem_from_data
  malloc: malloc_elem_join_adjacent_free can return null
  malloc: check result of rte_mem_virt2memseg_list
  malloc: check result of rte_fbarray_get
  malloc: check result of rte_mem_virt2memseg
  malloc: check result of malloc_elem_free
  malloc: check result of elem_start_pt
  bus/vdev: check result of rte_vdev_device_name

 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 6 files changed, 37 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v3 01/10] ethdev: check return result of rte_eth_dev_info_get

[okaya]

From: Sinan Kaya <okaya@kernel.org>

rte_class_eth: eth_mac_cmp: The status of this call to rte_eth_dev_info_get
is not checked, potentially leaving dev_info uninitialized.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/ethdev/rte_class_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
index 838b3a8f9f..559ce22491 100644
--- a/lib/ethdev/rte_class_eth.c
+++ b/lib/ethdev/rte_class_eth.c
@@ -50,8 +50,10 @@ eth_mac_cmp(const char *key __rte_unused,
 	if (rte_ether_unformat_addr(value, &mac) < 0)
 		return -1; /* invalid devargs value */
 
+	if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0)
+		return -1;
+
 	/* Return 0 if devargs MAC is matching one of the device MACs. */
-	rte_eth_dev_info_get(data->port_id, &dev_info);
 	for (index = 0; index < dev_info.max_mac_addrs; index++)
 		if (rte_is_same_ether_addr(&mac, &data->mac_addrs[index]))
 			return 0;
-- 
2.25.1

[PATCH v3 02/10] memzone: check result of rte_fbarray_get

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In memzone_lookup_thread_unsafe result of call to rte_fbarray_get
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memzone.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_memzone.c b/lib/eal/common/eal_common_memzone.c
index 860fb5fb64..8d472505eb 100644
--- a/lib/eal/common/eal_common_memzone.c
+++ b/lib/eal/common/eal_common_memzone.c
@@ -41,7 +41,7 @@ memzone_lookup_thread_unsafe(const char *name)
 	i = rte_fbarray_find_next_used(arr, 0);
 	while (i >= 0) {
 		mz = rte_fbarray_get(arr, i);
-		if (mz->addr != NULL &&
+		if ((mz != NULL) && (mz->addr != NULL) &&
 				!strncmp(name, mz->name, RTE_MEMZONE_NAMESIZE))
 			return mz;
 		i = rte_fbarray_find_next_used(arr, i + 1);
@@ -358,6 +358,10 @@ dump_memzone(const struct rte_memzone *mz, void *arg)
 	fprintf(f, "physical segments used:\n");
 	ms_idx = RTE_PTR_DIFF(mz->addr, msl->base_va) / page_sz;
 	ms = rte_fbarray_get(&msl->memseg_arr, ms_idx);
+	if (ms == NULL) {
+		RTE_LOG(DEBUG, EAL, "Skipping bad memzone\n");
+		return;
+	}
 
 	do {
 		fprintf(f, "  addr: %p iova: 0x%" PRIx64 " "
-- 
2.25.1

[PATCH v3 03/10] memzone: check result of malloc_elem_from_data

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In memzone_reserve_aligned_thread_unsafe result of call
to malloc_elem_from_data is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memzone.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/eal/common/eal_common_memzone.c b/lib/eal/common/eal_common_memzone.c
index 8d472505eb..930fee5fdc 100644
--- a/lib/eal/common/eal_common_memzone.c
+++ b/lib/eal/common/eal_common_memzone.c
@@ -169,6 +169,10 @@ memzone_reserve_aligned_thread_unsafe(const char *name, size_t len,
 	}
 
 	struct malloc_elem *elem = malloc_elem_from_data(mz_addr);
+	if (elem == NULL) {
+		rte_errno = ENOSPC;
+		return NULL;
+	}
 
 	/* fill the zone in config */
 	mz_idx = rte_fbarray_find_next_free(arr, 0);
-- 
2.25.1

[PATCH v3 04/10] malloc: malloc_elem_join_adjacent_free can return null

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In malloc_heap_add_memory result of call to malloc_elem_join_adjacent_free
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_heap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index d7c410b786..503e551bf9 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -97,6 +97,8 @@ malloc_heap_add_memory(struct malloc_heap *heap, struct rte_memseg_list *msl,
 	malloc_elem_insert(elem);
 
 	elem = malloc_elem_join_adjacent_free(elem);
+	if (elem == NULL)
+		return NULL;
 
 	malloc_elem_free_list_insert(elem);
 
-- 
2.25.1

[PATCH v3 05/10] malloc: check result of rte_mem_virt2memseg_list

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In alloc_pages_on_heap result of call to rte_mem_virt2memseg_list
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_heap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 503e551bf9..3f41430e42 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -323,6 +323,8 @@ alloc_pages_on_heap(struct malloc_heap *heap, uint64_t pg_sz, size_t elt_size,
 
 	map_addr = ms[0]->addr;
 	msl = rte_mem_virt2memseg_list(map_addr);
+	if (msl == NULL)
+		return NULL;
 
 	/* check if we wanted contiguous memory but didn't get it */
 	if (contig && !eal_memalloc_is_contig(msl, map_addr, alloc_sz)) {
-- 
2.25.1

[PATCH v3 06/10] malloc: check result of rte_fbarray_get

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In eal_memalloc_is_contig result of call to rte_fbarray_get
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memalloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_memalloc.c b/lib/eal/common/eal_common_memalloc.c
index ab04479c1c..24506f8447 100644
--- a/lib/eal/common/eal_common_memalloc.c
+++ b/lib/eal/common/eal_common_memalloc.c
@@ -126,6 +126,9 @@ eal_memalloc_is_contig(const struct rte_memseg_list *msl, void *start,
 
 		/* skip first iteration */
 		ms = rte_fbarray_get(&msl->memseg_arr, start_seg);
+		if (ms == NULL)
+			return false;
+
 		cur = ms->iova;
 		expected = cur + pgsz;
 
@@ -137,7 +140,7 @@ eal_memalloc_is_contig(const struct rte_memseg_list *msl, void *start,
 				cur_seg++, expected += pgsz) {
 			ms = rte_fbarray_get(&msl->memseg_arr, cur_seg);
 
-			if (ms->iova != expected)
+			if ((ms != NULL) && (ms->iova != expected))
 				return false;
 		}
 	}
-- 
2.25.1

[PATCH v3 07/10] malloc: check result of rte_mem_virt2memseg

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In malloc_elem_find_max_iova_contig result of call to rte_mem_virt2memseg
is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_elem.c | 11 ++++++++---
 lib/eal/common/malloc_heap.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/eal/common/malloc_elem.c b/lib/eal/common/malloc_elem.c
index 83f05497cc..8f49812846 100644
--- a/lib/eal/common/malloc_elem.c
+++ b/lib/eal/common/malloc_elem.c
@@ -63,6 +63,8 @@ malloc_elem_find_max_iova_contig(struct malloc_elem *elem, size_t align)
 
 	cur_page = RTE_PTR_ALIGN_FLOOR(contig_seg_start, page_sz);
 	ms = rte_mem_virt2memseg(cur_page, elem->msl);
+	if (ms == NULL)
+		return 0;
 
 	/* do first iteration outside the loop */
 	page_end = RTE_PTR_ADD(cur_page, page_sz);
@@ -91,9 +93,12 @@ malloc_elem_find_max_iova_contig(struct malloc_elem *elem, size_t align)
 			 * we're not blowing past data end.
 			 */
 			ms = rte_mem_virt2memseg(contig_seg_start, elem->msl);
-			cur_page = ms->addr;
-			/* don't trigger another recalculation */
-			expected_iova = ms->iova;
+			if (ms != NULL) {
+				cur_page = ms->addr;
+
+				/* don't trigger another recalculation */
+				expected_iova = ms->iova;
+			}
 			continue;
 		}
 		/* cur_seg_end ends on a page boundary or on data end. if we're
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 3f41430e42..88270ce4d2 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -930,7 +930,7 @@ malloc_heap_free(struct malloc_elem *elem)
 		const struct rte_memseg *tmp =
 				rte_mem_virt2memseg(aligned_start, msl);
 
-		if (tmp->flags & RTE_MEMSEG_FLAG_DO_NOT_FREE) {
+		if ((tmp != NULL) && (tmp->flags & RTE_MEMSEG_FLAG_DO_NOT_FREE)) {
 			/* this is an unfreeable segment, so move start */
 			aligned_start = RTE_PTR_ADD(tmp->addr, tmp->len);
 		}
-- 
2.25.1

[PATCH v3 08/10] malloc: check result of malloc_elem_free

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In malloc_heap_free result of call to malloc_elem_free is dereferenced
here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_heap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 88270ce4d2..6eb6fcda5e 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -892,6 +892,9 @@ malloc_heap_free(struct malloc_elem *elem)
 	/* anything after this is a bonus */
 	ret = 0;
 
+	if (elem == NULL)
+		goto free_unlock;
+
 	/* ...of which we can't avail if we are in legacy mode, or if this is an
 	 * externally allocated segment.
 	 */
-- 
2.25.1

[PATCH v3 09/10] malloc: check result of elem_start_pt

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In malloc_elem_alloc result of call to elem_start_pt is dereferenced
here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/malloc_elem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/common/malloc_elem.c b/lib/eal/common/malloc_elem.c
index 8f49812846..26296f2dba 100644
--- a/lib/eal/common/malloc_elem.c
+++ b/lib/eal/common/malloc_elem.c
@@ -435,6 +435,9 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
 {
 	struct malloc_elem *new_elem = elem_start_pt(elem, size, align, bound,
 			contig);
+	if (new_elem == NULL)
+		return NULL;
+
 	const size_t old_elem_size = (uintptr_t)new_elem - (uintptr_t)elem;
 	const size_t trailer_size = elem->size - old_elem_size - size -
 		MALLOC_ELEM_OVERHEAD;
-- 
2.25.1

[PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

[okaya]

From: Sinan Kaya <okaya@kernel.org>

In rte_eth_vdev_allocate result of call to rte_vdev_device_name is
dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/ethdev/ethdev_vdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/ethdev/ethdev_vdev.h b/lib/ethdev/ethdev_vdev.h
index 364f140f91..6d94a65d97 100644
--- a/lib/ethdev/ethdev_vdev.h
+++ b/lib/ethdev/ethdev_vdev.h
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;
 
 	eth_dev = rte_eth_dev_allocate(name);
 	if (!eth_dev)
-- 
2.25.1

RE: [PATCH v3 01/10] ethdev: check return result of rte_eth_dev_info_get

[Morten Brørup]

> From: okaya@kernel.org [mailto:okaya@kernel.org]
> Sent: Friday, 20 January 2023 05.42
> 
> From: Sinan Kaya <okaya@kernel.org>
> 
> rte_class_eth: eth_mac_cmp: The status of this call to
> rte_eth_dev_info_get
> is not checked, potentially leaving dev_info uninitialized.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> ---
>  lib/ethdev/rte_class_eth.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
> index 838b3a8f9f..559ce22491 100644
> --- a/lib/ethdev/rte_class_eth.c
> +++ b/lib/ethdev/rte_class_eth.c
> @@ -50,8 +50,10 @@ eth_mac_cmp(const char *key __rte_unused,
>  	if (rte_ether_unformat_addr(value, &mac) < 0)
>  		return -1; /* invalid devargs value */
> 
> +	if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0)
> +		return -1;
> +
>  	/* Return 0 if devargs MAC is matching one of the device MACs. */
> -	rte_eth_dev_info_get(data->port_id, &dev_info);
>  	for (index = 0; index < dev_info.max_mac_addrs; index++)
>  		if (rte_is_same_ether_addr(&mac, &data->mac_addrs[index]))
>  			return 0;
> --
> 2.25.1
> 

Although rte_eth_dev_info_get() can return various error codes, eth_mac_cmp() returns only 0 or -1, so this is correct.

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

[Stephen Hemminger]

On Thu, 19 Jan 2023 23:41:40 -0500
okaya@kernel.org wrote:

> diff --git a/lib/ethdev/ethdev_vdev.h b/lib/ethdev/ethdev_vdev.h
> index 364f140f91..6d94a65d97 100644
> --- a/lib/ethdev/ethdev_vdev.h
> +++ b/lib/ethdev/ethdev_vdev.h
> @@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
>  {
>  	struct rte_eth_dev *eth_dev;
>  	const char *name = rte_vdev_device_name(dev);
> +	if (name == NULL)
> +		return NULL;


Please add a blank line after declarations and before code.
For some reason the DPDK version of checkpatch suppresses this warning.

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

[Thomas Monjalon]

20/01/2023 17:47, Stephen Hemminger:
> On Thu, 19 Jan 2023 23:41:40 -0500
> okaya@kernel.org wrote:
> 
> > diff --git a/lib/ethdev/ethdev_vdev.h b/lib/ethdev/ethdev_vdev.h
> > index 364f140f91..6d94a65d97 100644
> > --- a/lib/ethdev/ethdev_vdev.h
> > +++ b/lib/ethdev/ethdev_vdev.h
> > @@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
> >  {
> >  	struct rte_eth_dev *eth_dev;
> >  	const char *name = rte_vdev_device_name(dev);
> > +	if (name == NULL)
> > +		return NULL;
> 
> Please add a blank line after declarations and before code.
> For some reason the DPDK version of checkpatch suppresses this warning.

The check of "name" is related to the line above,
so I don't think we should insert a blank line.

Re: [PATCH v3 00/10] codeql fixes for various subsystems

[Sinan Kaya]

On Thu, 2023-01-19 at 23:41 -0500, okaya@kernel.org wrote:
> From: Sinan Kaya <okaya@kernel.org>
> 
> Following up the codeql reported problems first submitted
> by Stephen Hemminger here:
> 
> https://lore.kernel.org/all/20220527161210.77212d0b@hermes.local/t/
> 
> Posting a series of fixes about potential null pointer accesses.
> 
> Changes from v3:
> - Dropped net/tap: check if name is null
> - Moved the comment to the appropriate position in ethdev: check
> return result of rte_eth_dev_info_get
> 
> Changes from v2:
> - Remove braces around single line statements
> - use NULL comparisons
> 
> 
> Sinan Kaya (10):
>   ethdev: check return result of rte_eth_dev_info_get
>   memzone: check result of rte_fbarray_get
>   memzone: check result of malloc_elem_from_data
>   malloc: malloc_elem_join_adjacent_free can return null
>   malloc: check result of rte_mem_virt2memseg_list
>   malloc: check result of rte_fbarray_get
>   malloc: check result of rte_mem_virt2memseg
>   malloc: check result of malloc_elem_free
>   malloc: check result of elem_start_pt
>   bus/vdev: check result of rte_vdev_device_name
> 
>  lib/eal/common/eal_common_memalloc.c |  5 ++++-
>  lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
>  lib/eal/common/malloc_elem.c         | 14 +++++++++++---
>  lib/eal/common/malloc_heap.c         |  9 ++++++++-
>  lib/ethdev/ethdev_vdev.h             |  2 ++
>  lib/ethdev/rte_class_eth.c           |  4 +++-
>  6 files changed, 37 insertions(+), 7 deletions(-)
> 

Anatoly, any feedback on the series? You seem to be a maintainer for
2-9 patches.

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

[Sinan Kaya]

On Sun, 2023-01-22 at 21:51 +0100, Thomas Monjalon wrote:
> 20/01/2023 17:47, Stephen Hemminger:
> > On Thu, 19 Jan 2023 23:41:40 -0500
> > okaya@kernel.org wrote:
> > 
> > > diff --git a/lib/ethdev/ethdev_vdev.h b/lib/ethdev/ethdev_vdev.h
> > > index 364f140f91..6d94a65d97 100644
> > > --- a/lib/ethdev/ethdev_vdev.h
> > > +++ b/lib/ethdev/ethdev_vdev.h
> > > @@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device
> > > *dev, size_t private_data_size)
> > >  {
> > >  	struct rte_eth_dev *eth_dev;
> > >  	const char *name = rte_vdev_device_name(dev);
> > > +	if (name == NULL)
> > > +		return NULL;
> > 
> > Please add a blank line after declarations and before code.
> > For some reason the DPDK version of checkpatch suppresses this
> > warning.
> 
> The check of "name" is related to the line above,
> so I don't think we should insert a blank line.
> 
> 

While I'm waiting for maintainer feedback, let me know which
way I should go. I can update this for next rev.

Re: [PATCH v3 10/10] bus/vdev: check result of rte_vdev_device_name

[Thomas Monjalon]

06/02/2023 17:01, Sinan Kaya:
> On Sun, 2023-01-22 at 21:51 +0100, Thomas Monjalon wrote:
> > 20/01/2023 17:47, Stephen Hemminger:
> > > On Thu, 19 Jan 2023 23:41:40 -0500
> > > okaya@kernel.org wrote:
> > > 
> > > > diff --git a/lib/ethdev/ethdev_vdev.h b/lib/ethdev/ethdev_vdev.h
> > > > index 364f140f91..6d94a65d97 100644
> > > > --- a/lib/ethdev/ethdev_vdev.h
> > > > +++ b/lib/ethdev/ethdev_vdev.h
> > > > @@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device
> > > > *dev, size_t private_data_size)
> > > >  {
> > > >  	struct rte_eth_dev *eth_dev;
> > > >  	const char *name = rte_vdev_device_name(dev);
> > > > +	if (name == NULL)
> > > > +		return NULL;
> > > 
> > > Please add a blank line after declarations and before code.
> > > For some reason the DPDK version of checkpatch suppresses this
> > > warning.
> > 
> > The check of "name" is related to the line above,
> > so I don't think we should insert a blank line.
> 
> While I'm waiting for maintainer feedback, let me know which
> way I should go. I can update this for next rev.

It's just a matter of taste, not a big deal.
I think you can keep it as-is.

Re: [PATCH v3 00/10] codeql fixes for various subsystems

[Stephen Hemminger]

On Thu, 19 Jan 2023 23:41:30 -0500
okaya@kernel.org wrote:

> From: Sinan Kaya <okaya@kernel.org>
> 
> Following up the codeql reported problems first submitted
> by Stephen Hemminger here:
> 
> https://lore.kernel.org/all/20220527161210.77212d0b@hermes.local/t/
> 
> Posting a series of fixes about potential null pointer accesses.
> 
> Changes from v3:
> - Dropped net/tap: check if name is null
> - Moved the comment to the appropriate position in ethdev: check return result of rte_eth_dev_info_get
> 
> Changes from v2:
> - Remove braces around single line statements
> - use NULL comparisons
> 
> 
> Sinan Kaya (10):
>   ethdev: check return result of rte_eth_dev_info_get
>   memzone: check result of rte_fbarray_get
>   memzone: check result of malloc_elem_from_data
>   malloc: malloc_elem_join_adjacent_free can return null
>   malloc: check result of rte_mem_virt2memseg_list
>   malloc: check result of rte_fbarray_get
>   malloc: check result of rte_mem_virt2memseg
>   malloc: check result of malloc_elem_free
>   malloc: check result of elem_start_pt
>   bus/vdev: check result of rte_vdev_device_name
> 
>  lib/eal/common/eal_common_memalloc.c |  5 ++++-
>  lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
>  lib/eal/common/malloc_elem.c         | 14 +++++++++++---
>  lib/eal/common/malloc_heap.c         |  9 ++++++++-
>  lib/ethdev/ethdev_vdev.h             |  2 ++
>  lib/ethdev/rte_class_eth.c           |  4 +++-
>  6 files changed, 37 insertions(+), 7 deletions(-)

The patches are fine, only warning was from the requirement that all
submitters be in .mailmap now.  Do you mind if I rebase these, add a mailmap
and combine the like ones together?

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH v3 00/10] codeql fixes for various subsystems

[Sinan Kaya]

On Thu, 2023-07-06 at 15:43 -0700, Stephen Hemminger wrote:
> >   lib/eal/common/eal_common_memalloc.c |  5 ++++-
> >   lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
> >   lib/eal/common/malloc_elem.c         | 14 +++++++++++---
> >   lib/eal/common/malloc_heap.c         |  9 ++++++++-
> >   lib/ethdev/ethdev_vdev.h             |  2 ++
> >   lib/ethdev/rte_class_eth.c           |  4 +++-
> >   6 files changed, 37 insertions(+), 7 deletions(-)
> 
> 
> The patches are fine, only warning was from the requirement that all
> 
> submitters be in .mailmap now.  Do you mind if I rebase these, add a
> mailmap
> 
> and combine the like ones together?
> 

Please go ahead.

> 
> 
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH v4 0/5] fixes for problems found by codeql analyzer

[Stephen Hemminger]

CodeQL is an analyzer (like Coverity) for static analysis
https://codeql.github.com/

Followup from earlier set from Sinan.
Rebased to main and consolidated for easier review

Sinan Kaya (4):
  ethdev: check return result of rte_eth_dev_info_get
  memzone: check result of rte_fbarray_get and malloc_elem_from_data
  malloc: codeql fixes
  bus/vdev: check result of rte_vdev_device_name

Stephen Hemminger (1):
  mailmap: add Sinan

 .mailmap                             |  1 +
 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 lib/ethdev/ethdev_vdev.h             |  2 ++
 lib/ethdev/rte_class_eth.c           |  4 +++-
 7 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH v4 1/5] mailmap: add Sinan

[Stephen Hemminger]

Patches get flagged as in error if mailmap missing.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .mailmap | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index d200f363394d..91557940e3d9 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1285,6 +1285,7 @@ Simon Ellmann <simon.ellmann@tum.de>
 Simon Horman <simon.horman@corigine.com> <simon.horman@netronome.com>
 Simon Kagstrom <simon.kagstrom@netinsight.net>
 Simon Kuenzer <simon.kuenzer@neclab.eu>
+Sinan Kaya <okaya@kernel.org>
 Siobhan Butler <siobhan.a.butler@intel.com>
 Sirshak Das <sirshak.das@arm.com>
 Sivaprasad Tummala <sivaprasad.tummala@amd.com> <sivaprasad.tummala@intel.com>
-- 
2.39.2

[PATCH v4 2/5] ethdev: check return result of rte_eth_dev_info_get

[Stephen Hemminger]

From: Sinan Kaya <okaya@kernel.org>

rte_class_eth: eth_mac_cmp: The status of this call to rte_eth_dev_info_get
is not checked, potentially leaving dev_info uninitialized.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/ethdev/rte_class_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
index b61dae849dce..05c100efbba3 100644
--- a/lib/ethdev/rte_class_eth.c
+++ b/lib/ethdev/rte_class_eth.c
@@ -50,8 +50,10 @@ eth_mac_cmp(const char *key __rte_unused,
 	if (rte_ether_unformat_addr(value, &mac) < 0)
 		return -1; /* invalid devargs value */
 
+	if (rte_eth_dev_info_get(data->port_id, &dev_info) < 0)
+		return -1;
+
 	/* Return 0 if devargs MAC is matching one of the device MACs. */
-	rte_eth_dev_info_get(data->port_id, &dev_info);
 	for (index = 0; index < dev_info.max_mac_addrs; index++)
 		if (rte_is_same_ether_addr(&mac, &data->mac_addrs[index]))
 			return 0;
-- 
2.39.2

[PATCH v4 3/5] memzone: check result of rte_fbarray_get and malloc_elem_from_data

[Stephen Hemminger]

From: Sinan Kaya <okaya@kernel.org>

In memzone_lookup_thread_unsafe result of call to rte_fbarray_get
is dereferenced here and may be null.

In memzone_reserve_aligned_thread_unsafe result of call
to malloc_elem_from_data is dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/common/eal_common_memzone.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_memzone.c b/lib/eal/common/eal_common_memzone.c
index 1f3e7014995b..c96053b41e63 100644
--- a/lib/eal/common/eal_common_memzone.c
+++ b/lib/eal/common/eal_common_memzone.c
@@ -77,7 +77,7 @@ memzone_lookup_thread_unsafe(const char *name)
 	i = rte_fbarray_find_next_used(arr, 0);
 	while (i >= 0) {
 		mz = rte_fbarray_get(arr, i);
-		if (mz->addr != NULL &&
+		if ((mz != NULL) && (mz->addr != NULL) &&
 				!strncmp(name, mz->name, RTE_MEMZONE_NAMESIZE))
 			return mz;
 		i = rte_fbarray_find_next_used(arr, i + 1);
@@ -206,6 +206,10 @@ memzone_reserve_aligned_thread_unsafe(const char *name, size_t len,
 	}
 
 	struct malloc_elem *elem = malloc_elem_from_data(mz_addr);
+	if (elem == NULL) {
+		rte_errno = ENOSPC;
+		return NULL;
+	}
 
 	/* fill the zone in config */
 	mz_idx = rte_fbarray_find_next_free(arr, 0);
@@ -395,6 +399,10 @@ dump_memzone(const struct rte_memzone *mz, void *arg)
 	fprintf(f, "physical segments used:\n");
 	ms_idx = RTE_PTR_DIFF(mz->addr, msl->base_va) / page_sz;
 	ms = rte_fbarray_get(&msl->memseg_arr, ms_idx);
+	if (ms == NULL) {
+		RTE_LOG(DEBUG, EAL, "Skipping bad memzone\n");
+		return;
+	}
 
 	do {
 		fprintf(f, "  addr: %p iova: 0x%" PRIx64 " "
-- 
2.39.2

[PATCH v4 4/5] malloc: codeql fixes

[Stephen Hemminger]

From: Sinan Kaya <okaya@kernel.org>

In malloc_heap_add_memory result of call to malloc_elem_join_adjacent_free
is dereferenced here and may be null.

In alloc_pages_on_heap result of call to rte_mem_virt2memseg_list
is dereferenced here and may be null.

In eal_memalloc_is_contig result of call to rte_fbarray_get
is dereferenced here and may be null.

In malloc_elem_find_max_iova_contig result of call to rte_mem_virt2memseg
is dereferenced here and may be null.

In malloc_heap_free result of call to malloc_elem_free is dereferenced
here and may be null.

In malloc_elem_alloc result of call to elem_start_pt is dereferenced
here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/common/eal_common_memalloc.c |  5 ++++-
 lib/eal/common/malloc_elem.c         | 14 +++++++++++---
 lib/eal/common/malloc_heap.c         |  9 ++++++++-
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/lib/eal/common/eal_common_memalloc.c b/lib/eal/common/eal_common_memalloc.c
index ab04479c1cc5..24506f8447d7 100644
--- a/lib/eal/common/eal_common_memalloc.c
+++ b/lib/eal/common/eal_common_memalloc.c
@@ -126,6 +126,9 @@ eal_memalloc_is_contig(const struct rte_memseg_list *msl, void *start,
 
 		/* skip first iteration */
 		ms = rte_fbarray_get(&msl->memseg_arr, start_seg);
+		if (ms == NULL)
+			return false;
+
 		cur = ms->iova;
 		expected = cur + pgsz;
 
@@ -137,7 +140,7 @@ eal_memalloc_is_contig(const struct rte_memseg_list *msl, void *start,
 				cur_seg++, expected += pgsz) {
 			ms = rte_fbarray_get(&msl->memseg_arr, cur_seg);
 
-			if (ms->iova != expected)
+			if ((ms != NULL) && (ms->iova != expected))
 				return false;
 		}
 	}
diff --git a/lib/eal/common/malloc_elem.c b/lib/eal/common/malloc_elem.c
index 619c040aa3e8..443ae26d283a 100644
--- a/lib/eal/common/malloc_elem.c
+++ b/lib/eal/common/malloc_elem.c
@@ -63,6 +63,8 @@ malloc_elem_find_max_iova_contig(struct malloc_elem *elem, size_t align)
 
 	cur_page = RTE_PTR_ALIGN_FLOOR(contig_seg_start, page_sz);
 	ms = rte_mem_virt2memseg(cur_page, elem->msl);
+	if (ms == NULL)
+		return 0;
 
 	/* do first iteration outside the loop */
 	page_end = RTE_PTR_ADD(cur_page, page_sz);
@@ -91,9 +93,12 @@ malloc_elem_find_max_iova_contig(struct malloc_elem *elem, size_t align)
 			 * we're not blowing past data end.
 			 */
 			ms = rte_mem_virt2memseg(contig_seg_start, elem->msl);
-			cur_page = ms->addr;
-			/* don't trigger another recalculation */
-			expected_iova = ms->iova;
+			if (ms != NULL) {
+				cur_page = ms->addr;
+
+				/* don't trigger another recalculation */
+				expected_iova = ms->iova;
+			}
 			continue;
 		}
 		/* cur_seg_end ends on a page boundary or on data end. if we're
@@ -430,6 +435,9 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
 {
 	struct malloc_elem *new_elem = elem_start_pt(elem, size, align, bound,
 			contig);
+	if (new_elem == NULL)
+		return NULL;
+
 	const size_t old_elem_size = (uintptr_t)new_elem - (uintptr_t)elem;
 	const size_t trailer_size = elem->size - old_elem_size - size -
 		MALLOC_ELEM_OVERHEAD;
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 6b6cf9174cd3..0abaaa8c57f8 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -97,6 +97,8 @@ malloc_heap_add_memory(struct malloc_heap *heap, struct rte_memseg_list *msl,
 	malloc_elem_insert(elem);
 
 	elem = malloc_elem_join_adjacent_free(elem);
+	if (elem == NULL)
+		return NULL;
 
 	malloc_elem_free_list_insert(elem);
 
@@ -321,6 +323,8 @@ alloc_pages_on_heap(struct malloc_heap *heap, uint64_t pg_sz, size_t elt_size,
 
 	map_addr = ms[0]->addr;
 	msl = rte_mem_virt2memseg_list(map_addr);
+	if (msl == NULL)
+		return NULL;
 
 	/* check if we wanted contiguous memory but didn't get it */
 	if (contig && !eal_memalloc_is_contig(msl, map_addr, alloc_sz)) {
@@ -897,6 +901,9 @@ malloc_heap_free(struct malloc_elem *elem)
 	/* anything after this is a bonus */
 	ret = 0;
 
+	if (elem == NULL)
+		goto free_unlock;
+
 	/* ...of which we can't avail if we are in legacy mode, or if this is an
 	 * externally allocated segment.
 	 */
@@ -935,7 +942,7 @@ malloc_heap_free(struct malloc_elem *elem)
 		const struct rte_memseg *tmp =
 				rte_mem_virt2memseg(aligned_start, msl);
 
-		if (tmp->flags & RTE_MEMSEG_FLAG_DO_NOT_FREE) {
+		if ((tmp != NULL) && (tmp->flags & RTE_MEMSEG_FLAG_DO_NOT_FREE)) {
 			/* this is an unfreeable segment, so move start */
 			aligned_start = RTE_PTR_ADD(tmp->addr, tmp->len);
 		}
-- 
2.39.2

[PATCH v4 5/5] bus/vdev: check result of rte_vdev_device_name

[Stephen Hemminger]

From: Sinan Kaya <okaya@kernel.org>

In rte_eth_vdev_allocate result of call to rte_vdev_device_name is
dereferenced here and may be null.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/ethdev/ethdev_vdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/ethdev/ethdev_vdev.h b/lib/ethdev/ethdev_vdev.h
index 364f140f91b5..6d94a65d975d 100644
--- a/lib/ethdev/ethdev_vdev.h
+++ b/lib/ethdev/ethdev_vdev.h
@@ -34,6 +34,8 @@ rte_eth_vdev_allocate(struct rte_vdev_device *dev, size_t private_data_size)
 {
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
+	if (name == NULL)
+		return NULL;
 
 	eth_dev = rte_eth_dev_allocate(name);
 	if (!eth_dev)
-- 
2.39.2

Re: [PATCH v4 0/5] fixes for problems found by codeql analyzer

[fengchengwen]

On 2023/7/11 1:07, Stephen Hemminger wrote:
> CodeQL is an analyzer (like Coverity) for static analysis
> https://codeql.github.com/
> 
> Followup from earlier set from Sinan.
> Rebased to main and consolidated for easier review
> 
> Sinan Kaya (4):
>   ethdev: check return result of rte_eth_dev_info_get
>   memzone: check result of rte_fbarray_get and malloc_elem_from_data
>   malloc: codeql fixes
>   bus/vdev: check result of rte_vdev_device_name

Please add Cc.
Series-acked-by: Chengwen Feng <fengchengwen@huawei.com>

> 
> Stephen Hemminger (1):
>   mailmap: add Sinan
> 
>  .mailmap                             |  1 +
>  lib/eal/common/eal_common_memalloc.c |  5 ++++-
>  lib/eal/common/eal_common_memzone.c  | 10 +++++++++-
>  lib/eal/common/malloc_elem.c         | 14 +++++++++++---
>  lib/eal/common/malloc_heap.c         |  9 ++++++++-
>  lib/ethdev/ethdev_vdev.h             |  2 ++
>  lib/ethdev/rte_class_eth.c           |  4 +++-
>  7 files changed, 38 insertions(+), 7 deletions(-)
> 

Re: [PATCH v4 2/5] ethdev: check return result of rte_eth_dev_info_get

[Andrew Rybchenko]

On 7/10/23 20:07, Stephen Hemminger wrote:
> From: Sinan Kaya <okaya@kernel.org>
> 
> rte_class_eth: eth_mac_cmp: The status of this call to rte_eth_dev_info_get
> is not checked, potentially leaving dev_info uninitialized.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH v4 2/5] ethdev: check return result of rte_eth_dev_info_get

[Andrew Rybchenko]

On 12/16/23 13:06, Andrew Rybchenko wrote:
> On 7/10/23 20:07, Stephen Hemminger wrote:
>> From: Sinan Kaya <okaya@kernel.org>
>>
>> rte_class_eth: eth_mac_cmp: The status of this call to 
>> rte_eth_dev_info_get
>> is not checked, potentially leaving dev_info uninitialized.
>>
>> Signed-off-by: Sinan Kaya <okaya@kernel.org>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

except of patch summary which should not mention function name

Re: [PATCH v4 5/5] bus/vdev: check result of rte_vdev_device_name

[Andrew Rybchenko]

On 7/10/23 20:08, Stephen Hemminger wrote:
> From: Sinan Kaya <okaya@kernel.org>
> 
> In rte_eth_vdev_allocate result of call to rte_vdev_device_name is
> dereferenced here and may be null.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Patch summary should be human-readable and do not mention
internals (function name).

Other than that:
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>