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

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

[Greg Kroah-Hartman]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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 <dmitry.torokhov@gmail.com>
Cc: <linux-input@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/serio/serio.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

--- 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,

[PATCH 02/02] input: serio: remove bus usage of dev_attrs

[Greg Kroah-Hartman]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

The dev_attrs field of struct bus_type is going away soon, so move the
remaining sysfs files that are being described with this field to use
dev_groups instead.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: <linux-input@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/serio/serio.c |   62 +++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -365,7 +365,7 @@ static ssize_t serio_show_description(st
 	return sprintf(buf, "%s\n", serio->name);
 }
 
-static ssize_t serio_show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct serio *serio = to_serio_port(dev);
 
@@ -397,30 +397,7 @@ static ssize_t extra_show(struct device
 	return sprintf(buf, "%02x\n", serio->id.extra);
 }
 
-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,
-	&dev_attr_proto.attr,
-	&dev_attr_id.attr,
-	&dev_attr_extra.attr,
-	NULL
-};
-
-static struct attribute_group serio_id_attr_group = {
-	.name	= "id",
-	.attrs	= serio_device_id_attrs,
-};
-
-static const struct attribute_group *serio_device_attr_groups[] = {
-	&serio_id_attr_group,
-	NULL
-};
-
-static ssize_t serio_rebind_driver(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
+static ssize_t drvctl_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct serio *serio = to_serio_port(dev);
 	struct device_driver *drv;
@@ -474,14 +451,36 @@ static ssize_t serio_set_bind_mode(struc
 	return retval;
 }
 
-static struct device_attribute serio_device_attrs[] = {
-	__ATTR(description, S_IRUGO, serio_show_description, NULL),
-	__ATTR(modalias, S_IRUGO, serio_show_modalias, NULL),
-	__ATTR(drvctl, S_IWUSR, NULL, serio_rebind_driver),
-	__ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode),
-	__ATTR_NULL
+static DEVICE_ATTR_RO(type);
+static DEVICE_ATTR_RO(proto);
+static DEVICE_ATTR_RO(id);
+static DEVICE_ATTR_RO(extra);
+static DEVICE_ATTR_RO(modalias);
+static DEVICE_ATTR_WO(drvctl);
+static DEVICE_ATTR(description, S_IRUGO, serio_show_description, NULL);
+static DEVICE_ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode);
+
+static struct attribute *serio_device_id_attrs[] = {
+	&dev_attr_type.attr,
+	&dev_attr_proto.attr,
+	&dev_attr_id.attr,
+	&dev_attr_extra.attr,
+	&dev_attr_modalias.attr,
+	&dev_attr_description.attr,
+	&dev_attr_drvctl.attr,
+	&dev_attr_bind_mode.attr,
+	NULL
+};
+
+static struct attribute_group serio_id_attr_group = {
+	.name	= "id",
+	.attrs	= serio_device_id_attrs,
 };
 
+static const struct attribute_group *serio_device_attr_groups[] = {
+	&serio_id_attr_group,
+	NULL
+};
 
 static void serio_release_port(struct device *dev)
 {
@@ -996,7 +995,6 @@ EXPORT_SYMBOL(serio_interrupt);
 
 static struct bus_type serio_bus = {
 	.name		= "serio",
-	.dev_attrs	= serio_device_attrs,
 	.drv_groups	= serio_driver_groups,
 	.match		= serio_bus_match,
 	.uevent		= serio_uevent,

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

[Dmitry Torokhov]

Hi Greg,

On Mon, Oct 07, 2013 at 06:08:23PM -0700, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 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.

-- 
Dmitry

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

[Greg Kroah-Hartman]

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 <gregkh@linuxfoundation.org>
> > 
> > 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

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

[Dmitry Torokhov]

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 <gregkh@linuxfoundation.org>
> > > 
> > > 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.

-- 
Dmitry