driver-staging: vsoc.c: Add sysfs support for examining the permissions of regions.

PATCH In review Jerry Lin 3 participants 8 messages Nov 7, 2018 – Nov 20, 2018 1 message not in the archive Staging

lore

Share & tools

lore

Jerry Lin

author

2018-11-07 02:30 UTC #0 lore raw

PATCH v3 1/1 driver-staging: vsoc.c: Add sysfs support for examining the permissions of regions.

Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.

This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x

Signed-off-by Jerry Lin <[email protected]>

---
 drivers/staging/android/vsoc.c | 48 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

drivers/staging/android/vsoc.c +45 -3

index 22571ab..8ce3604 100644
@@ -128,9 +128,10 @@ struct vsoc_device {
 
 static struct vsoc_device vsoc_dev;
 
-/*
- * TODO(ghartman): Add a /sys filesystem entry that summarizes the permissions.
- */
+static ssize_t permissions_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf);
+static DEVICE_ATTR_RO(permissions);
 
 struct fd_scoped_permission_node {
 	struct fd_scoped_permission permission;

@@ -718,6 +719,37 @@ static ssize_t vsoc_write(struct file *filp, const char __user *buffer,
 	return len;
 }
 
+static ssize_t permissions_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buffer)
+{
+	struct fd_scoped_permission_node *node;
+	char *row;
+	int ret;
+	ssize_t written = 0;
+
+	row = kzalloc(sizeof(char) * 128, GFP_KERNEL);
+	if (!row)
+		return 0;
+	mutex_lock(&vsoc_dev.mtx);
+	list_for_each_entry(node, &vsoc_dev.permissions, list) {
+		ret = snprintf(row, 128, "%x\t%x\t%x\t%x\n",
+			       node->permission.begin_offset,
+			       node->permission.end_offset,
+			       node->permission.owner_offset,
+			       node->permission.owned_value);
+		if (ret < 0)
+			goto done;
+		memcpy(buffer + written, row, ret);
+		written += ret;
+	}
+
+done:
+	mutex_unlock(&vsoc_dev.mtx);
+	kfree(row);
+	return written;
+}
+
 static irqreturn_t vsoc_interrupt(int irq, void *region_data_v)
 {
 	struct vsoc_region_data *region_data =

@@ -942,6 +974,15 @@ static int vsoc_probe_device(struct pci_dev *pdev,
 		}
 		vsoc_dev.regions_data[i].device_created = true;
 	}
+	/*
+	 * Create permission attribute on device node.
+	 */
+	result = device_create_file(&pdev->dev, &dev_attr_permissions);
+	if (result) {
+		dev_err(&vsoc_dev.dev->dev, "device_create_file failed\n");
+		vsoc_remove_device(pdev);
+		return -EFAULT;
+	}
 	return 0;
 }

@@ -967,6 +1008,7 @@ static void vsoc_remove_device(struct pci_dev *pdev)
 	if (!pdev || !vsoc_dev.dev)
 		return;
 	dev_info(&pdev->dev, "remove_device\n");
