Re: [PATCH V2,2/2] mm: madvise: skip unmapped vma holes passed to process_madvise

From: Michal Hocko <mhocko@suse.com>
To: Charan Teja Kalla <quic_charante@quicinc.com>
Cc: akpm@linux-foundation.org, surenb@google.com, vbabka@suse.cz,
	rientjes@google.com, sfr@canb.auug.org.au,
	edgararriaga@google.com, minchan@kernel.org,
	nadav.amit@gmail.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org,
	"# 5 . 10+" <stable@vger.kernel.org>
Subject: Re: [PATCH V2,2/2] mm: madvise: skip unmapped vma holes passed to process_madvise
Date: Tue, 22 Mar 2022 09:40:56 +0100	[thread overview]
Message-ID: <YjmLmBUmROr+hshO@dhcp22.suse.cz> (raw)
In-Reply-To: <7207b2f5-6b3e-aea4-aa1b-9c6d849abe34@quicinc.com>

On Tue 22-03-22 12:40:24, Charan Teja Kalla wrote:
> Thanks Michal for the inputs.
> 
> On 3/21/2022 9:04 PM, Michal Hocko wrote:
> > On Fri 11-03-22 20:59:06, Charan Teja Kalla wrote:
> >> The process_madvise() system call is expected to skip holes in vma
> >> passed through 'struct iovec' vector list.
> > Where is this assumption coming from? From the man page I can see:
> > : The advice might be applied to only a part of iovec if one of its
> > : elements points to an invalid memory region in the remote
> > : process.  No further elements will be processed beyond that
> > : point.  
> 
> I assumed this while processing a single element of a iovec. In a
> scenario where a range passed contains multiple VMA's + holes, on
> encountering the VMA with VM_LOCKED|VM_HUGETLB|VM_PFNMAP, we are
> immediately stopping further processing of that iovec element with
> EINVAL return. Where as on encountering a hole, we are simply
> remembering it as ENOMEM but continues processing that iovec element and
> in the end returns ENOMEM. This means that complete range is processed
> but still returning ENOMEM, hence the assumption of skipping holes in a
> vma.
> 
> The other problem is, in an individual iovec element, though some bytes
> are processed we may still endup in returning EINVAL which is hard for
> the user to take decisions i.e. he doesn't know at which address it is
> exactly failed to advise.
> 
> Anyway, both these will be addressed in the next version of this patch
> with the suggestions from minchan [1] where it mentioned that: "it
> should represent exact bytes it addressed with exacts ranges like
> process_vm_readv/writev. Poviding valid ranges is responsiblity from the
> user."

I would tend to agree that the userspace should be providing sensible
ranges (either subsets or full existing mappings). Whenever multiple
vmas are defined by a single iovec, things get more complicated. IMO
process_madvise should mimic the madvise semantic applied to each iovec.
That means to bail out on an error. That applies to ENOMEM even when the
last iovec has been processed completely.

This would allow to learn about address space change that the caller is
not aware of. That being said, your first patch should be good enough.
-- 
Michal Hocko
SUSE Labs