linux-kernel - Re: [PATCH 2/2] mm: madvise: return exact bytes advised with 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Yjxutr35QLGhjJ57@dhcp22.suse.cz>
Date:   Thu, 24 Mar 2022 14:14:30 +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 2/2] mm: madvise: return exact bytes advised with
 process_madvise under error

On Wed 23-03-22 20:54:10, Charan Teja Kalla wrote:
> From: Charan Teja Reddy <quic_charante@...cinc.com>
> 
> The commit 5bd009c7c9a9 ("mm: madvise: return correct bytes advised with
> process_madvise") fixes the issue to return number of bytes that are
> successfully advised before hitting error with iovec elements
> processing. But, when the user passed unmapped ranges in iovec, the
> syscall ignores these holes and continues processing and returns ENOMEM
> in the end, which is same as madvise semantic. This is a problem for
> vector processing where user may want to know how many bytes were
> exactly processed in a iovec element to make better decissions in the
> user space. As in ENOMEM case, we processed all bytes in a iovec element
> but still returned error which will confuse the user whether it is
> failed or succeeded to advise.

Do you have any specific example where the initial semantic is really
problematic or is this mostly a theoretical problem you have found when
reading the code?


> As an example, consider below ranges were passed by the user in struct
> iovec: iovec1(ranges: vma1), iovec2(ranges: vma2 -- vma3 -- hole) and
> iovec3(ranges: vma4). In the current implementation, it fully advise
> iovec1 and iovec2 but just returns number of processed bytes as iovec1
> range. Then user may repeat the processing of iovec2, which is already
> processed, which then returns with ENOMEM. Then user may want to skip
> iovec2 and starts processing from iovec3. Here because of wrong return
> processed bytes, iovec2 is processed twice.

I think you should be much more specific why this is actually a problem.
This would surely be less optimal but is this a correctness issue?

[...]
> +	vma = find_vma_prev(mm, start, &prev);
> +	if (vma && start > vma->vm_start)
> +		prev = vma;
> +
> +	blk_start_plug(&plug);
> +	for (;;) {
> +		/*
> +		 * It it hits a unmapped address range in the [start, end),
> +		 * stop processing and return ENOMEM.
> +		 */
> +		if (!vma || start < vma->vm_start) {
> +			error = -ENOMEM;
> +			goto out;
> +		}
> +
> +		tmp = vma->vm_end;
> +		if (end < tmp)
> +			tmp = end;
> +
> +		error = madvise_vma_behavior(vma, &prev, start, tmp, behavior);
> +		if (error)
> +			goto out;
> +		tmp_bytes_advised += tmp - start;
> +		start = tmp;
> +		if (prev && start < prev->vm_end)
> +			start = prev->vm_end;
> +		if (start >= end)
> +			goto out;
> +		if (prev)
> +			vma = prev->vm_next;
> +		else
> +			vma = find_vma(mm, start);
> +	}
> +out:
> +	/*
> +	 * partial_bytes_advised may contain non-zero bytes indicating
> +	 * the number of bytes advised before failure. Holds zero incase
> +	 * of success.
> +	 */
> +	*partial_bytes_advised = error ? tmp_bytes_advised : 0;

Although this looks like a fix I am not sure it is future proof.
madvise_vma_behavior doesn't report which part of the range has been
really processed. I do not think that currently supported madvise modes
for process_madvise support an early break out with return to the
userspace (madvise_cold_or_pageout_pte_range bails on fatal signals for
example) but this can change in the future and then you are back to
"imprecise" return value problem. Yes, this is a theoretical problem
but so it sounds the problem you are trying to fix IMHO. I think it
would be better to live with imprecise return values reporting rather
than aiming for perfection which would be fragile and add a future
maintenance burden.

On the other hand if there are _real_ workloads which suffer from the
existing semantic then sure the above seems to be an appropriate fix
AFAICS.
-- 
Michal Hocko
SUSE Labs