Skip to contentSkip to conversation

cgroups: show correct file mode

PATCH In review Li Zefan 10 participants 30 messages Mar 2, 2009 – Mar 3, 2009 9 messages not in the archive

Subsystems (2)
Security (LSM) cgroup
lore
Share & tools
lore

Li Zefan

author
#0 lore raw

cover cgroups: show correct file mode

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

author
#1 lore raw

PATCH 1/4 cgroup debug: 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(-)
kernel/cgroup_debug.c +5 -0
index daca620..2ba54c6 100644
@@ -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

author
#2 lore raw

PATCH 2/4 cpuacct: show correct file mode

In reply to Li Zefan

cpuacct.usage_percpu is read-only.

Signed-off-by Li Zefan <[email protected]>
---
 kernel/sched.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
kernel/sched.c +1 -0
index e66f009..638aa4d 100644
@@ -9734,6 +9734,7 @@ static struct cftype files[] = {
 	{
 		.name = "usage_percpu",
 		.read_seq_string = cpuacct_percpu_seq_read,
+		.mode = 0444,
 	},
 
 };

-- 1.5.4.rc3

author
#3 lore raw

PATCH 3/4 devcgroup: show correct file mode

In reply to Li Zefan

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(-)
security/device_cgroup.c +3 -0
index 3aacd0f..b13fbb8 100644
@@ -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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Acked-by
#5 lore raw

Re: [PATCH 3/4] devcgroup: show correct file mode

Quoted 3 lines · Li Zefan

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 <serue-r/[email protected]>

thanks,
-serge

Quoted 29 lines
---
 security/device_cgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
security/device_cgroup.c +3 -1
index 3aacd0f..b13fbb8 100644
@@ -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
Acked-by on v1 3/4
#6 lore raw

Re: [PATCH 3/4] devcgroup: show correct file mode

In reply to Li Zefan
Quoted 3 lines · Li Zefan

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

Quoted 29 lines
---
 security/device_cgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
security/device_cgroup.c +3 -1
index 3aacd0f..b13fbb8 100644
@@ -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
author
#7 lore raw

PATCH 4/4 cpuset: show correct file mode

In reply to Li Zefan

cpuset.memory_pressure is read-only.

Signed-off-by Li Zefan <[email protected]>
---
 kernel/cpuset.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
kernel/cpuset.c +1 -0
index a46d693..1ed507c 100644
@@ -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

A message that is not in the archive (sent off-list or not yet received): [email protected]

#9 lore raw

PATCH 3/4 devcgroup: show correct file mode

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(-)
security/device_cgroup.c +3 -0
index 3aacd0f..b13fbb8 100644
@@ -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

#10 lore raw

PATCH 4/4 cpuset: show correct file mode

In reply to a message not in the archive

cpuset.memory_pressure is read-only.

Signed-off-by Li Zefan <[email protected]>
---
 kernel/cpuset.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
kernel/cpuset.c +1 -0
index a46d693..1ed507c 100644
@@ -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

#11 lore raw
In reply to a message not in the archive
Quoted 2 lines · Li Zefan

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

Quoted 10 lines

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

#12 lore raw
In reply to Li Zefan
Quoted 2 lines · Li Zefan

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

Quoted 10 lines

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

A message that is not in the archive (sent off-list or not yet received): 6599ad830903021019p3b29c173oc7772af6679d90e0-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#14 lore raw
Quoted 10 lines · On Mon, 2 Mar 2009 10:19:15 -0800 Paul Menage…
Quoted 2 lines · Li Zefan

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

#15 lore raw
In reply to Paul Menage
Quoted 10 lines · On Mon, 2 Mar 2009 10:19:15 -0800 Paul Menage <[email protected]> wrote:
Quoted 2 lines · Li Zefan

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

A message that is not in the archive (sent off-list or not yet received): 20090303090950.60cea53a.kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org

#17 lore raw
Quoted 8 lines · KAMEZAWA Hiroyuki
       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

#18 lore raw
In reply to KAMEZAWA Hiroyuki
Quoted 8 lines · KAMEZAWA Hiroyuki
       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

