[PATCH 1/2] base: memory: fix soft/hard_offline_page permissions

[PATCH 1/2] base: memory: fix soft/hard_offline_page permissions

[Felipe Balbi]

those two sysfs files don't have a 'show' method,
so they shouldn't have a read permission. Thanks
to Greg Kroah-Hartman for actually looking into
the source code and figuring out we had a real bug
with these two files.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/base/memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 987604d..83d0b17 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -494,8 +494,8 @@ store_hard_offline_page(struct device *dev,
 	return ret ? ret : count;
 }
 
-static DEVICE_ATTR(soft_offline_page, 0644, NULL, store_soft_offline_page);
-static DEVICE_ATTR(hard_offline_page, 0644, NULL, store_hard_offline_page);
+static DEVICE_ATTR(soft_offline_page, S_IWUSR, NULL, store_soft_offline_page);
+static DEVICE_ATTR(hard_offline_page, S_IWUSR, NULL, store_hard_offline_page);
 
 static __init int memory_fail_init(void)
 {
-- 
1.8.1.rc1.5.g7e0651a

[BONUS PATCH 2/2] base: core: WARN() about bogus permissions on device attributes

[Felipe Balbi]

Whenever a struct device_attribute is registered
with mismatched permissions - read permission without
a show routine or write permission without store
routine - we will issue a big warning so we catch
those early enough.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---

This is completely untested. I have only compile tested
to make sure I'm not breaking anything.

Greg, do you think this would be nice to have ? I could
fire up kvm tomorrow and run on a few of my OMAP-based
boards to make sure it works as expected.

 drivers/base/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index a235085..14e6a92 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -563,8 +563,15 @@ int device_create_file(struct device *dev,
 		       const struct device_attribute *attr)
 {
 	int error = 0;
-	if (dev)
+
+	if (dev) {
+		WARN(((attr->attr.mode & S_IWUGO) && !attr->store),
+				"Write permission without 'store'\n");
+		WARN(((attr->attr.mode & S_IRUGO) && !attr->show),
+				"Read permission without 'show'\n");
 		error = sysfs_create_file(&dev->kobj, &attr->attr);
+	}
+
 	return error;
 }
 
-- 
1.8.1.rc1.5.g7e0651a

Re: [BONUS PATCH 2/2] base: core: WARN() about bogus permissions on device attributes

[Greg KH]

On Mon, Feb 18, 2013 at 09:09:04PM +0200, Felipe Balbi wrote:
> Whenever a struct device_attribute is registered
> with mismatched permissions - read permission without
> a show routine or write permission without store
> routine - we will issue a big warning so we catch
> those early enough.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> This is completely untested. I have only compile tested
> to make sure I'm not breaking anything.
> 
> Greg, do you think this would be nice to have ? I could
> fire up kvm tomorrow and run on a few of my OMAP-based
> boards to make sure it works as expected.

Looks good, but you better fix up all of the big offenders that this
points out, before I apply it, otherwise we will get a ton of bug
reports :)

thanks,

greg k-h

Re: [BONUS PATCH 2/2] base: core: WARN() about bogus permissions on device attributes

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

On Mon, Feb 18, 2013 at 11:17:19AM -0800, Greg KH wrote:
> On Mon, Feb 18, 2013 at 09:09:04PM +0200, Felipe Balbi wrote:
> > Whenever a struct device_attribute is registered
> > with mismatched permissions - read permission without
> > a show routine or write permission without store
> > routine - we will issue a big warning so we catch
> > those early enough.
> > 
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > 
> > This is completely untested. I have only compile tested
> > to make sure I'm not breaking anything.
> > 
> > Greg, do you think this would be nice to have ? I could
> > fire up kvm tomorrow and run on a few of my OMAP-based
> > boards to make sure it works as expected.
> 
> Looks good, but you better fix up all of the big offenders that this
> points out, before I apply it, otherwise we will get a ton of bug
> reports :)

sure, will do that tomorrow though ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [BONUS PATCH 2/2] base: core: WARN() about bogus permissions on device attributes

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

Hi,

On Mon, Feb 18, 2013 at 09:20:10PM +0200, Felipe Balbi wrote:
> On Mon, Feb 18, 2013 at 11:17:19AM -0800, Greg KH wrote:
> > On Mon, Feb 18, 2013 at 09:09:04PM +0200, Felipe Balbi wrote:
> > > Whenever a struct device_attribute is registered
> > > with mismatched permissions - read permission without
> > > a show routine or write permission without store
> > > routine - we will issue a big warning so we catch
> > > those early enough.
> > > 
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > ---
> > > 
> > > This is completely untested. I have only compile tested
> > > to make sure I'm not breaking anything.
> > > 
> > > Greg, do you think this would be nice to have ? I could
> > > fire up kvm tomorrow and run on a few of my OMAP-based
> > > boards to make sure it works as expected.
> > 
> > Looks good, but you better fix up all of the big offenders that this
> > points out, before I apply it, otherwise we will get a ton of bug
> > reports :)
> 
> sure, will do that tomorrow though ;-)

Just tested $SUBJECT and it didn't trigger any extra warnings. Only the
one whose fix you already applied (I reverted it to make sure $SUBJECT
was working as expected).

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [BONUS PATCH 2/2] base: core: WARN() about bogus permissions on device attributes

[Greg KH]

On Tue, Feb 19, 2013 at 11:59:10AM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Feb 18, 2013 at 09:20:10PM +0200, Felipe Balbi wrote:
> > On Mon, Feb 18, 2013 at 11:17:19AM -0800, Greg KH wrote:
> > > On Mon, Feb 18, 2013 at 09:09:04PM +0200, Felipe Balbi wrote:
> > > > Whenever a struct device_attribute is registered
> > > > with mismatched permissions - read permission without
> > > > a show routine or write permission without store
> > > > routine - we will issue a big warning so we catch
> > > > those early enough.
> > > > 
> > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > ---
> > > > 
> > > > This is completely untested. I have only compile tested
> > > > to make sure I'm not breaking anything.
> > > > 
> > > > Greg, do you think this would be nice to have ? I could
> > > > fire up kvm tomorrow and run on a few of my OMAP-based
> > > > boards to make sure it works as expected.
> > > 
> > > Looks good, but you better fix up all of the big offenders that this
> > > points out, before I apply it, otherwise we will get a ton of bug
> > > reports :)
> > 
> > sure, will do that tomorrow though ;-)
> 
> Just tested $SUBJECT and it didn't trigger any extra warnings. Only the
> one whose fix you already applied (I reverted it to make sure $SUBJECT
> was working as expected).

Ok, care to resend so that I can queue it up after 3.9-rc1 is out, for
inclusion in 3.10, and we can have lots of time to see what falls out?

thanks,

greg k-h

[PATCH] base: core: WARN() about bogus permissions on device attributes

[Felipe Balbi]

Whenever a struct device_attribute is registered
with mismatched permissions - read permission without
a show routine or write permission without store
routine - we will issue a big warning so we catch
those early enough.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/base/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index a235085..14e6a92 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -563,8 +563,15 @@ int device_create_file(struct device *dev,
 		       const struct device_attribute *attr)
 {
 	int error = 0;
-	if (dev)
+
+	if (dev) {
+		WARN(((attr->attr.mode & S_IWUGO) && !attr->store),
+				"Write permission without 'store'\n");
+		WARN(((attr->attr.mode & S_IRUGO) && !attr->show),
+				"Read permission without 'show'\n");
 		error = sysfs_create_file(&dev->kobj, &attr->attr);
+	}
+
 	return error;
 }
 
-- 
1.8.1.rc1.5.g7e0651a