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 18CC2C7619A for ; Sat, 15 Apr 2023 11:27:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 27EE16B0072; Sat, 15 Apr 2023 07:27:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 22DC96B0075; Sat, 15 Apr 2023 07:27:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0CEEF6B0078; Sat, 15 Apr 2023 07:27:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id EDABB6B0072 for ; Sat, 15 Apr 2023 07:27:18 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id A9231AB414 for ; Sat, 15 Apr 2023 11:27:18 +0000 (UTC) X-FDA: 80683399356.16.2D7960E Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) by imf12.hostedemail.com (Postfix) with ESMTP id CE2E440009 for ; Sat, 15 Apr 2023 11:27:16 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=OSRc51dS; spf=pass (imf12.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.45 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=1681558036; 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=Kd5MmsBf8CECOG+Q9e/Du3+V2fqX2JlI3U1aqIIAEq4=; b=MAPoSsuqPjRwswFWo+viSYpZr2Y7PtmDfK+zx9JoWCAaHZyIku1ltTOT7PRJtHUMnHNKla OS+E64sNpc4jO6hd0gKL8nZg+exqWwuGKFgyOOHd/OUgC1fBsgD84shG+wcv36Nh7sUkOd EcKvIMDwDzpU502glDsneeG8qDTafwU= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=OSRc51dS; spf=pass (imf12.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.45 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=1681558036; a=rsa-sha256; cv=none; b=E3hja6HAJ9++/28LWUXskli8DOGqtwmIq4G7rLto+9za/MEDUr8+PhDg6XzacX9DylAnoZ kULExaMSz4xyPchM1EDifCCB5Tz1o3T9V5NPzcXMW/y5anuxjqSn3OjMt2Mrh+Q/ap5uBE iO5ELCWlPFyaYbAhDJ1pTwZ1SFACL50= Received: by mail-wm1-f45.google.com with SMTP id l10-20020a05600c1d0a00b003f04bd3691eso21875038wms.5 for ; Sat, 15 Apr 2023 04:27:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1681558035; x=1684150035; 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=Kd5MmsBf8CECOG+Q9e/Du3+V2fqX2JlI3U1aqIIAEq4=; b=OSRc51dSXShNWcrdH8P6Yp8aRMcnt9EgoPr1Fux06iyeQoPYk+eloqkVagMAcYv51T irO091w875QoIpII5MxmxC1CfX/+zbHuFY8u1PhBMBXuLlFA8YICayrxSz2oju3CNMe8 GmuWbDMP+gmrmtDwTsw60uG/7FDvuJSor/ZpOfnQnmo0IO+CeIfSpVuQoaCKGuVMsWzL lWoGZ4AKoFrRJOS9S8FXA26++lbpOvMuMPCWwzLckkQ1R0aTm3+ewkwzOxI+mvo25tdK 8ICNTx+cW5RUsc1OEPbDS7PkpwPqwgtScLucMr+1ARPmpw5WbYcoN2Lm4caS3Qo/2ILa jXlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681558035; x=1684150035; 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=Kd5MmsBf8CECOG+Q9e/Du3+V2fqX2JlI3U1aqIIAEq4=; b=dOmRhEzw+mntEL3inkhikfNHhecXiu3dSjXJoZiKmlI15djcKGdvl8u6CaMhixEVKL NctdktKgZuGrdVmZhJoqxOAmjVTV0CRATqWUokJX6r5kujopfKu0LNidcHgrAgbBqwSN /TiZxKcMn4sufIf4jpx0GCiQDV7a9Yd4cVjbpf2kFSGfbKtJyb/DpetY3Vzv+Lb6FXsd yH9yZeNK5109L0jclylvW2jlGLVJ9MxTOGFcr+WtfL1aQOZWVLyY+Mo6WAsOKwgXOjdE lnQJ65RL9K3xIcY255L9I0xByAnF0InmSWiUvLhpa4fRKG6aNbUxK+/hSCwcQx30//qr PjEA== X-Gm-Message-State: AAQBX9fFpF/OX5bdWi6ujSw+XYecoBNalGnYrKttw0JYPDpSuKJjWHTS ffH5MMSk/gKIqq1lRYjmarc= X-Google-Smtp-Source: AKy350b8KNVDjCd9ld2enXCO8Ye/49/7l9oDn3EwLZPpm5PiEJizPJcXT6oNmYsWQO8ACXyDgI5Pxg== X-Received: by 2002:a7b:cb8b:0:b0:3f1:65f6:a69 with SMTP id m11-20020a7bcb8b000000b003f165f60a69mr1401318wmi.0.1681558035053; Sat, 15 Apr 2023 04:27:15 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id c9-20020a05600c0a4900b003ee6aa4e6a9sm10306776wmq.5.2023.04.15.04.27.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 15 Apr 2023 04:27:13 -0700 (PDT) Date: Sat, 15 Apr 2023 12:27:13 +0100 From: Lorenzo Stoakes To: Tetsuo Handa Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 3/7] mm/gup: remove vmas parameter from get_user_pages_remote() Message-ID: <64ae6557-0e46-48a8-badd-af8993645330@lucifer.local> References: <631001ecc556c5e348ff4f47719334c31f7bd592.1681547405.git.lstoakes@gmail.com> <63d92734-2185-439c-bbc7-53a4720f5d4a@lucifer.local> <65e79cf8-8713-4d2d-7a50-76d7e2aa454a@I-love.SAKURA.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <65e79cf8-8713-4d2d-7a50-76d7e2aa454a@I-love.SAKURA.ne.jp> X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: mfrde1byxp1nqocze6awxs59rfx4ed38 X-Rspamd-Queue-Id: CE2E440009 X-HE-Tag: 1681558036-665092 X-HE-Meta: U2FsdGVkX18XXXHvKVv3NJqBiW55A5qULEdMQC+AFT7ELQEacf45fAonLpdIZlWQoV0V3628CoSugfcJIVN+8JMPA/piTmTVjT003CAxHD4s6X8wmkj4tPiTqoOV6LOXBv/mEerUg9LeJlqQkdgvu//6u1LW4l2vHWdbJ3pNvwj5bw2aVSgjRg5DnJlPJyD52cMqGkRFQ7N869vfc36joOiGMDEepN/vwJjbGO6F6FjQsa5y8GWUGhNF4TKeP6xDL7isND1/YBxfK/YjbzvJi+TFeLpMgEa5H16X1ZoXbDAbbcYiGsLtnOvV9vB5MlvMg4xqKzAYAJPXPGK8vz3bg42++zpKs+oEPpxCp3NsKdym+cBQNN629iWNnXE+IhWZJ8PAA61rkSBA3b5Fsgywm32ZnaBQ4oTBNGPVxHH+eLop/lKN+3YIT0jlBEurx2iQQpOWV+RmRTJX7lYP2N7xjy8xJGay+O+s9C2drpLN8m4+74iflrdGpqzUiGxx221gyN6aW9rOQYKm1ejQGfmo491am9+lrU4HTAP8GiV/SePWhtMpYZHV/0DIdYgjMgKoz3sBXojRu990kzKRZ8OeeQxAWop5fySL/lHdqYQAoLHXgWTvj5q9daJkKWsL7/IJeswQu7na1J9oOiLdAzUsTsEcoC1W8uLj+psOl3F/GrpN+CirbEfGzXYQMIlIRFJEdm4SnLuFu7nuLx9fPyIUamzd6quJgGKAFDrilZiJy0e8yJWrZDJf+cTItOBwrXvfSrRZr3aH8B3nakUwURwdV8cUzS6rkcHofTJPMcSPuDY6p1T+q7rLfAjHYCNPVw96jtSb8Ud1sCSNg4zUq1jBcx5Rgv7//tTPwJHjSmreS4Ugcr7DQDi4+MzZxp3TMGywRT2OiYa6gDCAMqBBeVfhJoXF3niZanYxTfTmDiYZjTMd7M3ftLp/EfgzdMwOzKsNeeowh3Kci4qNgrDDzXA 1i5fhUfv Lm5ALG/rTfllZiDX8lgL/88URAZN2/0InCE/fckcwbbRIAwfolVcOdFgoatTehd555ol81sqB2Awwa6q+QaXHAoGWl7Hfdt1KUKfeFydO86MG17B5NJoE6BU0yfmvFBaB4iNfz2W7dWrZbqeVwWoASub98yjDev5yiTwNG7QiStyio9WMo6+71PUuCpbZDnx24lmGCGnYNh5o3VlkFQm+itQWml7b+gZRSNrv98cQaaHU3Poc2QLqgvkP/TzqP2vl0gyLTVnZDv9OKEPUfkqcIZhJujlQb6EXICfmoFqm92C2s5fxlTpepv8b9HREyVCueEx/wmVEG97bMOvKvuXO65iDan4mScB95eCN7btYtfWfFHjlzZ/lFmDcJ3FEdF9KPJpmbbDoLYk8lYu18IEB9Bq34ThayyBIS+uaauvX2s/GadD4RAxg1CBaGABvHnenhC3+X7TTGlAfms2yXaKj/otfCQ== 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 Sat, Apr 15, 2023 at 07:36:06PM +0900, Tetsuo Handa wrote: > On 2023/04/15 19:14, Lorenzo Stoakes wrote: > > On Sat, Apr 15, 2023 at 06:52:41PM +0900, Tetsuo Handa wrote: > >> On 2023/04/15 18:08, Lorenzo Stoakes wrote: > >>> @@ -475,10 +474,14 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > >>> gup_flags |= FOLL_SPLIT_PMD; > >>> /* Read the page with vaddr into memory */ > >>> ret = get_user_pages_remote(mm, vaddr, 1, gup_flags, > >>> - &old_page, &vma, NULL); > >>> + &old_page, NULL); > >>> if (ret <= 0) > >>> return ret; > >>> > >>> + vma = vma_lookup(mm, vaddr); > >>> + if (!vma) > >>> + goto put_old; > >>> + > >>> ret = verify_opcode(old_page, vaddr, &opcode); > >>> if (ret <= 0) > >>> goto put_old; > >> > >> This conversion looks wrong. > >> This causes returning a positive number when vma_lookup() returned NULL. > >> > >> * Return 0 (success) or a negative errno. > >> > > > > In reality it shouldn't be possible for vma to return NULL, I'm adding the > > checks to be extra careful. > > > > In any case you're right, attaching a -fix patch to avoid spam:- > > If you want to return -EINVAL when vma_lookup() returned NULL for whatever > unexpected reason, returning -EOPNOTSUPP in below path looks strange. > This feels a little pedantic, this is not a condition that is expected to occur in practice, I'm not sure users will be writing code to differentiate between the two, and certainly vma being NULL implies MTE is not supported. To differentiate with minimal churn, I'll add a WARN_ON_ONCE() here and in each other case where an impossible condition arises as it would be indicative of a kernel bug. > > @@ -448,7 +448,8 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > > * would cause the existing tags to be cleared if the page > > * was never mapped with PROT_MTE. > > */ > > - if (!(vma->vm_flags & VM_MTE)) { > > + vma = vma_lookup(mm, addr); > > + if (!vma || !(vma->vm_flags & VM_MTE)) { > > ret = -EOPNOTSUPP; > > put_page(page); > > break; > > Also, > > > @@ -5591,7 +5591,9 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf, > > struct page *page = NULL; > > > > ret = get_user_pages_remote(mm, addr, 1, > > - gup_flags, &page, &vma, NULL); > > + gup_flags, &page, NULL); > > + vma = vma_lookup(mm, addr); > > + > > if (ret <= 0) { > > #ifndef CONFIG_HAVE_IOREMAP_PROT > > break; > > @@ -5600,7 +5602,6 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf, > > * Check if this is a VM_IO | VM_PFNMAP VMA, which > > * we can access using slightly different code. > > */ > > - vma = vma_lookup(mm, addr); > > if (!vma) > > break; > > if (vma->vm_ops && vma->vm_ops->access) > > @@ -5617,11 +5618,11 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf, > > bytes = PAGE_SIZE-offset; > > > > maddr = kmap(page); > > - if (write) { > > + if (write && vma) { > > copy_to_user_page(vma, page, addr, > > maddr + offset, buf, bytes); > > set_page_dirty_lock(page); > > - } else { > > + } else if (vma) { > > copy_from_user_page(vma, page, addr, > > buf, maddr + offset, bytes); > > } > > not calling copy_{from,to}_user_page() if vma == NULL is not sufficient for > propagating an error to caller. > This is a product of wanting to avoid churn, again this condition is simply impossible. Also as a pedantic side note - the loop explicitly indicates no errors are propagated, so there is no need to do so. However, I want to be consistent with how I handle this and also I think it's sensible to add warnings for violation of this 'impossible' condition so I'll add a branch for it. Since I'd end up confusingly fixing up a fix-patch (and I want to change another patch in series to be consistent), I'll do a respin, apologies for spam in advance...