linux-kernel - Re: [PATCH 1/2] Revert "mm: madvise: skip unmapped vma holes passed to process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yjxoha4PLlKjEayl@dhcp22.suse.cz>
Date:   Thu, 24 Mar 2022 13:48:05 +0100
From:   Michal Hocko <mhocko@...e.com>
To:     Charan Teja Kalla <quic_charante@...cinc.com>
Cc:     akpm@...ux-foundation.org, minchan@...nel.org, surenb@...gle.com,
        vbabka@...e.cz, rientjes@...gle.com, nadav.amit@...il.com,
        edgararriaga@...gle.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] Revert "mm: madvise: skip unmapped vma holes passed
 to process_madvise"

On Wed 23-03-22 20:54:09, Charan Teja Kalla wrote:
> This reverts commit 08095d6310a7 ("mm: madvise: skip unmapped vma holes
> passed to process_madvise") as process_madvise() fails to return exact
> processed bytes at other cases too. As an example: if the
> process_madvise() hits mlocked pages after processing some initial bytes
> passed in [start, end), it just returns EINVAL though some bytes are
> processed. Thus making an exception only for ENOMEM is partially fixing
> the problem of returning the proper advised bytes.
> 
> Thus revert this patch and return proper bytes advised, if there any,
> for all the error types in the following patch.

I do agree with the revert. I am not sure the above really is a proper
justification though. 08095d6310a7 was changing one (arguably) dubious
semantic by another one without a proper justification and wider
consensus which I would expect from a patch which changes an existing
semantic. Not to mention it being marked for stable tree.

But let's not nit pick on that now. Let's send this revert ASAP and use
some more time to discuss the semantic and whether any change is really
required.

> Signed-off-by: Charan Teja Kalla <quic_charante@...cinc.com>

Acked-by: Michal Hocko <mhocko@...e.com>

> ---
>  mm/madvise.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 39b712f..0d8fd17 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1433,16 +1433,9 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
>  
>  	while (iov_iter_count(&iter)) {
>  		iovec = iov_iter_iovec(&iter);
> -		/*
> -		 * do_madvise returns ENOMEM if unmapped holes are present
> -		 * in the passed VMA. process_madvise() is expected to skip
> -		 * unmapped holes passed to it in the 'struct iovec' list
> -		 * and not fail because of them. Thus treat -ENOMEM return
> -		 * from do_madvise as valid and continue processing.
> -		 */
>  		ret = do_madvise(mm, (unsigned long)iovec.iov_base,
>  					iovec.iov_len, behavior);
> -		if (ret < 0 && ret != -ENOMEM)
> +		if (ret < 0)
>  			break;
>  		iov_iter_advance(&iter, iovec.iov_len);
>  	}
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs