[PATCH 0/3] EAL memory fixes

[PATCH 0/3] EAL memory fixes

[Jake Freeland]

Hi there,

This patchset contains a number of EAL-specific memory fixes that I've
made over the last year. Two pertain to FreeBSD, one pertains to Linux.

Let me know if you have any feedback. Thanks.

Jake Freeland (3):
  eal/freebsd: Do not use prev_ms_idx for hole detection
  eal/freebsd: Do not index out of bounds in memseg list
  eal/linux: Check hugepage access permissions

 lib/eal/freebsd/eal_memory.c      | 8 +++++---
 lib/eal/linux/eal_hugepage_info.c | 6 ++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

--
2.47.2

[PATCH 1/3] eal/freebsd: Do not use prev_ms_idx for hole detection

[Jake Freeland]

Use rte_fbarray_is_used() to check if the previous fbarray entry is
already empty.

Using prev_ms_idx to do this is flawed in cases where we loop through
multiple memseg lists. Each memseg list has its own count and length,
so using a prev_ms_idx from one memseg list to check for used entries
in another non-empty memseg list can lead to incorrect hole placement.

Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
---
 lib/eal/freebsd/eal_memory.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/eal/freebsd/eal_memory.c b/lib/eal/freebsd/eal_memory.c
index 3b72e13506..bcf5a6f986 100644
--- a/lib/eal/freebsd/eal_memory.c
+++ b/lib/eal/freebsd/eal_memory.c
@@ -104,7 +104,6 @@ rte_eal_hugepage_init(void)
 	for (i = 0; i < internal_conf->num_hugepage_sizes; i++) {
 		struct hugepage_info *hpi;
 		rte_iova_t prev_end = 0;
-		int prev_ms_idx = -1;
 		uint64_t page_sz, mem_needed;
 		unsigned int n_pages, max_pages;
 
@@ -168,9 +167,9 @@ rte_eal_hugepage_init(void)
 				if (ms_idx < 0)
 					continue;
 
-				if (need_hole && prev_ms_idx == ms_idx - 1)
+				if (need_hole &&
+				    rte_fbarray_is_used(arr, ms_idx - 1))
 					ms_idx++;
-				prev_ms_idx = ms_idx;
 
 				break;
 			}
-- 
2.47.2

Re: [PATCH 1/3] eal/freebsd: Do not use prev_ms_idx for hole detection

[Burakov, Anatoly]

On 5/6/2025 7:50 PM, Jake Freeland wrote:
> Use rte_fbarray_is_used() to check if the previous fbarray entry is
> already empty.
> 
> Using prev_ms_idx to do this is flawed in cases where we loop through
> multiple memseg lists. Each memseg list has its own count and length,
> so using a prev_ms_idx from one memseg list to check for used entries
> in another non-empty memseg list can lead to incorrect hole placement.
> 
> Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH 1/3] eal/freebsd: Do not use prev_ms_idx for hole detection

[Burakov, Anatoly]

On 5/6/2025 7:50 PM, Jake Freeland wrote:
> Use rte_fbarray_is_used() to check if the previous fbarray entry is
> already empty.
> 
> Using prev_ms_idx to do this is flawed in cases where we loop through
> multiple memseg lists. Each memseg list has its own count and length,
> so using a prev_ms_idx from one memseg list to check for used entries
> in another non-empty memseg list can lead to incorrect hole placement.
> 
> Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
> ---
>   lib/eal/freebsd/eal_memory.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/eal/freebsd/eal_memory.c b/lib/eal/freebsd/eal_memory.c
> index 3b72e13506..bcf5a6f986 100644
> --- a/lib/eal/freebsd/eal_memory.c
> +++ b/lib/eal/freebsd/eal_memory.c
> @@ -104,7 +104,6 @@ rte_eal_hugepage_init(void)
>   	for (i = 0; i < internal_conf->num_hugepage_sizes; i++) {
>   		struct hugepage_info *hpi;
>   		rte_iova_t prev_end = 0;
> -		int prev_ms_idx = -1;
>   		uint64_t page_sz, mem_needed;
>   		unsigned int n_pages, max_pages;
>   
> @@ -168,9 +167,9 @@ rte_eal_hugepage_init(void)
>   				if (ms_idx < 0)
>   					continue;

I guess an alternative fix would be to reset prev_ms_idx after (ms_idx < 
0) check above?

>   
> -				if (need_hole && prev_ms_idx == ms_idx - 1)
> +				if (need_hole &&
> +				    rte_fbarray_is_used(arr, ms_idx - 1))
>   					ms_idx++;
> -				prev_ms_idx = ms_idx;
>   
>   				break;
>   			}


