Re: [PATCH RFC v2 4/4] mm/mempolicy: add nodes_empty check in SYSC_migrate_pages

On 11/06/2017 02:31 AM, Yisheng Xie wrote:

Hi Vlastimil,

On 2017/10/31 17:46, Vlastimil Babka wrote:

quoted

+CC Andi and Christoph

On 10/27/2017 12:14 PM, Yisheng Xie wrote:

quoted

As manpage of migrate_pages, the errno should be set to EINVAL when none
of the specified nodes contain memory. However, when new_nodes is null,
i.e. the specified nodes also do not have memory, as the following case:

	new_nodes = 0;
	old_nodes = 0xf;
	ret = migrate_pages(pid, old_nodes, new_nodes, MAX);

The ret will be 0 and no errno is set.

This patch is to add nodes_empty check to fix above case.

Hmm, I think we have a bigger problem than "empty set is a subset of
anything" here.

The existing checks are:

        task_nodes = cpuset_mems_allowed(task);
        if (!nodes_subset(*new, task_nodes) && !capable(CAP_SYS_NICE)) {
                err = -EPERM;
                goto out_put;
        }

        if (!nodes_subset(*new, node_states[N_MEMORY])) {
                err = -EINVAL;
                goto out_put;
        }


And manpage says:

       EINVAL The value specified by maxnode exceeds a kernel-imposed
limit.  Or, old_nodes or new_nodes specifies one or more node IDs that
are greater than the maximum supported node
              ID.  *Or, none of the node IDs specified by new_nodes are
on-line and allowed by the process's current cpuset context, or none of
the specified nodes contain memory.*

       EPERM  Insufficient privilege (CAP_SYS_NICE) to move pages of the
process specified by pid, or insufficient privilege (CAP_SYS_NICE) to
access the specified target nodes.

- it says "none ... are allowed", but checking for subset means we check
if "all ... are allowed". Shouldn't we be checking for a non-empty
intersection?

You are absolutely right. To follow the manpage, we should check non-empty
of intersection instead of subset. I mean：
         nodes_and(*new, *new, task_nodes);
         if (!node_empty(*new) && !capable(CAP_SYS_NICE)) {
                 err = -EPERM;
                 goto out_put;
         }

         nodes_and(*new, *new, node_states[N_MEMORY]);
         if (!node_empty(*new)) {
                 err = -EINVAL;
                 goto out_put;
         }

Maybe not exactly like this, see below.

So finally, we should only migrate the smallest intersection of all the node
set, right?

That's right.

So if new_nodes AND task_nodes AND node_states[N_MEMORY] is empty, then
EINVAL.

I'm not sure what exactly is the EPERM intention. Should really the
capability of THIS process override the cpuset restriction of the TARGET
process? Maybe yes. Then, does "insufficient privilege (CAP_SYS_NICE) to
access the specified target nodes." mean that at least some nodes must
be allowed, or all of them? Maybe the subset check is after all OK for
the EPERM check, but still wrong for the EINVAL check.

quoted

- there doesn't seem to be any EINVAL check for "process's current
cpuset context", there's just an EPERM check for "target process's
cpuset context".

This also need to be checked as manpage.

Thanks
Yisheng Xie