[PATCH] mm/mempolicy: prevent the risk of division by 0

[PATCH] mm/mempolicy: prevent the risk of division by 0

[Chelsy Ratnawat]

If no bits are set in the policy's node mask, then nodes will be 0.
This patch adds a check if nodes == 0 before dividing.

Signed-off-by: Chelsy Ratnawat <chelsyratnawat2001@gmail.com>
---
 mm/mempolicy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index eb83cff7db8c..faacc604fc16 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2530,6 +2530,8 @@ static unsigned long alloc_pages_bulk_interleave(gfp_t gfp,
 	unsigned long total_allocated = 0;
 
 	nodes = nodes_weight(pol->nodes);
+	if (nodes == 0)
+		return 0;
 	nr_pages_per_node = nr_pages / nodes;
 	delta = nr_pages - nodes * nr_pages_per_node;
 
-- 
2.47.3

Re: [PATCH] mm/mempolicy: prevent the risk of division by 0

[Joshua Hahn]

On Sun,  7 Sep 2025 09:08:29 -0700 Chelsy Ratnawat <chelsyratnawat2001@gmail.com> wrote:

> If no bits are set in the policy's node mask, then nodes will be 0.
> This patch adds a check if nodes == 0 before dividing.

Hello Chelsy,

Thank you for the patch! I will start off by saying that it probably does not
hurt to add a check like this. With that said, I think it's best if we clarify:

(1) Have we seen this happen before?
(2) Should we add a Fixes tag?
(3) Do we expect to hit the nodes == 0 case, or is this just to be safe?
(4) If so, should we do more than just return 0 (i.e. WARN_ON?)

From what I can tell, I think it should not be possible for the policy to be
interleave, and the nodes to be null. If we look at mpol_new, there is an
explicit call to nodes_empty that checks if the nodes are empty, and returns
an error if that is the case (of course, excluding MPOL_DEFAULT, which actually
throws an error if the nodes are not empty).

After all, it doesn't really make semantic sense to allow interleaving across
0 nodes ; -)

But as always, it is very possible that I am missing something that you are
seeing : -) Please feel free to correct me if you believe that is the case.

With all of this said, I do agree that it's a bit scary to do a div0, even if
I am confident that the value shouldn't be zero. I don't have any strong
feelings about adding redundant checks, so I will let other mempolicy reviewers
chime in with thier opinions. One thing I do feel strongly though, is that if
we are to move forward in the patch, some explicit clarification that we
have/have not seen it happen before, and a WARN_ON would make sense. 

Thanks again for the patch. I hope you have a great rest of your weekend!
Joshua

> Signed-off-by: Chelsy Ratnawat <chelsyratnawat2001@gmail.com>
> ---
>  mm/mempolicy.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index eb83cff7db8c..faacc604fc16 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2530,6 +2530,8 @@ static unsigned long alloc_pages_bulk_interleave(gfp_t gfp,
>  	unsigned long total_allocated = 0;
>  
>  	nodes = nodes_weight(pol->nodes);
> +	if (nodes == 0)
> +		return 0;
>  	nr_pages_per_node = nr_pages / nodes;
>  	delta = nr_pages - nodes * nr_pages_per_node;
>  
> -- 
> 2.47.3

Sent using hkml (https://github.com/sjp38/hackermail)

Re: [PATCH] mm/mempolicy: prevent the risk of division by 0

[Huang, Ying]

Joshua Hahn <joshua.hahnjy@gmail.com> writes:

> On Sun,  7 Sep 2025 09:08:29 -0700 Chelsy Ratnawat <chelsyratnawat2001@gmail.com> wrote:
>
>> If no bits are set in the policy's node mask, then nodes will be 0.
>> This patch adds a check if nodes == 0 before dividing.
>
> Hello Chelsy,
>
> Thank you for the patch! I will start off by saying that it probably does not
> hurt to add a check like this. With that said, I think it's best if we clarify:
>
> (1) Have we seen this happen before?
> (2) Should we add a Fixes tag?
> (3) Do we expect to hit the nodes == 0 case, or is this just to be safe?
> (4) If so, should we do more than just return 0 (i.e. WARN_ON?)
>
> From what I can tell, I think it should not be possible for the policy to be
> interleave, and the nodes to be null. If we look at mpol_new, there is an
> explicit call to nodes_empty that checks if the nodes are empty, and returns
> an error if that is the case (of course, excluding MPOL_DEFAULT, which actually
> throws an error if the nodes are not empty).
>
> After all, it doesn't really make semantic sense to allow interleaving across
> 0 nodes ; -)
>
> But as always, it is very possible that I am missing something that you are
> seeing : -) Please feel free to correct me if you believe that is the case.
>
> With all of this said, I do agree that it's a bit scary to do a div0, even if
> I am confident that the value shouldn't be zero. I don't have any strong
> feelings about adding redundant checks, so I will let other mempolicy reviewers
> chime in with thier opinions. One thing I do feel strongly though, is that if
> we are to move forward in the patch, some explicit clarification that we
> have/have not seen it happen before, and a WARN_ON would make sense. 

IIUC, the patch is to catch the bug during kernel development.  And, if
an exception can be generated for dividing by 0, it can be used to catch
the bug already.  If so, IMHO, the patch appears unnecessary.

[snip]

---
Best Regards,
Huang, Ying

Re: [PATCH] mm/mempolicy: prevent the risk of division by 0

[David Hildenbrand]