-- 
Thanks,
Anatoly

[PATCH 2/3] eal/freebsd: Do not index out of bounds in memseg list

[Jake Freeland]

It is possible for rte_fbarray_find_next_n_free() to misreport that there
are n contiguous open spots. If we need two contiguous entries for a
hole, make sure that we're not indexing out-of-bounds in the fbarray.

The `arr->len - arr->count < n` condition in fbarray_find_n() is meant to
safeguard against this, but we are not updating arr->count when inserting
holes, so an undesired index may be returned.

Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
---
 lib/eal/freebsd/eal_memory.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/freebsd/eal_memory.c b/lib/eal/freebsd/eal_memory.c
index bcf5a6f986..bdbac0c3f3 100644
--- a/lib/eal/freebsd/eal_memory.c
+++ b/lib/eal/freebsd/eal_memory.c
@@ -171,6 +171,9 @@ rte_eal_hugepage_init(void)
 				    rte_fbarray_is_used(arr, ms_idx - 1))
 					ms_idx++;
 
+				if (ms_idx == (int)arr->len)
+					continue;
+
 				break;
 			}
 			if (msl_idx == RTE_MAX_MEMSEG_LISTS) {
-- 
2.47.2

Re: [PATCH 2/3] eal/freebsd: Do not index out of bounds in memseg list

[Burakov, Anatoly]

On 5/6/2025 7:50 PM, Jake Freeland wrote:
> It is possible for rte_fbarray_find_next_n_free() to misreport that there
> are n contiguous open spots. If we need two contiguous entries for a
> hole, make sure that we're not indexing out-of-bounds in the fbarray.
> 
> The `arr->len - arr->count < n` condition in fbarray_find_n() is meant to
> safeguard against this, but we are not updating arr->count when inserting
> holes, so an undesired index may be returned.
> 
> Signed-off-by: Jake Freeland <jfree@FreeBSD.org>

I don't quite get how this happens.

If we "need hole" that means the memseg list isn't empty, and previous 
segment is occupied. When we need hole, we request 2 free spots from the 
memseg. Let's assume there's only 2 free spots left in the memseg list, 
so we get ms_idx equal to (arr->len - 2).

We get ms_idx - that is, the index of where the free spot starts, and 
there's 2 spots guaranteed to be free at that point. We need a hole, so 
we increment ms_idx once to arrive at last free spot (arr->len - 1).

Where would the overflow come from?

(also, now that I think of it, I suspect we should not be starting our 
search from index 0 every time, because if we "don't need a hole" 
because the segment is adjacent to previous one, that means 
find_next_n(1) starting from 0 will find us a hole we left previously 
for some other segment - perhaps we should find first unoccupied spot 
from the end, and start the find_next_n search from there?)

-- 
Thanks,
Anatoly

[PATCH 3/3] eal/linux: Check hugepage access permissions

[Jake Freeland]

Currently, hugepage mountpoints will be used irrespective of permissions,
leading to potential EACCES errors during memory allocation. Fix this by
not using a mountpoint if we do not have read/write permissions on it.

Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
---
 lib/eal/linux/eal_hugepage_info.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage_info.c
index d47a19c56a..dbfa38b05c 100644
--- a/lib/eal/linux/eal_hugepage_info.c
+++ b/lib/eal/linux/eal_hugepage_info.c
@@ -260,6 +260,12 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
 				continue;
 		}
 
