From patchwork Sun Jul 1 16:10:57 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 10498783 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 280306035E for ; Sun, 1 Jul 2018 16:11:03 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 172E32886A for ; Sun, 1 Jul 2018 16:11:03 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0B4B428876; Sun, 1 Jul 2018 16:11:03 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9FD022886A for ; Sun, 1 Jul 2018 16:11:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752470AbeGAQK7 (ORCPT ); Sun, 1 Jul 2018 12:10:59 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:39907 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752459AbeGAQK6 (ORCPT ); Sun, 1 Jul 2018 12:10:58 -0400 Received: by mail-oi0-f68.google.com with SMTP id d189-v6so3890164oib.6 for ; Sun, 01 Jul 2018 09:10:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=chUtP72mDLUXEZqqsLQcLhLEhT0qqcBkll7PUfkZSLY=; b=Q7oFxTckDHLhNjR4OQ6o+QG3FHBkmV8oL9M06upQIzPQ9CmQBZj8/yicPdBoqKYiK/ bL66Etv9ae4o5/ese4P+Fw1SYAhp17FXeHlugCexIbaUSZ+ET/IomR9l6/qoJSHjV76u +lFMU1d0147MgUqeuf+y8AyfACA0X8c/8zI3xikWWWcBSZyhQ6jC6hB6stz+hcjXYnIj tT4qk1TWY8W0Tvijhn1YndS3MbbKMY1r4IJzmY2+6Mae93tnFXqxC3IW1moaYIkvg31C 7hDpSECMcLVGAQc8OEVM4Mko1ImYqALEZPOd3MeDGW4As+y1fN/zielAtNIt6JBZqyj1 vGRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=chUtP72mDLUXEZqqsLQcLhLEhT0qqcBkll7PUfkZSLY=; b=sGxQHkyLYxdMUbvQeAhdu7jFJbQU80JAlHqnaje4uhZIfuOuNKFyqAwKJEq4SK5iS/ Yq8y5WyPZX8th2GuUYL1txoI2tvIZPUQ1igoNmId+BqhelmpR0YiTlIkgQb8c3Z1JZe5 YzgbK52Ks+Kk1jjjTy91pXRcPKMQbyCt5xFHXUimUwSR2xx4dvPJY4Ta0C6lMOcn4ufS 36MJRtTQCkqELmZmnWWbgeMFTMCYxOBsvGVf+uLRoyocTtlRJwjkGYGg6wyRkt/ZODwv mbQzZbxbvMQgRzIv63DH360aOXa+/FIEExdNF0nJyvSW2+UDmEz2u1cLOsUkCGsm+bo7 Dwcw== X-Gm-Message-State: APt69E2xYTQsFoeyoTzbP62/g+9T9B19NXR38mVxciiwdC5ZhlCIsx2P RgCBO8GMHnXmOkG66hpArJnHKOeSdbFBqvqNv6OmUg== X-Google-Smtp-Source: AAOMgpcuXlPLUT7P21wTEl1XHMJyK9kjl6N1mrrD5cx9DCvaLSHUy1EU3un3VM3P0pZIC1UDomNxmbTkQOBE428wmTk= X-Received: by 2002:aca:745:: with SMTP id 66-v6mr11895560oih.295.1530461458188; Sun, 01 Jul 2018 09:10:58 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a9d:3495:0:0:0:0:0 with HTTP; Sun, 1 Jul 2018 09:10:57 -0700 (PDT) In-Reply-To: <20180630141748.41f0b1c7@t450s.home> References: <20180629173150.GA24833@ziepe.ca> <20180630141748.41f0b1c7@t450s.home> From: Dan Williams Date: Sun, 1 Jul 2018 09:10:57 -0700 Message-ID: Subject: Re: [PATCH vfio] vfio: Use get_user_pages_longterm correctly To: Alex Williamson Cc: Jason Gunthorpe , linux-rdma , Michal Hocko , KVM list , Haozhong Zhang , Christoph Hellwig , Huy Nguyen , Leon Romanovsky Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sat, Jun 30, 2018 at 1:17 PM, Alex Williamson wrote: > On Fri, 29 Jun 2018 11:31:50 -0600 > Jason Gunthorpe wrote: > >> The patch noted in the fixes below converted get_user_pages_fast() to >> get_user_pages_longterm(), however the two calls differ in a few ways. >> >> First _fast() is documented to not require the mmap_sem, while _longterm() >> is documented to need it. Hold the mmap sem as required. >> >> Second, _fast accepts an 'int write' while _longterm uses 'unsigned int >> gup_flags', so the expression '!!(prot & IOMMU_WRITE)' is only working by >> luck as FOLL_WRITE is currently == 0x1. Use the expected FOLL_WRITE >> constant instead. >> >> Fixes: 94db151dc892 ("vfio: disable filesystem-dax page pinning") >> Cc: >> Signed-off-by: Jason Gunthorpe >> --- >> drivers/vfio/vfio_iommu_type1.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > Minor change as shown below, we don't need both branches coming up with > the FOLL_WRITE flag in slightly different ways. > >> I noticed this while trying to review some RDMA code that was touching >> our get_user_pages_longterm() call site and wanted to see what others >> are doing.. >> >> If someone can explain that get_user_pages_longterm() is safe to call >> without the mmap_sem held I'd love to here it! > > Me too :-\ > >> The comments in gup.c do seem to pretty clearly state the >> __get_user_pages_locked() called internally by >> get_user_pages_longterm() needs mmap_sem held.. >> >> This is confusing me because this is the only >> get_user_pages_longterm() callsite that doesn't hold the mmap_sem, and >> if it really isn't required I'd like to remove it from the RDMA code >> as well :) > > commit 0e81a8fc0411c9baec88f3f65154285fede473f6 > Author: Jason Gunthorpe > Date: Fri Jun 29 11:31:50 2018 -0600 > > vfio: Use get_user_pages_longterm correctly > > The patch noted in the fixes below converted get_user_pages_fast() to > get_user_pages_longterm(), however the two calls differ in a few ways. > > First _fast() is documented to not require the mmap_sem, while _longterm() > is documented to need it. Hold the mmap sem as required. > > Second, _fast accepts an 'int write' while _longterm uses 'unsigned int > gup_flags', so the expression '!!(prot & IOMMU_WRITE)' is only working by > luck as FOLL_WRITE is currently == 0x1. Use the expected FOLL_WRITE > constant instead. > > Fixes: 94db151dc892 ("vfio: disable filesystem-dax page pinning") > Cc: > Signed-off-by: Jason Gunthorpe > Signed-off-by: Alex Williamson Ugh, yes. Acked-by: Dan Williams I think we also need the following clue bat for people like me in the future: BUG_ON(vmas); diff --git a/mm/gup.c b/mm/gup.c index b70d7ba7cc13..388a5c799fa5 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -873,6 +873,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *ts long ret, pages_done; bool lock_dropped; + lockdep_assert_held_read(&mm->mmap_sem); + if (locked) { /* if VM_FAULT_RETRY can be returned, vmas become invalid */