* [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap [not found] <20201127164131.2244124-1-daniel.vetter@ffwll.ch> @ 2020-11-27 16:41 ` Daniel Vetter 2020-11-27 16:41 ` [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps Daniel Vetter 2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter 2 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw) To: DRI Development, LKML Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media, Daniel Vetter, Bjorn Helgaas, Daniel Vetter, Jason Gunthorpe, Kees Cook, Dan Williams, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, linux-pci There's three ways to access PCI BARs from userspace: /dev/mem, sysfs files, and the old proc interface. Two check against iomem_is_exclusive, proc never did. And with CONFIG_IO_STRICT_DEVMEM, this starts to matter, since we don't want random userspace having access to PCI BARs while a driver is loaded and using it. Fix this by adding the same iomem_is_exclusive() check we already have on the sysfs side in pci_mmap_resource(). Acked-by: Bjorn Helgaas <bhelgaas@google.com> References: 90a545e98126 ("restrict /dev/mem to idle io memory ranges") Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Jérôme Glisse <jglisse@redhat.com> Cc: Jan Kara <jack@suse.cz> Cc: Dan Williams <dan.j.williams@intel.com> 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: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> -- v2: Improve commit message (Bjorn) --- drivers/pci/proc.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c index d35186b01d98..3a2f90beb4cb 100644 --- a/drivers/pci/proc.c +++ b/drivers/pci/proc.c @@ -274,6 +274,11 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma) else return -EINVAL; } + + if (dev->resource[i].flags & IORESOURCE_MEM && + iomem_is_exclusive(dev->resource[i].start)) + return -EINVAL; + ret = pci_mmap_page_range(dev, i, vma, fpriv->mmap_state, write_combine); if (ret < 0) -- 2.29.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps [not found] <20201127164131.2244124-1-daniel.vetter@ffwll.ch> 2020-11-27 16:41 ` [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap Daniel Vetter @ 2020-11-27 16:41 ` Daniel Vetter 2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter 2 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw) To: DRI Development, LKML Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media, Daniel Vetter, Greg Kroah-Hartman, Daniel Vetter, Jason Gunthorpe, Kees Cook, Dan Williams, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Bjorn Helgaas, linux-pci, Rafael J. Wysocki, Christian Brauner, David S. Miller, Michael Ellerman, Sourabh Jain, Mauro Carvalho Chehab, Nayna Jain We want to be able to revoke pci mmaps so that the same access rules applies as for /dev/kmem. Revoke support for devmem was added in 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region"). The simplest way to achieve this is by having the same filp->f_mapping for all mappings, so that unmap_mapping_range can find them all, no matter through which file they've been created. Since this must be set at open time we need sysfs support for this. Add an optional mapping parameter bin_attr, which is only consulted when there's also an mmap callback, since without mmap support allowing to adjust the ->f_mapping makes no sense. Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Jérôme Glisse <jglisse@redhat.com> Cc: Jan Kara <jack@suse.cz> Cc: Dan Williams <dan.j.williams@intel.com> 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: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Christian Brauner <christian.brauner@ubuntu.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Sourabh Jain <sourabhjain@linux.ibm.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Cc: Nayna Jain <nayna@linux.ibm.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- fs/sysfs/file.c | 11 +++++++++++ include/linux/sysfs.h | 2 ++ 2 files changed, 13 insertions(+) diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 96d0da65e088..9aefa7779b29 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -170,6 +170,16 @@ static int sysfs_kf_bin_mmap(struct kernfs_open_file *of, return battr->mmap(of->file, kobj, battr, vma); } +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; + + return 0; +} + void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr) { struct kernfs_node *kn = kobj->sd, *tmp; @@ -241,6 +251,7 @@ static const struct kernfs_ops sysfs_bin_kfops_mmap = { .read = sysfs_kf_bin_read, .write = sysfs_kf_bin_write, .mmap = sysfs_kf_bin_mmap, + .open = sysfs_kf_bin_open, }; int sysfs_add_file_mode_ns(struct kernfs_node *parent, diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index 2caa34c1ca1a..d76a1ddf83a3 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -164,11 +164,13 @@ __ATTRIBUTE_GROUPS(_name) struct file; struct vm_area_struct; +struct address_space; struct bin_attribute { struct attribute attr; size_t size; void *private; + struct address_space *mapping; 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 *, -- 2.29.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v7 12/17] PCI: Revoke mappings like devmem [not found] <20201127164131.2244124-1-daniel.vetter@ffwll.ch> 2020-11-27 16:41 ` [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap Daniel Vetter 2020-11-27 16:41 ` [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps Daniel Vetter @ 2020-11-27 16:41 ` Daniel Vetter 2021-01-19 8:17 ` Daniel Vetter 2 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2020-11-27 16:41 UTC (permalink / raw) To: DRI Development, LKML Cc: kvm, linux-mm, linux-arm-kernel, linux-samsung-soc, linux-media, Daniel Vetter, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Greg Kroah-Hartman, linux-pci Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region") /dev/kmem zaps ptes when the kernel requests exclusive acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is the default for all driver uses. Except there's two more ways to access PCI BARs: sysfs and proc mmap support. Let's plug that hole. For revoke_devmem() to work we need to link our vma into the same address_space, with consistent vma->vm_pgoff. ->pgoff is already adjusted, because that's how (io_)remap_pfn_range works, but for the mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is to adjust this at at ->open time: - for sysfs this is easy, now that binary attributes support this. We just set bin_attr->mapping when mmap is supported - for procfs it's a bit more tricky, since procfs pci access has only one file per device, and access to a specific resources first needs to be set up with some ioctl calls. But mmap is only supported for the same resources as sysfs exposes with mmap support, and otherwise rejected, so we can set the mapping unconditionally at open time without harm. A special consideration is for arch_can_pci_mmap_io() - we need to make sure that the ->f_mapping doesn't alias between ioport and iomem space. There's only 2 ways in-tree to support mmap of ioports: generic pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single architecture hand-rolling. Both approach support ioport mmap through a special pfn range and not through magic pte attributes. Aliasing is therefore not a problem. The only difference in access checks left is that sysfs PCI mmap does not check for CAP_RAWIO. I'm not really sure whether that should be added or not. Acked-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Kees Cook <keescook@chromium.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Jérôme Glisse <jglisse@redhat.com> Cc: Jan Kara <jack@suse.cz> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> -- v2: - Totally new approach: Adjust filp->f_mapping at open time. Note that this now works on all architectures, not just those support ARCH_GENERIC_PCI_MMAP_RESOURCE --- drivers/pci/pci-sysfs.c | 4 ++++ drivers/pci/proc.c | 1 + 2 files changed, 5 insertions(+) diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index d15c881e2e7e..3f1c31bc0b7c 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) b->legacy_io->read = pci_read_legacy_io; b->legacy_io->write = pci_write_legacy_io; b->legacy_io->mmap = pci_mmap_legacy_io; + b->legacy_io->mapping = iomem_get_mapping(); pci_adjust_legacy_attr(b, pci_mmap_io); error = device_create_bin_file(&b->dev, b->legacy_io); if (error) @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) b->legacy_mem->size = 1024*1024; b->legacy_mem->attr.mode = 0600; b->legacy_mem->mmap = pci_mmap_legacy_mem; + b->legacy_io->mapping = iomem_get_mapping(); pci_adjust_legacy_attr(b, pci_mmap_mem); error = device_create_bin_file(&b->dev, b->legacy_mem); if (error) @@ -1156,6 +1158,8 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine) res_attr->mmap = pci_mmap_resource_uc; } } + if (res_attr->mmap) + 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/drivers/pci/proc.c b/drivers/pci/proc.c index 3a2f90beb4cb..9bab07302bbf 100644 --- a/drivers/pci/proc.c +++ b/drivers/pci/proc.c @@ -298,6 +298,7 @@ static int proc_bus_pci_open(struct inode *inode, struct file *file) fpriv->write_combine = 0; file->private_data = fpriv; + file->f_mapping = iomem_get_mapping(); return 0; } -- 2.29.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem 2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter @ 2021-01-19 8:17 ` Daniel Vetter 2021-01-19 14:32 ` Greg Kroah-Hartman 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2021-01-19 8:17 UTC (permalink / raw) To: DRI Development, LKML, Stephen Rothwell Cc: KVM list, Linux MM, Linux ARM, linux-samsung-soc, open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Greg Kroah-Hartman, Linux PCI On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims > the region") /dev/kmem zaps ptes when the kernel requests exclusive > acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is > the default for all driver uses. > > Except there's two more ways to access PCI BARs: sysfs and proc mmap > support. Let's plug that hole. > > For revoke_devmem() to work we need to link our vma into the same > address_space, with consistent vma->vm_pgoff. ->pgoff is already > adjusted, because that's how (io_)remap_pfn_range works, but for the > mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is > to adjust this at at ->open time: > > - for sysfs this is easy, now that binary attributes support this. We > just set bin_attr->mapping when mmap is supported > - for procfs it's a bit more tricky, since procfs pci access has only > one file per device, and access to a specific resources first needs > to be set up with some ioctl calls. But mmap is only supported for > the same resources as sysfs exposes with mmap support, and otherwise > rejected, so we can set the mapping unconditionally at open time > without harm. > > A special consideration is for arch_can_pci_mmap_io() - we need to > make sure that the ->f_mapping doesn't alias between ioport and iomem > space. There's only 2 ways in-tree to support mmap of ioports: generic > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single > architecture hand-rolling. Both approach support ioport mmap through a > special pfn range and not through magic pte attributes. Aliasing is > therefore not a problem. > > The only difference in access checks left is that sysfs PCI mmap does > not check for CAP_RAWIO. I'm not really sure whether that should be > added or not. > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Kees Cook <keescook@chromium.org> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > 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: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > -- > v2: > - Totally new approach: Adjust filp->f_mapping at open time. Note that > this now works on all architectures, not just those support > ARCH_GENERIC_PCI_MMAP_RESOURCE > --- > drivers/pci/pci-sysfs.c | 4 ++++ > drivers/pci/proc.c | 1 + > 2 files changed, 5 insertions(+) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index d15c881e2e7e..3f1c31bc0b7c 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) > b->legacy_io->read = pci_read_legacy_io; > b->legacy_io->write = pci_write_legacy_io; > b->legacy_io->mmap = pci_mmap_legacy_io; > + b->legacy_io->mapping = iomem_get_mapping(); > pci_adjust_legacy_attr(b, pci_mmap_io); > error = device_create_bin_file(&b->dev, b->legacy_io); > if (error) > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) > b->legacy_mem->size = 1024*1024; > b->legacy_mem->attr.mode = 0600; > b->legacy_mem->mmap = pci_mmap_legacy_mem; > + b->legacy_io->mapping = iomem_get_mapping(); Unlike the normal pci stuff below, the legacy files here go boom because they're set up much earlier in the boot sequence. This only affects HAVE_PCI_LEGACY architectures, which aren't that many. So what should we do here now: - drop the devmem revoke for these - rework the init sequence somehow to set up these files a lot later - redo the sysfs patch so that it doesn't take an address_space pointer, but instead a callback to get at that (since at open time everything is set up). Imo rather ugly - ditch this part of the series (since there's not really any takers for the latter parts it might just not make sense to push for this) - something else? Bjorn, Greg, thoughts? Issuge got reported by Stephen on a powerpc when trying to build linux-next with this patch included. Thanks, Daniel > pci_adjust_legacy_attr(b, pci_mmap_mem); > error = device_create_bin_file(&b->dev, b->legacy_mem); > if (error) > @@ -1156,6 +1158,8 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine) > res_attr->mmap = pci_mmap_resource_uc; > } > } > + if (res_attr->mmap) > + 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/drivers/pci/proc.c b/drivers/pci/proc.c > index 3a2f90beb4cb..9bab07302bbf 100644 > --- a/drivers/pci/proc.c > +++ b/drivers/pci/proc.c > @@ -298,6 +298,7 @@ static int proc_bus_pci_open(struct inode *inode, struct file *file) > fpriv->write_combine = 0; > > file->private_data = fpriv; > + file->f_mapping = iomem_get_mapping(); > > return 0; > } > -- > 2.29.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem 2021-01-19 8:17 ` Daniel Vetter @ 2021-01-19 14:32 ` Greg Kroah-Hartman 2021-01-19 14:34 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Greg Kroah-Hartman @ 2021-01-19 14:32 UTC (permalink / raw) To: Daniel Vetter Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM, Linux ARM, linux-samsung-soc, open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Linux PCI On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote: > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims > > the region") /dev/kmem zaps ptes when the kernel requests exclusive > > acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is > > the default for all driver uses. > > > > Except there's two more ways to access PCI BARs: sysfs and proc mmap > > support. Let's plug that hole. > > > > For revoke_devmem() to work we need to link our vma into the same > > address_space, with consistent vma->vm_pgoff. ->pgoff is already > > adjusted, because that's how (io_)remap_pfn_range works, but for the > > mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is > > to adjust this at at ->open time: > > > > - for sysfs this is easy, now that binary attributes support this. We > > just set bin_attr->mapping when mmap is supported > > - for procfs it's a bit more tricky, since procfs pci access has only > > one file per device, and access to a specific resources first needs > > to be set up with some ioctl calls. But mmap is only supported for > > the same resources as sysfs exposes with mmap support, and otherwise > > rejected, so we can set the mapping unconditionally at open time > > without harm. > > > > A special consideration is for arch_can_pci_mmap_io() - we need to > > make sure that the ->f_mapping doesn't alias between ioport and iomem > > space. There's only 2 ways in-tree to support mmap of ioports: generic > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single > > architecture hand-rolling. Both approach support ioport mmap through a > > special pfn range and not through magic pte attributes. Aliasing is > > therefore not a problem. > > > > The only difference in access checks left is that sysfs PCI mmap does > > not check for CAP_RAWIO. I'm not really sure whether that should be > > added or not. > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: John Hubbard <jhubbard@nvidia.com> > > Cc: Jérôme Glisse <jglisse@redhat.com> > > Cc: Jan Kara <jack@suse.cz> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > 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: Bjorn Helgaas <bhelgaas@google.com> > > Cc: linux-pci@vger.kernel.org > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > -- > > v2: > > - Totally new approach: Adjust filp->f_mapping at open time. Note that > > this now works on all architectures, not just those support > > ARCH_GENERIC_PCI_MMAP_RESOURCE > > --- > > drivers/pci/pci-sysfs.c | 4 ++++ > > drivers/pci/proc.c | 1 + > > 2 files changed, 5 insertions(+) > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > index d15c881e2e7e..3f1c31bc0b7c 100644 > > --- a/drivers/pci/pci-sysfs.c > > +++ b/drivers/pci/pci-sysfs.c > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > b->legacy_io->read = pci_read_legacy_io; > > b->legacy_io->write = pci_write_legacy_io; > > b->legacy_io->mmap = pci_mmap_legacy_io; > > + b->legacy_io->mapping = iomem_get_mapping(); > > pci_adjust_legacy_attr(b, pci_mmap_io); > > error = device_create_bin_file(&b->dev, b->legacy_io); > > if (error) > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > b->legacy_mem->size = 1024*1024; > > b->legacy_mem->attr.mode = 0600; > > b->legacy_mem->mmap = pci_mmap_legacy_mem; > > + b->legacy_io->mapping = iomem_get_mapping(); > > Unlike the normal pci stuff below, the legacy files here go boom > because they're set up much earlier in the boot sequence. This only > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what > should we do here now: > - drop the devmem revoke for these > - rework the init sequence somehow to set up these files a lot later > - redo the sysfs patch so that it doesn't take an address_space > pointer, but instead a callback to get at that (since at open time > everything is set up). Imo rather ugly > - ditch this part of the series (since there's not really any takers > for the latter parts it might just not make sense to push for this) > - something else? > > Bjorn, Greg, thoughts? What sysfs patch are you referring to here? thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem 2021-01-19 14:32 ` Greg Kroah-Hartman @ 2021-01-19 14:34 ` Daniel Vetter 2021-01-19 15:20 ` Greg Kroah-Hartman 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2021-01-19 14:34 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM, Linux ARM, linux-samsung-soc, open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Linux PCI On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote: > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims > > > the region") /dev/kmem zaps ptes when the kernel requests exclusive > > > acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is > > > the default for all driver uses. > > > > > > Except there's two more ways to access PCI BARs: sysfs and proc mmap > > > support. Let's plug that hole. > > > > > > For revoke_devmem() to work we need to link our vma into the same > > > address_space, with consistent vma->vm_pgoff. ->pgoff is already > > > adjusted, because that's how (io_)remap_pfn_range works, but for the > > > mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is > > > to adjust this at at ->open time: > > > > > > - for sysfs this is easy, now that binary attributes support this. We > > > just set bin_attr->mapping when mmap is supported > > > - for procfs it's a bit more tricky, since procfs pci access has only > > > one file per device, and access to a specific resources first needs > > > to be set up with some ioctl calls. But mmap is only supported for > > > the same resources as sysfs exposes with mmap support, and otherwise > > > rejected, so we can set the mapping unconditionally at open time > > > without harm. > > > > > > A special consideration is for arch_can_pci_mmap_io() - we need to > > > make sure that the ->f_mapping doesn't alias between ioport and iomem > > > space. There's only 2 ways in-tree to support mmap of ioports: generic > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single > > > architecture hand-rolling. Both approach support ioport mmap through a > > > special pfn range and not through magic pte attributes. Aliasing is > > > therefore not a problem. > > > > > > The only difference in access checks left is that sysfs PCI mmap does > > > not check for CAP_RAWIO. I'm not really sure whether that should be > > > added or not. > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > Cc: John Hubbard <jhubbard@nvidia.com> > > > Cc: Jérôme Glisse <jglisse@redhat.com> > > > Cc: Jan Kara <jack@suse.cz> > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > 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: Bjorn Helgaas <bhelgaas@google.com> > > > Cc: linux-pci@vger.kernel.org > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > -- > > > v2: > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that > > > this now works on all architectures, not just those support > > > ARCH_GENERIC_PCI_MMAP_RESOURCE > > > --- > > > drivers/pci/pci-sysfs.c | 4 ++++ > > > drivers/pci/proc.c | 1 + > > > 2 files changed, 5 insertions(+) > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > > index d15c881e2e7e..3f1c31bc0b7c 100644 > > > --- a/drivers/pci/pci-sysfs.c > > > +++ b/drivers/pci/pci-sysfs.c > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > b->legacy_io->read = pci_read_legacy_io; > > > b->legacy_io->write = pci_write_legacy_io; > > > b->legacy_io->mmap = pci_mmap_legacy_io; > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > pci_adjust_legacy_attr(b, pci_mmap_io); > > > error = device_create_bin_file(&b->dev, b->legacy_io); > > > if (error) > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > b->legacy_mem->size = 1024*1024; > > > b->legacy_mem->attr.mode = 0600; > > > b->legacy_mem->mmap = pci_mmap_legacy_mem; > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > Unlike the normal pci stuff below, the legacy files here go boom > > because they're set up much earlier in the boot sequence. This only > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what > > should we do here now: > > - drop the devmem revoke for these > > - rework the init sequence somehow to set up these files a lot later > > - redo the sysfs patch so that it doesn't take an address_space > > pointer, but instead a callback to get at that (since at open time > > everything is set up). Imo rather ugly > > - ditch this part of the series (since there's not really any takers > > for the latter parts it might just not make sense to push for this) > > - something else? > > > > Bjorn, Greg, thoughts? > > What sysfs patch are you referring to here? Currently in linux-next: commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD -> topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup) Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Fri Nov 27 17:41:25 2020 +0100 sysfs: Support zapping of binary attr mmaps Or the patch right before this one in this submission here: https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/ Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem 2021-01-19 14:34 ` Daniel Vetter @ 2021-01-19 15:20 ` Greg Kroah-Hartman 2021-01-19 16:03 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Greg Kroah-Hartman @ 2021-01-19 15:20 UTC (permalink / raw) To: Daniel Vetter Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM, Linux ARM, linux-samsung-soc, open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Linux PCI On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote: > On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote: > > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > > Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims > > > > the region") /dev/kmem zaps ptes when the kernel requests exclusive > > > > acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is > > > > the default for all driver uses. > > > > > > > > Except there's two more ways to access PCI BARs: sysfs and proc mmap > > > > support. Let's plug that hole. > > > > > > > > For revoke_devmem() to work we need to link our vma into the same > > > > address_space, with consistent vma->vm_pgoff. ->pgoff is already > > > > adjusted, because that's how (io_)remap_pfn_range works, but for the > > > > mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is > > > > to adjust this at at ->open time: > > > > > > > > - for sysfs this is easy, now that binary attributes support this. We > > > > just set bin_attr->mapping when mmap is supported > > > > - for procfs it's a bit more tricky, since procfs pci access has only > > > > one file per device, and access to a specific resources first needs > > > > to be set up with some ioctl calls. But mmap is only supported for > > > > the same resources as sysfs exposes with mmap support, and otherwise > > > > rejected, so we can set the mapping unconditionally at open time > > > > without harm. > > > > > > > > A special consideration is for arch_can_pci_mmap_io() - we need to > > > > make sure that the ->f_mapping doesn't alias between ioport and iomem > > > > space. There's only 2 ways in-tree to support mmap of ioports: generic > > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single > > > > architecture hand-rolling. Both approach support ioport mmap through a > > > > special pfn range and not through magic pte attributes. Aliasing is > > > > therefore not a problem. > > > > > > > > The only difference in access checks left is that sysfs PCI mmap does > > > > not check for CAP_RAWIO. I'm not really sure whether that should be > > > > added or not. > > > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > > > Cc: Kees Cook <keescook@chromium.org> > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > > Cc: John Hubbard <jhubbard@nvidia.com> > > > > Cc: Jérôme Glisse <jglisse@redhat.com> > > > > Cc: Jan Kara <jack@suse.cz> > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > 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: Bjorn Helgaas <bhelgaas@google.com> > > > > Cc: linux-pci@vger.kernel.org > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > -- > > > > v2: > > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that > > > > this now works on all architectures, not just those support > > > > ARCH_GENERIC_PCI_MMAP_RESOURCE > > > > --- > > > > drivers/pci/pci-sysfs.c | 4 ++++ > > > > drivers/pci/proc.c | 1 + > > > > 2 files changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > > > index d15c881e2e7e..3f1c31bc0b7c 100644 > > > > --- a/drivers/pci/pci-sysfs.c > > > > +++ b/drivers/pci/pci-sysfs.c > > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > b->legacy_io->read = pci_read_legacy_io; > > > > b->legacy_io->write = pci_write_legacy_io; > > > > b->legacy_io->mmap = pci_mmap_legacy_io; > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > pci_adjust_legacy_attr(b, pci_mmap_io); > > > > error = device_create_bin_file(&b->dev, b->legacy_io); > > > > if (error) > > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > b->legacy_mem->size = 1024*1024; > > > > b->legacy_mem->attr.mode = 0600; > > > > b->legacy_mem->mmap = pci_mmap_legacy_mem; > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > > > Unlike the normal pci stuff below, the legacy files here go boom > > > because they're set up much earlier in the boot sequence. This only > > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what > > > should we do here now: > > > - drop the devmem revoke for these > > > - rework the init sequence somehow to set up these files a lot later > > > - redo the sysfs patch so that it doesn't take an address_space > > > pointer, but instead a callback to get at that (since at open time > > > everything is set up). Imo rather ugly > > > - ditch this part of the series (since there's not really any takers > > > for the latter parts it might just not make sense to push for this) > > > - something else? > > > > > > Bjorn, Greg, thoughts? > > > > What sysfs patch are you referring to here? > > Currently in linux-next: > > commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD -> > topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup) > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Fri Nov 27 17:41:25 2020 +0100 > > sysfs: Support zapping of binary attr mmaps > > Or the patch right before this one in this submission here: > > https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/ Ah. Hm, a callback in the sysfs file logic seems really hairy, so I would prefer that not happen. If no one really needs this stuff, why not just drop it like you mention? thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem 2021-01-19 15:20 ` Greg Kroah-Hartman @ 2021-01-19 16:03 ` Daniel Vetter 2021-02-03 16:14 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2021-01-19 16:03 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM, Linux ARM, linux-samsung-soc, open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Linux PCI On Tue, Jan 19, 2021 at 4:20 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote: > > On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote: > > > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > > > > Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims > > > > > the region") /dev/kmem zaps ptes when the kernel requests exclusive > > > > > acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is > > > > > the default for all driver uses. > > > > > > > > > > Except there's two more ways to access PCI BARs: sysfs and proc mmap > > > > > support. Let's plug that hole. > > > > > > > > > > For revoke_devmem() to work we need to link our vma into the same > > > > > address_space, with consistent vma->vm_pgoff. ->pgoff is already > > > > > adjusted, because that's how (io_)remap_pfn_range works, but for the > > > > > mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is > > > > > to adjust this at at ->open time: > > > > > > > > > > - for sysfs this is easy, now that binary attributes support this. We > > > > > just set bin_attr->mapping when mmap is supported > > > > > - for procfs it's a bit more tricky, since procfs pci access has only > > > > > one file per device, and access to a specific resources first needs > > > > > to be set up with some ioctl calls. But mmap is only supported for > > > > > the same resources as sysfs exposes with mmap support, and otherwise > > > > > rejected, so we can set the mapping unconditionally at open time > > > > > without harm. > > > > > > > > > > A special consideration is for arch_can_pci_mmap_io() - we need to > > > > > make sure that the ->f_mapping doesn't alias between ioport and iomem > > > > > space. There's only 2 ways in-tree to support mmap of ioports: generic > > > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single > > > > > architecture hand-rolling. Both approach support ioport mmap through a > > > > > special pfn range and not through magic pte attributes. Aliasing is > > > > > therefore not a problem. > > > > > > > > > > The only difference in access checks left is that sysfs PCI mmap does > > > > > not check for CAP_RAWIO. I'm not really sure whether that should be > > > > > added or not. > > > > > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > > > > Cc: Kees Cook <keescook@chromium.org> > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > > > Cc: John Hubbard <jhubbard@nvidia.com> > > > > > Cc: Jérôme Glisse <jglisse@redhat.com> > > > > > Cc: Jan Kara <jack@suse.cz> > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > 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: Bjorn Helgaas <bhelgaas@google.com> > > > > > Cc: linux-pci@vger.kernel.org > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > -- > > > > > v2: > > > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that > > > > > this now works on all architectures, not just those support > > > > > ARCH_GENERIC_PCI_MMAP_RESOURCE > > > > > --- > > > > > drivers/pci/pci-sysfs.c | 4 ++++ > > > > > drivers/pci/proc.c | 1 + > > > > > 2 files changed, 5 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > > > > index d15c881e2e7e..3f1c31bc0b7c 100644 > > > > > --- a/drivers/pci/pci-sysfs.c > > > > > +++ b/drivers/pci/pci-sysfs.c > > > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > > b->legacy_io->read = pci_read_legacy_io; > > > > > b->legacy_io->write = pci_write_legacy_io; > > > > > b->legacy_io->mmap = pci_mmap_legacy_io; > > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > > pci_adjust_legacy_attr(b, pci_mmap_io); > > > > > error = device_create_bin_file(&b->dev, b->legacy_io); > > > > > if (error) > > > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > > b->legacy_mem->size = 1024*1024; > > > > > b->legacy_mem->attr.mode = 0600; > > > > > b->legacy_mem->mmap = pci_mmap_legacy_mem; > > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > > > > > Unlike the normal pci stuff below, the legacy files here go boom > > > > because they're set up much earlier in the boot sequence. This only > > > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what > > > > should we do here now: > > > > - drop the devmem revoke for these > > > > - rework the init sequence somehow to set up these files a lot later > > > > - redo the sysfs patch so that it doesn't take an address_space > > > > pointer, but instead a callback to get at that (since at open time > > > > everything is set up). Imo rather ugly > > > > - ditch this part of the series (since there's not really any takers > > > > for the latter parts it might just not make sense to push for this) > > > > - something else? > > > > > > > > Bjorn, Greg, thoughts? > > > > > > What sysfs patch are you referring to here? > > > > Currently in linux-next: > > > > commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD -> > > topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup) > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Fri Nov 27 17:41:25 2020 +0100 > > > > sysfs: Support zapping of binary attr mmaps > > > > Or the patch right before this one in this submission here: > > > > https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/ > > Ah. Hm, a callback in the sysfs file logic seems really hairy, so I > would prefer that not happen. If no one really needs this stuff, why > not just drop it like you mention? Well it is needed, but just on architectures I don't care about much. Most relevant is perhaps powerpc (that's where Stephen hit the issue). I do wonder whether we could move the legacy pci files setup to where the modern stuff is set up from pci_create_resource_files() or maybe pci_create_sysfs_dev_files() even for HAVE_PCI_LEGACY. I think that might work, but since it's legacy flow on some funny architectures (alpha, itanium, that kind of stuff) I have no idea what kind of monsters I'm going to anger :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem 2021-01-19 16:03 ` Daniel Vetter @ 2021-02-03 16:14 ` Daniel Vetter 2021-02-04 10:23 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2021-02-03 16:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM, Linux ARM, linux-samsung-soc, open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Linux PCI On Tue, Jan 19, 2021 at 5:03 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Tue, Jan 19, 2021 at 4:20 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote: > > > On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote: > > > > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > > > > > > Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims > > > > > > the region") /dev/kmem zaps ptes when the kernel requests exclusive > > > > > > acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is > > > > > > the default for all driver uses. > > > > > > > > > > > > Except there's two more ways to access PCI BARs: sysfs and proc mmap > > > > > > support. Let's plug that hole. > > > > > > > > > > > > For revoke_devmem() to work we need to link our vma into the same > > > > > > address_space, with consistent vma->vm_pgoff. ->pgoff is already > > > > > > adjusted, because that's how (io_)remap_pfn_range works, but for the > > > > > > mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is > > > > > > to adjust this at at ->open time: > > > > > > > > > > > > - for sysfs this is easy, now that binary attributes support this. We > > > > > > just set bin_attr->mapping when mmap is supported > > > > > > - for procfs it's a bit more tricky, since procfs pci access has only > > > > > > one file per device, and access to a specific resources first needs > > > > > > to be set up with some ioctl calls. But mmap is only supported for > > > > > > the same resources as sysfs exposes with mmap support, and otherwise > > > > > > rejected, so we can set the mapping unconditionally at open time > > > > > > without harm. > > > > > > > > > > > > A special consideration is for arch_can_pci_mmap_io() - we need to > > > > > > make sure that the ->f_mapping doesn't alias between ioport and iomem > > > > > > space. There's only 2 ways in-tree to support mmap of ioports: generic > > > > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single > > > > > > architecture hand-rolling. Both approach support ioport mmap through a > > > > > > special pfn range and not through magic pte attributes. Aliasing is > > > > > > therefore not a problem. > > > > > > > > > > > > The only difference in access checks left is that sysfs PCI mmap does > > > > > > not check for CAP_RAWIO. I'm not really sure whether that should be > > > > > > added or not. > > > > > > > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > > > > > Cc: Kees Cook <keescook@chromium.org> > > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > > > > Cc: John Hubbard <jhubbard@nvidia.com> > > > > > > Cc: Jérôme Glisse <jglisse@redhat.com> > > > > > > Cc: Jan Kara <jack@suse.cz> > > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > 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: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Cc: linux-pci@vger.kernel.org > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > -- > > > > > > v2: > > > > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that > > > > > > this now works on all architectures, not just those support > > > > > > ARCH_GENERIC_PCI_MMAP_RESOURCE > > > > > > --- > > > > > > drivers/pci/pci-sysfs.c | 4 ++++ > > > > > > drivers/pci/proc.c | 1 + > > > > > > 2 files changed, 5 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > > > > > index d15c881e2e7e..3f1c31bc0b7c 100644 > > > > > > --- a/drivers/pci/pci-sysfs.c > > > > > > +++ b/drivers/pci/pci-sysfs.c > > > > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > > > b->legacy_io->read = pci_read_legacy_io; > > > > > > b->legacy_io->write = pci_write_legacy_io; > > > > > > b->legacy_io->mmap = pci_mmap_legacy_io; > > > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > > > pci_adjust_legacy_attr(b, pci_mmap_io); > > > > > > error = device_create_bin_file(&b->dev, b->legacy_io); > > > > > > if (error) > > > > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > > > b->legacy_mem->size = 1024*1024; > > > > > > b->legacy_mem->attr.mode = 0600; > > > > > > b->legacy_mem->mmap = pci_mmap_legacy_mem; > > > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > > > > > > > Unlike the normal pci stuff below, the legacy files here go boom > > > > > because they're set up much earlier in the boot sequence. This only > > > > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what > > > > > should we do here now: > > > > > - drop the devmem revoke for these > > > > > - rework the init sequence somehow to set up these files a lot later > > > > > - redo the sysfs patch so that it doesn't take an address_space > > > > > pointer, but instead a callback to get at that (since at open time > > > > > everything is set up). Imo rather ugly > > > > > - ditch this part of the series (since there's not really any takers > > > > > for the latter parts it might just not make sense to push for this) > > > > > - something else? > > > > > > > > > > Bjorn, Greg, thoughts? > > > > > > > > What sysfs patch are you referring to here? > > > > > > Currently in linux-next: > > > > > > commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD -> > > > topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup) > > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Date: Fri Nov 27 17:41:25 2020 +0100 > > > > > > sysfs: Support zapping of binary attr mmaps > > > > > > Or the patch right before this one in this submission here: > > > > > > https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/ > > > > Ah. Hm, a callback in the sysfs file logic seems really hairy, so I > > would prefer that not happen. If no one really needs this stuff, why > > not just drop it like you mention? > > Well it is needed, but just on architectures I don't care about much. > Most relevant is perhaps powerpc (that's where Stephen hit the issue). > I do wonder whether we could move the legacy pci files setup to where > the modern stuff is set up from pci_create_resource_files() or maybe > pci_create_sysfs_dev_files() even for HAVE_PCI_LEGACY. I think that > might work, but since it's legacy flow on some funny architectures > (alpha, itanium, that kind of stuff) I have no idea what kind of > monsters I'm going to anger :-) Back from a week of vacation, I looked at this again and I think shouldn't be hard to fix this with the sam trick pci_create_sysfs_dev_files() uses: As long as sysfs_initialized isn't set we skip, and then later on when the vfs is up&running we can initialize everything. To be able to apply the same thing to pci_create_legacy_files() I think all I need is to iterate overa all struct pci_bus in pci_sysfs_init() and we're good. Unfortunately I didn't find any for_each_pci_bus(), so how do I do that? Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v7 12/17] PCI: Revoke mappings like devmem 2021-02-03 16:14 ` Daniel Vetter @ 2021-02-04 10:23 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2021-02-04 10:23 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: DRI Development, LKML, Stephen Rothwell, KVM list, Linux MM, Linux ARM, linux-samsung-soc, open list:DMA BUFFER SHARING FRAMEWORK, Bjorn Helgaas, Dan Williams, Daniel Vetter, Jason Gunthorpe, Kees Cook, Andrew Morton, John Hubbard, Jérôme Glisse, Jan Kara, Linux PCI On Wed, Feb 3, 2021 at 5:14 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Tue, Jan 19, 2021 at 5:03 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > On Tue, Jan 19, 2021 at 4:20 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Tue, Jan 19, 2021 at 03:34:47PM +0100, Daniel Vetter wrote: > > > > On Tue, Jan 19, 2021 at 3:32 PM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Tue, Jan 19, 2021 at 09:17:55AM +0100, Daniel Vetter wrote: > > > > > > On Fri, Nov 27, 2020 at 5:42 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > > > > > > > > Since 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims > > > > > > > the region") /dev/kmem zaps ptes when the kernel requests exclusive > > > > > > > acccess to an iomem region. And with CONFIG_IO_STRICT_DEVMEM, this is > > > > > > > the default for all driver uses. > > > > > > > > > > > > > > Except there's two more ways to access PCI BARs: sysfs and proc mmap > > > > > > > support. Let's plug that hole. > > > > > > > > > > > > > > For revoke_devmem() to work we need to link our vma into the same > > > > > > > address_space, with consistent vma->vm_pgoff. ->pgoff is already > > > > > > > adjusted, because that's how (io_)remap_pfn_range works, but for the > > > > > > > mapping we need to adjust vma->vm_file->f_mapping. The cleanest way is > > > > > > > to adjust this at at ->open time: > > > > > > > > > > > > > > - for sysfs this is easy, now that binary attributes support this. We > > > > > > > just set bin_attr->mapping when mmap is supported > > > > > > > - for procfs it's a bit more tricky, since procfs pci access has only > > > > > > > one file per device, and access to a specific resources first needs > > > > > > > to be set up with some ioctl calls. But mmap is only supported for > > > > > > > the same resources as sysfs exposes with mmap support, and otherwise > > > > > > > rejected, so we can set the mapping unconditionally at open time > > > > > > > without harm. > > > > > > > > > > > > > > A special consideration is for arch_can_pci_mmap_io() - we need to > > > > > > > make sure that the ->f_mapping doesn't alias between ioport and iomem > > > > > > > space. There's only 2 ways in-tree to support mmap of ioports: generic > > > > > > > pci mmap (ARCH_GENERIC_PCI_MMAP_RESOURCE), and sparc as the single > > > > > > > architecture hand-rolling. Both approach support ioport mmap through a > > > > > > > special pfn range and not through magic pte attributes. Aliasing is > > > > > > > therefore not a problem. > > > > > > > > > > > > > > The only difference in access checks left is that sysfs PCI mmap does > > > > > > > not check for CAP_RAWIO. I'm not really sure whether that should be > > > > > > > added or not. > > > > > > > > > > > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > > > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > > > > > > Cc: Kees Cook <keescook@chromium.org> > > > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > > > > > Cc: John Hubbard <jhubbard@nvidia.com> > > > > > > > Cc: Jérôme Glisse <jglisse@redhat.com> > > > > > > > Cc: Jan Kara <jack@suse.cz> > > > > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > > 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: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > Cc: linux-pci@vger.kernel.org > > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > -- > > > > > > > v2: > > > > > > > - Totally new approach: Adjust filp->f_mapping at open time. Note that > > > > > > > this now works on all architectures, not just those support > > > > > > > ARCH_GENERIC_PCI_MMAP_RESOURCE > > > > > > > --- > > > > > > > drivers/pci/pci-sysfs.c | 4 ++++ > > > > > > > drivers/pci/proc.c | 1 + > > > > > > > 2 files changed, 5 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > > > > > > > index d15c881e2e7e..3f1c31bc0b7c 100644 > > > > > > > --- a/drivers/pci/pci-sysfs.c > > > > > > > +++ b/drivers/pci/pci-sysfs.c > > > > > > > @@ -929,6 +929,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > > > > b->legacy_io->read = pci_read_legacy_io; > > > > > > > b->legacy_io->write = pci_write_legacy_io; > > > > > > > b->legacy_io->mmap = pci_mmap_legacy_io; > > > > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > > > > pci_adjust_legacy_attr(b, pci_mmap_io); > > > > > > > error = device_create_bin_file(&b->dev, b->legacy_io); > > > > > > > if (error) > > > > > > > @@ -941,6 +942,7 @@ void pci_create_legacy_files(struct pci_bus *b) > > > > > > > b->legacy_mem->size = 1024*1024; > > > > > > > b->legacy_mem->attr.mode = 0600; > > > > > > > b->legacy_mem->mmap = pci_mmap_legacy_mem; > > > > > > > + b->legacy_io->mapping = iomem_get_mapping(); > > > > > > > > > > > > Unlike the normal pci stuff below, the legacy files here go boom > > > > > > because they're set up much earlier in the boot sequence. This only > > > > > > affects HAVE_PCI_LEGACY architectures, which aren't that many. So what > > > > > > should we do here now: > > > > > > - drop the devmem revoke for these > > > > > > - rework the init sequence somehow to set up these files a lot later > > > > > > - redo the sysfs patch so that it doesn't take an address_space > > > > > > pointer, but instead a callback to get at that (since at open time > > > > > > everything is set up). Imo rather ugly > > > > > > - ditch this part of the series (since there's not really any takers > > > > > > for the latter parts it might just not make sense to push for this) > > > > > > - something else? > > > > > > > > > > > > Bjorn, Greg, thoughts? > > > > > > > > > > What sysfs patch are you referring to here? > > > > > > > > Currently in linux-next: > > > > > > > > commit 74b30195395c406c787280a77ae55aed82dbbfc7 (HEAD -> > > > > topic/iomem-mmap-vs-gup, drm/topic/iomem-mmap-vs-gup) > > > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Date: Fri Nov 27 17:41:25 2020 +0100 > > > > > > > > sysfs: Support zapping of binary attr mmaps > > > > > > > > Or the patch right before this one in this submission here: > > > > > > > > https://lore.kernel.org/dri-devel/20201127164131.2244124-12-daniel.vetter@ffwll.ch/ > > > > > > Ah. Hm, a callback in the sysfs file logic seems really hairy, so I > > > would prefer that not happen. If no one really needs this stuff, why > > > not just drop it like you mention? > > > > Well it is needed, but just on architectures I don't care about much. > > Most relevant is perhaps powerpc (that's where Stephen hit the issue). > > I do wonder whether we could move the legacy pci files setup to where > > the modern stuff is set up from pci_create_resource_files() or maybe > > pci_create_sysfs_dev_files() even for HAVE_PCI_LEGACY. I think that > > might work, but since it's legacy flow on some funny architectures > > (alpha, itanium, that kind of stuff) I have no idea what kind of > > monsters I'm going to anger :-) > > Back from a week of vacation, I looked at this again and I think > shouldn't be hard to fix this with the sam trick > pci_create_sysfs_dev_files() uses: As long as sysfs_initialized isn't > set we skip, and then later on when the vfs is up&running we can > initialize everything. > > To be able to apply the same thing to pci_create_legacy_files() I > think all I need is to iterate overa all struct pci_bus in > pci_sysfs_init() and we're good. Unfortunately I didn't find any > for_each_pci_bus(), so how do I do that? pci_find_next_bus() seems to be the answer I want. I'll see whether that works and then send out new patches. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-04 10:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20201127164131.2244124-1-daniel.vetter@ffwll.ch> 2020-11-27 16:41 ` [PATCH v7 08/17] PCI: Obey iomem restrictions for procfs mmap Daniel Vetter 2020-11-27 16:41 ` [PATCH v7 11/17] sysfs: Support zapping of binary attr mmaps Daniel Vetter 2020-11-27 16:41 ` [PATCH v7 12/17] PCI: Revoke mappings like devmem Daniel Vetter 2021-01-19 8:17 ` Daniel Vetter 2021-01-19 14:32 ` Greg Kroah-Hartman 2021-01-19 14:34 ` Daniel Vetter 2021-01-19 15:20 ` Greg Kroah-Hartman 2021-01-19 16:03 ` Daniel Vetter 2021-02-03 16:14 ` Daniel Vetter 2021-02-04 10:23 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).