Re: [rfc] /dev/devstat permissions patch

2014-03-20 Thread Maksim Yevmenkin 
On Thu, Mar 20, 2014 at 7:05 AM, John Baldwin  wrote:
> On Tuesday, March 18, 2014 3:29:32 pm Maksim Yevmenkin wrote:
>> hello,
>>
>> would anyone object to the following patch?
>
> I think this is fine.
>
> While you are at it, can you test this patch to remove D_NEEDGIANT?
>
> Index: subr_devstat.c
> ===
> --- subr_devstat.c  (revision 263302)
> +++ subr_devstat.c  (working copy)
> @@ -460,7 +460,6 @@ static d_mmap_t devstat_mmap;
>
>  static struct cdevsw devstat_cdevsw = {
> .d_version =D_VERSION,
> -   .d_flags =  D_NEEDGIANT,
> .d_mmap =   devstat_mmap,
> .d_name =   "devstat",
>  };
> @@ -482,13 +481,16 @@ devstat_mmap(struct cdev *dev, vm_ooffset_t offset
>
> if (nprot != VM_PROT_READ)
> return (-1);
> +   mtx_lock(&devstat_mutex);
> TAILQ_FOREACH(spp, &pagelist, list) {
> if (offset == 0) {
> *paddr = vtophys(spp->stat);
> +   mtx_unlock(&devstat_mutex);
> return (0);
> }
> offset -= PAGE_SIZE;
> }
> +   mtx_unlock(&devstat_mutex);
> return (-1);
>  }

seems to work fine for me. so, i guess, i will commit combined patch, then

==

Index: subr_devstat.c
===
--- subr_devstat.c (revision 3427)
+++ subr_devstat.c (working copy)
@@ -462,7 +462,6 @@

 static struct cdevsw devstat_cdevsw = {
  .d_version = D_VERSION,
- .d_flags = D_NEEDGIANT,
  .d_mmap = devstat_mmap,
  .d_name = "devstat",
 };
@@ -484,13 +483,16 @@

  if (nprot != VM_PROT_READ)
  return (-1);
+ mtx_lock(&devstat_mutex);
  TAILQ_FOREACH(spp, &pagelist, list) {
  if (offset == 0) {
  *paddr = vtophys(spp->stat);
+ mtx_unlock(&devstat_mutex);
  return (0);
  }
  offset -= PAGE_SIZE;
  }
+ mtx_unlock(&devstat_mutex);
  return (-1);
 }

@@ -505,7 +507,7 @@
  mtx_assert(&devstat_mutex, MA_NOTOWNED);
  if (!once) {
  make_dev_credf(MAKEDEV_ETERNAL | MAKEDEV_CHECKNAME,
-&devstat_cdevsw, 0, NULL, UID_ROOT, GID_WHEEL, 0400,
+&devstat_cdevsw, 0, NULL, UID_ROOT, GID_WHEEL, 0444,
 DEVSTAT_DEVICE_NAME);
  once = 1;
  }

==


thanks
max
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: [rfc] /dev/devstat permissions patch

2014-03-20 Thread John Baldwin 
On Tuesday, March 18, 2014 3:29:32 pm Maksim Yevmenkin wrote:
> hello,
> 
> would anyone object to the following patch?

I think this is fine.

While you are at it, can you test this patch to remove D_NEEDGIANT?

Index: subr_devstat.c
===
--- subr_devstat.c  (revision 263302)
+++ subr_devstat.c  (working copy)
@@ -460,7 +460,6 @@ static d_mmap_t devstat_mmap;
 
 static struct cdevsw devstat_cdevsw = {
.d_version =D_VERSION,
-   .d_flags =  D_NEEDGIANT,
.d_mmap =   devstat_mmap,
.d_name =   "devstat",
 };
