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 1B86CC56202 for ; Wed, 21 Oct 2020 19:26:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A8B4924179 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" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2504935AbgJUT0F (ORCPT ); Wed, 21 Oct 2020 15:26:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54762 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2441904AbgJUT0E (ORCPT ); Wed, 21 Oct 2020 15:26:04 -0400 Received: from mail-oi1-x243.google.com (mail-oi1-x243.google.com [IPv6:2607:f8b0:4864:20::243]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8851FC0613CE for ; Wed, 21 Oct 2020 12:26:03 -0700 (PDT) Received: by mail-oi1-x243.google.com with SMTP id j7so3264284oie.12 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=bjTUHkr7OLtMs7xlK03SPwORB6zFCIdrVnGfoinECpWQJIgvKGUrvIMHjDn99aMLBq XAg7VZA1QF0vN7wQnqw3N65XwptfGYOvWWT0OBDvo1PcpXgHBjSgyF7/QTkX2zMutEHH GWeF0ZIvXg83E8buyAeZe99Andg2uBWKijYir2eIV1kxb5kelv3al7Jt3oAmeX/ovtRF i6THqgJKJ9zwAHEtixVK13W9RciwhNOTDM/RcQ0YMEtgH+n1In8rHxhIpDfuhLdyZjaM ezEIsiTItHN3BzF6RkrzCIs22j+71veEdw+XYyBzsBgKYQpIF3ZdjkY2PFe6ijYPgZty PW3g== X-Gm-Message-State: AOAM532NNT6uNQJPwIM9VsNSf9TV506bVJTgrxJAHAsn9SbahCXTyV7R UWf70ejZ6s5aUSuGBhzawiAJojuFAT1SuB3tkM6Wrw== 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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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