+	device_remove_file(&pdev->dev, &dev_attr_permissions);
 	if (vsoc_dev.regions_data) {
 		for (i = 0; i < vsoc_dev.layout->region_count; ++i) {
 			if (vsoc_dev.regions_data[i].device_created) {

Signature · 1 line

2.7.4

Greg Kroah-Hartman

2018-11-07 09:15 UTC #1 lore raw

Quoted 11 lines · Jerry Lin

Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.
This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x
Signed-off-by Jerry Lin <[email protected]>
---
 drivers/staging/android/vsoc.c | 48 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

What changed from v2? And where was v2? What about v1?

You need a change log here of what you did different from the previous patches.

And why ignore my response saying that this type of sysfs file is not ok at all?

thanks,

greg k-h

wahahab

author

2018-11-08 00:49 UTC #2 lore raw

Quoted 16 lines · Greg Kroah-Hartman

Quoted 11 lines · Jerry Lin
Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.
This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x
Signed-off-by Jerry Lin <[email protected]>
---
drivers/staging/android/vsoc.c | 48 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 45 insertions(+), 3 deletions(-)

Quoted 4 lines

What changed from v2? And where was v2? What about v1?
You need a change log here of what you did different from the previous patches.

Sorry for the mistakes I made, I shall read the document about patches more carefully. Here is the change logs:

Changes in v2:
- Display permissions information in sysfs insureds of debufs.
Changes in v3:
- Remove unnecessary null terminator after snprintf.

Quoted 4 lines

And why ignore my response saying that this type of sysfs file is not ok at all?

I didn’t mean to ignore it but I haven’t receive the response you described, May you send the response to me again so I can do further revision as well as change logs and resubmit the patch again?

Thanks for your patience

Jerry

Greg Kroah-Hartman

2018-11-08 02:38 UTC #3 lore raw

Quoted 30 lines · wahahab

Quoted 16 lines · Greg Kroah-Hartman
Quoted 11 lines · Jerry Lin
Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.
This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x
Signed-off-by Jerry Lin <[email protected]>
---
drivers/staging/android/vsoc.c | 48 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 45 insertions(+), 3 deletions(-)
Quoted 4 lines
What changed from v2? And where was v2? What about v1?
You need a change log here of what you did different from the previous patches.
Sorry for the mistakes I made, I shall read the document about patches more carefully. Here is the change logs:
Changes in v2:
- Display permissions information in sysfs insureds of debufs.
Changes in v3:
- Remove unnecessary null terminator after snprintf.
Quoted 1 line

Quoted 7 lines

Quoted 3 lines
And why ignore my response saying that this type of sysfs file is not ok at all?
I didn’t mean to ignore it but I haven’t receive the response you described, May you send the response to me again so I can do further revision as well as change logs and resubmit the patch again?

You can not have multiple values in a single sysfs file. sysfs is "one value per file". This needs to be individual files, if you really need this. And never a "header" for a sysfs file, that's never something that should ever be in a sysfs file.

And finally, you need a Documentation/ABI/ update for any sysfs file changes.

thanks,

greg k-h

Dan Carpenter

2018-11-09 17:16 UTC #4 lore raw

In reply to Jerry Lin

Quoted 7 lines · Jerry Lin

Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.
This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x

(I'm not totally an expert on sysfs rules).

Sysfs are supposed to be one value per file, so instead of doing this you would create a directory with four files like vsoc_device/begin_offset vsoc_device/end_offset etc. And each would just hae a %x output.

drivers/staging/android/vsoc.c static ssize_t vsoc_write(struct file *filp, const char __user *buffer, · line 733

Quoted 12 lines

drivers/staging/android/vsoc.c +12 -0

+static ssize_t permissions_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buffer)
+{
+	struct fd_scoped_permission_node *node;
+	char *row;
+	int ret;
+	ssize_t written = 0;
+
+	row = kzalloc(sizeof(char) * 128, GFP_KERNEL);
+	if (!row)
+		return 0;

Don't allocate this, just snprintf() directly into the buffer.

regards,
dan carpenter

A message that is not in the archive (sent off-list or not yet received): [email protected]

Greg Kroah-Hartman

2018-11-12 12:40 UTC #6 lore raw

Quoted 23 lines · wahahab

Quoted 12 lines · On 10 Nov 2018, at 1:15 AM, Dan Carpenter <[email protected]…
Quoted 7 lines · Jerry Lin
Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.
This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x
Quoted 6 lines
(I'm not totally an expert on sysfs rules).
Sysfs are supposed to be one value per file, so instead of doing this you would create a directory with four files like vsoc_device/begin_offset vsoc_device/end_offset etc. And each would just hae a %x output.
Thanks for your advice. I have started with this approach.
But when I trying to create this kind of sysfs hierarchy. I encountered the difficulties of file organizing.

Quoted 7 lines

My current thought is to create a folder under the device node called permissions and user can examine permission though file path like vsoc_device/permissions/permission1/begin_offset. But there comes a problem that it seems hard to determine the correct index of permission to make folder name unique.
The solution I come up with is to use memory address of permission node to be the index of permission. So the path will be something like vsoc_device/permissions/0x4d23f/begin_offset.
Is this OK for sysfs?

Ick, that is messy. What exactly are you trying to export and why use sysfs? Is this just debugging information? Who is going to use this data?

If it is just for debugging, use debugfs, that is what it is there for. If you use sysfs, you then have to support your new api for a very long time, as you can not break userspace reliance on it.

thanks,

greg k-h

wahahab

author

2018-11-13 03:33 UTC #7 lore raw

Quoted 31 lines · Greg Kroah-Hartman

Quoted 27 lines · wahahab
Quoted 12 lines · On 10 Nov 2018, at 1:15 AM, Dan Carpenter <[email protected]…
Quoted 7 lines · Jerry Lin
Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.
This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x
Quoted 6 lines
(I'm not totally an expert on sysfs rules).
Sysfs are supposed to be one value per file, so instead of doing this you would create a directory with four files like vsoc_device/begin_offset vsoc_device/end_offset etc. And each would just hae a %x output.
Thanks for your advice. I have started with this approach.
But when I trying to create this kind of sysfs hierarchy. I encountered the difficulties of file organizing.
My current thought is to create a folder under the device node called permissions and user can examine permission though file path like vsoc_device/permissions/permission1/begin_offset. But there comes a problem that it seems hard to determine the correct index of permission to make folder name unique.

Quoted 7 lines

Quoted 3 lines
The solution I come up with is to use memory address of permission node to be the index of permission. So the path will be something like vsoc_device/permissions/0x4d23f/begin_offset.
Is this OK for sysfs?
Ick, that is messy. What exactly are you trying to export and why use sysfs? Is this just debugging information? Who is going to use this data?

I felt that exporting these information in sysfs will add lots of complexities in this driver. And I’m not sure these informations are for user space or just for debugging.

It seems there is a conflict of TODO messages between TODO file and the comment in vsoc.c.

Should I use debugfs first for this patch?

Thanks

Jerry Lin

Quoted 8 lines

If it is just for debugging, use debugfs, that is what it is there for. If you use sysfs, you then have to support your new api for a very long time, as you can not break userspace reliance on it.
thanks,
greg k-h

Greg Kroah-Hartman

2018-11-20 09:56 UTC #8 lore raw

Quoted 39 lines · wahahab

Quoted 31 lines · Greg Kroah-Hartman
Quoted 27 lines · wahahab
Quoted 12 lines · On 10 Nov 2018, at 1:15 AM, Dan Carpenter <[email protected]…
Quoted 7 lines · Jerry Lin
Add a attribute called permissions under vsoc device node for examining current granted permissions in vsoc_device.
This file will display permissions in following format:
begin_offset end_offset owner_offset owned_value
%x %x %x %x
Quoted 6 lines
(I'm not totally an expert on sysfs rules).
Sysfs are supposed to be one value per file, so instead of doing this you would create a directory with four files like vsoc_device/begin_offset vsoc_device/end_offset etc. And each would just hae a %x output.
Thanks for your advice. I have started with this approach.
But when I trying to create this kind of sysfs hierarchy. I encountered the difficulties of file organizing.
My current thought is to create a folder under the device node called permissions and user can examine permission though file path like vsoc_device/permissions/permission1/begin_offset. But there comes a problem that it seems hard to determine the correct index of permission to make folder name unique.
Quoted 7 lines
Quoted 3 lines
The solution I come up with is to use memory address of permission node to be the index of permission. So the path will be something like vsoc_device/permissions/0x4d23f/begin_offset.
Is this OK for sysfs?
Ick, that is messy. What exactly are you trying to export and why use sysfs? Is this just debugging information? Who is going to use this data?

Quoted 7 lines

I felt that exporting these information in sysfs will add lots of complexities in this driver. And I’m not sure these informations are for user space or just for debugging.
It seems there is a conflict of TODO messages between TODO file and the comment in vsoc.c.
Should I use debugfs first for this patch?

If it is for debugging, yes. As I have no idea what this code is doing, or what wants that information, it is hard to determine, sorry.

good luck!

greg k-h