[PATCH] powerpc/powernv: Add mmap to opal export sysfs nodes

[PATCH] powerpc/powernv: Add mmap to opal export sysfs nodes

[Jordan Niethe]

The sysfs nodes created under /opal/exports/ do not currently support
mmap. Skiboot trace buffers are exported here with in the series
https://patchwork.ozlabs.org/cover/1073501/. Adding mmap support makes
it possible to use the functions for reading traces in external/trace.
This improves on the current read/lseek method as it handles cases like
the buffer wrapping and overflowing.

Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: ensure only whole pages can be mapped
---
 arch/powerpc/platforms/powernv/opal.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b0eca104f86..3611b5b9c5d2 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -714,6 +714,15 @@ static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
 				       bin_attr->size);
 }
 
+static int export_attr_mmap(struct file *fp, struct kobject *kobj,
+			    struct bin_attribute *attr,
+			    struct vm_area_struct *vma)
+{
+	return remap_pfn_range(vma, vma->vm_start,
+			       __pa(attr->private) >> PAGE_SHIFT,
+			       attr->size, PAGE_READONLY);
+}
+
 /*
  * opal_export_attrs: creates a sysfs node for each property listed in
  * the device-tree under /ibm,opal/firmware/exports/
@@ -759,6 +768,9 @@ static void opal_export_attrs(void)
 		attr->attr.name = kstrdup(prop->name, GFP_KERNEL);
 		attr->attr.mode = 0400;
 		attr->read = export_attr_read;
+		/* Ensure only whole pages are mapped */
+		if (vals[0] % PAGE_SIZE == 0 && vals[1] % PAGE_SIZE == 0)
+			attr->mmap = export_attr_mmap;
 		attr->private = __va(vals[0]);
 		attr->size = vals[1];
 
-- 
2.20.1

[PATCH] powerpc/powernv: Add mmap to opal export sysfs nodes

[Jordan Niethe]

The sysfs nodes created under /opal/exports/ do not currently support
mmap. Skiboot trace buffers are not yet added to this location but
this is a suitable for them to be exported to. Adding mmap support makes
using these trace buffers more convenient. The state in the header of
the trace buffer is needed to ensure the read position has not been
overwritten. Thus the header of the buffer must be read, then the
read position itself. Using lseek/read to do this introduces a delay
that could result in incorrect reads if the read position is overwritten
after the header is read.

Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b0eca104f86..3cfc683bb060 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -714,6 +714,15 @@ static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
 				       bin_attr->size);
 }
 
+static int export_attr_mmap(struct file *fp, struct kobject *kobj,
+				struct bin_attribute *attr,
+				struct vm_area_struct *vma)
+{
+	return remap_pfn_range(vma, vma->vm_start,
+				__pa(attr->private) >> PAGE_SHIFT,
+				attr->size, PAGE_READONLY);
+}
+
 /*
  * opal_export_attrs: creates a sysfs node for each property listed in
  * the device-tree under /ibm,opal/firmware/exports/
@@ -759,6 +768,7 @@ static void opal_export_attrs(void)
 		attr->attr.name = kstrdup(prop->name, GFP_KERNEL);
 		attr->attr.mode = 0400;
 		attr->read = export_attr_read;
+		attr->mmap = export_attr_mmap;
 		attr->private = __va(vals[0]);
 		attr->size = vals[1];
 
-- 
2.20.1

Re: [PATCH] powerpc/powernv: Add mmap to opal export sysfs nodes

[Oliver]

On Fri, Mar 15, 2019 at 11:55 AM Jordan Niethe <jniethe5@gmail.com> wrote:
>
> The sysfs nodes created under /opal/exports/ do not currently support
> mmap. Skiboot trace buffers are not yet added to this location but
> this is a suitable for them to be exported to. Adding mmap support makes
> using these trace buffers more convenient. The state in the header of
> the trace buffer is needed to ensure the read position has not been
> overwritten. Thus the header of the buffer must be read, then the
> read position itself. Using lseek/read to do this introduces a delay
> that could result in incorrect reads if the read position is overwritten
> after the header is read.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
>  arch/powerpc/platforms/powernv/opal.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 2b0eca104f86..3cfc683bb060 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -714,6 +714,15 @@ static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
>                                        bin_attr->size);
>  }
>
> +static int export_attr_mmap(struct file *fp, struct kobject *kobj,
> +                               struct bin_attribute *attr,
> +                               struct vm_area_struct *vma)
> +{
> +       return remap_pfn_range(vma, vma->vm_start,
> +                               __pa(attr->private) >> PAGE_SHIFT,
> +                               attr->size, PAGE_READONLY);
> +}
> +
>  /*
>   * opal_export_attrs: creates a sysfs node for each property listed in
>   * the device-tree under /ibm,opal/firmware/exports/
> @@ -759,6 +768,7 @@ static void opal_export_attrs(void)
>                 attr->attr.name = kstrdup(prop->name, GFP_KERNEL);
>                 attr->attr.mode = 0400;
>                 attr->read = export_attr_read;
> +               attr->mmap = export_attr_mmap;

You are assuming that it's safe to allow the export to be mmap()ed
here and that's not always the case. e.g.

[16:56 oliveroh@localhost .../firmware/exports]$ lsprop
symbol_map       00000000 30125640 00000000 0003598d
hdat_map         00000000 31200000 00000000 00800000
phandle          100002e8 (268436200)
name             "exports"

The symbol_map export there does not start (or end) at a page
boundary. If you allow user programs to mmap() that export it will
make everything from 0x30120000 to 0x30160000 and the additional space
might contain internal firmware data structures that we'd rather not
allow userspace to access.You can protect against that by validating
the export starts and ends on a page boundary and only populating the
.mmap when it's safe to do so.

>                 attr->private = __va(vals[0]);
>                 attr->size = vals[1];
>
> --
> 2.20.1
>