Now a cgroup subsystem can set default file mode of its control files,
so here is a patchset to correct file mode of each subsystem's files.
Should be applied after:
cgroup-allow-subsys-to-set-default-mode-of-its-own-file.patch
[PATCH 1/4] cgroup debug: show correct file mode
[PATCH 2/4] cpuacct: show correct file mode
[PATCH 3/4] devcgroup: show correct file mode
[PATCH 4/4] cpuset: show correct file mode
All control files in debug subsystem are read-only.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/cgroup_debug.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/kernel/cgroup_debug.c b/kernel/cgroup_debug.c
index daca620..2ba54c6 100644
--- a/kernel/cgroup_debug.c
+++ b/kernel/cgroup_debug.c
@@ -71,25 +71,30 @@ static struct cftype files[] = {
{
.name = "cgroup_refcount",
.read_u64 = cgroup_refcount_read,
+ .mode = 0444,
},
{
.name = "taskcount",
.read_u64 = taskcount_read,
+ .mode = 0444,
},
{
.name = "current_css_set",
.read_u64 = current_css_set_read,
+ .mode = 0444,
},
{
.name = "current_css_set_refcount",
.read_u64 = current_css_set_refcount_read,
+ .mode = 0444,
},
{
.name = "releasable",
.read_u64 = releasable_read,
+ .mode = 0444,
},
};
-- 1.5.4.rc3
cpuacct.usage_percpu is read-only.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/sched.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/kernel/sched.c b/kernel/sched.c
index e66f009..638aa4d 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -9734,6 +9734,7 @@ static struct cftype files[] = {
{
.name = "usage_percpu",
.read_seq_string = cpuacct_percpu_seq_read,
+ .mode = 0444,
},
};
-- 1.5.4.rc3
devices.allow and devices.deny are write-only, and devices.list is read-only.
Signed-off-by: Li Zefan <[email protected]>
---
security/device_cgroup.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 3aacd0f..b13fbb8 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -439,16 +439,19 @@ static struct cftype dev_cgroup_files[] = {
.name = "allow",
.write_string = devcgroup_access_write,
.private = DEVCG_ALLOW,
+ .mode = 0200,
},
{
.name = "deny",
.write_string = devcgroup_access_write,
.private = DEVCG_DENY,
+ .mode = 0200,
},
{
.name = "list",
.read_seq_string = devcgroup_seq_read,
.private = DEVCG_LIST,
+ .mode = 0444,
},
};
-- 1.5.4.rc3
cpuset.memory_pressure is read-only.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/cpuset.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index a46d693..1ed507c 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1671,6 +1671,7 @@ static struct cftype files[] = {
.read_u64 = cpuset_read_u64,
.write_u64 = cpuset_write_u64,
.private = FILE_MEMORY_MIGRATE,
+ .mode = 0444,
},
{
-- 1.5.4.rc3
Quoting Li Zefan ([email protected]):
> devices.allow and devices.deny are write-only, and devices.list is read-only.
>
> Signed-off-by: Li Zefan <[email protected]>
Yup that should be intuitive for people.
Acked-by: Serge Hallyn <[email protected]>
thanks,
-serge
> ---
> security/device_cgroup.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index 3aacd0f..b13fbb8 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -439,16 +439,19 @@ static struct cftype dev_cgroup_files[] = {
> .name = "allow",
> .write_string = devcgroup_access_write,
> .private = DEVCG_ALLOW,
> + .mode = 0200,
> },
> {
> .name = "deny",
> .write_string = devcgroup_access_write,
> .private = DEVCG_DENY,
> + .mode = 0200,
> },
> {
> .name = "list",
> .read_seq_string = devcgroup_seq_read,
> .private = DEVCG_LIST,
> + .mode = 0444,
> },
> };
>
> -- 1.5.4.rc3
On Sun, Mar 1, 2009 at 6:13 PM, Li Zefan <[email protected]> wrote:
> Now a cgroup subsystem can set default file mode of its control files,
> so here is a patchset to correct file mode of each subsystem's files.
I really think that we should be defaulting this based on whether the
control file has read or write handlers.
Sure, there are special cases like "tasks" that we'd need to set a
manual value for, but most of these patches would be unnecessary.
Paul
>
> Should be applied after:
> ? ? ? ?cgroup-allow-subsys-to-set-default-mode-of-its-own-file.patch
>
> [PATCH 1/4] cgroup debug: show correct file mode
> [PATCH 2/4] cpuacct: show correct file mode
> [PATCH 3/4] devcgroup: show correct file mode
> [PATCH 4/4] cpuset: show correct file mode
>
>
On Mon, 2 Mar 2009 10:19:15 -0800
Paul Menage <[email protected]> wrote:
> On Sun, Mar 1, 2009 at 6:13 PM, Li Zefan <[email protected]> wrote:
> > Now a cgroup subsystem can set default file mode of its control files,
> > so here is a patchset to correct file mode of each subsystem's files.
>
> I really think that we should be defaulting this based on whether the
> control file has read or write handlers.
>
> Sure, there are special cases like "tasks" that we'd need to set a
> manual value for, but most of these patches would be unnecessary.
>
like this ?
int mode;
if (cft->mode)
mode = cft->mode;
else if (cft->write_xxx || .....)
mode = 0644;
else
mode = 0444;
Thanks,
-Kame
On Mon, Mar 2, 2009 at 4:09 PM, KAMEZAWA Hiroyuki
<[email protected]> wrote:
>
> int mode;
> if (cft->mode)
> mode = cft->mode;
> else if (cft->write_xxx || .....)
> mode = 0644;
> else
> mode = 0444;
Almost:
int mode;
if (cft->mode) {
mode = cft->mode;
} else {
if (cft->write_xxx || ....)
mode |= 0600;
if (cft->read_xxx || ...)
mode |= 0444;
}
Paul
On Mon, 2 Mar 2009 16:15:51 -0800
Paul Menage <[email protected]> wrote:
> On Mon, Mar 2, 2009 at 4:09 PM, KAMEZAWA Hiroyuki
> <[email protected]> wrote:
> >
> > int mode;
> > if (cft->mode)
> > mode = cft->mode;
> > else if (cft->write_xxx || .....)
> > mode = 0644;
> > else
> > mode = 0444;
>
> Almost:
>
> int mode;
> if (cft->mode) {
> mode = cft->mode;
> } else {
> if (cft->write_xxx || ....)
> mode |= 0600;
> if (cft->read_xxx || ...)
> mode |= 0444;
> }
>
Oh, I see. But int mode=0, at first ;)
I have no objections but please forgive subsys to set mode=0644 explicitly.
Thanks,
-Kame
On Mon, Mar 2, 2009 at 4:20 PM, KAMEZAWA Hiroyuki
<[email protected]> wrote:
>>
>> int mode;
>> if (cft->mode) {
>> mode = cft->mode;
>> } else {
>> if (cft->write_xxx || ....)
>> mode |= 0600;
>> if (cft->read_xxx || ...)
>> mode |= 0444;
>> }
>>
> Oh, I see. But int mode=0, at first ;)
Oops, yes.
> I have no objections but please forgive subsys to set mode=0644 explicitly.
Do you mean "allow" rather than "forgive"? if so, then yes, a
subsystem should be able to override this to whatever it wants.
Paul
KAMEZAWA Hiroyuki wrote:
> On Mon, 2 Mar 2009 16:15:51 -0800
> Paul Menage <[email protected]> wrote:
>
>> On Mon, Mar 2, 2009 at 4:09 PM, KAMEZAWA Hiroyuki
>> <[email protected]> wrote:
>>> int mode;
>>> if (cft->mode)
>>> mode = cft->mode;
>>> else if (cft->write_xxx || .....)
>>> mode = 0644;
>>> else
>>> mode = 0444;
>> Almost:
>>
>> int mode;
>> if (cft->mode) {
>> mode = cft->mode;
>> } else {
>> if (cft->write_xxx || ....)
>> mode |= 0600;
0200 ?
>> if (cft->read_xxx || ...)
>> mode |= 0444;
>> }
>>
> Oh, I see. But int mode=0, at first ;)
> I have no objections but please forgive subsys to set mode=0644 explicitly.
>
> Thanks,
> -Kame
>
> _______________________________________________
> Containers mailing list
> [email protected]
> https://lists.linux-foundation.org/mailman/listinfo/containers
On Mon, Mar 2, 2009 at 4:23 PM, Mike Waychison <[email protected]> wrote:
>>> if (cft->write_xxx || ....)
>>> mode |= 0600;
>
> 0200 ?
Oops (again). Yes, that's what I meant. Coding in email considered harmful :-)
Paul
Paul Menage wrote:
> On Sun, Mar 1, 2009 at 6:13 PM, Li Zefan <[email protected]> wrote:
>> Now a cgroup subsystem can set default file mode of its control files,
>> so here is a patchset to correct file mode of each subsystem's files.
>
> I really think that we should be defaulting this based on whether the
> control file has read or write handlers.
>
> Sure, there are special cases like "tasks" that we'd need to set a
> manual value for, but most of these patches would be unnecessary.
>
Those patches are small and trivial, but it's ok for me to do this
automatically. How about below patch.
Note cpuset.memory_pressure is read-only though it has read handler.
Since if the read handler is removed, it'll return EINVAL instead of
the current EACCESS, I think it's better to leave as it is.
=====================
cgroups: show correct file mode
Signed-off-by: Li Zefan <[email protected]>
---
include/linux/cgroup.h | 5 +++++
kernel/cgroup.c | 30 ++++++++++++++++++++++++++++++
kernel/cpuset.c | 1 +
3 files changed, 36 insertions(+)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 6ad1989..af3c10f 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -258,6 +258,11 @@ struct cftype {
*/
char name[MAX_CFTYPE_NAME];
int private;
+ /*
+ * If not 0, file mode is set to this value, otherwise it will
+ * be figured out automatically
+ */
+ int mode;
/*
* If non-zero, defines the maximum length of string that can
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 379baa3..0b19204 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1750,6 +1750,33 @@ static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry,
return error;
}
+/**
+ * cgroup_file_mode - deduce file mode of a control file
+ * @cft: the control file in question
+ *
+ * returns cftype->mode if ->mode is not 0
+ * returns 0644 if it has both a read and a write handler
+ * returns 0444 if it has only a read handler
+ * returns 0200 if it has only a write hander
+ */
+static int cgroup_file_mode(const struct cftype *cft)
+{
+ int mode = 0;
+
+ if (cft->mode)
+ return cft->mode;
+
+ if (cft->read || cft->read_u64 || cft->read_s64 ||
+ cft->read_map || cft->read_seq_string)
+ mode += 0444;
+
+ if (cft->write || cft->write_u64 || cft->write_s64 ||
+ cft->write_string || cft->trigger)
+ mode += 0200;
+
+ return mode;
+}
+
int cgroup_add_file(struct cgroup *cgrp,
struct cgroup_subsys *subsys,
const struct cftype *cft)
@@ -1757,6 +1784,7 @@ int cgroup_add_file(struct cgroup *cgrp,
struct dentry *dir = cgrp->dentry;
struct dentry *dentry;
int error;
+ int mode;
char name[MAX_CGROUP_TYPE_NAMELEN + MAX_CFTYPE_NAME + 2] = { 0 };
if (subsys && !test_bit(ROOT_NOPREFIX, &cgrp->root->flags)) {
@@ -1767,6 +1795,7 @@ int cgroup_add_file(struct cgroup *cgrp,
BUG_ON(!mutex_is_locked(&dir->d_inode->i_mutex));
dentry = lookup_one_len(name, dir, strlen(name));
if (!IS_ERR(dentry)) {
+ mode = cgroup_file_mode(cft);
error = cgroup_create_file(dentry, 0644 | S_IFREG,
cgrp->root->sb);
if (!error)
@@ -2349,6 +2378,7 @@ static struct cftype files[] = {
.write_u64 = cgroup_tasks_write,
.release = cgroup_tasks_release,
.private = FILE_TASKLIST,
+ .mode = 0644,
},
{
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index a46d693..31e28b3 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1678,6 +1678,7 @@ static struct cftype files[] = {
.read_u64 = cpuset_read_u64,
.write_u64 = cpuset_write_u64,
.private = FILE_MEMORY_PRESSURE,
+ .mode = 0444,
},
{
On Tue, 03 Mar 2009 09:08:23 +0800
Li Zefan <[email protected]> wrote:
> Paul Menage wrote:
> > On Sun, Mar 1, 2009 at 6:13 PM, Li Zefan <[email protected]> wrote:
> >> Now a cgroup subsystem can set default file mode of its control files,
> >> so here is a patchset to correct file mode of each subsystem's files.
> >
> > I really think that we should be defaulting this based on whether the
> > control file has read or write handlers.
> >
> > Sure, there are special cases like "tasks" that we'd need to set a
> > manual value for, but most of these patches would be unnecessary.
> >
>
> Those patches are small and trivial, but it's ok for me to do this
> automatically. How about below patch.
>
> Note cpuset.memory_pressure is read-only though it has read handler.
> Since if the read handler is removed, it'll return EINVAL instead of
> the current EACCESS, I think it's better to leave as it is.
>
> =====================
>
> cgroups: show correct file mode
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> include/linux/cgroup.h | 5 +++++
> kernel/cgroup.c | 30 ++++++++++++++++++++++++++++++
> kernel/cpuset.c | 1 +
> 3 files changed, 36 insertions(+)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 6ad1989..af3c10f 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -258,6 +258,11 @@ struct cftype {
> */
> char name[MAX_CFTYPE_NAME];
> int private;
> + /*
> + * If not 0, file mode is set to this value, otherwise it will
> + * be figured out automatically
> + */
> + int mode;
>
> /*
> * If non-zero, defines the maximum length of string that can
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 379baa3..0b19204 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1750,6 +1750,33 @@ static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry,
> return error;
> }
>
> +/**
> + * cgroup_file_mode - deduce file mode of a control file
> + * @cft: the control file in question
> + *
> + * returns cftype->mode if ->mode is not 0
> + * returns 0644 if it has both a read and a write handler
> + * returns 0444 if it has only a read handler
> + * returns 0200 if it has only a write hander
> + */
> +static int cgroup_file_mode(const struct cftype *cft)
> +{
> + int mode = 0;
> +
> + if (cft->mode)
> + return cft->mode;
> +
> + if (cft->read || cft->read_u64 || cft->read_s64 ||
> + cft->read_map || cft->read_seq_string)
> + mode += 0444;
> +
> + if (cft->write || cft->write_u64 || cft->write_s64 ||
> + cft->write_string || cft->trigger)
> + mode += 0200;
> +
+= is not |=...
Thanks,
-Kame
>> +/**
>> + * cgroup_file_mode - deduce file mode of a control file
>> + * @cft: the control file in question
>> + *
>> + * returns cftype->mode if ->mode is not 0
>> + * returns 0644 if it has both a read and a write handler
>> + * returns 0444 if it has only a read handler
>> + * returns 0200 if it has only a write hander
>> + */
>> +static int cgroup_file_mode(const struct cftype *cft)
>> +{
>> + int mode = 0;
>> +
>> + if (cft->mode)
>> + return cft->mode;
>> +
>> + if (cft->read || cft->read_u64 || cft->read_s64 ||
>> + cft->read_map || cft->read_seq_string)
>> + mode += 0444;
>> +
>> + if (cft->write || cft->write_u64 || cft->write_s64 ||
>> + cft->write_string || cft->trigger)
>> + mode += 0200;
>> +
>
> += is not |=...
>
Ah, yes, though both happen to result in 0644.
On Mon, Mar 2, 2009 at 5:08 PM, Li Zefan <[email protected]> wrote:
>
> Those patches are small and trivial, but it's ok for me to do this
> automatically. How about below patch.
>
> Note cpuset.memory_pressure is read-only though it has read handler.
> Since if the read handler is removed, it'll return EINVAL instead of
> the current EACCESS, I think it's better to leave as it is.
Hmm. I find it hard to believe that anyone would have behaviour that
depends on cpuset.memory_pressure returning EACCES rather than EINVAL,
but I guess it's not a big deal.
On the += or |= issue, I think |= is slightly more familiar to most
readers given that you're doing bit options, although the end result
is the same in this case.
Acked-by: Paul Menage <[email protected]>
>
> =====================
>
> cgroups: show correct file mode
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> include/linux/cgroup.h | 5 +++++
> kernel/cgroup.c | 30 ++++++++++++++++++++++++++++++
> kernel/cpuset.c | 1 +
> 3 files changed, 36 insertions(+)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 6ad1989..af3c10f 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -258,6 +258,11 @@ struct cftype {
> */
> char name[MAX_CFTYPE_NAME];
> int private;
> + /*
> + * If not 0, file mode is set to this value, otherwise it will
> + * be figured out automatically
> + */
> + int mode;
>
> /*
> * If non-zero, defines the maximum length of string that can
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 379baa3..0b19204 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1750,6 +1750,33 @@ static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry,
> return error;
> }
>
> +/**
> + * cgroup_file_mode - deduce file mode of a control file
> + * @cft: the control file in question
> + *
> + * returns cftype->mode if ->mode is not 0
> + * returns 0644 if it has both a read and a write handler
> + * returns 0444 if it has only a read handler
> + * returns 0200 if it has only a write hander
> + */
> +static int cgroup_file_mode(const struct cftype *cft)
> +{
> + int mode = 0;
> +
> + if (cft->mode)
> + return cft->mode;
> +
> + if (cft->read || cft->read_u64 || cft->read_s64 ||
> + cft->read_map || cft->read_seq_string)
> + mode += 0444;
> +
> + if (cft->write || cft->write_u64 || cft->write_s64 ||
> + cft->write_string || cft->trigger)
> + mode += 0200;
> +
> + return mode;
> +}
> +
> int cgroup_add_file(struct cgroup *cgrp,
> struct cgroup_subsys *subsys,
> const struct cftype *cft)
> @@ -1757,6 +1784,7 @@ int cgroup_add_file(struct cgroup *cgrp,
> struct dentry *dir = cgrp->dentry;
> struct dentry *dentry;
> int error;
> + int mode;
>
> char name[MAX_CGROUP_TYPE_NAMELEN + MAX_CFTYPE_NAME + 2] = { 0 };
> if (subsys && !test_bit(ROOT_NOPREFIX, &cgrp->root->flags)) {
> @@ -1767,6 +1795,7 @@ int cgroup_add_file(struct cgroup *cgrp,
> BUG_ON(!mutex_is_locked(&dir->d_inode->i_mutex));
> dentry = lookup_one_len(name, dir, strlen(name));
> if (!IS_ERR(dentry)) {
> + mode = cgroup_file_mode(cft);
> error = cgroup_create_file(dentry, 0644 | S_IFREG,
> cgrp->root->sb);
> if (!error)
> @@ -2349,6 +2378,7 @@ static struct cftype files[] = {
> .write_u64 = cgroup_tasks_write,
> .release = cgroup_tasks_release,
> .private = FILE_TASKLIST,
> + .mode = 0644,
> },
>
> {
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index a46d693..31e28b3 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1678,6 +1678,7 @@ static struct cftype files[] = {
> .read_u64 = cpuset_read_u64,
> .write_u64 = cpuset_write_u64,
> .private = FILE_MEMORY_PRESSURE,
> + .mode = 0444,
> },
>
> {
>
>
On Tue, 03 Mar 2009 09:15:42 +0800
Li Zefan <[email protected]> wrote:
> >> +/**
> >> + * cgroup_file_mode - deduce file mode of a control file
> >> + * @cft: the control file in question
> >> + *
> >> + * returns cftype->mode if ->mode is not 0
> >> + * returns 0644 if it has both a read and a write handler
> >> + * returns 0444 if it has only a read handler
> >> + * returns 0200 if it has only a write hander
> >> + */
> >> +static int cgroup_file_mode(const struct cftype *cft)
> >> +{
> >> + int mode = 0;
> >> +
> >> + if (cft->mode)
> >> + return cft->mode;
> >> +
> >> + if (cft->read || cft->read_u64 || cft->read_s64 ||
> >> + cft->read_map || cft->read_seq_string)
> >> + mode += 0444;
> >> +
> >> + if (cft->write || cft->write_u64 || cft->write_s64 ||
> >> + cft->write_string || cft->trigger)
> >> + mode += 0200;
> >> +
> >
> > += is not |=...
> >
>
> Ah, yes, though both happen to result in 0644.
>
yes but I like |= rather than += in bit ops.
Reviewed-by; KAMEZAWA Hiroyuki <[email protected]>