From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 84D60CD611D for ; Mon, 9 Oct 2023 18:19:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DEDFA6B024B; Mon, 9 Oct 2023 14:19:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D9DE56B024D; Mon, 9 Oct 2023 14:19:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C8CA06B024E; Mon, 9 Oct 2023 14:19:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id BAA986B024B for ; Mon, 9 Oct 2023 14:19:33 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 832F712039F for ; Mon, 9 Oct 2023 18:19:33 +0000 (UTC) X-FDA: 81326735826.20.7880848 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by imf26.hostedemail.com (Postfix) with ESMTP id 9755D140024 for ; Mon, 9 Oct 2023 18:19:31 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=aM00HomL; spf=pass (imf26.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.48 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1696875571; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=++ijn284zfH6P1kONLuhHN+YYK1WM6GxC/tHCJj4268=; b=QF/MM3TRP7Ym1IOzbc3EG03RY3tM6Vbll6/OUfb62aFcErnyxphAFKz9uGwltJlR8xWDGW dCDNllqcz4Z0cf7GmMAJV8Pta9fqspCoOzGOjzTAxrFtyW/oWBXt4ZXMkZjp+VmZpcoQ4D 8kwdLX6fbuYMRnNQq4kyLqyCb0tn+Xc= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=aM00HomL; spf=pass (imf26.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.48 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1696875571; a=rsa-sha256; cv=none; b=gDYgNwSNlWfDy59njWkk/FkgeXK5hSEYWbRu9Ol1dl01+GrfyaKJQp2m8rSsQ98mMKBrJ2 X+pGgztVRsOkVaXAC5gIzpOniDTrmbokqnS/OVeen24r/ugRFBj1cTFjiudc2imcwdrH1s 40ebPQ53q5mP2t6PKM8ZT7zsbOMAgJk= Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-4053c6f0db8so44948125e9.3 for ; Mon, 09 Oct 2023 11:19:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1696875570; x=1697480370; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=++ijn284zfH6P1kONLuhHN+YYK1WM6GxC/tHCJj4268=; b=aM00HomLG9NhflLwC8d9DvGUW7mVcHZ1YGbyHd83rINjRgLEjF7j7emtFuhGjaUCd0 ax0WWs6M9GQHF4zWYTxwRFIZeCIM76TGgl6sIOcVwFHrOIXtBlqYJKcbrgTxSOh+Y1Qe HLfLeviDqKa8fG3MEyGAT54Hpp6xubOkJPAZOYFEMji+OBc0wYHk6puIwQUvAqgDJkOU /78XKs/Pse3tWIX0xfdUZcR3aIJG0ZOgJlV5UyKw4wgD88kl0MoJOW64jW0lVraQE69Q 4yq4OlJd7ce8dgHWXJ9sTcKqDnBEBBD6N5wqJ3tH1ZL2nDpRL7+aBzP3jHE2DngsGtAS cinA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696875570; x=1697480370; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=++ijn284zfH6P1kONLuhHN+YYK1WM6GxC/tHCJj4268=; b=NanqmkHUVJZQTNUuR6/lPSm22hIlb4P0hM2JlQwAdN+07P37cT2qHzJlaTR5DwXFdW D1eF6+/cc/TDOYFQL+U1e5e3pQ6Y18tAIQ41rWA9rjwaCsIr1L0E87TmJAM6KZgxdsdZ Zdil3NHnkXcX+SFktrumBlBXX8zkbcjSyWWUIT3G1LJsSbB07vRIG4444LFxoATSI9cu RqCpAbxeHJJBTLUfWigMUYfS9xAgFc5GU9i1p0ouIi26LecBPCt0LmkfUinpqt4fFKyS rx/zIQYGIhyycqkZHe6DR93lYAZqGvx4wHOaSSQB0vtMtKU1/CaCTP4/H+c6UFJgMKYP FcaQ== X-Gm-Message-State: AOJu0YxCpBf2S+e/xokXAA1GVVkQxOumjsKoN9IvcYt0HPrwcP+DEhV/ 995JcSWvi6wmkkrGEyQFSOuvJwvYMss= X-Google-Smtp-Source: AGHT+IHSRLck2Pb7IQLpIWXEUMb/xfnErOkkK568VmsUtVQM+T+bQElHaXCDuYfJcfc70QcgkvHKHQ== X-Received: by 2002:a7b:c40f:0:b0:403:bb04:2908 with SMTP id k15-20020a7bc40f000000b00403bb042908mr14196717wmi.23.1696875569774; Mon, 09 Oct 2023 11:19:29 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id z3-20020adfec83000000b0032327b70ef6sm10400770wrn.70.2023.10.09.11.19.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Oct 2023 11:19:28 -0700 (PDT) Date: Mon, 9 Oct 2023 19:19:27 +0100 From: Lorenzo Stoakes To: Vlastimil Babka Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Alexander Viro , Christian Brauner , "=Liam R . Howlett" , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 1/4] mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al. Message-ID: <5f5274b8-f2fe-4e4a-850b-2a383778c4d3@lucifer.local> References: <6feb6f37-dfb9-0fe1-1303-2744ad2758d9@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6feb6f37-dfb9-0fe1-1303-2744ad2758d9@suse.cz> X-Rspamd-Queue-Id: 9755D140024 X-Rspam-User: X-Stat-Signature: de5jbwko3m3eecmcc41qips89hqxiiqt X-Rspamd-Server: rspam01 X-HE-Tag: 1696875571-990739 X-HE-Meta: U2FsdGVkX1++gSLDZDfpNkgHMGfhhcpq2QLJSRGHh23k2thAAM9MyNuvhyNw4BVOIqDIqyX9MDdV7HND14VFN8cfr/vNk06mzpJAHNU+vJxAOtGrXN7uaiggu5oRMrL/HK9JJicU1dsA4z0Q8yYYiJLcPXzn0mLlqBDlnRvpu6hYc4H00tJl0g+493utLtwSC9YL/tYprVseYinDLY5KrNhkjfnU1LXFl5XUcj5NsH5aQhkQlyQPHHVVdd3eHbFGOw6bpTukkdK6amPNu7i4vHK72tYluG4UoJEU98noWA/VYYNpq0Fxh6RARLU7xo9v6dxu/tNyzPQX2Bp7xCXlXxtmBI7Gm+tWl6CBkhh1pgwnxpUMJeHcIBgW/LCg5ZdXAuB0zzd2+XHjoNbyBcoCBTeAWfL3PvBrhC7l8Makv/GpNJ8pH4igo0B0At6pFhZnTkywBUKbb5enW1Dp1Xi44s1NGgv15AyWktuPGATrAt4VflFw5KKAkLPMuAol+BJkCHhpPf/NmrDFfFu3PuHcOSmQhuR931QWQkxGCNk86xU0Tyd3rs1SdI1eIoy+Er1YZqVCtH4hCf61SeLE/AD4gAlTT7iklu03pUxDQTYyR/XFgnofbwRTf5+QEYfUcytJAYsiQj+SdgfBQpgR1XGAM7HotaaicE8gBe0l8eyCXCTZXS+t7hqzaAjaaHShM1hkEPmE3LY8g/Ojfu2LcdTk+HK2TEgKTO8CrDn3eusIg9vYh/Lb/pfs88DyOyn/RypNebr4nZqp8ILAHCv8t1EYIShFSGjxZfgoJGQUhlfUh/zrw5JIgJShYkAyVs36c4D+Z13zTPG+RNgnnWmHWGUVN5ZaPgST7g0im+HYTM0lNhFDOsQ8F165BEzcfbZiJJLIFLsScqUKrr2Z476EWlr+NJM9MT8dD8FyQM9NCZxcu84VudUIB2mDRE2UPMdcTsu32QmsU2BUn6XbCkn4OK+ MY54UJun CxjGf68wLcI+MIVPDHROd4XCE10+b3BBU6N1GJEXKfPimgbnymPcTrMcq8EiPZACyh2hkNvDm/6dyy5d9egEEAPDQRPDivi8HeOi1uYswj5Lg03k20+nMXF2kgMqy6TJakszq/RNukfBwT1F1m3Gayh3jxxmy2V4VlW5QYTjo1oN4jMmTU4G6lJucnK9Gfe5L0cRcnPVtPr48/payGJ8Zx0+rDRhGW8DdwEEDlUeGSoonEESbfqEtPu4nUjjTGT8HquVke+VEsjQgPSCCCBQoxmaLDeUpj7o6vNmzDRcr81jhxBRxeyiOT4AbyxfFiC6XfXAsPKfQe8IFX+IE+0qWo6AaYF+2htO9dRLF2MIWO249QCEk0nkQuDFnjijtVDPKovmuQhptzuvlnU7ZcUFMRf7jizFZOIYSNog5lIbWPIfS6vjYpu1u0W4910bEOWcKpHbzaCITG10RJDAPp8+xTVsoGisHdvDr8flI/zIxx2HZR7j6nToJmRJe8g6m3bf6rXRT X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Oct 09, 2023 at 05:22:33PM +0200, Vlastimil Babka wrote: > On 10/8/23 22:23, Lorenzo Stoakes wrote: > > mprotect() and other functions which change VMA parameters over a range > > each employ a pattern of:- > > > > 1. Attempt to merge the range with adjacent VMAs. > > 2. If this fails, and the range spans a subset of the VMA, split it > > accordingly. > > > > This is open-coded and duplicated in each case. Also in each case most of > > the parameters passed to vma_merge() remain the same. > > > > Create a new static function, vma_modify(), which abstracts this operation, > > accepting only those parameters which can be changed. > > > > To avoid the mess of invoking each function call with unnecessary > > parameters, create wrapper functions for each of the modify operations, > > parameterised only by what is required to perform the action. > > Nice! Thanks :) > > > Note that the userfaultfd_release() case works even though it does not > > split VMAs - since start is set to vma->vm_start and end is set to > > vma->vm_end, the split logic does not trigger. > > > > In addition, since we calculate pgoff to be equal to vma->vm_pgoff + (start > > - vma->vm_start) >> PAGE_SHIFT, and start - vma->vm_start will be 0 in this > > instance, this invocation will remain unchanged. > > > > Signed-off-by: Lorenzo Stoakes > > --- > > fs/userfaultfd.c | 53 +++++++++----------------- > > include/linux/mm.h | 23 ++++++++++++ > > mm/madvise.c | 25 ++++--------- > > mm/mempolicy.c | 20 ++-------- > > mm/mlock.c | 24 ++++-------- > > mm/mmap.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ > > mm/mprotect.c | 27 ++++---------- > > 7 files changed, 157 insertions(+), 108 deletions(-) > > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > > index a7c6ef764e63..9e5232d23927 100644 > > --- a/fs/userfaultfd.c > > +++ b/fs/userfaultfd.c > > @@ -927,11 +927,10 @@ static int userfaultfd_release(struct inode *inode, struct file *file) > > continue; > > } > > new_flags = vma->vm_flags & ~__VM_UFFD_FLAGS; > > - prev = vma_merge(&vmi, mm, prev, vma->vm_start, vma->vm_end, > > - new_flags, vma->anon_vma, > > - vma->vm_file, vma->vm_pgoff, > > - vma_policy(vma), > > - NULL_VM_UFFD_CTX, anon_vma_name(vma)); > > + prev = vma_modify_uffd(&vmi, prev, vma, vma->vm_start, > > + vma->vm_end, new_flags, > > + NULL_VM_UFFD_CTX); > > + > > if (prev) { > > vma = prev; > > } else { > > @@ -1331,7 +1330,6 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > > unsigned long start, end, vma_end; > > struct vma_iterator vmi; > > bool wp_async = userfaultfd_wp_async_ctx(ctx); > > - pgoff_t pgoff; > > > > user_uffdio_register = (struct uffdio_register __user *) arg; > > > > @@ -1484,26 +1482,18 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > > vma_end = min(end, vma->vm_end); > > > > new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags; > > - pgoff = vma->vm_pgoff + ((start - vma->vm_start) >> PAGE_SHIFT); > > - prev = vma_merge(&vmi, mm, prev, start, vma_end, new_flags, > > - vma->anon_vma, vma->vm_file, pgoff, > > - vma_policy(vma), > > - ((struct vm_userfaultfd_ctx){ ctx }), > > - anon_vma_name(vma)); > > + prev = vma_modify_uffd(&vmi, prev, vma, start, vma_end, > > + new_flags, > > + ((struct vm_userfaultfd_ctx){ ctx })); > > if (prev) { > > This will hit also for IS_ERR(prev), no? > > > /* vma_merge() invalidated the mas */ > > vma = prev; > > goto next; > > } > > - if (vma->vm_start < start) { > > - ret = split_vma(&vmi, vma, start, 1); > > - if (ret) > > - break; > > - } > > - if (vma->vm_end > end) { > > - ret = split_vma(&vmi, vma, end, 0); > > - if (ret) > > - break; > > + > > + if (IS_ERR(prev)) { > > So here's too late to test for it. AFAICS the other usages are like this as > well. Oh dear :) yes you're right, I will rework this in v2 for all cases. > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index a7b667786cde..c069813f215f 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -3253,6 +3253,29 @@ extern struct vm_area_struct *copy_vma(struct vm_area_struct **, > > unsigned long addr, unsigned long len, pgoff_t pgoff, > > bool *need_rmap_locks); > > extern void exit_mmap(struct mm_struct *); > > +struct vm_area_struct *vma_modify_flags(struct vma_iterator *vmi, > > + struct vm_area_struct *prev, > > + struct vm_area_struct *vma, > > + unsigned long start, unsigned long end, > > + unsigned long new_flags); > > +struct vm_area_struct *vma_modify_flags_name(struct vma_iterator *vmi, > > + struct vm_area_struct *prev, > > + struct vm_area_struct *vma, > > + unsigned long start, > > + unsigned long end, > > + unsigned long new_flags, > > + struct anon_vma_name *new_name); > > +struct vm_area_struct *vma_modify_policy(struct vma_iterator *vmi, > > + struct vm_area_struct *prev, > > + struct vm_area_struct *vma, > > + unsigned long start, unsigned long end, > > + struct mempolicy *new_pol); > > +struct vm_area_struct *vma_modify_uffd(struct vma_iterator *vmi, > > + struct vm_area_struct *prev, > > + struct vm_area_struct *vma, > > + unsigned long start, unsigned long end, > > + unsigned long new_flags, > > + struct vm_userfaultfd_ctx new_ctx); > > Could these be instead static inline wrappers, and vma_modify exported > instead of static? I started by trying this but sadly the vma_policy() helper needs the mempolicy header and trying to important that into mm.h produces a horror show of things breaking. As discussed via IRC, will look to see whether we can sensibly move this define into mm_types.h and then we can shift these. > > Maybe we could also move this to mm/internal.h? Which would mean > fs/userfaultfd.c would have to start including it, but as it's already so > much rooted in mm, it shouldn't be wrong? I'm not a fan of trying to have fs/userfaultfd.c to important mm/internal.h, seems like a bridge too far there. I think it's a bit odd that the fs bit invokes mm bits but the mm bit doesn't, but this might be an artifact of how uffd is implemented. I do in principle like the idea, as we can then seriously shift what I consider to be impl details (mergey/splitty) to being as internal as we can make it, but I think perhaps it's something we can address later if it makes sense to move some uffd bits around. > > > > > static inline int check_data_rlimit(unsigned long rlim, > > unsigned long new, > > diff --git a/mm/madvise.c b/mm/madvise.c > > index a4a20de50494..73024693d5c8 100644 >