Patchwork SYSFS "errors"

login
register
mail settings
Submitter Mauro Carvalho Chehab
Date Feb. 19, 2013, 12:16 p.m.
Message ID <20130219091610.2b746a30@redhat.com>
Download mbox | patch
Permalink /patch/221689/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Mauro Carvalho Chehab - Feb. 19, 2013, 12:16 p.m.
Em Tue, 19 Feb 2013 12:43:46 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> On Tue, Feb 19, 2013 at 08:11:49AM -0300, Mauro Carvalho Chehab wrote:
> > > > I remember I saw some discussions about it in the past at bluesmoke ML,
> > > > saying that -ENODEV is the expected behavior when this is not supported.
> > > > 
> > > > Changing from -ENODEV to "N/A" will break anything that would be relying
> > > > on the previous behavior. So, I think that such change will for sure break
> > > > userspace.
> > > > 
> > > > If we're willing to change it, not creating the "sdram_scrub_rate" sysfs 
> > > > node is less likely to affect userspace.
> 
> This will break scripts which assume this file's presence implicitly.

If are there any script like that, then yes.

> [ … ]
> 
> > @@ -1017,6 +1010,14 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
> >  		return err;
> >  	}
> >  
> > +	if (mci->set_sdram_scrub_rate && mci->get_sdram_scrub_rate) {
> 
> This will break cpc925_edac.c because it defines a
> ->get_sdram_scrub_rate but not a ->set_sdram_scrub_rate.

True. Patch below fixes it.

> I think a maybe better fix would be to figure out the sysfs file
> permissions based on the presence of the two functions and *then* add
> the attribute.
> 
> This way, the only visible change to userspace is the corrected sysfs
> file permissions.

I'm not sure if is there a way to pass fs permissions to something similar
to device_create_file().

On both cases, an error will happen at open:
	- if file doesn't exist (this approach), it will return -ENOENT;
	- if file is opened with wrong permissions, open will return -EPERM.

However, if the file is not created, readdir() won't show the file.
So, if userspace first seeks for the sdram_scrub_rate, it won't fail.
That makes the logic below a little safer, IMO.


[PATCH EDAC] edac: only create sdram_scrub_rate where supported

Currently, sdram_scrub_rate sysfs node is created even if the device
doesn't support get/set the scub rate. Change the logic to only
create this device node when the operation is supported.

If only read or only write is supported, it will keep returning -ENODEV
just like before.

Reported-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov - Feb. 19, 2013, 12:35 p.m.
On Tue, Feb 19, 2013 at 09:16:10AM -0300, Mauro Carvalho Chehab wrote:
> I'm not sure if is there a way to pass fs permissions to something similar
> to device_create_file().

struct device_attribute.attr.mode? I.e., second arg.

> On both cases, an error will happen at open:
> 	- if file doesn't exist (this approach), it will return -ENOENT;
> 	- if file is opened with wrong permissions, open will return -EPERM.
> 
> However, if the file is not created, readdir() won't show the file.

Right, and in that case userspace which *assumes* it is always created -
like it is now - will fail when accessing it.

If simply you adjust the attributes accordingly but *always* create the
file and it has the correct permissions, everyone is happy. Right?
Mauro Carvalho Chehab - Feb. 19, 2013, 12:46 p.m.
Em Tue, 19 Feb 2013 13:35:02 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> On Tue, Feb 19, 2013 at 09:16:10AM -0300, Mauro Carvalho Chehab wrote:
> > I'm not sure if is there a way to pass fs permissions to something similar
> > to device_create_file().
> 
> struct device_attribute.attr.mode? I.e., second arg.

Ah, now I see what you're meaning. That would require to dynamically
create a per-mci DEVICE_ATTR().

> > On both cases, an error will happen at open:
> > 	- if file doesn't exist (this approach), it will return -ENOENT;
> > 	- if file is opened with wrong permissions, open will return -EPERM.
> > 
> > However, if the file is not created, readdir() won't show the file.
> 
> Right, and in that case userspace which *assumes* it is always created -
> like it is now - will fail when accessing it.
> 
> If simply you adjust the attributes accordingly but *always* create the
> file and it has the correct permissions, everyone is happy. Right?

No, on both cases, open() will return an error (-ENOENT against -EPERM). 

If userspace doesn't check if open() failed, I can't see why
changing the open return error code would help.
Borislav Petkov - Feb. 19, 2013, 1:06 p.m.
On Tue, Feb 19, 2013 at 09:46:40AM -0300, Mauro Carvalho Chehab wrote:
> Ah, now I see what you're meaning. That would require to dynamically
> create a per-mci DEVICE_ATTR().

Dude, look at the code, we do that already. And you're using it with
dev_attr_sdram_scrub_rate.

Simply change the permissions of the attribute before calling
device_create_file.

pseudo:

	umode_t mode = 0;

	if (mci->set...)
		mode |= S_IWUSR;

	if (mci->get...)
		mode |= S_IRUGO;


	dev_attr_sdram_scrub_rate.attr.mode = mode;

	device_create_file(&mci->dev, &dev_attr_sdram_scrub_rate);

> No, on both cases, open() will return an error (-ENOENT against -EPERM).

What if it is a shell script doing:

cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate

or similar?

Simply fixing the permissions fixes *all* possible cases.
Felipe Balbi - Feb. 19, 2013, 1:15 p.m.
On Tue, Feb 19, 2013 at 02:06:26PM +0100, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 09:46:40AM -0300, Mauro Carvalho Chehab wrote:
> > Ah, now I see what you're meaning. That would require to dynamically
> > create a per-mci DEVICE_ATTR().
> 
> Dude, look at the code, we do that already. And you're using it with
> dev_attr_sdram_scrub_rate.
> 
> Simply change the permissions of the attribute before calling
> device_create_file.
> 
> pseudo:
> 
> 	umode_t mode = 0;
> 
> 	if (mci->set...)
> 		mode |= S_IWUSR;
> 
> 	if (mci->get...)
> 		mode |= S_IRUGO;
> 
> 
> 	dev_attr_sdram_scrub_rate.attr.mode = mode;
> 
> 	device_create_file(&mci->dev, &dev_attr_sdram_scrub_rate);
> 
> > No, on both cases, open() will return an error (-ENOENT against -EPERM).
> 
> What if it is a shell script doing:
> 
> cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate

what's the problem with that ?

$ cat /path/to/file/that/doesnt/exist.txt
cat: /path/to/file/that/doesnt/exist.txt: No such file or directory

Didn't see any gates to hell opening here...
Borislav Petkov - Feb. 19, 2013, 1:28 p.m.
On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> what's the problem with that ?

Not a problem - simply annoying.

$ ./test.sh
cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
Setting scrubrate to:

I'm sure someone would ask themselves why all of a sudden the file is
gone.

> $ cat /path/to/file/that/doesnt/exist.txt
> cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> 
> Didn't see any gates to hell opening here...

And I don't see why we have to debate such a trivial thing when we can
fix it *properly* without *absolutely* *not* upsetting userspace.
Felipe Balbi - Feb. 19, 2013, 1:38 p.m.
Hi,

On Tue, Feb 19, 2013 at 02:28:54PM +0100, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> > what's the problem with that ?
> 
> Not a problem - simply annoying.
> 
> $ ./test.sh
> cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> Setting scrubrate to:
> 
> I'm sure someone would ask themselves why all of a sudden the file is
> gone.
> 
> > $ cat /path/to/file/that/doesnt/exist.txt
> > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > 
> > Didn't see any gates to hell opening here...
> 
> And I don't see why we have to debate such a trivial thing when we can
> fix it *properly* without *absolutely* *not* upsetting userspace.

because changing the permission will cause the same issue:

$ cat temporary.txt 
cat: temporary.txt: Permission denied
$ ls -l temporary.txt 
---------- 1 balbi balbi 0 Feb 19 15:37 temporary.txt
$ cat temporary.txt 
cat: temporary.txt: Permission denied
$ cat non-existent.txt
cat: non-existent.txt: No such file or directory

just a different error code is returned...
Mauro Carvalho Chehab - Feb. 19, 2013, 1:42 p.m.
Em Tue, 19 Feb 2013 14:06:26 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> > No, on both cases, open() will return an error (-ENOENT against -EPERM).
> 
> What if it is a shell script doing:
> 
> cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> 
> or similar?

Well, cat will return "1" if an error is found, no matter what error happened. 

With an existing file (-ENOSYS):

	$ cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate; echo $?

	cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate: No such device
	1

When the file doesn't exist (-ENOENT):
	$ cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate_not_exist; echo $?
	cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate_not_exist: No such file or directory
	1

When permission doesn't match (-EPERM):
	$ cat /sys/devices/system/cpu/probe; echo $?
	cat: /sys/devices/system/cpu/probe: Permission denied
	1

When everything is ok, it will return 0
	$ cat /sys/devices/system/edac/mc/mc0/ce_count; echo $? >/dev/null
	0

A script ready to handle -ENOSYS would be doing, instead:

	RATE=$(cat /sys/devices/system/edac/mc/mc0/ce_count 2>/dev/null)
	if [ "$?" == "0" ]; then echo "Scrub rate: $RATE"; fi

So, a bash script won't notice any difference.

The only difference will be noticed if the script is written on some other 
language and the script is dumb enough to assume success if the errno is
different than -ENOSYS.
Mauro Carvalho Chehab - Feb. 19, 2013, 1:50 p.m.
Em Tue, 19 Feb 2013 15:38:09 +0200
Felipe Balbi <balbi@ti.com> escreveu:

> Hi,
> 
> On Tue, Feb 19, 2013 at 02:28:54PM +0100, Borislav Petkov wrote:
> > On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> > > what's the problem with that ?
> > 
> > Not a problem - simply annoying.
> > 
> > $ ./test.sh
> > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > Setting scrubrate to:
> > 
> > I'm sure someone would ask themselves why all of a sudden the file is
> > gone.
> > 
> > > $ cat /path/to/file/that/doesnt/exist.txt
> > > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > > 
> > > Didn't see any gates to hell opening here...
> > 
> > And I don't see why we have to debate such a trivial thing when we can
> > fix it *properly* without *absolutely* *not* upsetting userspace.
> 
> because changing the permission will cause the same issue:

It is actually worse, as if someone is using a C code to open the device
with
	fp = open ("/sys/devices/system/edac/mc/mc0/sdram_scrub_rate", O_RDWR);

It will now start to fail if the device doesn't have both permissions.

Regards,
Mauro
Borislav Petkov - Feb. 19, 2013, 1:55 p.m.
On Tue, Feb 19, 2013 at 10:50:48AM -0300, Mauro Carvalho Chehab wrote:
> It is actually worse, as if someone is using a C code to open the device
> with
> 	fp = open ("/sys/devices/system/edac/mc/mc0/sdram_scrub_rate", O_RDWR);
> 
> It will now start to fail if the device doesn't have both permissions.

That's fine - you're supposed to handle open()'s retval properly anyway. See my
other mail.

Patch

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 9c58da6..29b66a2 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -7,7 +7,7 @@ 
  *
  * Written Doug Thompson <norsk5@xmission.com> www.softwarebitmaker.com
  *
- * (c) 2012 - Mauro Carvalho Chehab <mchehab@redhat.com>
+ * (c) 2012-2013 - Mauro Carvalho Chehab <mchehab@redhat.com>
  *	The entire API were re-written, and ported to use struct device
  *
  */
@@ -882,7 +882,6 @@  static struct attribute *mci_attrs[] = {
 	&dev_attr_ce_noinfo_count.attr,
 	&dev_attr_ue_count.attr,
 	&dev_attr_ce_count.attr,
-	&dev_attr_sdram_scrub_rate.attr,
 	&dev_attr_max_location.attr,
 	NULL
 };
@@ -1017,6 +1016,14 @@  int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
 		return err;
 	}
 
+	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
+		err = device_create_file(&mci->dev,
+					 &dev_attr_sdram_scrub_rate);
+		if (err) {
+			edac_dbg(1, "failure: create sdram_scrub_rate\n");
+			goto fail2;
+		}
+	}
 	/*
 	 * Create the dimm/rank devices
 	 */
@@ -1061,6 +1068,7 @@  fail:
 			continue;
 		device_unregister(&dimm->dev);
 	}
+fail2:
 	device_unregister(&mci->dev);
 	bus_unregister(&mci->bus);
 	kfree(mci->bus.name);