+		if (access(splitstr[MOUNTPT], R_OK | W_OK) < 0) {
+			EAL_LOG(NOTICE, "Missing r/w permissions on huge dir: "
+			    "'%s'. Skipping it", splitstr[MOUNTPT]);
+			continue;
+		}
+
 		/*
 		 * If no --huge-dir option has been given, we're done.
 		 */
-- 
2.47.2

Re: [PATCH 3/3] eal/linux: Check hugepage access permissions

[Stephen Hemminger]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

Please don't split message a across multiple lines.
Open and access are not the same in all security checks, so not a great
idea.
Some analyzer tools may flag as time of check, time of use issue.

On Wed, May 7, 2025, 02:50 Jake Freeland <jfree@freebsd.org> wrote:

> Currently, hugepage mountpoints will be used irrespective of permissions,
> leading to potential EACCES errors during memory allocation. Fix this by
> not using a mountpoint if we do not have read/write permissions on it.
>
> Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
> ---
>  lib/eal/linux/eal_hugepage_info.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/lib/eal/linux/eal_hugepage_info.c
> b/lib/eal/linux/eal_hugepage_info.c
> index d47a19c56a..dbfa38b05c 100644
> --- a/lib/eal/linux/eal_hugepage_info.c
> +++ b/lib/eal/linux/eal_hugepage_info.c
> @@ -260,6 +260,12 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir,
> int len)
>                                 continue;
>                 }
>
> +               if (access(splitstr[MOUNTPT], R_OK | W_OK) < 0) {
> +                       EAL_LOG(NOTICE, "Missing r/w permissions on huge
> dir: "
> +                           "'%s'. Skipping it", splitstr[MOUNTPT]);
> +                       continue;
> +               }
> +
>                 /*
>                  * If no --huge-dir option has been given, we're done.
>                  */
> -
>

>

[-- Attachment #2: Type: text/html, Size: 2186 bytes --]

Re: [PATCH 3/3] eal/linux: Check hugepage access permissions

[Jake Freeland]

On Wed May 7, 2025 at 3:52 AM CDT, Stephen Hemminger wrote:
> Please don't split message a across multiple lines.
> Open and access are not the same in all security checks, so not a great
> idea.

What do you mean by this? In this case, access() is just verifying that
we can read and write to the hugepage directory. What extra security
check does open() perform that would be needed here?

Keep in mind that open() will still be used to open the hugepages and
perform it’s own checks later on. The purpose of this access() call is
to avoid saving inaccessible hugepage paths up front.

> Some analyzer tools may flag as time of check, time of use issue.

If this access() check succeeds (i.e. we have r/w access) and the
permissions of the hugepage directory change to read-only before we call
open(), then open() will fail like it does without this patch. This
seems like reasonable behavior to me and is better than having no
initial r/w check at all.

Thanks,
Jake Freeland

>
> On Wed, May 7, 2025, 02:50 Jake Freeland <jfree@freebsd.org> wrote:
>
> > Currently, hugepage mountpoints will be used irrespective of permissions,
> > leading to potential EACCES errors during memory allocation. Fix this by
> > not using a mountpoint if we do not have read/write permissions on it.
> >
> > Signed-off-by: Jake Freeland <jfree@FreeBSD.org>
> > ---
> >  lib/eal/linux/eal_hugepage_info.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/lib/eal/linux/eal_hugepage_info.c
> > b/lib/eal/linux/eal_hugepage_info.c
> > index d47a19c56a..dbfa38b05c 100644
> > --- a/lib/eal/linux/eal_hugepage_info.c
> > +++ b/lib/eal/linux/eal_hugepage_info.c
> > @@ -260,6 +260,12 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir,
> > int len)
> >                                 continue;
> >                 }
> >
> > +               if (access(splitstr[MOUNTPT], R_OK | W_OK) < 0) {
> > +                       EAL_LOG(NOTICE, "Missing r/w permissions on huge
> > dir: "
> > +                           "'%s'. Skipping it", splitstr[MOUNTPT]);
> > +                       continue;
> > +               }
> > +
> >                 /*
> >                  * If no --huge-dir option has been given, we're done.
> >                  */
> > -
> >
>
> >