On 08.09.25 05:48, Huang, Ying wrote:
> Joshua Hahn <joshua.hahnjy@gmail.com> writes:
> 
>> On Sun,  7 Sep 2025 09:08:29 -0700 Chelsy Ratnawat <chelsyratnawat2001@gmail.com> wrote:
>>
>>> If no bits are set in the policy's node mask, then nodes will be 0.
>>> This patch adds a check if nodes == 0 before dividing.
>>
>> Hello Chelsy,
>>
>> Thank you for the patch! I will start off by saying that it probably does not
>> hurt to add a check like this. With that said, I think it's best if we clarify:
>>
>> (1) Have we seen this happen before?
>> (2) Should we add a Fixes tag?
>> (3) Do we expect to hit the nodes == 0 case, or is this just to be safe?
>> (4) If so, should we do more than just return 0 (i.e. WARN_ON?)
>>
>>  From what I can tell, I think it should not be possible for the policy to be
>> interleave, and the nodes to be null. If we look at mpol_new, there is an
>> explicit call to nodes_empty that checks if the nodes are empty, and returns
>> an error if that is the case (of course, excluding MPOL_DEFAULT, which actually
>> throws an error if the nodes are not empty).
>>
>> After all, it doesn't really make semantic sense to allow interleaving across
>> 0 nodes ; -)
>>
>> But as always, it is very possible that I am missing something that you are
>> seeing : -) Please feel free to correct me if you believe that is the case.
>>
>> With all of this said, I do agree that it's a bit scary to do a div0, even if
>> I am confident that the value shouldn't be zero. I don't have any strong
>> feelings about adding redundant checks, so I will let other mempolicy reviewers
>> chime in with thier opinions. One thing I do feel strongly though, is that if
>> we are to move forward in the patch, some explicit clarification that we
>> have/have not seen it happen before, and a WARN_ON would make sense.
> 
> IIUC, the patch is to catch the bug during kernel development.  And, if
> an exception can be generated for dividing by 0, it can be used to catch
> the bug already.  If so, IMHO, the patch appears unnecessary.

We don't want to add runtime checks for things that are supposed to be 
caught early during testing. Usually we use VM_WARN_* for sanity 
checking such things, but the div0 would also already trigger early 
during testing, no?


-- 
Cheers

David / dhildenb

Re: [PATCH] mm/mempolicy: prevent the risk of division by 0

[Gregory Price]

On Sun, Sep 07, 2025 at 09:08:29AM -0700, Chelsy Ratnawat wrote:
> If no bits are set in the policy's node mask, then nodes will be 0.
> This patch adds a check if nodes == 0 before dividing.
> 
> Signed-off-by: Chelsy Ratnawat <chelsyratnawat2001@gmail.com>
> ---
>  mm/mempolicy.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index eb83cff7db8c..faacc604fc16 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2530,6 +2530,8 @@ static unsigned long alloc_pages_bulk_interleave(gfp_t gfp,
>  	unsigned long total_allocated = 0;
>  
>  	nodes = nodes_weight(pol->nodes);
> +	if (nodes == 0)
> +		return 0;

3 second look at this code would tell you the only way this can happen
is a mempolicy where MPOL_INTERLEAVE is set and the nodemask is empty

The only way this can occur is

1) someone setting that explicitly, which is invalid and protected by
   a line in mpol_new:

   else if (nodes_empty(*nodes))
	return ERR_PTR(-EINVAL);

2) or a rebind event occurring due to a cgroup task migration or
   cpuset change - which is protected by a check in the rebind:

   if (nodes_empty(tmp))
	tmp = *nodes;


I doubt this bug was observed in action.

>  	nr_pages_per_node = nr_pages / nodes;
>  	delta = nr_pages - nodes * nr_pages_per_node;
>  
> -- 
> 2.47.3
> 
> 

Re: [PATCH] mm/mempolicy: prevent the risk of division by 0

[Gregory Price]

On Mon, Sep 08, 2025 at 02:12:29PM -0400, Gregory Price wrote:
> On Sun, Sep 07, 2025 at 09:08:29AM -0700, Chelsy Ratnawat wrote:
> > If no bits are set in the policy's node mask, then nodes will be 0.
> > This patch adds a check if nodes == 0 before dividing.
> > 
> > Signed-off-by: Chelsy Ratnawat <chelsyratnawat2001@gmail.com>
> > ---
> >  mm/mempolicy.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index eb83cff7db8c..faacc604fc16 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -2530,6 +2530,8 @@ static unsigned long alloc_pages_bulk_interleave(gfp_t gfp,
> >  	unsigned long total_allocated = 0;
> >  
> >  	nodes = nodes_weight(pol->nodes);
> > +	if (nodes == 0)
> > +		return 0;
> 
> 3 second look at this code would tell you the only way this can happen
> is a mempolicy where MPOL_INTERLEAVE is set and the nodemask is empty
>

after some more thought... there is actually a race condition here that
is non-obvious

in some places we take a cpusets cookie to prevent pol->nodes from ever
showing up empty while iterating over the policy nodes.

Excerpt from: alloc_pages_bulk_weighted_interleave

```
        /* read the nodes onto the stack, retry if done during rebind */
        do {
                cpuset_mems_cookie = read_mems_allowed_begin();
                nnodes = read_once_policy_nodemask(pol, &nodes);
        } while (read_mems_allowed_retry(cpuset_mems_cookie));

        /* if the nodemask has become invalid, we cannot do anything */
        if (!nnodes)
                return 0;
```

So this actually can happen - otherwise I wouldn't have bothered with
the stack read in this code chunk anyway.

So now i've argued with and against myself, i think this patch
as-written is not sufficient - and in fact there's a very subtle bug in
the normal interleave that isn't present in the weighted interleave
because we take a copy of the nodes.

So mea culpa for the snap read and response.  But could you please
take a look at the weighted interleave code and model the fix off
of that?

~Gregory