Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3846d4ff-c8ab-3c44-1974-fee451894c0d@quicinc.com>
Date:   Thu, 10 Mar 2022 15:04:26 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH] mm: madvise: return correct bytes advised with
 process_madvise
Content-Language: en-US
To:     Nadav Amit <nadav.amit@gmail.com>, Minchan Kim <minchan@kernel.org>
CC:     Andrew Morton <akpm@linux-foundation.org>, <yuehaibing@huawei.com>,
        Stephen Rothwell <sfr@canb.auug.org.au>,
        David Rientjes <rientjes@google.com>,
        <edgararriaga@google.com>, Michal Hocko <mhocko@suse.com>,
        Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <1646803679-11433-1-git-send-email-quic_charante@quicinc.com>
 <YijaP7cC6Sclxc29@google.com>
 <B156E4D9-C3DD-4EBD-ACE2-CA55A8B3A3F5@gmail.com>
From:   Charan Teja Kalla <quic_charante@quicinc.com>
In-Reply-To: <B156E4D9-C3DD-4EBD-ACE2-CA55A8B3A3F5@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thanks Amit for the inputs!!

On 3/10/2022 12:20 AM, Nadav Amit wrote:
> ---
> mm/madvise.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 38d0f51..d3b49b3 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1426,15 +1426,21 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> 
> 	while (iov_iter_count(&iter)) {
> 		iovec = iov_iter_iovec(&iter);
> +		/*
> +		 * Even when [start, end) passed to do_madvise covers
> +		 * some unmapped addresses, it continues processing with
> +		 * returning ENOMEM at the end. Thus consider the range
> +		 * as processed when do_madvise() returns ENOMEM.
> +		 * This makes process_madvise() never returns ENOMEM.
> +		 */
> 
> I fully understand and relate to the basic motivation of this
> patch.
> 
> The ENOMEM that this patch checks for, IIUC, is the ENOMEM that is
> returned on unmapped holes. Such ENOMEM does not appear, according to
> the man page, to be a valid reason to return ENOMEM to userspace.
> Presumably process_madvise() is expected to skip unmapped holes
> and not to fail because of them>
True, that ENOMEM represents the VMA passed contains the unmapped holes.
Pasting the Documentation of do_madvise():
 *  -ENOMEM - addresses in the specified range are not currently
 *              mapped, or are outside the AS of the process.

Internally process_madvise() calls do_madvise() in a loop by passing the
vma it received in 'struct iovec'.  And I too agree here that
process_madvise() is expected to process the unmapped holes.

> Having said that, I do not think that the check that the patch does
> is clean or clearly documented.

If it is about the Documentation, how about adding: "Since
process_madvise() is expected to process unmapped holes, never return
ENOMEM received from do_madvise() to user". If the code changes can be
made further cleaner, please suggest.

> 
> In addition, this patch (and some work on process_madvise()) raise
> in my mind a couple of questions:
> 
> 1. There are other errors that process_madvise might encounter
>    and can be propagated back to userspace, but are not
>    documented. For instance if can_madv_lru_vma() fails on
>    MADV_COLD, userspace will get EINVAL. EINVAL is not documented
>    as a valid error-code for such case in either madvise() and
>    process_madvise() man pages.

I agree here with the man page documentations too and felt the same
while going through them. For the mentioned case too, in the madvise[1]
man page, EINVAL return type is only talked for MADV_DONTNEED  and
MADV_REMOVE. It should also contains for MADV_PAGEOUT, MADV_COLD and as
well for MADV_FREE. The other missing return types, which I came across,
in process_madvise are:
EINVAL - return from process_madvise_behavior_valid().
EINTR  - from mm_access()
EACCES - from mm_access()

Thanks,
Charan