#19 lore raw
Quoted 18 lines · On Mon, 2 Mar 2009 16:15:51 -0800 Paul Menage…
Quoted 8 lines · KAMEZAWA Hiroyuki
       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 || ....)

Quoted 5 lines
    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

A message that is not in the archive (sent off-list or not yet received): 20090303092021.93e69b96.kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org

#21 lore raw
Quoted 11 lines · KAMEZAWA Hiroyuki
Quoted 11 lines

int mode;
if (cft->mode) {
mode = cft->mode;
} else {
if (cft->write_xxx || ....)

    mode |= 0600;
  if (cft->read_xxx || ...)
    mode |= 0444;
}
Quoted 1 line

Oh, I see. But int mode=0, at first ;)

Oops, yes.

Quoted 1 line

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

#22 lore raw
In reply to a message not in the archive
Quoted 12 lines · KAMEZAWA Hiroyuki
Quoted 9 lines · On Mon, 2 Mar 2009 16:15:51 -0800 Paul Menage…
Quoted 7 lines · KAMEZAWA Hiroyuki
       int mode;
       if (cft->mode)
               mode = cft->mode;
       else if (cft->write_xxx || .....)
               mode = 0644;
       else
               mode = 0444;
Quoted 8 lines
Quoted 8 lines

Almost:

int mode;
if (cft->mode) {
mode = cft->mode;
} else {
if (cft->write_xxx || ....)

    mode |= 0600;

0200 ?

Quoted 14 lines
Quoted 4 lines

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 Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

#23 lore raw
In reply to KAMEZAWA Hiroyuki
Quoted 11 lines · KAMEZAWA Hiroyuki
Quoted 11 lines

int mode;
if (cft->mode) {
mode = cft->mode;
} else {
if (cft->write_xxx || ....)

    mode |= 0600;
  if (cft->read_xxx || ...)
    mode |= 0444;
}
Quoted 1 line

Oh, I see. But int mode=0, at first ;)

Oops, yes.

Quoted 1 line

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

#24 lore raw
In reply to KAMEZAWA Hiroyuki
Quoted 12 lines · KAMEZAWA Hiroyuki
Quoted 9 lines · On Mon, 2 Mar 2009 16:15:51 -0800 Paul Menage <[email protected]> wrote:
Quoted 7 lines · KAMEZAWA Hiroyuki
       int mode;
       if (cft->mode)
               mode = cft->mode;
       else if (cft->write_xxx || .....)
               mode = 0644;
       else
               mode = 0444;
Quoted 8 lines
Quoted 8 lines

Almost:

int mode;
if (cft->mode) {
mode = cft->mode;
} else {
if (cft->write_xxx || ....)

    mode |= 0600;

0200 ?

Quoted 14 lines
Quoted 4 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

#26 lore raw
Quoted 4 lines · Mike Waychison
Quoted 2 lines
Quoted 2 lines

if (cft->write_xxx || ....)
mode |= 0600;

0200 ?

Oops (again). Yes, that's what I meant. Coding in email considered harmful :-)

Paul

#27 lore raw
In reply to Mike Waychison
Quoted 4 lines · Mike Waychison
Quoted 2 lines
Quoted 2 lines

if (cft->write_xxx || ....)
mode |= 0600;

0200 ?

Oops (again). Yes, that's what I meant. Coding in email considered harmful :-)

Paul

author
#28 lore raw
In reply to Paul Menage
Quoted 10 lines · Paul Menage
Quoted 2 lines · Li Zefan

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(+)
include/linux/cgroup.h +5 -0
index 6ad1989..af3c10f 100644
@@ -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
kernel/cgroup.c +30 -0
index 379baa3..0b19204 100644
@@ -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,
 	},
 
 	{
kernel/cpuset.c +1 -0
index a46d693..31e28b3 100644
@@ -1678,6 +1678,7 @@ static struct cftype files[] = {
 		.read_u64 = cpuset_read_u64,
 		.write_u64 = cpuset_write_u64,
 		.private = FILE_MEMORY_PRESSURE,
+		.mode = 0444,
 	},
 
 	{

A message that is not in the archive (sent off-list or not yet received): [email protected]

#30 lore raw
Quoted 68 lines · On Tue, 03 Mar 2009 09:08:23 +0800 Li Zefan…
Quoted 10 lines · Paul Menage
Quoted 2 lines · Li Zefan

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(+)
include/linux/cgroup.h +5 -0
index 6ad1989..af3c10f 100644
@@ -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
kernel/cgroup.c +14 -0
index 379baa3..0b19204 100644
@@ -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)

kernel/cgroup.c static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry, · line 1776

Quoted 10 lines
kernel/cgroup.c +10 -0
+		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

#31 lore raw
In reply to Li Zefan
Quoted 68 lines · On Tue, 03 Mar 2009 09:08:23 +0800 Li Zefan <[email protected]> wrote:
Quoted 10 lines · Paul Menage
Quoted 2 lines · Li Zefan

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(+)
include/linux/cgroup.h +5 -0
index 6ad1989..af3c10f 100644
@@ -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
kernel/cgroup.c +14 -0
index 379baa3..0b19204 100644
@@ -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)

