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

Andrew Morton
2019-02-26T22:36:25Z

>
> The number of node specific huge pages can be set via a file such as:
> /sys/devices/system/node/node1/hugepages/hugepages-2048kB/nr_hugepages
> When a node specific value is specified, the global number of huge
> pages must also be adjusted. This adjustment is calculated as the
> specified node specific value + (global value - current node value).
> If the node specific value provided by the user is large enough, this
> calculation could overflow an unsigned long leading to a smaller
> than expected number of huge pages.
>
> To fix, check the calculation for overflow. If overflow is detected,
> use ULONG_MAX as the requested value. This is inline with the user
> request to allocate as many huge pages as possible.
>
> It was also noticed that the above calculation was done outside the
> hugetlb_lock. Therefore, the values could be inconsistent and result
> in underflow. To fix, the calculation is moved to within the routine
> set_max_huge_pages() where the lock is held.
>
> ...
>
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2274,7 +2274,7 @@ static int adjust_pool_surplus(struct hstate *h,
> nodemask_t *nodes_allowed,

Please tweak that email client to prevent the wordwraps.

> + /*
> + * Check for a node specific request. Adjust global count, but
> + * restrict alloc/free to the specified node.
> + */
> + if (nid != NUMA_NO_NODE) {
> + unsigned long old_count = count;
> + count += h->nr_huge_pages - h->nr_huge_pages_node[nid];
> + /*
> + * If user specified count causes overflow, set to
> + * largest possible value.
> + */
> + if (count < old_count)
> + count = ULONG_MAX;
> + }

The above two comments explain the code, but do not reveal the
reasoning behind the policy decisions which that code implements.

> ...
>
> + } else {
> /*
> - * per node hstate attribute: adjust count to global,
> - * but restrict alloc/free to the specified node.
> + * Node specific request, but we could not allocate
> + * node mask. Pass in ALL nodes, and clear nid.
> */

Ditto here, somewhat.

The old mantra: comments should explain "why", not "what". Reading the
code tells us the "what".

Thanks.