@@ -482,13 +481,16 @@ devstat_mmap(struct cdev *dev, vm_ooffset_t offset
 
if (nprot != VM_PROT_READ)
return (-1);
+   mtx_lock(&devstat_mutex);
TAILQ_FOREACH(spp, &pagelist, list) {
if (offset == 0) {
*paddr = vtophys(spp->stat);
+   mtx_unlock(&devstat_mutex);
return (0);
}
offset -= PAGE_SIZE;
}
+   mtx_unlock(&devstat_mutex);
return (-1);
 }
 

-- 
John Baldwin
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: [rfc] /dev/devstat permissions patch

2014-03-18 Thread Xin Li 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Hi,

Adding phk@ to cc since the 400 is from his changeset (r112001).

On 03/18/14 12:29, Maksim Yevmenkin wrote:
> hello,
> 
> would anyone object to the following patch?
> 
> ==
> 
> Index: subr_devstat.c 
> ===
>
> 
- --- subr_devstat.c (revision 263311)
> +++ subr_devstat.c (working copy) @@ -503,7 +503,7 @@ 
> mtx_assert(&devstat_mutex, MA_NOTOWNED); if (!once) { 
> make_dev_credf(MAKEDEV_ETERNAL | MAKEDEV_CHECKNAME, -
> &devstat_cdevsw, 0, NULL, UID_ROOT, GID_WHEEL, 0400, +
> &devstat_cdevsw, 0, NULL, UID_ROOT, GID_WHEEL, 0444, 
> DEVSTAT_DEVICE_NAME); once = 1; }
> 
> ==
> 
> i'm not sure why /dev/devstat has such restrictive permissions.
> can someone please explain the reason for it? having gstat(8)
> require super-user privilege seems like an overkill me. iostat(8)
> and systat(1) do not require super-user privileges to work.
> 
> and, yes, i know i can override permissions with /etc/devfs.conf,
> just curious what are we protecting from in /dev/devstat

I have similar change locally (except it's GID_OPERATOR and 0440) and
I think your proposed change would be a sensible default.

Cheers,
- -- 
Xin LI https://www.delphij.net/
FreeBSD - The Power to Serve!   Live free or die
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (FreeBSD)

iQIcBAEBCgAGBQJTKMofAAoJEJW2GBstM+nspPYQAKt8UiAqDBQwe0KTeH+ykpis
4EG4+oM43Ze8WCc3DgsbB+Dnq4en63z3SXyK7b78ZDN9xVzSmR4Cb6N0W63cuACI
4pE2Wl72P7v6eVgOrrgMJoRjI7BwX0nlOXKCvmwkHznbZSmpjgYTzx9ADYl1T4pP
SKtvgOtCyFXdpGP2adE8kJMRcFvBpbs61Y4hiSLwKE1lGywgLwYWfwkZMWFxGaNW
SU3H7qew5SRoFSF7ZhurhKENwyNR1EHEHXW+Se77TcTUzBIGCQop+78Od+Pxwi/v
KJYFKHS+Z72BRVbpaxowxQGRNSPzqC4dB2nMhrcQaOU8gXRret9OXCfBc7Fmrv31
ot0ewo3GapmNh/9ypMuYNQ+3XsjEmx96ckSeS0oX6lKLR2qIu8+JIMd9Oq0ogNHk
tMdjrX0dkpwedN9UiakbQq8Ws7u/XRfkQEUD8nsDu5gK+f3KlRldboA+GFAjYgX6
F+E4JHfRGWCFYQuzcl48Nkzg4Glw/r8HCHHE+cGqwXXPIGfjtwSIyGGZzw0Nb2Nr
jYfs4aYuGCwFmwUO/hVn47Wbbzmpr7rVbf7EW3PXwZuxPKTVxrEUpYklvCUmkDMi
jYEwQMcIfV7pI+nD1M9bocOk3TQ4nYWqlts2E6J+/qEC/ayXpo4kk/93swimj7wP
p6xDXw3sAX6Xaj0bZqcB
=ktrj
-END PGP SIGNATURE-
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"