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=-6.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 222D8C47423 for ; Wed, 7 Oct 2020 18:01:59 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 72BC32173E for ; Wed, 7 Oct 2020 18:01:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="XlsYCfTV" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 72BC32173E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7BA456B005C; Wed, 7 Oct 2020 14:01:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 742566B0062; Wed, 7 Oct 2020 14:01:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5BB096B0068; Wed, 7 Oct 2020 14:01:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0001.hostedemail.com [216.40.44.1]) by kanga.kvack.org (Postfix) with ESMTP id 23F196B005C for ; Wed, 7 Oct 2020 14:01:57 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 3B270362C for ; Wed, 7 Oct 2020 18:01:56 +0000 (UTC) X-FDA: 77345897832.09.root65_3c02920271d1 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin09.hostedemail.com (Postfix) with ESMTP id 2F3DA180AD815 for ; Wed, 7 Oct 2020 18:01:55 +0000 (UTC) X-HE-Tag: root65_3c02920271d1 X-Filterd-Recvd-Size: 7577 Received: from mail-oo1-f67.google.com (mail-oo1-f67.google.com [209.85.161.67]) by imf45.hostedemail.com (Postfix) with ESMTP for ; Wed, 7 Oct 2020 18:01:54 +0000 (UTC) Received: by mail-oo1-f67.google.com with SMTP id o20so848268ook.1 for ; Wed, 07 Oct 2020 11:01:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=KjKZykFuT09vi8gvWSxWfIoih5+HtjIRxuS5MgzFTGo=; b=XlsYCfTVgW+6nmGAiv0O62GLPsC5r7eB+oidAEHNavDRGpWicEbtNZg9to7A5saHMp zaYRD/gT5inYX9JmqOP5ZzySgAIOp0+59TPao6y2kIbw9G7KU0mdvCuhCQrafShecj0J tHZf4mGHu3aegW49dmX0ZJptYBRUrRuO4MkvA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=KjKZykFuT09vi8gvWSxWfIoih5+HtjIRxuS5MgzFTGo=; b=ks+dSJ9JE/DZSlxwREsemz1kt1vI21E9Er1jR9DxIdE28CWZIxAwV3xxAruCZ+LtkZ u5o2ZFDZrr3I+c0z2Heyv0OI+HyTuCNb+xDkJm0mHYKWDFL86OnZHmym2ekFsJ2Oc3kZ dOC+FYx1ayPt1tyNS/so2LzYDvhr7XFMgDrVWrW7O7qV+3+QYmcYsVf5/tCnLU4VYIhL SSXfeV6hfiv7iTE7S8RlyjozFuFcW31+SxFUrBVuhuyuxDY5AEQSisF1UJX6wi/zIrEM SAWsnl6Q8YSOHt7LISb0PC9EUMc4lVErPHmTsde2gToA0mqE7HYYk77Li0QBAmNwFKV8 djqA== X-Gm-Message-State: AOAM531l3IgfXg/uef8Ke9FcHhqRjApKj74iUQh9e4zgmbKNUPv46Ywe N3ZOGZ6S+FMOOv9JcjwwxcYjr+62doRZa0qJrGxRGw== X-Google-Smtp-Source: ABdhPJwJzYY7PIFYfmYEAKc+uMwrEIxle3PFN75o5nMMKSdxenHBbi1fgd2mUNbWFOGRvdaIUhxGwTNij1DRYovGNtE= X-Received: by 2002:a4a:c011:: with SMTP id v17mr2806667oop.89.1602093713265; Wed, 07 Oct 2020 11:01:53 -0700 (PDT) MIME-Version: 1.0 References: <20201007164426.1812530-1-daniel.vetter@ffwll.ch> <20201007164426.1812530-8-daniel.vetter@ffwll.ch> <20201007172746.GU5177@ziepe.ca> In-Reply-To: <20201007172746.GU5177@ziepe.ca> From: Daniel Vetter Date: Wed, 7 Oct 2020 20:01:42 +0200 Message-ID: Subject: Re: [PATCH 07/13] mm: close race in generic_access_phys To: Jason Gunthorpe Cc: DRI Development , LKML , kvm@vger.kernel.org, Linux MM , Linux ARM , linux-samsung-soc , "open list:DMA BUFFER SHARING FRAMEWORK" , linux-s390@vger.kernel.org, Dan Williams , Kees Cook , Rik van Riel , Benjamin Herrensmidt , Dave Airlie , Hugh Dickins , Andrew Morton , John Hubbard , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Jan Kara , Daniel Vetter Content-Type: text/plain; charset="UTF-8" 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 Wed, Oct 7, 2020 at 7:27 PM Jason Gunthorpe wrote: > > On Wed, Oct 07, 2020 at 06:44:20PM +0200, Daniel Vetter wrote: > > Way back it was a reasonable assumptions that iomem mappings never > > change the pfn range they point at. But this has changed: > > > > - gpu drivers dynamically manage their memory nowadays, invalidating > > ptes with unmap_mapping_range when buffers get moved > > > > - contiguous dma allocations have moved from dedicated carvetouts to > > cma regions. This means if we miss the unmap the pfn might contain > > pagecache or anon memory (well anything allocated with GFP_MOVEABLE) > > > > - even /dev/mem now invalidates mappings when the kernel requests that > > iomem region when CONFIG_IO_STRICT_DEVMEM is set, see 3234ac664a87 > > ("/dev/mem: Revoke mappings when a driver claims the region") > > > > Accessing pfns obtained from ptes without holding all the locks is > > therefore no longer a good idea. Fix this. > > > > Since ioremap might need to manipulate pagetables too we need to drop > > the pt lock and have a retry loop if we raced. > > > > While at it, also add kerneldoc and improve the comment for the > > vma_ops->access function. It's for accessing, not for moving the > > memory from iomem to system memory, as the old comment seemed to > > suggest. > > > > References: 28b2ee20c7cb ("access_process_vm device memory infrastructu= re") > > Cc: Jason Gunthorpe > > Cc: Dan Williams > > Cc: Kees Cook > > Cc: Rik van Riel > > Cc: Benjamin Herrensmidt > > Cc: Dave Airlie > > Cc: Hugh Dickins > > Cc: Andrew Morton > > Cc: John Hubbard > > Cc: J=C3=A9r=C3=B4me Glisse > > Cc: Jan Kara > > Cc: Dan Williams > > Cc: linux-mm@kvack.org > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-samsung-soc@vger.kernel.org > > Cc: linux-media@vger.kernel.org > > Signed-off-by: Daniel Vetter > > --- > > include/linux/mm.h | 3 ++- > > mm/memory.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 44 insertions(+), 3 deletions(-) > > This does seem to solve the race with revoke_devmem(), but it is really u= gly. > > It would be much nicer to wrap a rwsem around this access and the unmap. > > Any place using it has a nice linear translation from vm_off to pfn, > so I don't think there is a such a good reason to use follow_pte in > the first place. > > ie why not the helper be this: > > int generic_access_phys(unsigned long pfn, unsigned long pgprot, > void *buf, size_t len, bool write) > > Then something like dev/mem would compute pfn and obtain the lock: > > dev_access(struct vm_area_struct *vma, unsigned long addr, void *buf, int= len, int write) > { > cpu_addr =3D vma->vm_pgoff*PAGE_SIZE + (addr - vma->vm_start)); > > /* FIXME: Has to be over each page of len */ > if (!devmem_is_allowed_access(PHYS_PFN(cpu_addr/4096))) > return -EPERM; > > down_read(&mem_sem); > generic_access_phys(cpu_addr/4096, pgprot_val(vma->vm_page_prot), > buf, len, write); > up_read(&mem_sem); > } > > The other cases looked simpler because they don't revoke, here the > mmap_sem alone should be enough protection, they would just need to > provide the linear translation to pfn. > > What do you think? I think it'd fix the bug, until someone wires ->access up for drivers/gpu, or the next subsystem. This is also just for ptrace, so we really don't care when we stall the vm badly and other silly things. So I figured the somewhat ugly, but full generic solution is the better one, so that people who want to be able to ptrace read/write their iomem mmaps can just sprinkle this wherever they feel like. But yeah if we go with most minimal fix, i.e. only trying to fix the current users, then your thing should work and is simpler. But it leaves the door open for future problems. -Daniel --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch