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=-8.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS 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 910F3C47080 for ; Thu, 3 Jun 2021 04:15:50 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 15BFD613E6 for ; Thu, 3 Jun 2021 04:15:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 15BFD613E6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4A6506B006C; Thu, 3 Jun 2021 00:15:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 456496B006E; Thu, 3 Jun 2021 00:15:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2D0C26B0070; Thu, 3 Jun 2021 00:15:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0201.hostedemail.com [216.40.44.201]) by kanga.kvack.org (Postfix) with ESMTP id EACBE6B006C for ; Thu, 3 Jun 2021 00:15:48 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 87101180AD804 for ; Thu, 3 Jun 2021 04:15:48 +0000 (UTC) X-FDA: 78211099176.12.562478B Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) by imf27.hostedemail.com (Postfix) with ESMTP id 3F7A280192FE for ; Thu, 3 Jun 2021 04:15:34 +0000 (UTC) Received: by mail-pl1-f176.google.com with SMTP id e7so2219893plj.7 for ; Wed, 02 Jun 2021 21:15:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ACX9rMbN+WVThypXBQEdnoXRc0uA+XrZOXTgO72q+S8=; b=dLgNK1Vx41z9q44npf1xeO2FjKBfyHRsKkRclb3uBXu1+oovIkoA3xl5AOneHjlVHX pXPC3bMruhQyhvxDOlMlOm2EgmsXxHfalSe4JsOlcakqvaXigk8wqMhDTctSvH4ZkYe0 yL2H/WlZnRgaATssaPoK9pXQFHAiO1hNxzgc/KsEgV4hUlcdIdWm0HGFulCsDYokjXK3 MiZ6RjSkpr3UgDm/WTxG5fbYZOjGHgYae6TxUh+c46MZffzNZgqK2lzSp6evw2bU4vmP HkCAVIHV211KRlfsC1q9qX7frF+0QEEs7M7LQ/QFtgrUcRVzE4bw2FJxdEbduGRMtgqZ Ibzg== 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; bh=ACX9rMbN+WVThypXBQEdnoXRc0uA+XrZOXTgO72q+S8=; b=CMh4hmBTzC8VLe1Q8bGhz2fuDxPV9qlUkxydu7dS/nzzFThMXFO4V5MiSjKz79g2lJ 67ARqfrT14j5Pu2CtrgksonFTJG9F4N5nNYik81E1zHlx0L3+YcMYHWHLWbYSEnZUTj7 x/OOKt9Qs2awmRWmp2szd+h/zArV8sI+GmQzSYZHHb88F2VvfYU48rG98hTsAHeItcOG BM4F8iS8QvDsPFyGREjBCEotDfROMVFzxMzdnwmf00XqsVDR8tX1w7RrzuwXfyIYz8j0 uuOOi4m5kbp0O2NOupQcP8hD8H0Ma1KE0TN9TM9EnBSTw1wI3yRgTb+wUs26aaAj2LFD 3vmw== X-Gm-Message-State: AOAM531yNz1qJl0FhC1Pj671YHXOCbM59kpujcdj8dSaLkMPNY/6Y7uL uhxmCDMLvXzvgnrD/7SJgo0giqgR4y1WA1Z9nw4qmA== X-Google-Smtp-Source: ABdhPJzCoBS5kd86/SCGEZcD+gXz+gdgubc9i0tipov1et+C/LNyDYoVYfDdAhhd/mI7cjZ23uDL5REjVYf5ybzO404= X-Received: by 2002:a17:902:820a:b029:10c:a12b:e524 with SMTP id x10-20020a170902820ab029010ca12be524mr4082153pln.79.1622693746180; Wed, 02 Jun 2021 21:15:46 -0700 (PDT) MIME-Version: 1.0 References: <20210603033958.GA2069090@bjorn-Precision-5520> In-Reply-To: <20210603033958.GA2069090@bjorn-Precision-5520> From: Dan Williams Date: Wed, 2 Jun 2021 21:15:35 -0700 Message-ID: Subject: Re: [PATCH v4] /dev/mem: Revoke mappings when a driver claims the region To: Bjorn Helgaas Cc: Greg KH , Arnd Bergmann , Ingo Molnar , Kees Cook , Matthew Wilcox , Russell King , Andrew Morton , Linux Kernel Mailing List , Linux MM , Linux PCI , Daniel Vetter , =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= , Jason Gunthorpe , Christoph Hellwig , =?UTF-8?Q?Pali_Roh=C3=A1r?= , "Oliver O'Halloran" Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=intel-com.20150623.gappssmtp.com header.s=20150623 header.b=dLgNK1Vx; spf=none (imf27.hostedemail.com: domain of dan.j.williams@intel.com has no SPF policy when checking 209.85.214.176) smtp.mailfrom=dan.j.williams@intel.com; dmarc=fail reason="No valid SPF, DKIM not aligned (relaxed)" header.from=intel.com (policy=none) X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 3F7A280192FE X-Stat-Signature: 7x757nztkmp5mg5ct8hkhkfzba3jy1nb X-HE-Tag: 1622693734-215923 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, Jun 2, 2021 at 8:40 PM Bjorn Helgaas wrote: > > [+cc Pali, Oliver] > > On Thu, May 27, 2021 at 02:30:31PM -0700, Dan Williams wrote: > > On Thu, May 27, 2021 at 1:58 PM Bjorn Helgaas wrote: > > > > > > [+cc Daniel, Krzysztof, Jason, Christoph, linux-pci] > > > > > > On Thu, May 21, 2020 at 02:06:17PM -0700, Dan Williams wrote: > > > > Close the hole of holding a mapping over kernel driver takeover event of > > > > a given address range. > > > > > > > > Commit 90a545e98126 ("restrict /dev/mem to idle io memory ranges") > > > > introduced CONFIG_IO_STRICT_DEVMEM with the goal of protecting the > > > > kernel against scenarios where a /dev/mem user tramples memory that a > > > > kernel driver owns. However, this protection only prevents *new* read(), > > > > write() and mmap() requests. Established mappings prior to the driver > > > > calling request_mem_region() are left alone. > > > > > > > > Especially with persistent memory, and the core kernel metadata that is > > > > stored there, there are plentiful scenarios for a /dev/mem user to > > > > violate the expectations of the driver and cause amplified damage. > > > > > > > > Teach request_mem_region() to find and shoot down active /dev/mem > > > > mappings that it believes it has successfully claimed for the exclusive > > > > use of the driver. Effectively a driver call to request_mem_region() > > > > becomes a hole-punch on the /dev/mem device. > > > > > > This idea of hole-punching /dev/mem has since been extended to PCI > > > BARs via [1]. > > > > > > Correct me if I'm wrong: I think this means that if a user process has > > > mmapped a PCI BAR via sysfs, and a kernel driver subsequently requests > > > that region via pci_request_region() or similar, we punch holes in the > > > the user process mmap. The driver might be happy, but my guess is the > > > user starts seeing segmentation violations for no obvious reason and > > > is not happy. > > > > > > Apart from the user process issue, the implementation of [1] is > > > problematic for PCI because the mmappable sysfs attributes now depend > > > on iomem_init_inode(), an fs_initcall, which means they can't be > > > static attributes, which ultimately leads to races in creating them. > > > > See the comments in iomem_get_mapping(), and revoke_iomem(): > > > > /* > > * Check that the initialization has completed. Losing the race > > * is ok because it means drivers are claiming resources before > > * the fs_initcall level of init and prevent iomem_get_mapping users > > * from establishing mappings. > > */ > > > > ...the observation being that it is ok for the revocation inode to > > come on later in the boot process because userspace won't be able to > > use the fs yet. So any missed calls to revoke_iomem() would fall back > > to userspace just seeing the resource busy in the first instance. I.e. > > through the normal devmem_is_allowed() exclusion. > > I did see that comment, but the race I meant is different. Pali wrote > up a nice analysis of it [3]. > > Here's the typical enumeration flow for PCI: > > acpi_pci_root_add <-- subsys_initcall (4) > pci_acpi_scan_root > ... > pci_device_add > device_initialize > device_add > device_add_attrs <-- static sysfs attributes created > ... > pci_bus_add_devices > pci_bus_add_device > pci_create_sysfs_dev_files > if (!sysfs_initialized) return; <-- Ugh :) > ... > attr->mmap = pci_mmap_resource_uc > attr->mapping = iomem_get_mapping() <-- new dependency > return iomem_inode->i_mapping > sysfs_create_bin_file <-- dynamic sysfs attributes created > > iomem_init_inode <-- fs_initcall (5) > iomem_inode = ... <-- now iomem_get_mapping() works > > pci_sysfs_init <-- late_initcall (7) > sysfs_initialized = 1 <-- Ugh (see above) > for_each_pci_dev(dev) <-- Ugh > pci_create_sysfs_dev_files(dev) > > The race is between the pci_sysfs_init() initcall (intended for > boot-time devices) and the pci_bus_add_device() path (used for all > devices including hot-added ones). Pali outlined cases where we call > pci_create_sysfs_dev_files() from both paths for the same device. > > "sysfs_initialized" is a gross hack that prevents this most of the > time, but not always. I want to get rid of it and pci_sysfs_init(). > > Oliver had the excellent idea of using static sysfs attributes to do > this cleanly [4]. If we can convert things to static attributes, the > device core creates them in device_add(), so we don't have to create > them in pci_create_sysfs_dev_files(). > > Krzysztof recently did some very nice work to convert most things to > static attributes, e.g., [5]. But we can't do this for the PCI BAR > attributes because they support ->mmap(), which now depends on > iomem_get_mapping(), which IIUC doesn't work until after fs_initcalls. Ah, sorry, yes, I see the race now. And yes, anything that gets in the way of the static attribute conversion needs fixing. How about something like this? diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index beb8d1f4fafe..c8bc249750d6 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -1195,7 +1195,7 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine) } } if (res_attr->mmap) - res_attr->mapping = iomem_get_mapping(); + res_attr->mapping = iomem_get_mapping; res_attr->attr.name = res_attr_name; res_attr->attr.mode = 0600; res_attr->size = pci_resource_len(pdev, num); diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 9aefa7779b29..a3ee4c32a264 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -175,7 +175,7 @@ static int sysfs_kf_bin_open(struct kernfs_open_file *of) struct bin_attribute *battr = of->kn->priv; if (battr->mapping) - of->file->f_mapping = battr->mapping; + of->file->f_mapping = battr->mapping(); return 0; } diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index d76a1ddf83a3..fbb7c7df545c 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -170,7 +170,7 @@ struct bin_attribute { struct attribute attr; size_t size; void *private; - struct address_space *mapping; + struct address_space *(*mapping)(void); ssize_t (*read)(struct file *, struct kobject *, struct bin_attribute *, char *, loff_t, size_t); ssize_t (*write)(struct file *, struct kobject *, struct bin_attribute *,