Re: [PATCH] mm/mempolicy: fix mpol_rebind_nodemask() for MPOL_F_NUMA_BALANCING

[Jinjiang Tu <tujinjiang@huawei.com>]

在 2025/12/20 3:20, Gregory Price 写道:
> On Sun, Dec 14, 2025 at 04:04:59PM -0800, Andrew Morton wrote:
>> On Sat, 13 Dec 2025 16:29:11 +0800 Jinjiang Tu <tujinjiang@huawei.com> wrote:
> ...
>> The intended behavior was for the rebinding logic to remain the same as
>> the default `MPOL_BIND` behavior.  However, the function
>> `mpol_store_user_nodemask()` was incorrectly returning `true` for
>> policies containing `MPOL_F_NUMA_BALANCING`.
>>
>> This led to a bug where:
>>
>> 1.  `mempolicy.w.cpuset_mems_allowed` stored the **user's passed
>>      nodemask** instead of the actual nodemask allowed by the current
>>      cpuset context (`cpuset_current_mems_allowed`).
>>
> Hm... these things are a union.
>
> It's probably simpler to state the following
>
> ----
>
> Setting MPOL_F_NUMA_BALANCING causes pol->w.cpuset_mems_allowed to be
> erroneously overwritten, causing mpol_rebind_nodemask to rebind the
> policy based on the wrong nodemask.
>
> 1. The intended rebind behavior of MPOL_F_NUMA_BALANCING when neither
>     MPOL_F_STATIC_NODES or MPOL_F_RELATIVE_NODES flags are present is
>     to remap nodes based on mempolicy.w.cpuset_mems_allowed.
>
> 2. `mempolicy.w.cpuset_mems_allowed` is overwritten by mpol_set_nodemask
>     setting `mempolicy.w.user_nodemask` (these are unioned) when
>     MPOL_F_NUMA_BALANCING is set because it mpol_store_user_nodemask()
>     check for any mode flag.
>
>     union {
>         nodemask_t cpuset_mems_allowed; /* relative to these nodes */
>         nodemask_t user_nodemask;       /* nodemask passed by user */
>     } w;
>
>     static inline int mpol_store_user_nodemask(const struct mempolicy *pol)
>     {
>          return pol->flags & MPOL_MODE_FLAGS;
>     }
>
>     static int mpol_set_nodemask(...)
>     {
>          if (mpol_store_user_nodemask(pol))
>                  pol->w.user_nodemask = *nodes;
>          else
>                  pol->w.cpuset_mems_allowed = cpuset_current_mems_allowed;
>     }
>
>
> 3. `mpol_rebind_nodemask()` consequently ends up rebinding based on the
>     user-passed nodemask rather than the cpuset_mems_allowed nodemask
>     as intended.
>
>     static void mpol_rebind_nodemask()
>     {
>          if (pol->flags & MPOL_F_STATIC_NODES)
>                  nodes_and(tmp, pol->w.user_nodemask, *nodes);
>          else if (pol->flags & MPOL_F_RELATIVE_NODES)
>                  mpol_relative_nodemask(&tmp, &pol->w.user_nodemask, nodes);
>          else
>                  nodes_remap(tmp, pol->nodes, pol->w.cpuset_mems_allowed,
>                                                                  *nodes);
>         ...
>     }
>
> To fix this, only store the user nodemask if MPOL_F_STATIC_NODES or
> MPOL_F_RELATIVE_NODES are present.
>
> -----------

Thanks for review, I will update the changelog and send v2.

> On another note... what's even the reason for this union to exist if you
> need to know the flag state to determine which one to access????
>
> and they're both nodemask_t!
>
> May as well call it `pol->w.rebind_mask` or something and let the flags do
> the talking.

Yes, the only difference is which type of nodemask is stored.

> Otherwise the fix looks good, I will respond to the original with a
> review tag.
>
> ~Gregory