| Message ID | 20130219091610.2b746a30@redhat.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
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.
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.
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...
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.
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...
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.
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
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.
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);