Re: [PATCH 0/1] nvme: Export CSTS register via sysfs

[Keith Busch <kbusch@kernel.org>]

On Thu, Mar 18, 2021 at 06:39:37PM +0000, Alan Adamson wrote:
> 
> 
> > On Mar 18, 2021, at 9:52 AM, Keith Busch <kbusch@kernel.org> wrote:
> > 
> > On Thu, Mar 18, 2021 at 04:28:24PM +0000, Alan Adamson wrote:
> >> 
> >> 
> >>> On Mar 17, 2021, at 9:38 PM, Christoph Hellwig <hch@lst.de> wrote:
> >>> 
> >>> On Wed, Mar 17, 2021 at 04:46:14PM -0400, Alan Adamson wrote:
> >>>> This patch exports the NVMe Controller CSTS register via sysfs.  This
> >>>> feature can be used by userland executables that accessed CSTS and
> >>>> possibly other registers by mapping them into user space.  Since this ability
> >>>> may not always available, exporting certain registers via sysfs provides
> >>>> a safe/read-only way to access registers from outside the kernel.
> >>> 
> >>> So what is this application doings with it?  Should we just have a
> >>> ready attribute instead of exporting a raw register encoding?
> >> 
> >> 
> >> Was using nvme-cli show-regs to manage nvme devices. This is no longer an option.
> > 
> > Most distributions ship with kernel CONFIG_IO_STRICT_DEVMEM these days,
> > so that user command will very rarely work on PCIe targets anymore.
> > 
> > Perhaps we could introduce a driver option allowing read-only mmap on
> > this memory?  While you're currently asking for just one register
> > attribute, it would be nice if we can make all future requests available
> > without piling on more sysfs properties.
> 
> This could be a good solution (along with a nvme-cli change).  I’ll code this up.

I don't think it should require user space modifications. Just mmap the
current resource as-is but let the driver allow read-only in the
precense of CONFIG_IO_STRICT_DEVMEM. The below is a rough idea of what I
had in mind (compile tested only, and a proper solution should restrict
access to a specific address range)

---
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d47bb18b976a..c70965135276 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2362,6 +2362,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 		return result;
 
 	pci_set_master(pdev);
+	pdev->allow_ro_mmap = true;
 
 	if (dev->ctrl.quirks & NVME_QUIRK_DMA_ADDRESS_BITS_48)
 		dma_address_bits = 48;
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index f8afd54ca3e1..b92f2172497c 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1028,7 +1028,10 @@ static int pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
 	if (ret)
 		return ret;
 
-	if (res->flags & IORESOURCE_MEM && iomem_is_exclusive(res->start))
+	if (res->flags & IORESOURCE_MEM && iomem_is_exclusive(res->start) &&
+	    !(pdev->allow_ro_mmap &&
+			!((vma->vm_flags & VM_SHARED) ||
+			  (vma->vm_flags & VM_MAYWRITE))))
 		return -EINVAL;
 
 	if (!pci_mmap_fits(pdev, bar, vma, PCI_MMAP_SYSFS))
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..ca1598fa46d5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -379,6 +379,7 @@ struct pci_dev {
 						      user sysfs */
 	unsigned int	clear_retrain_link:1;	/* Need to clear Retrain Link
 						   bit manually */
+	unsigned int	allow_ro_mmap;	/* allow read only even if CONFIG_IO_STRICT_DEVMEM */
 	unsigned int	d3hot_delay;	/* D3hot->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
 
--

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme