[dpdk-dev] [PATCH] mem: fix undefined behavior in NUMA code

[dpdk-dev] [PATCH] mem: fix undefined behavior in NUMA code

[Anatoly Burakov]

When NUMA-aware hugepages config option is set, we rely on
libnuma to tell the kernel to allocate hugepages on a specific
NUMA node. However, we allocate node mask before we check if
NUMA is available in the first place, which, according to
the manpage [1], causes undefined behaviour.

Fix by only using nodemask when we have NUMA available.

[1] https://linux.die.net/man/3/numa_alloc_onnode

Bugzilla ID: 20

Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
Cc: i.maximets@samsung.com
Cc: stable@dpdk.org

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 28 ++++++++++++++----------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index dbf19499e..4976eeacd 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -263,7 +263,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 	int node_id = -1;
 	int essential_prev = 0;
 	int oldpolicy;
-	struct bitmask *oldmask = numa_allocate_nodemask();
+	struct bitmask *oldmask = NULL;
 	bool have_numa = true;
 	unsigned long maxnode = 0;
 
@@ -275,6 +275,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 
 	if (have_numa) {
 		RTE_LOG(DEBUG, EAL, "Trying to obtain current memory policy.\n");
+		oldmask = numa_allocate_nodemask();
 		if (get_mempolicy(&oldpolicy, oldmask->maskp,
 				  oldmask->size + 1, 0, 0) < 0) {
 			RTE_LOG(ERR, EAL,
@@ -390,19 +391,22 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 
 out:
 #ifdef RTE_EAL_NUMA_AWARE_HUGEPAGES
-	if (maxnode) {
-		RTE_LOG(DEBUG, EAL,
-			"Restoring previous memory policy: %d\n", oldpolicy);
-		if (oldpolicy == MPOL_DEFAULT) {
-			numa_set_localalloc();
-		} else if (set_mempolicy(oldpolicy, oldmask->maskp,
-					 oldmask->size + 1) < 0) {
-			RTE_LOG(ERR, EAL, "Failed to restore mempolicy: %s\n",
-				strerror(errno));
-			numa_set_localalloc();
+	if (have_numa) {
+		if (maxnode) {
+			RTE_LOG(DEBUG, EAL,
+				"Restoring previous memory policy: %d\n",
+					oldpolicy);
+			if (oldpolicy == MPOL_DEFAULT) {
+				numa_set_localalloc();
+			} else if (set_mempolicy(oldpolicy, oldmask->maskp,
+						 oldmask->size + 1) < 0) {
+				RTE_LOG(ERR, EAL, "Failed to restore mempolicy: %s\n",
+					strerror(errno));
+				numa_set_localalloc();
+			}
 		}
+		numa_free_cpumask(oldmask);
 	}
-	numa_free_cpumask(oldmask);
 #endif
 	return i;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH] mem: fix undefined behavior in NUMA code

[Ilya Maximets]

Hi.
Thanks for the fix.
Comments inline.

Best regards, Ilya Maximets.

On 29.08.2018 15:21, Anatoly Burakov wrote:
> When NUMA-aware hugepages config option is set, we rely on
> libnuma to tell the kernel to allocate hugepages on a specific
> NUMA node. However, we allocate node mask before we check if
> NUMA is available in the first place, which, according to
> the manpage [1], causes undefined behaviour.
> 
> Fix by only using nodemask when we have NUMA available.
> 
> [1] https://linux.die.net/man/3/numa_alloc_onnode
> 
> Bugzilla ID: 20
> 
> Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
> Cc: i.maximets@samsung.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_memory.c | 28 ++++++++++++++----------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index dbf19499e..4976eeacd 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -263,7 +263,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>  	int node_id = -1;
>  	int essential_prev = 0;
>  	int oldpolicy;
> -	struct bitmask *oldmask = numa_allocate_nodemask();
> +	struct bitmask *oldmask = NULL;
>  	bool have_numa = true;
>  	unsigned long maxnode = 0;
>  
> @@ -275,6 +275,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>  
>  	if (have_numa) {
>  		RTE_LOG(DEBUG, EAL, "Trying to obtain current memory policy.\n");
> +		oldmask = numa_allocate_nodemask();
>  		if (get_mempolicy(&oldpolicy, oldmask->maskp,
>  				  oldmask->size + 1, 0, 0) < 0) {
>  			RTE_LOG(ERR, EAL,
> @@ -390,19 +391,22 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>  
>  out:
>  #ifdef RTE_EAL_NUMA_AWARE_HUGEPAGES
> -	if (maxnode) {
> -		RTE_LOG(DEBUG, EAL,
> -			"Restoring previous memory policy: %d\n", oldpolicy);
> -		if (oldpolicy == MPOL_DEFAULT) {
> -			numa_set_localalloc();
> -		} else if (set_mempolicy(oldpolicy, oldmask->maskp,
> -					 oldmask->size + 1) < 0) {
> -			RTE_LOG(ERR, EAL, "Failed to restore mempolicy: %s\n",
> -				strerror(errno));
> -			numa_set_localalloc();
> +	if (have_numa) {
> +		if (maxnode) {
> +			RTE_LOG(DEBUG, EAL,
> +				"Restoring previous memory policy: %d\n",
> +					oldpolicy);
> +			if (oldpolicy == MPOL_DEFAULT) {
> +				numa_set_localalloc();
> +			} else if (set_mempolicy(oldpolicy, oldmask->maskp,
> +						 oldmask->size + 1) < 0) {
> +				RTE_LOG(ERR, EAL, "Failed to restore mempolicy: %s\n",
> +					strerror(errno));
> +				numa_set_localalloc();
> +			}
>  		}
> +		numa_free_cpumask(oldmask);
>  	}
> -	numa_free_cpumask(oldmask);

The original intend was to avoid ugly nested 'if's as possible.
'maxnode' is only initialized in NUMA case. So, there is no need
to check for 'has_numa'. 'numa_free_cpumask' has 'free' semantics
and checks for the argument. It is safe to call it with NULL.
If you want to be fully compliant with man page, you may use less
invasive change like this:

---
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index dbf19499e..d0b9f3a2f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -390,7 +390,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 
 out:
 #ifdef RTE_EAL_NUMA_AWARE_HUGEPAGES
-       if (maxnode) {
+       if (have_numa && maxnode) {
                RTE_LOG(DEBUG, EAL,
                        "Restoring previous memory policy: %d\n", oldpolicy);
                if (oldpolicy == MPOL_DEFAULT) {
@@ -402,7 +402,8 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
                        numa_set_localalloc();
                }
        }
-       numa_free_cpumask(oldmask);
+       if (oldmask)
+               numa_free_cpumask(oldmask);
 #endif
        return i;
 }
---

But still, checking both 'have_numa && maxnode', IMHO, is unnecessary.

As this change is cosmetic (issue doesn't produce any real bug),
I'd like to avoid changing the functional code to something less readable.
This also will complicate 'git blame' process.

What do you think?

>  #endif
>  	return i;
>  }
> 

[dpdk-dev] [PATCH v2] mem: fix undefined behavior in NUMA code

[Anatoly Burakov]

When NUMA-aware hugepages config option is set, we rely on
libnuma to tell the kernel to allocate hugepages on a specific
NUMA node. However, we allocate node mask before we check if
NUMA is available in the first place, which, according to
the manpage [1], causes undefined behaviour.

Fix by only using nodemask when we have NUMA available.

[1] https://linux.die.net/man/3/numa_alloc_onnode

Bugzilla ID: 20

Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
Cc: i.maximets@samsung.com
Cc: stable@dpdk.org

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index dbf19499e..1a2a84a65 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -263,7 +263,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 	int node_id = -1;
 	int essential_prev = 0;
 	int oldpolicy;
-	struct bitmask *oldmask = numa_allocate_nodemask();
+	struct bitmask *oldmask = NULL;
 	bool have_numa = true;
 	unsigned long maxnode = 0;
 
@@ -275,6 +275,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 
 	if (have_numa) {
 		RTE_LOG(DEBUG, EAL, "Trying to obtain current memory policy.\n");
+		oldmask = numa_allocate_nodemask();
 		if (get_mempolicy(&oldpolicy, oldmask->maskp,
 				  oldmask->size + 1, 0, 0) < 0) {
 			RTE_LOG(ERR, EAL,
@@ -401,8 +402,8 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 				strerror(errno));
 			numa_set_localalloc();
 		}
+		numa_free_cpumask(oldmask);
 	}
-	numa_free_cpumask(oldmask);
 #endif
 	return i;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] mem: fix undefined behavior in NUMA code

[Ilya Maximets]

On 20.09.2018 15:50, Anatoly Burakov wrote:
> When NUMA-aware hugepages config option is set, we rely on
> libnuma to tell the kernel to allocate hugepages on a specific
> NUMA node. However, we allocate node mask before we check if
> NUMA is available in the first place, which, according to
> the manpage [1], causes undefined behaviour.
> 
> Fix by only using nodemask when we have NUMA available.
> 
> [1] https://linux.die.net/man/3/numa_alloc_onnode
> 
> Bugzilla ID: 20
> 
> Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
> Cc: i.maximets@samsung.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_memory.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index dbf19499e..1a2a84a65 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -263,7 +263,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>  	int node_id = -1;
>  	int essential_prev = 0;
>  	int oldpolicy;
> -	struct bitmask *oldmask = numa_allocate_nodemask();
> +	struct bitmask *oldmask = NULL;
>  	bool have_numa = true;
>  	unsigned long maxnode = 0;
>  
> @@ -275,6 +275,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>  
>  	if (have_numa) {
>  		RTE_LOG(DEBUG, EAL, "Trying to obtain current memory policy.\n");
> +		oldmask = numa_allocate_nodemask();
>  		if (get_mempolicy(&oldpolicy, oldmask->maskp,
>  				  oldmask->size + 1, 0, 0) < 0) {
>  			RTE_LOG(ERR, EAL,
> @@ -401,8 +402,8 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>  				strerror(errno));
>  			numa_set_localalloc();
>  		}
> +		numa_free_cpumask(oldmask);
>  	}
> -	numa_free_cpumask(oldmask);

There will be 'oldmask' leak in case no 'socket-mem' requested.

>  #endif
>  	return i;
>  }
> 

Re: [dpdk-dev] [PATCH v2] mem: fix undefined behavior in NUMA code

[Burakov, Anatoly]

On 21-Sep-18 7:47 AM, Ilya Maximets wrote:
> On 20.09.2018 15:50, Anatoly Burakov wrote:
>> When NUMA-aware hugepages config option is set, we rely on
>> libnuma to tell the kernel to allocate hugepages on a specific
>> NUMA node. However, we allocate node mask before we check if
>> NUMA is available in the first place, which, according to
>> the manpage [1], causes undefined behaviour.
>>
>> Fix by only using nodemask when we have NUMA available.
>>
>> [1] https://linux.die.net/man/3/numa_alloc_onnode
>>
>> Bugzilla ID: 20
>>
>> Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
>> Cc: i.maximets@samsung.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_memory.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
>> index dbf19499e..1a2a84a65 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
>> @@ -263,7 +263,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>>   	int node_id = -1;
>>   	int essential_prev = 0;
>>   	int oldpolicy;
>> -	struct bitmask *oldmask = numa_allocate_nodemask();
>> +	struct bitmask *oldmask = NULL;
>>   	bool have_numa = true;
>>   	unsigned long maxnode = 0;
>>   
>> @@ -275,6 +275,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>>   
>>   	if (have_numa) {
>>   		RTE_LOG(DEBUG, EAL, "Trying to obtain current memory policy.\n");
>> +		oldmask = numa_allocate_nodemask();
>>   		if (get_mempolicy(&oldpolicy, oldmask->maskp,
>>   				  oldmask->size + 1, 0, 0) < 0) {
>>   			RTE_LOG(ERR, EAL,
>> @@ -401,8 +402,8 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
>>   				strerror(errno));
>>   			numa_set_localalloc();
>>   		}
>> +		numa_free_cpumask(oldmask);
>>   	}
>> -	numa_free_cpumask(oldmask);
> 
> There will be 'oldmask' leak in case no 'socket-mem' requested.

Oh, right, you're correct!

> 
>>   #endif
>>   	return i;
>>   }
>>
> 


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v3] mem: fix undefined behavior in NUMA code

[Anatoly Burakov]

When NUMA-aware hugepages config option is set, we rely on
libnuma to tell the kernel to allocate hugepages on a specific
NUMA node. However, we allocate node mask before we check if
NUMA is available in the first place, which, according to
the manpage [1], causes undefined behaviour.

Fix by only using nodemask when we have NUMA available.

[1] https://linux.die.net/man/3/numa_alloc_onnode

Bugzilla ID: 20

Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
Cc: i.maximets@samsung.com
Cc: stable@dpdk.org

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

Notes:
    v3:
    - Fix potential memory leak if socket-mem was not specified
    
    v2:
    - Improve readability as per Ilya's comment

 lib/librte_eal/linuxapp/eal/eal_memory.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index dbf19499e..7747ee6df 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -263,7 +263,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 	int node_id = -1;
 	int essential_prev = 0;
 	int oldpolicy;
-	struct bitmask *oldmask = numa_allocate_nodemask();
+	struct bitmask *oldmask = NULL;
 	bool have_numa = true;
 	unsigned long maxnode = 0;
 
@@ -275,6 +275,7 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 
 	if (have_numa) {
 		RTE_LOG(DEBUG, EAL, "Trying to obtain current memory policy.\n");
+		oldmask = numa_allocate_nodemask();
 		if (get_mempolicy(&oldpolicy, oldmask->maskp,
 				  oldmask->size + 1, 0, 0) < 0) {
 			RTE_LOG(ERR, EAL,
@@ -402,7 +403,8 @@ map_all_hugepages(struct hugepage_file *hugepg_tbl, struct hugepage_info *hpi,
 			numa_set_localalloc();
 		}
 	}
-	numa_free_cpumask(oldmask);
+	if (oldmask != NULL)
+		numa_free_cpumask(oldmask);
 #endif
 	return i;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3] mem: fix undefined behavior in NUMA code

[Ilya Maximets]

On 21.09.2018 12:27, Anatoly Burakov wrote:
> When NUMA-aware hugepages config option is set, we rely on
> libnuma to tell the kernel to allocate hugepages on a specific
> NUMA node. However, we allocate node mask before we check if
> NUMA is available in the first place, which, according to
> the manpage [1], causes undefined behaviour.
> 
> Fix by only using nodemask when we have NUMA available.
> 
> [1] https://linux.die.net/man/3/numa_alloc_onnode
> 
> Bugzilla ID: 20
> 
> Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
> Cc: i.maximets@samsung.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     v3:
>     - Fix potential memory leak if socket-mem was not specified
>     
>     v2:
>     - Improve readability as per Ilya's comment
> 
>  lib/librte_eal/linuxapp/eal/eal_memory.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

LGTM,
Acked-by: Ilya Maximets <i.maximets@samsung.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] mem: fix undefined behavior in NUMA code

[Thomas Monjalon]

21/09/2018 13:02, Ilya Maximets:
> On 21.09.2018 12:27, Anatoly Burakov wrote:
> > When NUMA-aware hugepages config option is set, we rely on
> > libnuma to tell the kernel to allocate hugepages on a specific
> > NUMA node. However, we allocate node mask before we check if
> > NUMA is available in the first place, which, according to
> > the manpage [1], causes undefined behaviour.
> > 
> > Fix by only using nodemask when we have NUMA available.
> > 
> > [1] https://linux.die.net/man/3/numa_alloc_onnode
> > 
> > Bugzilla ID: 20
> > 
> > Fixes: 1b72605d2416 ("mem: balanced allocation of hugepages")
> > Cc: i.maximets@samsung.com
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > ---
> > 
> > Notes:
> >     v3:
> >     - Fix potential memory leak if socket-mem was not specified
> >     
> >     v2:
> >     - Improve readability as per Ilya's comment
> > 
> >  lib/librte_eal/linuxapp/eal/eal_memory.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> LGTM,
> Acked-by: Ilya Maximets <i.maximets@samsung.com>

Applied, thanks