[PATCH] mm/hugetlb: Fix unsigned overflow in __nr_hugepages_store_common()

[PATCH] mm/hugetlb: Fix unsigned overflow in __nr_hugepages_store_common()

[Jingxiangfeng]

From: Jing Xiangfeng <jingxiangfeng@huawei.com>

We can use the following command to dynamically allocate huge pages:
	echo NR_HUGEPAGES > /proc/sys/vm/nr_hugepages
The count in  __nr_hugepages_store_common() is parsed from /proc/sys/vm/nr_hugepages,
The maximum number of count is ULONG_MAX,
the operation 'count += h->nr_huge_pages - h->nr_huge_pages_node[nid]' overflow and count will be a wrong number.
So check the overflow to fix this problem.

Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
---
 mm/hugetlb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index afef616..55173c3 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2423,7 +2423,12 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
 		 * per node hstate attribute: adjust count to global,
 		 * but restrict alloc/free to the specified node.
 		 */
+		unsigned long old_count = count;
 		count += h->nr_huge_pages - h->nr_huge_pages_node[nid];
+		if (count < old_count) {
+			err = -EINVAL;
+			goto out;
+		}
 		init_nodemask_of_node(nodes_allowed, nid);
 	} else
 		nodes_allowed = &node_states[N_MEMORY];
-- 
2.7.4

Re: [PATCH] mm/hugetlb: Fix unsigned overflow in __nr_hugepages_store_common()

[Michal Hocko]

On Sat 16-02-19 21:31:12, Jingxiangfeng wrote:
> From: Jing Xiangfeng <jingxiangfeng@huawei.com>
> 
> We can use the following command to dynamically allocate huge pages:
> 	echo NR_HUGEPAGES > /proc/sys/vm/nr_hugepages
> The count in  __nr_hugepages_store_common() is parsed from /proc/sys/vm/nr_hugepages,
> The maximum number of count is ULONG_MAX,
> the operation 'count += h->nr_huge_pages - h->nr_huge_pages_node[nid]' overflow and count will be a wrong number.

Could you be more specific of what is the runtime effect on the
overflow? I haven't checked closer but I would assume that we will
simply shrink the pool size because count will become a small number.

Is there any reason to report an error in that case? We do not report
errors when we cannot allocate the requested number of huge pages so why
is this case any different?

> So check the overflow to fix this problem.
> 
> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> ---
>  mm/hugetlb.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index afef616..55173c3 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2423,7 +2423,12 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
>  		 * per node hstate attribute: adjust count to global,
>  		 * but restrict alloc/free to the specified node.
>  		 */
> +		unsigned long old_count = count;
>  		count += h->nr_huge_pages - h->nr_huge_pages_node[nid];
> +		if (count < old_count) {
> +			err = -EINVAL;
> +			goto out;
> +		}
>  		init_nodemask_of_node(nodes_allowed, nid);
>  	} else
>  		nodes_allowed = &node_states[N_MEMORY];
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/hugetlb: Fix unsigned overflow in __nr_hugepages_store_common()

[Mike Kravetz]

On 2/18/19 1:27 AM, Michal Hocko wrote:
> On Sat 16-02-19 21:31:12, Jingxiangfeng wrote:
>> From: Jing Xiangfeng <jingxiangfeng@huawei.com>
>>
>> We can use the following command to dynamically allocate huge pages:
>> 	echo NR_HUGEPAGES > /proc/sys/vm/nr_hugepages
>> The count in  __nr_hugepages_store_common() is parsed from /proc/sys/vm/nr_hugepages,
>> The maximum number of count is ULONG_MAX,
>> the operation 'count += h->nr_huge_pages - h->nr_huge_pages_node[nid]' overflow and count will be a wrong number.
> 
> Could you be more specific of what is the runtime effect on the
> overflow? I haven't checked closer but I would assume that we will
> simply shrink the pool size because count will become a small number.
> 

Well, the first thing to note is that this code only applies to case where
someone is changing a node specific hugetlb count.  i.e.
/sys/devices/system/node/node1/hugepages/hugepages-2048kB
In this case, the calculated value of count is a maximum or minimum total
number of huge pages.  However, only the number of huge pages on the specified
node is adjusted to try and achieve this maximum or minimum.

