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 B7BBDC52D7C for ; Thu, 15 Aug 2024 15:41:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 41A856B012A; Thu, 15 Aug 2024 11:41:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3CAA26B012C; Thu, 15 Aug 2024 11:41:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 26ACA6B012E; Thu, 15 Aug 2024 11:41:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 087166B012A for ; Thu, 15 Aug 2024 11:41:50 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id A1D96121593 for ; Thu, 15 Aug 2024 15:41:49 +0000 (UTC) X-FDA: 82454895138.18.B3ECDD4 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf05.hostedemail.com (Postfix) with ESMTP id 7A9D4100022 for ; Thu, 15 Aug 2024 15:41:47 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=LsTDcnVI; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf05.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723736494; a=rsa-sha256; cv=none; b=UGi8OFF+rfJKIV7BdWMsWq0fwwujBw8Z0jFkAMj1PNI663mEKFUCAp51JbnFrvM3MW1zRH KJxsqXbqWRyjhp3o5FdTo3Qe05nItx9znX2Im1HEqquF3oraskcuu4SpeEH6BCz+eBFX+u EeXl2Ekm8DYNAm8PDNZ8Pcr4HHa0/W0= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=LsTDcnVI; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf05.hostedemail.com: domain of peterx@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=peterx@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723736494; 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=JsmHuSZ/h+8byKlDVdTBZriKwwSluzVcisJkUaNyJWc=; b=oEO/1Z5cP10FIcNp+4U6iQv1Cs+2+k/Qzn+wHE3qdwAZxQs9Ee6FttKJptzTqSnNAQokRN BKjYxAIjmYYuNS2RPsJ/YecNoPSvNwuKoGhpswhVAkhh/BLgFQU+6Jb4s9QEHPxMkVuSWr ornud8o20mP7EIQ+TKjSlo0mJqcoLIc= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1723736506; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JsmHuSZ/h+8byKlDVdTBZriKwwSluzVcisJkUaNyJWc=; b=LsTDcnVIK6xXuqu1vO3PgSkT/N+6qpSmOCXLnwYU4KqcNRL/8nLET2EzMe2mAnLmlGEa20 ZDsgTjZ/nMsu22rjIPJVGm1jzwkklgHnwkaQ4LPu9Gd4a+72dEwWr+GKCoP6EvMOT1v1fC 34TEV4zwe/FLpBZ1PS7vlQVe+o78X44= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-325-63_T1DU3Ma-onig-D16HKw-1; Thu, 15 Aug 2024 11:41:44 -0400 X-MC-Unique: 63_T1DU3Ma-onig-D16HKw-1 Received: by mail-qv1-f72.google.com with SMTP id 6a1803df08f44-6bf7b853f0dso288156d6.0 for ; Thu, 15 Aug 2024 08:41:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723736504; x=1724341304; 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=JsmHuSZ/h+8byKlDVdTBZriKwwSluzVcisJkUaNyJWc=; b=G+yb1EPkVCryV7qrvPv3/cMhw/x2WDeq6VWAHNK2n2QuY1vQw1aWxg5cHm+Jt6QwTI wCgwnKT7YqydMIx2VVX5D/hs18YJaKv3a+U8U9lH47EL5S71RoK3aNkMpkFbiflcoPkC VKGp4SVyOjOI/VeZXJ2m6u9iJHjajTNrFlWz8qLvUUS0NvgXFvTpcf2BtFx1OgM6chzA cA4NRoWcUSby31HtqYWc6oUjAYUE46GeX64BirIYAYXbNaymeCQ0JCbjG0tazfgkbZyI td/S9nIh6KYTWLFGoQCTM0egx5++NdvNHzrCH5mraWQylEnEIUAcL3eU7o90jAI0zw4Y 65Iw== X-Gm-Message-State: AOJu0YwK0zqSM3vLS4jzVoNecBIy+6ZYgpjdnIOV/ndlrgxPe+Hz9w/E 1WQ68uk2lqeltS8EJumj52l/EJQuKDQAPQT8i0ws9Wwc1t2y4N/Z5vbEOtiiPZzNtUq9oDvma6y FcoXOUmqUx9zKqwp02vDyZ0+ewDMeylUEFOM7bjOeEbNiJF02 X-Received: by 2002:a05:6214:c2a:b0:6b9:9417:c103 with SMTP id 6a1803df08f44-6bf5d15e1c2mr43262836d6.1.1723736503621; Thu, 15 Aug 2024 08:41:43 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFzOO1MXixhWkJJiUpcPb8vsKcxQhlblFzoLUtxY92DYm0+isa557XEa9BsKNxw5lcKbfHR8g== X-Received: by 2002:a05:6214:c2a:b0:6b9:9417:c103 with SMTP id 6a1803df08f44-6bf5d15e1c2mr43262466d6.1.1723736503043; Thu, 15 Aug 2024 08:41:43 -0700 (PDT) Received: from x1n (pool-99-254-121-117.cpe.net.cable.rogers.com. [99.254.121.117]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-6bf6ff0c354sm7266716d6.135.2024.08.15.08.41.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Aug 2024 08:41:42 -0700 (PDT) Date: Thu, 15 Aug 2024 11:41:39 -0400 From: Peter Xu To: Jason Gunthorpe Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sean Christopherson , Oscar Salvador , Axel Rasmussen , linux-arm-kernel@lists.infradead.org, x86@kernel.org, Will Deacon , Gavin Shan , Paolo Bonzini , Zi Yan , Andrew Morton , Catalin Marinas , Ingo Molnar , Alistair Popple , Borislav Petkov , David Hildenbrand , Thomas Gleixner , kvm@vger.kernel.org, Dave Hansen , Alex Williamson , Yan Zhao Subject: Re: [PATCH 09/19] mm: New follow_pfnmap API Message-ID: References: <20240809160909.1023470-1-peterx@redhat.com> <20240809160909.1023470-10-peterx@redhat.com> <20240814131954.GK2032816@nvidia.com> <20240814221441.GB2032816@nvidia.com> MIME-Version: 1.0 In-Reply-To: <20240814221441.GB2032816@nvidia.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Rspam-User: X-Rspamd-Queue-Id: 7A9D4100022 X-Rspamd-Server: rspam01 X-Stat-Signature: zhsnch6e583pp58nfyxxyf9o9eo9ehk1 X-HE-Tag: 1723736507-194832 X-HE-Meta: U2FsdGVkX1/3UmgvapZomzNGY3fKHG25sM/4rcqNSmHvrwj/CuEmz5JqRT+2a0rbaVj6kucX2chlsvUJyQNqlMVg6AHPwYo5uzfyTSwef365wwwP0slL5+tcx9A2GhVKoXEZy4pPFF27xIIwILxEWXxRXdv1F2eN8sI3ZHKR6zBfvdaQqnccVE4/+Vz6KI6HJt6TU5dN9AL/2czoe5CcUyoWdumqK1TcGSuonmqxD7GktRqqsxsae0ezr/3yIV8xDQiWeYkelEiSC+1Qe3aJa58rgjgsooaqnan6hXCQ4hz9xKWgW8c117DLHw75apKEUc/lOm97TOKLPCMcUxY21X+lGKLUotmH2HLQ5TlrJLLVqKqevTaQsOSQreMp/bBU5EBHeKeZCKAgROP+pOQURcMXFkMrOEyomtv3OOazZVQNHfSlWKa1vQ1RGgI5Hhssr/zEhyotFGENweTO2DvMAGeLU0znVu3/fF4/OsF8YyYdjYT+B87gN0TtgL0i9RhUTNcNV6WnOechPS3VxXzWPx4/xf74V1UcGg6YvYh7K4hUIvUnD53Cn7TaOhTwoJ03x4nQdF6uKcd7XK+QauTkCWVDc0AXfal7yOpDRb2DwS/ngnUkXK0dLwnXnl/mAEuTCBLml7BPgxnAMdS5ajgkdGHVSNGR3/XynVoJQGGJCb+XjHZ//f/qPEolQgCuLp2yiPM50bmosau60+owkJ5ZG/RrkXEW6GEP5W0+W8P1d1yhsu2cJLvCkGOaCyusdB9sHOcymiIZXdFZoDqBmyCk82NGm05J888JfcLLW4UM/+SNp4U1z4YqqHzeqn9QIxufg694VvXliOR5VyUbSYmnrWuVucDko66qx81iOo0QTs+z/pGDB9DtlX501Y9+xD4Z9DvQHvn/SVjYJGTItRFTDTO1LW4g9kSso28WQKfxnPkvUCGKeDvsIR7vwIffjGHrrrIPWDJSRZSXaPHCHO9 m7Wa4xea i46q2gWmZjJIVTJO1R0ajvLBNaxOMV2OXZURuD9SaL2UUk+Wzz5NmbrygOApC2efiR72VtR/f7XQu6yKbIJWwwnig9F+LsvwDVAkPWtWnx2cRG0/cMsbH770ofK5cxgIQRD8RpzXlW6BiZQI8eFC1lJoE12zf1TbYdDbiWIcELSNdIqu/nk4lsLYuVad+JT5MYXkdIMEYwSSwZk2NPrmnNMJOPkNJnc6OZjNDFzRKm3FwtsXnkxh6rOBizK24uDXAtRx4+MXbbImE1OW6WYdIZ52SjhE2qfcDvdUXn8Dss2i6ickob0DYARDPfesRAwKM4S4W5NY8gKICd7bhKHwOAWtTAltNmZs3lFCwztElxKgzF0IH/khM03MnYCQGGvODpyYHG1+uZdeT0eA= 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: List-Subscribe: List-Unsubscribe: On Wed, Aug 14, 2024 at 07:14:41PM -0300, Jason Gunthorpe wrote: > On Wed, Aug 14, 2024 at 02:24:47PM -0400, Peter Xu wrote: > > On Wed, Aug 14, 2024 at 10:19:54AM -0300, Jason Gunthorpe wrote: > > > On Fri, Aug 09, 2024 at 12:08:59PM -0400, Peter Xu wrote: > > > > > > > +/** > > > > + * follow_pfnmap_start() - Look up a pfn mapping at a user virtual address > > > > + * @args: Pointer to struct @follow_pfnmap_args > > > > + * > > > > + * The caller needs to setup args->vma and args->address to point to the > > > > + * virtual address as the target of such lookup. On a successful return, > > > > + * the results will be put into other output fields. > > > > + * > > > > + * After the caller finished using the fields, the caller must invoke > > > > + * another follow_pfnmap_end() to proper releases the locks and resources > > > > + * of such look up request. > > > > + * > > > > + * During the start() and end() calls, the results in @args will be valid > > > > + * as proper locks will be held. After the end() is called, all the fields > > > > + * in @follow_pfnmap_args will be invalid to be further accessed. > > > > + * > > > > + * If the PTE maps a refcounted page, callers are responsible to protect > > > > + * against invalidation with MMU notifiers; otherwise access to the PFN at > > > > + * a later point in time can trigger use-after-free. > > > > + * > > > > + * Only IO mappings and raw PFN mappings are allowed. > > > > > > What does this mean? The paragraph before said this can return a > > > refcounted page? > > > > This came from the old follow_pte(), I kept that as I suppose we should > > allow VM_IO | VM_PFNMAP just like before, even if in this case I suppose > > only the pfnmap matters where huge mappings can start to appear. > > If that is the intention it should actively block returning anything > that is vm_normal_page() not check the VM flags, see the other > discussion.. The restriction should only be applied to the vma attributes, not a specific pte mapping, IMHO. I mean, the comment was describing "which VMA is allowed to use this function", reflecting that we'll fail at anything !PFNMAP && !IO. It seems legal to have private mappings of them, where vm_normal_page() can return true here for some of the mappings under PFNMAP|IO. IIUC either the old follow_pte() or follow_pfnmap*() API cared much on this part yet so far. > > It makes sense as a restriction if you call the API follow pfnmap. I'm open to any better suggestion to names. Again, I think here it's more about the vma attribute, not "every mapping under the memory range". > > > > > + * The mmap semaphore > > > > + * should be taken for read, and the mmap semaphore cannot be released > > > > + * before the end() is invoked. > > > > > > This function is not safe for IO mappings and PFNs either, VFIO has a > > > known security issue to call it. That should be emphasised in the > > > comment. > > > > Any elaboration on this? I could have missed that.. > > Just because the memory is a PFN or IO doesn't mean it is safe to > access it without a refcount. There are many driver scenarios where > revoking a PFN from mmap needs to be a hard fence that nothing else > has access to that PFN. Otherwise it is a security problem for that > driver. Oh ok, I suppose you meant the VFIO whole thing on "zapping mapping when MMIO disabled"? If so I get it. More below. > > > I suppose so? As the pgtable is stable, I thought it means it's safe, but > > I'm not sure now when you mentioned there's a VFIO known issue, so I could > > have overlooked something. There's no address returned, but pfn, pgprot, > > write, etc. > > zap/etc will wait on the PTL, I think, so it should be safe for at > least the issues I am thinking of. > > > The user needs to do proper mapping if they need an usable address, > > e.g. generic_access_phys() does ioremap_prot() and recheck the pfn didn't > > change. > > No, you can't take the phys_addr_t outside the start/end region that > explicitly holds the lock protecting it. This is what the comment must > warn against doing. I think the comment has that part covered more or less: * During the start() and end() calls, the results in @args will be valid * as proper locks will be held. After the end() is called, all the fields * in @follow_pfnmap_args will be invalid to be further accessed. Feel free to suggest anything that will make it better. For generic_access_phys() as a specific example: I think it is safe to map the pfn even after end(). I meant here the "map" operation is benign with ioremap_prot(), afaiu: it doesn't include an access on top of the mapping yet. After the map, it rewalks the pgtable, making sure PFN is still there and valid, and it'll only access it this time before end(): if (write) memcpy_toio(maddr + offset, buf, len); else memcpy_fromio(buf, maddr + offset, len); ret = len; follow_pfnmap_end(&args); If PFN changed, it properly releases the mapping: if ((prot != pgprot_val(args.pgprot)) || (phys_addr != (args.pfn << PAGE_SHIFT)) || (writable != args.writable)) { follow_pfnmap_end(&args); iounmap(maddr); goto retry; } Then taking the example of VFIO: there's no risk of racing with a concurrent zapping as far as I can see, because otherwise it'll see pfn changed. Thanks, -- Peter Xu