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 X-Spam-Level: X-Spam-Status: No, score=-13.2 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C5E34C433E0 for ; Sat, 2 Jan 2021 12:25:04 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 36D0C221F5 for ; Sat, 2 Jan 2021 12:25:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 36D0C221F5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 485CD6B013D; Sat, 2 Jan 2021 07:25:03 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4368C6B013E; Sat, 2 Jan 2021 07:25:03 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2D6BC6B013F; Sat, 2 Jan 2021 07:25:03 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0084.hostedemail.com [216.40.44.84]) by kanga.kvack.org (Postfix) with ESMTP id 14D3B6B013D for ; Sat, 2 Jan 2021 07:25:03 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id CA529181AC9BF for ; Sat, 2 Jan 2021 12:25:02 +0000 (UTC) X-FDA: 77660754444.29.kitty25_1a153f7274bf Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin29.hostedemail.com (Postfix) with ESMTP id AB6E318086CB6 for ; Sat, 2 Jan 2021 12:25:02 +0000 (UTC) X-HE-Tag: kitty25_1a153f7274bf X-Filterd-Recvd-Size: 10198 Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by imf13.hostedemail.com (Postfix) with ESMTP for ; Sat, 2 Jan 2021 12:25:02 +0000 (UTC) Received: by mail-lf1-f48.google.com with SMTP id 23so53061117lfg.10 for ; Sat, 02 Jan 2021 04:25:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=L1dz/24VIsSJQdZXwZKj08ElRo0hBPY/t9e+l0aEAUo=; b=OEEBImD4k4+iSfpEhsmWKCZIF1yotOwxFUeoU1h8ROU/QNYZQP42uxisVgosTCgRQO 9obJrqseFYckRsoFg2DsayWpRgtMV6I+UOxhFVg4XBzTGR9CXzNqcCq7+9lScLfydVgl VUUb5OfSfv9ASX0Q2xh8VvlYSWW9ZRMwH3g6QwJ+zUCuFLTj6As68fuOY0qGRBY25393 u59sSLa1Q2kQoGk/uoxDsfJyDbSKLBqvA2J2+UptSodis/ObH/U5FIdnL5IHVeyrTRFL fBpy1YtcT5d838UqRqK2yW+CaFV2ndpCUF0x8E0wbUoewYvbeDgw/3Ry059oD7myAZt7 OH3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=L1dz/24VIsSJQdZXwZKj08ElRo0hBPY/t9e+l0aEAUo=; b=Yy6MFeA67YprEeH+u63BasxJYfRONxmvks2hzeJ52LW3SthhMyJcQFzPY18vtFvyql de/IMOfkAVFHeaNU9qA/SWMx9YD6u062Urz/F5qmHNY+9AdIWrZSrEk7JBM1tMI0Dxif TF8WO0Druf9SG4OoBC3IbatxbNZTigUij5nCKQhHQY6BxyfubmSVAoQlJYnTZ+8pBp2i 79Ig5pPHVQYnadGyUJQc36ZjYGwTmRIKHMyoIvPYyxAHDavhyqrq/vRB829evNcbEhuW LGHP22pIzNzqtf5i/NgnpjmGg5wG+w3i6brTvBAG2s+bjP/7Tx7SHgmgQLR2in1frte7 Dfbg== X-Gm-Message-State: AOAM5313tCXC3xF+xHXTbrCIwA2gZ3f9Q5bVXXIAEPAaYI2wkmS7ED1Q FJiSuKZm/JRR6uqSMXWK1Gg= X-Google-Smtp-Source: ABdhPJwjiiNjk6Z/7nJDI5Xe4SyxfHkldLNxq826IkjtznIclNSznjxHz+sOP4H4ZOmv0wzR9Rcxcw== X-Received: by 2002:a2e:9417:: with SMTP id i23mr33668390ljh.206.1609590300774; Sat, 02 Jan 2021 04:25:00 -0800 (PST) Received: from localhost.localdomain ([131.228.2.21]) by smtp.gmail.com with ESMTPSA id q2sm6695359lfn.67.2021.01.02.04.24.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 02 Jan 2021 04:25:00 -0800 (PST) Subject: Re: [PATCH] mm/mremap.c: refactor finding vma and checking vma is alllowed to expand To: John Hubbard , linux-mm@kvack.org Cc: akpm@linux-foundation.org, "Kirill A. Shutemov" References: <20201230075657.2720522-1-lixinhai.lxh@gmail.com> From: Li Xinhai Message-ID: Date: Sat, 2 Jan 2021 20:24:50 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 12/31/20 4:52 AM, John Hubbard wrote: > On 12/29/20 11:56 PM, Li Xinhai wrote: >> Function vma_to_resize)() is called to find the vma to be remapped and >> also check if expand size is allowed or not. This function assume that= =20 >> all >> call sites should make sure new_len >=3D old_len, and currently this >> assumption is fullfilled at those two call sites, so no real problem a= t >> present. >> >> After this patch, we explicitly check new_len < old_len case, and=20 >> separate >> a new function for checking if expand size is allowed or not. Also ren= ame >> vma_to_resize to vma_to_remap, since the vma to be remapped would not >> always require resize. >=20 > I don't see any clear motivation for this code churn, either above, or > implicitly in the patch itself. The new function names are not an=20 > improvement. >=20 > Probably best to just drop this, unless there is some sort of benefit t= hat > I'm missing? > The main issue is that in vma_to_size() there are code like below if (new_len =3D=3D old_len) return vma; ... locked +=3D new_len - old_len; ... unsigned long charged =3D (new_len - old_len) >> PAGE_SHIFT; ... the test didn't cover new_len < old_len case, then just do 'new_len -=20 old_len'. That looks like hiding potential bug. So this need be fixed. I tends to move out the code after the test into a separate function=20 which is only for new_len > old_len case, currently there are various=20 calculation/check around that test. So, we see which checks are for all=20 new_len and old_len cases, and which only for new_len > old_len case,=20 more clear when further change this part of code. maybe better name than vma_to_remap()? or keep using vma_to_resize()? >=20 > thanks, > --=20 > John Hubbard > NVIDIA >=20 >> >> Cc: John Hubbard >> Cc: "Kirill A. Shutemov" >> Signed-off-by: Li Xinhai >> --- >> =C2=A0 mm/mremap.c | 79 ++++++++++++++++++++++++++++++----------------= ------- >> =C2=A0 1 file changed, 45 insertions(+), 34 deletions(-) >> >> diff --git a/mm/mremap.c b/mm/mremap.c >> index c5590afe7165..22eb4e9f35d6 100644 >> --- a/mm/mremap.c >> +++ b/mm/mremap.c >> @@ -621,13 +621,52 @@ static unsigned long move_vma(struct=20 >> vm_area_struct *vma, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return new_addr; >> =C2=A0 } >> -static struct vm_area_struct *vma_to_resize(unsigned long addr, >> +static struct vm_area_struct *vma_allow_expand(struct vm_area_struct=20 >> *vma, >> +=C2=A0=C2=A0=C2=A0 unsigned long addr, unsigned long old_len, unsigne= d long new_len, >> +=C2=A0=C2=A0=C2=A0 unsigned long *p) >> +{ >> +=C2=A0=C2=A0=C2=A0 struct mm_struct *mm =3D current->mm; >> +=C2=A0=C2=A0=C2=A0 unsigned long pgoff; >> + >> +=C2=A0=C2=A0=C2=A0 pgoff =3D (addr - vma->vm_start) >> PAGE_SHIFT; >> +=C2=A0=C2=A0=C2=A0 pgoff +=3D vma->vm_pgoff; >> +=C2=A0=C2=A0=C2=A0 if (pgoff + (new_len >> PAGE_SHIFT) < pgoff) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(-EINVAL); >> + >> +=C2=A0=C2=A0=C2=A0 if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP)) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(-EFAULT); >> + >> +=C2=A0=C2=A0=C2=A0 if (vma->vm_flags & VM_LOCKED) { >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long locked, lock= _limit; >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 locked =3D mm->locked_vm <= < PAGE_SHIFT; >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lock_limit =3D rlimit(RLIM= IT_MEMLOCK); >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 locked +=3D new_len - old_= len; >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (locked > lock_limit &&= !capable(CAP_IPC_LOCK)) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 re= turn ERR_PTR(-EAGAIN); >> +=C2=A0=C2=A0=C2=A0 } >> + >> +=C2=A0=C2=A0=C2=A0 if (!may_expand_vm(mm, vma->vm_flags, >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 (new_len - old_len) >> PAGE_SHIFT)) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(-ENOMEM); >> + >> +=C2=A0=C2=A0=C2=A0 if (vma->vm_flags & VM_ACCOUNT) { >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long charged =3D = (new_len - old_len) >> PAGE_SHIFT; >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (security_vm_enough_mem= ory_mm(mm, charged)) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 re= turn ERR_PTR(-ENOMEM); >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *p =3D charged; >> +=C2=A0=C2=A0=C2=A0 } >> + >> +=C2=A0=C2=A0=C2=A0 return vma; >> +} >> + >> +static struct vm_area_struct *vma_to_remap(unsigned long addr, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long old_len, unsigned long ne= w_len, unsigned long flags, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long *p) >> =C2=A0 { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct mm_struct *mm =3D current->mm; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct vm_area_struct *vma =3D find_vma= (mm, addr); >> -=C2=A0=C2=A0=C2=A0 unsigned long pgoff; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!vma || vma->vm_start > addr) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(= -EFAULT); >> @@ -656,39 +695,11 @@ static struct vm_area_struct=20 >> *vma_to_resize(unsigned long addr, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (old_len > vma->vm_end - addr) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(= -EFAULT); >> -=C2=A0=C2=A0=C2=A0 if (new_len =3D=3D old_len) >> +=C2=A0=C2=A0=C2=A0 if (new_len <=3D old_len) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return vma; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Need to be careful about a growing m= apping */ >> -=C2=A0=C2=A0=C2=A0 pgoff =3D (addr - vma->vm_start) >> PAGE_SHIFT; >> -=C2=A0=C2=A0=C2=A0 pgoff +=3D vma->vm_pgoff; >> -=C2=A0=C2=A0=C2=A0 if (pgoff + (new_len >> PAGE_SHIFT) < pgoff) >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(-EINVAL); >> - >> -=C2=A0=C2=A0=C2=A0 if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP)) >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(-EFAULT); >> - >> -=C2=A0=C2=A0=C2=A0 if (vma->vm_flags & VM_LOCKED) { >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long locked, lock= _limit; >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 locked =3D mm->locked_vm <= < PAGE_SHIFT; >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lock_limit =3D rlimit(RLIM= IT_MEMLOCK); >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 locked +=3D new_len - old_= len; >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (locked > lock_limit &&= !capable(CAP_IPC_LOCK)) >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 re= turn ERR_PTR(-EAGAIN); >> -=C2=A0=C2=A0=C2=A0 } >> - >> -=C2=A0=C2=A0=C2=A0 if (!may_expand_vm(mm, vma->vm_flags, >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 (new_len - old_len) >> PAGE_SHIFT)) >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ERR_PTR(-ENOMEM); >> - >> -=C2=A0=C2=A0=C2=A0 if (vma->vm_flags & VM_ACCOUNT) { >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long charged =3D = (new_len - old_len) >> PAGE_SHIFT; >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (security_vm_enough_mem= ory_mm(mm, charged)) >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 re= turn ERR_PTR(-ENOMEM); >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *p =3D charged; >> -=C2=A0=C2=A0=C2=A0 } >> - >> -=C2=A0=C2=A0=C2=A0 return vma; >> +=C2=A0=C2=A0=C2=A0 return vma_allow_expand(vma, addr, old_len, new_le= n, p); >> =C2=A0 } >> =C2=A0 static unsigned long mremap_to(unsigned long addr, unsigned lon= g=20 >> old_len, >> @@ -743,7 +754,7 @@ static unsigned long mremap_to(unsigned long addr,= =20 >> unsigned long old_len, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 old_len =3D new= _len; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >> -=C2=A0=C2=A0=C2=A0 vma =3D vma_to_resize(addr, old_len, new_len, flag= s, &charged); >> +=C2=A0=C2=A0=C2=A0 vma =3D vma_to_remap(addr, old_len, new_len, flags= , &charged); >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (IS_ERR(vma)) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D PTR_ERR= (vma); >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto out; >> @@ -894,7 +905,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr,=20 >> unsigned long, old_len, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Ok, we need to grow.. >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */ >> -=C2=A0=C2=A0=C2=A0 vma =3D vma_to_resize(addr, old_len, new_len, flag= s, &charged); >> +=C2=A0=C2=A0=C2=A0 vma =3D vma_to_remap(addr, old_len, new_len, flags= , &charged); >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (IS_ERR(vma)) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D PTR_ERR= (vma); >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto out; >>