So, in the case of overflow the number of huge pages on the specified node
could be reduced.  I say 'could' because it really is dependent on previous
values.  In some situations the node specific value will be increased.

Minor point is that the description in the commit message does not match
the code changed.

> Is there any reason to report an error in that case? We do not report
> errors when we cannot allocate the requested number of huge pages so why
> is this case any different?

Another issue to consider is that h->nr_huge_pages is an unsigned long,
and h->nr_huge_pages_node[] is an unsigned int.  The sysfs store routines
treat them both as unsigned longs.  Ideally, the store routines should
distinguish between the two.

In reality, an actual overflow is unlikely.  If my math is correct (not
likely) it would take something like 8 Petabytes to overflow the node specific
counts.

In the case of a user entering a crazy high value and causing an overflow,
an error return might not be out of line.  Another option would be to simply
set count to ULONG_MAX if we detect overflow (or UINT_MAX if we are paranoid)
and continue on.  This may be more in line with user's intention of allocating
as many huge pages as possible.

Thoughts?
-- 
Mike Kravetz

Re: [PATCH] mm/hugetlb: Fix unsigned overflow in __nr_hugepages_store_common()

[Jing Xiangfeng]

On 2019/2/20 7:45, Mike Kravetz wrote:
> On 2/18/19 1:27 AM, Michal Hocko wrote:
>> On Sat 16-02-19 21:31:12, Jingxiangfeng wrote:
>>> From: Jing Xiangfeng <jingxiangfeng@huawei.com>
>>>
>>> We can use the following command to dynamically allocate huge pages:
>>> 	echo NR_HUGEPAGES > /proc/sys/vm/nr_hugepages
>>> The count in  __nr_hugepages_store_common() is parsed from /proc/sys/vm/nr_hugepages,
>>> The maximum number of count is ULONG_MAX,
>>> the operation 'count += h->nr_huge_pages - h->nr_huge_pages_node[nid]' overflow and count will be a wrong number.
>>
>> Could you be more specific of what is the runtime effect on the
>> overflow? I haven't checked closer but I would assume that we will
>> simply shrink the pool size because count will become a small number.
>>
> 
> Well, the first thing to note is that this code only applies to case where
> someone is changing a node specific hugetlb count.  i.e.
> /sys/devices/system/node/node1/hugepages/hugepages-2048kB
> In this case, the calculated value of count is a maximum or minimum total
> number of huge pages.  However, only the number of huge pages on the specified
> node is adjusted to try and achieve this maximum or minimum.
> 
> So, in the case of overflow the number of huge pages on the specified node
> could be reduced.  I say 'could' because it really is dependent on previous
> values.  In some situations the node specific value will be increased.
> 
> Minor point is that the description in the commit message does not match
> the code changed.
> 
Thanks for your reply.as you said, the case is where someone is changing a node
specific hugetlb count when CONFIG_NUMA is enable. I will modify the commit message.

>> Is there any reason to report an error in that case? We do not report
>> errors when we cannot allocate the requested number of huge pages so why
>> is this case any different?
> 
> Another issue to consider is that h->nr_huge_pages is an unsigned long,
> and h->nr_huge_pages_node[] is an unsigned int.  The sysfs store routines
> treat them both as unsigned longs.  Ideally, the store routines should
> distinguish between the two.
> 
> In reality, an actual overflow is unlikely.  If my math is correct (not
> likely) it would take something like 8 Petabytes to overflow the node specific
> counts.
> 
> In the case of a user entering a crazy high value and causing an overflow,
> an error return might not be out of line.  Another option would be to simply
> set count to ULONG_MAX if we detect overflow (or UINT_MAX if we are paranoid)
> and continue on.  This may be more in line with user's intention of allocating
> as many huge pages as possible.
> 
> Thoughts?
> 
It is better to set count to ULONG_MAX if we detect overflow, and continue to
allocate as many huge pages as possible.
I will send v2 soon.