[PATCH 2/2] cgroup/memcg show correct file mode

[PATCH 2/2] cgroup/memcg show correct file mode

[KAMEZAWA Hiroyuki]

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

Set correct permission to memcg's file.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 mm/memcontrol.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: mmotm-2.6.29-Feb24/mm/memcontrol.c
===================================================================
--- mmotm-2.6.29-Feb24.orig/mm/memcontrol.c
+++ mmotm-2.6.29-Feb24/mm/memcontrol.c
@@ -2218,6 +2218,7 @@ static struct cftype mem_cgroup_files[] 
 		.name = "usage_in_bytes",
 		.private = MEMFILE_PRIVATE(_MEM, RES_USAGE),
 		.read_u64 = mem_cgroup_read,
+		.mode = 0444
 	},
 	{
 		.name = "max_usage_in_bytes",
@@ -2240,10 +2241,12 @@ static struct cftype mem_cgroup_files[] 
 	{
 		.name = "stat",
 		.read_map = mem_control_stat_show,
+		.mode = 0444
 	},
 	{
 		.name = "force_empty",
 		.trigger = mem_cgroup_force_empty_write,
+		.mode = 0200
 	},
 	{
 		.name = "use_hierarchy",
@@ -2263,6 +2266,7 @@ static struct cftype memsw_cgroup_files[
 		.name = "memsw.usage_in_bytes",
 		.private = MEMFILE_PRIVATE(_MEMSWAP, RES_USAGE),
 		.read_u64 = mem_cgroup_read,
+		.mode = 0444
 	},
 	{
 		.name = "memsw.max_usage_in_bytes",

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[Li Zefan]

KAMEZAWA Hiroyuki wrote:
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 
> cgroup's subsys has "readonly" files, but default permission is always
> rw-r--r--. This allows to create r--r--r-- file to subsys.
> (The user can do chmod on this read-only file. But this behavior is not
>  different from current pseudo file systems as /proc.)
> 
> Reason:
>  When I wrote tools for maintain cgroup, I can't find which file is
>  writable intarfece or not via cgroup file systems. (finally, I did
>  dirty approach.)
>  IMHO, showing "this file is read-only" in explicit way is useful
>  for user-land (tools). In other story, a file whose name sounds read-only
>  may have "trigger" operation and support reseting. In this case,
>  "writable" is informative.
>  

I once planed to do so. I think there's one trial issue, that some files are
read-only in top-cgroup, but read-write in sub-dirs, like cpuset.cpus and
cpuset.mems. But for those files, they must have a write function, so I think
it's still valid to mask them as read-write even they are in top-cgroup.

Unless Paul has different option towards this:

Acked-by: Li Zefan <lizf-BthXqXjhjHX4m0rU41bmIw@public.gmane.org>

> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> ---
>  include/linux/cgroup.h |    2 ++
>  kernel/cgroup.c        |    5 ++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> Index: mmotm-2.6.29-Feb24/include/linux/cgroup.h
> ===================================================================
> --- mmotm-2.6.29-Feb24.orig/include/linux/cgroup.h
> +++ mmotm-2.6.29-Feb24/include/linux/cgroup.h
> @@ -258,6 +258,8 @@ struct cftype {
>  	 */
>  	char name[MAX_CFTYPE_NAME];
>  	int private;
> +	/* if not 0, mode is set to mode, otherwise 0644 */
> +	int mode;
>  
>  	/*
>  	 * If non-zero, defines the maximum length of string that can
> Index: mmotm-2.6.29-Feb24/kernel/cgroup.c
> ===================================================================
> --- mmotm-2.6.29-Feb24.orig/kernel/cgroup.c
> +++ mmotm-2.6.29-Feb24/kernel/cgroup.c
> @@ -1767,7 +1767,10 @@ 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)) {
> -		error = cgroup_create_file(dentry, 0644 | S_IFREG,
> +		int mode = 0644;
> +		if (cft->mode)
> +			mode = cft->mode;
> +		error = cgroup_create_file(dentry, mode | S_IFREG,
>  						cgrp->root->sb);
>  		if (!error)
>  			dentry->d_fsdata = (void *)cft;
> 

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> [2009-02-25 16:35:55]:

> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 
> cgroup's subsys has "readonly" files, but default permission is always
> rw-r--r--. This allows to create r--r--r-- file to subsys.
> (The user can do chmod on this read-only file. But this behavior is not
>  different from current pseudo file systems as /proc.)
> 
> Reason:
>  When I wrote tools for maintain cgroup, I can't find which file is
>  writable intarfece or not via cgroup file systems. (finally, I did
>  dirty approach.)
>  IMHO, showing "this file is read-only" in explicit way is useful
>  for user-land (tools). In other story, a file whose name sounds read-only
>  may have "trigger" operation and support reseting. In this case,
>  "writable" is informative.

Perfect, Thanks, Looks good to me

-- 
	Balbir

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[Andrew Morton]

On Wed, 25 Feb 2009 16:35:55 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:

>  When I wrote tools for maintain cgroup, I can't find which file is
>  writable intarfece or not via cgroup file systems. (finally, I did
>  dirty approach.)
>  IMHO, showing "this file is read-only" in explicit way is useful
>  for user-land (tools). In other story, a file whose name sounds read-only
>  may have "trigger" operation and support reseting. In this case,
>  "writable" is informative.

Well, we have compatibility issues here.  If we make this change, and
people write tools which depend upon that change then those tools might
break when run upon older kernels.  Or they need back-compatibility
additions, which increases the testing burden of those tools.

One way in which we could improve this situation is to backport these
changes into earlier kernels, although I don't know which versions.

What do we think?

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[KAMEZAWA Hiroyuki]

On Thu, 26 Feb 2009 13:00:05 -0800
Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:

> On Wed, 25 Feb 2009 16:35:55 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> 
> >  When I wrote tools for maintain cgroup, I can't find which file is
> >  writable intarfece or not via cgroup file systems. (finally, I did
> >  dirty approach.)
> >  IMHO, showing "this file is read-only" in explicit way is useful
> >  for user-land (tools). In other story, a file whose name sounds read-only
> >  may have "trigger" operation and support reseting. In this case,
> >  "writable" is informative.
> 
> Well, we have compatibility issues here.  If we make this change, and
> people write tools which depend upon that change then those tools might
> break when run upon older kernels.  Or they need back-compatibility
> additions, which increases the testing burden of those tools.
> 
> One way in which we could improve this situation is to backport these
> changes into earlier kernels, although I don't know which versions.
> 
> What do we think?
> 
It sounds problem to me.

Hmm..1st commit to kernel/cgroup.c is 2007-10-19, then 2.6.24 is the oldest one.
But I think distro's tools for cgroup is not as old as...
Hmm, backport to 2.6.25 is enough ?
Balbir, how do you think ? I think you are familiar with libcgroup.

Thanks,
-Kame

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[Paul Menage]

On Thu, Feb 26, 2009 at 1:00 PM, Andrew Morton
<akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> On Wed, 25 Feb 2009 16:35:55 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
>
>>  When I wrote tools for maintain cgroup, I can't find which file is
>>  writable intarfece or not via cgroup file systems. (finally, I did
>>  dirty approach.)
>>  IMHO, showing "this file is read-only" in explicit way is useful
>>  for user-land (tools). In other story, a file whose name sounds read-only
>>  may have "trigger" operation and support reseting. In this case,
>>  "writable" is informative.
>
> Well, we have compatibility issues here.  If we make this change, and
> people write tools which depend upon that change then those tools might
> break when run upon older kernels.

I don't think that's too big a deal - a write can always fail at the
whim of a cgroups subsystem, so this would just be a hint to a tool
that it shouldn't even bother trying to write to the file. It should
be able to handle a failure.

But I don't see why we can't figure out the mode automatically based
on whether or not there's a write handler defined for the control
file.

Paul

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[KAMEZAWA Hiroyuki]

On Thu, 26 Feb 2009 16:28:11 -0800
Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

> On Thu, Feb 26, 2009 at 1:00 PM, Andrew Morton
> <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> > On Wed, 25 Feb 2009 16:35:55 +0900
> > KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> >
> >>  When I wrote tools for maintain cgroup, I can't find which file is
> >>  writable intarfece or not via cgroup file systems. (finally, I did
> >>  dirty approach.)
> >>  IMHO, showing "this file is read-only" in explicit way is useful
> >>  for user-land (tools). In other story, a file whose name sounds read-only
> >>  may have "trigger" operation and support reseting. In this case,
> >>  "writable" is informative.
> >
> > Well, we have compatibility issues here.  If we make this change, and
> > people write tools which depend upon that change then those tools might
> > break when run upon older kernels.
> 
> I don't think that's too big a deal - a write can always fail at the
> whim of a cgroups subsystem, so this would just be a hint to a tool
> that it shouldn't even bother trying to write to the file. It should
> be able to handle a failure.
> 
> But I don't see why we can't figure out the mode automatically based
> on whether or not there's a write handler defined for the control
> file.
> 
That's because I wanted to allow -w------- or -w--w---- or some

Thanks,
-Kame



> Paul
> 

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[Li Zefan]

> But I don't see why we can't figure out the mode automatically based
> on whether or not there's a write handler defined for the control
> file.
> 

This almost works.. except for 'tasks' file, which has no 'read' handler
but is readable.

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[Dhaval Giani]

On Fri, Feb 27, 2009 at 09:20:31AM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 26 Feb 2009 13:00:05 -0800
> Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> 
> > On Wed, 25 Feb 2009 16:35:55 +0900
> > KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> > 
> > >  When I wrote tools for maintain cgroup, I can't find which file is
> > >  writable intarfece or not via cgroup file systems. (finally, I did
> > >  dirty approach.)
> > >  IMHO, showing "this file is read-only" in explicit way is useful
> > >  for user-land (tools). In other story, a file whose name sounds read-only
> > >  may have "trigger" operation and support reseting. In this case,
> > >  "writable" is informative.
> > 
> > Well, we have compatibility issues here.  If we make this change, and
> > people write tools which depend upon that change then those tools might
> > break when run upon older kernels.  Or they need back-compatibility
> > additions, which increases the testing burden of those tools.
> > 
> > One way in which we could improve this situation is to backport these
> > changes into earlier kernels, although I don't know which versions.
> > 
> > What do we think?
> > 
> It sounds problem to me.
> 
> Hmm..1st commit to kernel/cgroup.c is 2007-10-19, then 2.6.24 is the oldest one.
> But I think distro's tools for cgroup is not as old as...
> Hmm, backport to 2.6.25 is enough ?
> Balbir, how do you think ? I think you are familiar with libcgroup.
> 

I would like to have this backported to at least the distro kernels.

thanks,
-- 
regards,
Dhaval

Re: [PATCH 1/2] cgroup allow subsys to set default mode of its own file

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> [2009-02-27 09:20:31]:

> On Thu, 26 Feb 2009 13:00:05 -0800
> Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> 
> > On Wed, 25 Feb 2009 16:35:55 +0900
> > KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
> > 
> > >  When I wrote tools for maintain cgroup, I can't find which file is
> > >  writable intarfece or not via cgroup file systems. (finally, I did
> > >  dirty approach.)
> > >  IMHO, showing "this file is read-only" in explicit way is useful
> > >  for user-land (tools). In other story, a file whose name sounds read-only
> > >  may have "trigger" operation and support reseting. In this case,
> > >  "writable" is informative.
> > 
> > Well, we have compatibility issues here.  If we make this change, and
> > people write tools which depend upon that change then those tools might
> > break when run upon older kernels.  Or they need back-compatibility
> > additions, which increases the testing burden of those tools.
> > 
> > One way in which we could improve this situation is to backport these
> > changes into earlier kernels, although I don't know which versions.
> > 
> > What do we think?
> > 
> It sounds problem to me.
> 
> Hmm..1st commit to kernel/cgroup.c is 2007-10-19, then 2.6.24 is the oldest one.
> But I think distro's tools for cgroup is not as old as...
> Hmm, backport to 2.6.25 is enough ?
> Balbir, how do you think ? I think you are familiar with libcgroup.
>

Actually this problem fixes an issue that we've seen in libcgroup,
where a file would have read permission (force_empty), but reading it
would fail, Porting back to 2.6.25 would be nice (since the memcg
merge) 

-- 
	Balbir

Re: [PATCH 2/2] cgroup/memcg show correct file mode

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> [2009-02-25 16:37:29]:

> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> 
> Set correct permission to memcg's file.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> ---
>  mm/memcontrol.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: mmotm-2.6.29-Feb24/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.29-Feb24.orig/mm/memcontrol.c
> +++ mmotm-2.6.29-Feb24/mm/memcontrol.c
> @@ -2218,6 +2218,7 @@ static struct cftype mem_cgroup_files[] 
>  		.name = "usage_in_bytes",
>  		.private = MEMFILE_PRIVATE(_MEM, RES_USAGE),
>  		.read_u64 = mem_cgroup_read,
> +		.mode = 0444
>  	},
>  	{
>  		.name = "max_usage_in_bytes",
> @@ -2240,10 +2241,12 @@ static struct cftype mem_cgroup_files[] 
>  	{
>  		.name = "stat",
>  		.read_map = mem_control_stat_show,
> +		.mode = 0444
>  	},
>  	{
>  		.name = "force_empty",
>  		.trigger = mem_cgroup_force_empty_write,
> +		.mode = 0200
>  	},
>  	{
>  		.name = "use_hierarchy",
> @@ -2263,6 +2266,7 @@ static struct cftype memsw_cgroup_files[
>  		.name = "memsw.usage_in_bytes",
>  		.private = MEMFILE_PRIVATE(_MEMSWAP, RES_USAGE),
>  		.read_u64 = mem_cgroup_read,
> +		.mode = 0444
>  	},
>  	{
>  		.name = "memsw.max_usage_in_bytes",
> 
>

Yes, please, very much required

Acked-by: Balbir Singh <balbir-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
 

-- 
	Balbir

Re: [PATCH 2/2] cgroup/memcg show correct file mode

[Dhaval Giani]

On Wed, Feb 25, 2009 at 01:33:29PM +0530, Balbir Singh wrote:
> * KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> [2009-02-25 16:37:29]:
> 
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> > 
> > Set correct permission to memcg's file.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> > ---
> >  mm/memcontrol.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > Index: mmotm-2.6.29-Feb24/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-2.6.29-Feb24.orig/mm/memcontrol.c
> > +++ mmotm-2.6.29-Feb24/mm/memcontrol.c
> > @@ -2218,6 +2218,7 @@ static struct cftype mem_cgroup_files[] 
> >  		.name = "usage_in_bytes",
> >  		.private = MEMFILE_PRIVATE(_MEM, RES_USAGE),
> >  		.read_u64 = mem_cgroup_read,
> > +		.mode = 0444
> >  	},
> >  	{
> >  		.name = "max_usage_in_bytes",
> > @@ -2240,10 +2241,12 @@ static struct cftype mem_cgroup_files[] 
> >  	{
> >  		.name = "stat",
> >  		.read_map = mem_control_stat_show,
> > +		.mode = 0444
> >  	},
> >  	{
> >  		.name = "force_empty",
> >  		.trigger = mem_cgroup_force_empty_write,
> > +		.mode = 0200
> >  	},
> >  	{
> >  		.name = "use_hierarchy",
> > @@ -2263,6 +2266,7 @@ static struct cftype memsw_cgroup_files[
> >  		.name = "memsw.usage_in_bytes",
> >  		.private = MEMFILE_PRIVATE(_MEMSWAP, RES_USAGE),
> >  		.read_u64 = mem_cgroup_read,
> > +		.mode = 0444
> >  	},
> >  	{
> >  		.name = "memsw.max_usage_in_bytes",
> > 
> >
> 
> Yes, please, very much required
> 
> Acked-by: Balbir Singh <balbir-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

I agree,

Acked-by: Dhaval Giani <dhaval-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
-- 
regards,
Dhaval

[PATCH 1/2] cgroup allow subsys to set default mode of its own file

[KAMEZAWA Hiroyuki]

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

cgroup's subsys has "readonly" files, but default permission is always
rw-r--r--. This allows to create r--r--r-- file to subsys.
(The user can do chmod on this read-only file. But this behavior is not
 different from current pseudo file systems as /proc.)

Reason:
 When I wrote tools for maintain cgroup, I can't find which file is
 writable intarfece or not via cgroup file systems. (finally, I did
 dirty approach.)
 IMHO, showing "this file is read-only" in explicit way is useful
 for user-land (tools). In other story, a file whose name sounds read-only
 may have "trigger" operation and support reseting. In this case,
 "writable" is informative.
 

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---
 include/linux/cgroup.h |    2 ++
 kernel/cgroup.c        |    5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

Index: mmotm-2.6.29-Feb24/include/linux/cgroup.h
===================================================================
--- mmotm-2.6.29-Feb24.orig/include/linux/cgroup.h
+++ mmotm-2.6.29-Feb24/include/linux/cgroup.h
@@ -258,6 +258,8 @@ struct cftype {
 	 */
 	char name[MAX_CFTYPE_NAME];
 	int private;
+	/* if not 0, mode is set to mode, otherwise 0644 */
+	int mode;
 
 	/*
 	 * If non-zero, defines the maximum length of string that can
Index: mmotm-2.6.29-Feb24/kernel/cgroup.c
===================================================================
--- mmotm-2.6.29-Feb24.orig/kernel/cgroup.c
+++ mmotm-2.6.29-Feb24/kernel/cgroup.c
@@ -1767,7 +1767,10 @@ 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)) {
-		error = cgroup_create_file(dentry, 0644 | S_IFREG,
+		int mode = 0644;
+		if (cft->mode)
+			mode = cft->mode;
+		error = cgroup_create_file(dentry, mode | S_IFREG,
 						cgrp->root->sb);
 		if (!error)
 			dentry->d_fsdata = (void *)cft;