From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751431AbdKFBcB (ORCPT ); Sun, 5 Nov 2017 20:32:01 -0500 Received: from szxga06-in.huawei.com ([45.249.212.32]:55584 "EHLO huawei.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1750778AbdKFBb7 (ORCPT ); Sun, 5 Nov 2017 20:31:59 -0500 Subject: Re: [PATCH RFC v2 4/4] mm/mempolicy: add nodes_empty check in SYSC_migrate_pages To: Vlastimil Babka , , , , , , References: <1509099265-30868-1-git-send-email-xieyisheng1@huawei.com> <1509099265-30868-5-git-send-email-xieyisheng1@huawei.com> CC: , , , , Andi Kleen , Christoph Lameter From: Yisheng Xie Message-ID: Date: Mon, 6 Nov 2017 09:31:44 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.177.29.40] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Vlastimil, On 2017/10/31 17:46, Vlastimil Babka wrote: > +CC Andi and Christoph > > On 10/27/2017 12:14 PM, Yisheng Xie wrote: >> 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; } So finally, we should only migrate the smallest intersection of all the node set, right? > - 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