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=-9.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 D419BC388F9 for ; Wed, 21 Oct 2020 19:26:06 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 334CE24170 for ; Wed, 21 Oct 2020 19:26:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="U3XYCOdj" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 334CE24170 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 70BBE6B0062; Wed, 21 Oct 2020 15:26:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6BCDB6B0068; Wed, 21 Oct 2020 15:26:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 55F1E6B006C; Wed, 21 Oct 2020 15:26:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0046.hostedemail.com [216.40.44.46]) by kanga.kvack.org (Postfix) with ESMTP id 28A966B0062 for ; Wed, 21 Oct 2020 15:26:05 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id BF570180AD802 for ; Wed, 21 Oct 2020 19:26:04 +0000 (UTC) X-FDA: 77396913048.30.noise92_2b17f1f2724a Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin30.hostedemail.com (Postfix) with ESMTP id 8B703180B3C83 for ; Wed, 21 Oct 2020 19:26:04 +0000 (UTC) X-HE-Tag: noise92_2b17f1f2724a X-Filterd-Recvd-Size: 15808 Received: from mail-oi1-f196.google.com (mail-oi1-f196.google.com [209.85.167.196]) by imf28.hostedemail.com (Postfix) with ESMTP for ; Wed, 21 Oct 2020 19:26:03 +0000 (UTC) Received: by mail-oi1-f196.google.com with SMTP id k27so630981oij.11 for ; Wed, 21 Oct 2020 12:26:03 -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=a/Gb9HcqyOMYoS5cL3Mt5peyKXCWIbiLEmXmAVK4Hps=; b=U3XYCOdjuUNVKWCdmns9iMe0roViquFzK9csIWb7yZp6wryx/eNlxOudPUYOzPTKCW R7lbsTyx1KoDy1l//OTwTi/0983AWoWL+Ua3VHte5mlvaOxbpvb15Bv6y7VNPPwB6/vu tbUXkmbe0y5ZTXPxSG0YA1KiqHumdnE8YKYuY= 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=a/Gb9HcqyOMYoS5cL3Mt5peyKXCWIbiLEmXmAVK4Hps=; b=LO1AmPG3iSs+yUjvcXTSZrhazUCuvty1x75Sha2Xl+CUiz3YQ6RHQ32NpTTc6whKMG unyZu0wWBGEP5LNBlZUf62TI4xyh8ythhk0YYNzx1CYFQsbNcfnM+eIPiffJqIagWTc5 k9wLiv/xCJsR6tsJPvPJJM9qd1ysDbJ1gEOUaB8i37kQf26376PB87hT7RktbSgLWhPa Cy0eiedLBaMQna6g6b+vVFHAPtlWuTzGHSqqBv3+ooJcWkhaSb6l3nlDZ+486Nfobq22 aPNxfvDT4PD/TdgOhW7uQXAJ5leFrqu9rkPUT3acJVyrhqB8JYXnTodcjDJ29+SI99cs oANQ== X-Gm-Message-State: AOAM531/mvWDi2jRVK3zeBzpkakYB6C5qzLZ5uydJcE0oe+xpDA+t90g WnRbeoCeKQpD8a6RBJm2r+658KvMbqpiPOIWVL4KFw== X-Google-Smtp-Source: ABdhPJzKXpPxrAZFssctpwQnMpLd8XkoDOj8a4x2BaU8FAudQbPjd+HCkzpXS4SNO9WctUhK8bbnBXSWU4xkUF0giQE= X-Received: by 2002:aca:cc01:: with SMTP id c1mr3370927oig.128.1603308362927; Wed, 21 Oct 2020 12:26:02 -0700 (PDT) MIME-Version: 1.0 References: <20201021085655.1192025-1-daniel.vetter@ffwll.ch> <20201021085655.1192025-15-daniel.vetter@ffwll.ch> In-Reply-To: From: Daniel Vetter Date: Wed, 21 Oct 2020 21:25:52 +0200 Message-ID: Subject: Re: [PATCH v3 14/16] resource: Move devmem revoke code to resource framework To: Dan Williams Cc: DRI Development , LKML , KVM list , Linux MM , Linux ARM , linux-samsung-soc , "Linux-media@vger.kernel.org" , linux-s390 , Greg Kroah-Hartman , Daniel Vetter , Jason Gunthorpe , Kees Cook , Andrew Morton , John Hubbard , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Jan Kara , Arnd Bergmann , David Hildenbrand , "Rafael J. Wysocki" , 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 21, 2020 at 8:59 PM Dan Williams wro= te: > > On Wed, Oct 21, 2020 at 1:57 AM Daniel Vetter wr= ote: > > > > We want all iomem mmaps to consistently revoke ptes when the kernel > > takes over and CONFIG_IO_STRICT_DEVMEM is enabled. This includes the > > pci bar mmaps available through procfs and sysfs, which currently do > > not revoke mappings. > > > > To prepare for this, move the code from the /dev/kmem driver to > > kernel/resource.c. > > > > Reviewed-by: Greg Kroah-Hartman > > Signed-off-by: Daniel Vetter > > Cc: Jason Gunthorpe > > Cc: Kees Cook > > Cc: Dan Williams > > 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 > > Cc: Arnd Bergmann > > Cc: Greg Kroah-Hartman > > Cc: Daniel Vetter > > Cc: David Hildenbrand > > Cc: "Rafael J. Wysocki" > > Signed-off-by: Daniel Vetter > > -- > > v3: > > - add barrier for consistency and document why we don't have to check > > for NULL (Jason) > > --- > > drivers/char/mem.c | 85 +--------------------------------- > > include/linux/ioport.h | 6 +-- > > kernel/resource.c | 101 ++++++++++++++++++++++++++++++++++++++++- > > 3 files changed, 102 insertions(+), 90 deletions(-) > > > > diff --git a/drivers/char/mem.c b/drivers/char/mem.c > > index 5502f56f3655..53338aad8d28 100644 > > --- a/drivers/char/mem.c > > +++ b/drivers/char/mem.c > > @@ -31,9 +31,6 @@ > > #include > > #include > > #include > > -#include > > -#include > > -#include > > > > #ifdef CONFIG_IA64 > > # include > > @@ -809,42 +806,6 @@ static loff_t memory_lseek(struct file *file, loff= _t offset, int orig) > > return ret; > > } > > > > -static struct inode *devmem_inode; > > - > > -#ifdef CONFIG_IO_STRICT_DEVMEM > > -void revoke_devmem(struct resource *res) > > -{ > > - /* pairs with smp_store_release() in devmem_init_inode() */ > > - struct inode *inode =3D smp_load_acquire(&devmem_inode); > > - > > - /* > > - * 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 /dev/mem from > > - * establishing mappings. > > - */ > > - if (!inode) > > - return; > > - > > - /* > > - * The expectation is that the driver has successfully marked > > - * the resource busy by this point, so devmem_is_allowed() > > - * should start returning false, however for performance this > > - * does not iterate the entire resource range. > > - */ > > - if (devmem_is_allowed(PHYS_PFN(res->start)) && > > - devmem_is_allowed(PHYS_PFN(res->end))) { > > - /* > > - * *cringe* iomem=3Drelaxed says "go ahead, what's the > > - * worst that can happen?" > > - */ > > - return; > > - } > > - > > - unmap_mapping_range(inode->i_mapping, res->start, resource_size= (res), 1); > > -} > > -#endif > > - > > static int open_port(struct inode *inode, struct file *filp) > > { > > int rc; > > @@ -864,7 +825,7 @@ static int open_port(struct inode *inode, struct fi= le *filp) > > * revocations when drivers want to take over a /dev/mem mapped > > * range. > > */ > > - filp->f_mapping =3D inode->i_mapping; > > + filp->f_mapping =3D iomem_get_mapping(); > > > > return 0; > > } > > @@ -995,48 +956,6 @@ static char *mem_devnode(struct device *dev, umode= _t *mode) > > > > static struct class *mem_class; > > > > -static int devmem_fs_init_fs_context(struct fs_context *fc) > > -{ > > - return init_pseudo(fc, DEVMEM_MAGIC) ? 0 : -ENOMEM; > > -} > > - > > -static struct file_system_type devmem_fs_type =3D { > > - .name =3D "devmem", > > - .owner =3D THIS_MODULE, > > - .init_fs_context =3D devmem_fs_init_fs_context, > > - .kill_sb =3D kill_anon_super, > > -}; > > - > > -static int devmem_init_inode(void) > > -{ > > - static struct vfsmount *devmem_vfs_mount; > > - static int devmem_fs_cnt; > > - struct inode *inode; > > - int rc; > > - > > - rc =3D simple_pin_fs(&devmem_fs_type, &devmem_vfs_mount, &devme= m_fs_cnt); > > - if (rc < 0) { > > - pr_err("Cannot mount /dev/mem pseudo filesystem: %d\n",= rc); > > - return rc; > > - } > > - > > - inode =3D alloc_anon_inode(devmem_vfs_mount->mnt_sb); > > - if (IS_ERR(inode)) { > > - rc =3D PTR_ERR(inode); > > - pr_err("Cannot allocate inode for /dev/mem: %d\n", rc); > > - simple_release_fs(&devmem_vfs_mount, &devmem_fs_cnt); > > - return rc; > > - } > > - > > - /* > > - * Publish /dev/mem initialized. > > - * Pairs with smp_load_acquire() in revoke_devmem(). > > - */ > > - smp_store_release(&devmem_inode, inode); > > - > > - return 0; > > -} > > - > > static int __init chr_dev_init(void) > > { > > int minor; > > @@ -1058,8 +977,6 @@ static int __init chr_dev_init(void) > > */ > > if ((minor =3D=3D DEVPORT_MINOR) && !arch_has_dev_port(= )) > > continue; > > - if ((minor =3D=3D DEVMEM_MINOR) && devmem_init_inode() = !=3D 0) > > - continue; > > > > device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor), > > NULL, devlist[minor].name); > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > > index 6c2b06fe8beb..8ffb61b36606 100644 > > --- a/include/linux/ioport.h > > +++ b/include/linux/ioport.h > > @@ -302,11 +302,7 @@ struct resource *devm_request_free_mem_region(stru= ct device *dev, > > struct resource *request_free_mem_region(struct resource *base, > > unsigned long size, const char *name); > > > > -#ifdef CONFIG_IO_STRICT_DEVMEM > > -void revoke_devmem(struct resource *res); > > -#else > > -static inline void revoke_devmem(struct resource *res) { }; > > -#endif > > +extern struct address_space *iomem_get_mapping(void); > > > > #endif /* __ASSEMBLY__ */ > > #endif /* _LINUX_IOPORT_H */ > > diff --git a/kernel/resource.c b/kernel/resource.c > > index 841737bbda9e..a92fed5b9997 100644 > > --- a/kernel/resource.c > > +++ b/kernel/resource.c > > @@ -18,12 +18,15 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > #include > > #include > > +#include > > #include > > +#include > > #include > > > > > > @@ -1112,6 +1115,58 @@ resource_size_t resource_alignment(struct resour= ce *res) > > > > static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait); > > > > +static struct inode *iomem_inode; > > + > > +#ifdef CONFIG_IO_STRICT_DEVMEM > > +static void revoke_iomem(struct resource *res) > > +{ > > + /* pairs with smp_store_release() in iomem_init_inode() */ > > + struct inode *inode =3D smp_load_acquire(&iomem_inode); > > + > > + /* > > + * 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 /dev/mem from > > How about: > > s,/dev/mem,iomem_get_mapping() users, > > ...now that this facility is generalized? > > > + * establishing mappings. > > + */ > > + if (!inode) > > + return; > > + > > + /* > > + * The expectation is that the driver has successfully marked > > + * the resource busy by this point, so devmem_is_allowed() > > + * should start returning false, however for performance this > > + * does not iterate the entire resource range. > > + */ > > + if (devmem_is_allowed(PHYS_PFN(res->start)) && > > + devmem_is_allowed(PHYS_PFN(res->end))) { > > + /* > > + * *cringe* iomem=3Drelaxed says "go ahead, what's the > > + * worst that can happen?" > > + */ > > + return; > > + } > > + > > + unmap_mapping_range(inode->i_mapping, res->start, resource_size= (res), 1); > > +} > > +struct address_space *iomem_get_mapping(void) > > +{ > > + /* > > + * This function is only called from file open paths, hence gua= ranteed > > + * that fs_initcalls have completed and no need to check for NU= LL. But > > + * since revoke_iomem can be called before the initcall we stil= l need > > + * the barrier to appease checkers. > > + */ > > + return smp_load_acquire(&iomem_inode)->i_mapping; > > +} > > +#else > > +static void revoke_iomem(struct resource *res) {} > > +struct address_space *iomem_get_mapping(void) > > +{ > > + return NULL; > > +} > > +#endif > > + > > /** > > * __request_region - create a new busy resource region > > * @parent: parent resource descriptor > > @@ -1179,7 +1234,7 @@ struct resource * __request_region(struct resourc= e *parent, > > write_unlock(&resource_lock); > > > > if (res && orig_parent =3D=3D &iomem_resource) > > - revoke_devmem(res); > > + revoke_iomem(res); > > > > return res; > > } > > @@ -1713,4 +1768,48 @@ static int __init strict_iomem(char *str) > > return 1; > > } > > > > +static int iomem_fs_init_fs_context(struct fs_context *fc) > > +{ > > + return init_pseudo(fc, DEVMEM_MAGIC) ? 0 : -ENOMEM; > > +} > > + > > +static struct file_system_type iomem_fs_type =3D { > > + .name =3D "iomem", > > + .owner =3D THIS_MODULE, > > + .init_fs_context =3D iomem_fs_init_fs_context, > > + .kill_sb =3D kill_anon_super, > > +}; > > + > > +static int __init iomem_init_inode(void) > > +{ > > + static struct vfsmount *iomem_vfs_mount; > > + static int iomem_fs_cnt; > > + struct inode *inode; > > + int rc; > > + > > + rc =3D simple_pin_fs(&iomem_fs_type, &iomem_vfs_mount, &iomem_f= s_cnt); > > + if (rc < 0) { > > + pr_err("Cannot mount iomem pseudo filesystem: %d\n", rc= ); > > + return rc; > > + } > > + > > + inode =3D alloc_anon_inode(iomem_vfs_mount->mnt_sb); > > + if (IS_ERR(inode)) { > > + rc =3D PTR_ERR(inode); > > + pr_err("Cannot allocate inode for iomem: %d\n", rc); > > + simple_release_fs(&iomem_vfs_mount, &iomem_fs_cnt); > > + return rc; > > + } > > + > > + /* > > + * Publish /dev/mem initialized. > > Similar potential fixup: > > "Publish iomem revocation inode initialized" Yeah makes sense I fix up the comments, I'll do that in v4. Need to fix up my mangeld sob line anyway :-) Thanks for taking a careful look at this! Cheers, Daniel > Other than that: > > Reviewed-by: Dan Williams --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch