[01/02] input: serio: use DEVICE_ATTR_RO()

Commit Message

Greg Kroah-Hartman Oct. 8, 2013, 1:08 a.m. UTC

From: Greg Kroah-Hartman <[e-mail]>

Convert the serio sysfs fiels to use the DEVICE_ATTR_RO() macros to make
it easier to audit the correct sysfs file permission usage.

Cc: Dmitry Torokhov <[e-mail]>
Cc: <[e-mail]>
Signed-off-by: Greg Kroah-Hartman <[e-mail]>
---
 drivers/input/serio/serio.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Dmitry Torokhov Oct. 21, 2013, 4:25 p.m. UTC | #1

Hi Greg,

On Mon, Oct 07, 2013 at 06:08:23PM -0700, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <[e-mail]>
> 
> Convert the serio sysfs fiels to use the DEVICE_ATTR_RO() macros to make
> it easier to audit the correct sysfs file permission usage.

Sorry for the delay. Frankly I am not a fan of DEVICE_ATTR macros as it
forces particular naming of the attribute structures and methods and
forces us to lose the module prefix in names. I prefer all functions and
file-scope variables have module prefix because in the absence of
language-supported namespaces this is the one thing that insulates
drivers from changes in shared headers.

Thanks.

Greg Kroah-Hartman Oct. 22, 2013, 6:12 a.m. UTC | #2

On Mon, Oct 21, 2013 at 09:25:32AM -0700, Dmitry Torokhov wrote:
> Hi Greg,
> 
> On Mon, Oct 07, 2013 at 06:08:23PM -0700, Greg Kroah-Hartman wrote:
> > From: Greg Kroah-Hartman <[e-mail]>
> > 
> > Convert the serio sysfs fiels to use the DEVICE_ATTR_RO() macros to make
> > it easier to audit the correct sysfs file permission usage.
> 
> Sorry for the delay. Frankly I am not a fan of DEVICE_ATTR macros as it
> forces particular naming of the attribute structures and methods and
> forces us to lose the module prefix in names. I prefer all functions and
> file-scope variables have module prefix because in the absence of
> language-supported namespaces this is the one thing that insulates
> drivers from changes in shared headers.

No sysfs read/write functions should ever be exported out of the local
file "scope", so why is this an issue?  I have run into it being a
problem for some files, where people have device/bus/class sysfs files
all in the same .c file, but that usage is very limited.

I'm trying to eventually convert the whole kernel to use the
DEVICE_ATTR_??() macros to make it so that we can audit the sysfs file
permissions easier, by making them impossible to get wrong.  We have had
problems in the past where device files had incorrect permissions and
unprivilidged users could do things they shouldn't have.  Using these
macros _should_ prevent almost all of that from even being possible.

Now odds are, I'll never be able to remove DEVICE_ATTR() from the tree,
as there are some strange files that use crazy permissions and we have
to live with that, but for all of the "simple" files, like are done
here, I'd really like to use them, especially as it provides a
kernel-wide uniformity to the naming of attribute function calls, which
is a huge benifit as well.

Note, I did apply this patch to my tree already, and it's queued up for
3.13-rc1.  If you really object to it, I'll revert it, and fix up the
02/02 patch in this series to not need it, but I'd really like you to
reconsider your objection.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov Oct. 22, 2013, 3:51 p.m. UTC | #3

On Tue, Oct 22, 2013 at 07:12:19AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Oct 21, 2013 at 09:25:32AM -0700, Dmitry Torokhov wrote:
> > Hi Greg,
> > 
> > On Mon, Oct 07, 2013 at 06:08:23PM -0700, Greg Kroah-Hartman wrote:
> > > From: Greg Kroah-Hartman <[e-mail]>
> > > 
> > > Convert the serio sysfs fiels to use the DEVICE_ATTR_RO() macros to make
> > > it easier to audit the correct sysfs file permission usage.
> > 
> > Sorry for the delay. Frankly I am not a fan of DEVICE_ATTR macros as it
> > forces particular naming of the attribute structures and methods and
> > forces us to lose the module prefix in names. I prefer all functions and
> > file-scope variables have module prefix because in the absence of
> > language-supported namespaces this is the one thing that insulates
> > drivers from changes in shared headers.
> 
> No sysfs read/write functions should ever be exported out of the local
> file "scope", so why is this an issue?  I have run into it being a
> problem for some files, where people have device/bus/class sysfs files
> all in the same .c file, but that usage is very limited.

It is not about exporting read/write functions, it is about newly
defined data from common headers stomping on the local module
definitions.

> 
> I'm trying to eventually convert the whole kernel to use the
> DEVICE_ATTR_??() macros to make it so that we can audit the sysfs file
> permissions easier, by making them impossible to get wrong.  We have had
> problems in the past where device files had incorrect permissions and
> unprivilidged users could do things they shouldn't have.  Using these
> macros _should_ prevent almost all of that from even being possible.

Sounds more like task for sparse to ensure we have sane permission bits.

> 
> Now odds are, I'll never be able to remove DEVICE_ATTR() from the tree,
> as there are some strange files that use crazy permissions and we have
> to live with that, but for all of the "simple" files, like are done
> here, I'd really like to use them, especially as it provides a
> kernel-wide uniformity to the naming of attribute function calls, which
> is a huge benifit as well.
> 
> Note, I did apply this patch to my tree already, and it's queued up for
> 3.13-rc1.  If you really object to it, I'll revert it, and fix up the
> 02/02 patch in this series to not need it, but I'd really like you to
> reconsider your objection.

Nah, that is fine.

Thanks.

Patch

--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -373,34 +373,34 @@  static ssize_t serio_show_modalias(struc
 			serio->id.type, serio->id.proto, serio->id.id, serio->id.extra);
 }
 
-static ssize_t serio_show_id_type(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t type_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct serio *serio = to_serio_port(dev);
 	return sprintf(buf, "%02x\n", serio->id.type);
 }
 
-static ssize_t serio_show_id_proto(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t proto_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct serio *serio = to_serio_port(dev);
 	return sprintf(buf, "%02x\n", serio->id.proto);
 }
 
-static ssize_t serio_show_id_id(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct serio *serio = to_serio_port(dev);
 	return sprintf(buf, "%02x\n", serio->id.id);
 }
 
-static ssize_t serio_show_id_extra(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t extra_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct serio *serio = to_serio_port(dev);
 	return sprintf(buf, "%02x\n", serio->id.extra);
 }
 
-static DEVICE_ATTR(type, S_IRUGO, serio_show_id_type, NULL);
-static DEVICE_ATTR(proto, S_IRUGO, serio_show_id_proto, NULL);
-static DEVICE_ATTR(id, S_IRUGO, serio_show_id_id, NULL);
-static DEVICE_ATTR(extra, S_IRUGO, serio_show_id_extra, NULL);
+static DEVICE_ATTR_RO(type);
+static DEVICE_ATTR_RO(proto);
+static DEVICE_ATTR_RO(id);
+static DEVICE_ATTR_RO(extra);
 
 static struct attribute *serio_device_id_attrs[] = {
 	&dev_attr_type.attr,