kernel/cgroup.c static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry, · line 1776

Quoted 10 lines
kernel/cgroup.c +10 -0
+		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

author
#32 lore raw
Quoted 18 lines
Quoted 18 lines
kernel/cgroup.c +18 -0
+/**
+ * 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)
Quoted 9 lines

kernel/cgroup.c static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry, · line 1776

Quoted 6 lines
kernel/cgroup.c +6 -0
+		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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#34 lore raw
Quoted 28 lines · On Tue, 03 Mar 2009 09:15:42 +0800 Li Zefan…
Quoted 18 lines
Quoted 18 lines
kernel/cgroup.c +18 -0
+/**
+ * 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)
Quoted 9 lines

kernel/cgroup.c static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry, · line 1776

Quoted 6 lines
kernel/cgroup.c +6 -0
+		mode += 0444;
+
+	if (cft->write || cft->write_u64 || cft->write_s64 ||
+	    cft->write_string || cft->trigger)
+		mode += 0200;
+

+= is not |=...

Quoted 2 lines

Ah, yes, though both happen to result in 0644.

yes but I like |= rather than += in bit ops.

Reviewed-by; KAMEZAWA Hiroyuki <[email protected]>

#35 lore raw
In reply to Li Zefan
Quoted 28 lines · On Tue, 03 Mar 2009 09:15:42 +0800 Li Zefan <[email protected]> wrote:
Quoted 18 lines
Quoted 18 lines
kernel/cgroup.c +18 -0
+/**
+ * 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)
Quoted 9 lines

kernel/cgroup.c static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry, · line 1776

Quoted 6 lines
kernel/cgroup.c +6 -0
+		mode += 0444;
+
+	if (cft->write || cft->write_u64 || cft->write_s64 ||
+	    cft->write_string || cft->trigger)
+		mode += 0200;
+

+= is not |=...

Quoted 2 lines

Ah, yes, though both happen to result in 0644.

yes but I like |= rather than += in bit ops.

Reviewed-by; KAMEZAWA Hiroyuki <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 20090303100827.43d82c3a.kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org

#37 lore raw
Quoted 18 lines
Quoted 18 lines
kernel/cgroup.c +18 -0
+/**
+ * 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)
Quoted 9 lines

kernel/cgroup.c static int cgroup_create_dir(struct cgroup *cgrp, struct dentry *dentry, · line 1776

Quoted 6 lines
kernel/cgroup.c +6 -0
+		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.

Acked-by
#38 lore raw
In reply to Li Zefan
Quoted 7 lines · Li Zefan

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]>
Quoted 104 lines

=====================

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(+)
include/linux/cgroup.h +5 -0
index 6ad1989..af3c10f 100644
@@ -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
kernel/cgroup.c +30 -0
index 379baa3..0b19204 100644
@@ -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,
       },
 
       {
kernel/cpuset.c +1 -0
index a46d693..31e28b3 100644
@@ -1678,6 +1678,7 @@ static struct cftype files[] = {
               .read_u64 = cpuset_read_u64,
               .write_u64 = cpuset_write_u64,
               .private = FILE_MEMORY_PRESSURE,
+               .mode = 0444,
       },
 
       {