[2.6 patch] kernel/cgroup.c: remove dead code

[2.6 patch] kernel/cgroup.c: remove dead code

[Adrian Bunk]

This patch removes dead code spotted by the Coverity checker
(look at the "(nbytes >= PATH_MAX)" check).

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---

 kernel/cgroup.c |   18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

--- linux-2.6/kernel/cgroup.c.old	2007-10-23 18:37:43.000000000 +0200
+++ linux-2.6/kernel/cgroup.c	2007-10-23 18:39:15.000000000 +0200
@@ -1320,90 +1320,88 @@ static ssize_t cgroup_common_file_write(
 
 	if (nbytes >= PATH_MAX)
 		return -E2BIG;
 
 	/* +1 for nul-terminator */
 	buffer = kmalloc(nbytes + 1, GFP_KERNEL);
 	if (buffer == NULL)
 		return -ENOMEM;
 
 	if (copy_from_user(buffer, userbuf, nbytes)) {
 		retval = -EFAULT;
 		goto out1;
 	}
 	buffer[nbytes] = 0;	/* nul-terminate */
 
 	mutex_lock(&cgroup_mutex);
 
 	if (cgroup_is_removed(cgrp)) {
 		retval = -ENODEV;
 		goto out2;
 	}
 
 	switch (type) {
 	case FILE_TASKLIST:
 		retval = attach_task_by_pid(cgrp, buffer);
 		break;
 	case FILE_NOTIFY_ON_RELEASE:
 		clear_bit(CGRP_RELEASABLE, &cgrp->flags);
 		if (simple_strtoul(buffer, NULL, 10) != 0)
 			set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
 		else
 			clear_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
 		break;
 	case FILE_RELEASE_AGENT:
 	{
 		struct cgroupfs_root *root = cgrp->root;
 		/* Strip trailing newline */
 		if (nbytes && (buffer[nbytes-1] == '\n')) {
 			buffer[nbytes-1] = 0;
 		}
-		if (nbytes < sizeof(root->release_agent_path)) {
-			/* We never write anything other than '\0'
-			 * into the last char of release_agent_path,
-			 * so it always remains a NUL-terminated
-			 * string */
-			strncpy(root->release_agent_path, buffer, nbytes);
-			root->release_agent_path[nbytes] = 0;
-		} else {
-			retval = -ENOSPC;
-		}
+
+		/* We never write anything other than '\0'
+		 * into the last char of release_agent_path,
+		 * so it always remains a NUL-terminated
+		 * string */
+		strncpy(root->release_agent_path, buffer, nbytes);
+		root->release_agent_path[nbytes] = 0;
+
 		break;
 	}
 	default:
 		retval = -EINVAL;
 		goto out2;
 	}
 
 	if (retval == 0)
 		retval = nbytes;
 out2:
 	mutex_unlock(&cgroup_mutex);
 out1:
 	kfree(buffer);
 	return retval;
 }
 
 static ssize_t cgroup_file_write(struct file *file, const char __user *buf,
 						size_t nbytes, loff_t *ppos)
 {
 	struct cftype *cft = __d_cft(file->f_dentry);
 	struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent);
 
 	if (!cft)
 		return -ENODEV;
 	if (cft->write)
 		return cft->write(cgrp, cft, file, buf, nbytes, ppos);
 	if (cft->write_uint)
 		return cgroup_write_uint(cgrp, cft, file, buf, nbytes, ppos);
 	return -EINVAL;
 }
 
 static ssize_t cgroup_read_uint(struct cgroup *cgrp, struct cftype *cft,
 				   struct file *file,
 				   char __user *buf, size_t nbytes,
 				   loff_t *ppos)
 {
 	char tmp[64];
 	u64 val = cft->read_uint(cgrp, cft);
 	int len = sprintf(tmp, "%llu\n", (unsigned long long) val);
 

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Paul Menage]

I think I'd rather not make this change - if we later changed the size
of release_agent_path[] this could silently fail. Can we get around
the coverity checker somehow?

Paul

On 10/24/07, Adrian Bunk <bunk@kernel.org> wrote:
> This patch removes dead code spotted by the Coverity checker
> (look at the "(nbytes >= PATH_MAX)" check).
>
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
>
> ---
>
>  kernel/cgroup.c |   18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> --- linux-2.6/kernel/cgroup.c.old       2007-10-23 18:37:43.000000000 +0200
> +++ linux-2.6/kernel/cgroup.c   2007-10-23 18:39:15.000000000 +0200
> @@ -1320,90 +1320,88 @@ static ssize_t cgroup_common_file_write(
>
>         if (nbytes >= PATH_MAX)
>                 return -E2BIG;
>
>         /* +1 for nul-terminator */
>         buffer = kmalloc(nbytes + 1, GFP_KERNEL);
>         if (buffer == NULL)
>                 return -ENOMEM;
>
>         if (copy_from_user(buffer, userbuf, nbytes)) {
>                 retval = -EFAULT;
>                 goto out1;
>         }
>         buffer[nbytes] = 0;     /* nul-terminate */
>
>         mutex_lock(&cgroup_mutex);
>
>         if (cgroup_is_removed(cgrp)) {
>                 retval = -ENODEV;
>                 goto out2;
>         }
>
>         switch (type) {
>         case FILE_TASKLIST:
>                 retval = attach_task_by_pid(cgrp, buffer);
>                 break;
>         case FILE_NOTIFY_ON_RELEASE:
>                 clear_bit(CGRP_RELEASABLE, &cgrp->flags);
>                 if (simple_strtoul(buffer, NULL, 10) != 0)
>                         set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
>                 else
>                         clear_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
>                 break;
>         case FILE_RELEASE_AGENT:
>         {
>                 struct cgroupfs_root *root = cgrp->root;
>                 /* Strip trailing newline */
>                 if (nbytes && (buffer[nbytes-1] == '\n')) {
>                         buffer[nbytes-1] = 0;
>                 }
> -               if (nbytes < sizeof(root->release_agent_path)) {
> -                       /* We never write anything other than '\0'
> -                        * into the last char of release_agent_path,
> -                        * so it always remains a NUL-terminated
> -                        * string */
> -                       strncpy(root->release_agent_path, buffer, nbytes);
> -                       root->release_agent_path[nbytes] = 0;
> -               } else {
> -                       retval = -ENOSPC;
> -               }
> +
> +               /* We never write anything other than '\0'
> +                * into the last char of release_agent_path,
> +                * so it always remains a NUL-terminated
> +                * string */
> +               strncpy(root->release_agent_path, buffer, nbytes);
> +               root->release_agent_path[nbytes] = 0;
> +
>                 break;
>         }
>         default:
>                 retval = -EINVAL;
>                 goto out2;
>         }
>
>         if (retval == 0)
>                 retval = nbytes;
>  out2:
>         mutex_unlock(&cgroup_mutex);
>  out1:
>         kfree(buffer);
>         return retval;
>  }
>
>  static ssize_t cgroup_file_write(struct file *file, const char __user *buf,
>                                                 size_t nbytes, loff_t *ppos)
>  {
>         struct cftype *cft = __d_cft(file->f_dentry);
>         struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent);
>
>         if (!cft)
>                 return -ENODEV;
>         if (cft->write)
>                 return cft->write(cgrp, cft, file, buf, nbytes, ppos);
>         if (cft->write_uint)
>                 return cgroup_write_uint(cgrp, cft, file, buf, nbytes, ppos);
>         return -EINVAL;
>  }
>
>  static ssize_t cgroup_read_uint(struct cgroup *cgrp, struct cftype *cft,
>                                    struct file *file,
>                                    char __user *buf, size_t nbytes,
>                                    loff_t *ppos)
>  {
>         char tmp[64];
>         u64 val = cft->read_uint(cgrp, cft);
>         int len = sprintf(tmp, "%llu\n", (unsigned long long) val);
>
>
>

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Adrian Bunk]

On Wed, Oct 24, 2007 at 09:30:23AM -0700, Paul Menage wrote:
> I think I'd rather not make this change - if we later changed the size
> of release_agent_path[] this could silently fail. Can we get around
> the coverity checker somehow?

I do not care what the Coverity checker thinks about the code, and 
there's no reason for changing code only for the sake of this checker.

Two questions:
- Is it really intended to perhaps change release_agent_path[] to have
  less than PATH_MAX size?
- If yes, do you want to return -E2BIG for (nbytes >= PATH_MAX) or for
  (nbytes >= sizeof(root->release_agent_path)) ?

> Paul
> 
> On 10/24/07, Adrian Bunk <bunk@kernel.org> wrote:
> > This patch removes dead code spotted by the Coverity checker
> > (look at the "(nbytes >= PATH_MAX)" check).
> >
> > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> >
> > ---
> >
> >  kernel/cgroup.c |   18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > --- linux-2.6/kernel/cgroup.c.old       2007-10-23 18:37:43.000000000 +0200
> > +++ linux-2.6/kernel/cgroup.c   2007-10-23 18:39:15.000000000 +0200
> > @@ -1320,90 +1320,88 @@ static ssize_t cgroup_common_file_write(
> >
> >         if (nbytes >= PATH_MAX)
> >                 return -E2BIG;
> >
> >         /* +1 for nul-terminator */
> >         buffer = kmalloc(nbytes + 1, GFP_KERNEL);
> >         if (buffer == NULL)
> >                 return -ENOMEM;
> >
> >         if (copy_from_user(buffer, userbuf, nbytes)) {
> >                 retval = -EFAULT;
> >                 goto out1;
> >         }
> >         buffer[nbytes] = 0;     /* nul-terminate */
> >
> >         mutex_lock(&cgroup_mutex);
> >
> >         if (cgroup_is_removed(cgrp)) {
> >                 retval = -ENODEV;
> >                 goto out2;
> >         }
> >
> >         switch (type) {
> >         case FILE_TASKLIST:
> >                 retval = attach_task_by_pid(cgrp, buffer);
> >                 break;
> >         case FILE_NOTIFY_ON_RELEASE:
> >                 clear_bit(CGRP_RELEASABLE, &cgrp->flags);
> >                 if (simple_strtoul(buffer, NULL, 10) != 0)
> >                         set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
> >                 else
> >                         clear_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
> >                 break;
> >         case FILE_RELEASE_AGENT:
> >         {
> >                 struct cgroupfs_root *root = cgrp->root;
> >                 /* Strip trailing newline */
> >                 if (nbytes && (buffer[nbytes-1] == '\n')) {
> >                         buffer[nbytes-1] = 0;
> >                 }
> > -               if (nbytes < sizeof(root->release_agent_path)) {
> > -                       /* We never write anything other than '\0'
> > -                        * into the last char of release_agent_path,
> > -                        * so it always remains a NUL-terminated
> > -                        * string */
> > -                       strncpy(root->release_agent_path, buffer, nbytes);
> > -                       root->release_agent_path[nbytes] = 0;
> > -               } else {
> > -                       retval = -ENOSPC;
> > -               }
> > +
> > +               /* We never write anything other than '\0'
> > +                * into the last char of release_agent_path,
> > +                * so it always remains a NUL-terminated
> > +                * string */
> > +               strncpy(root->release_agent_path, buffer, nbytes);
> > +               root->release_agent_path[nbytes] = 0;
> > +
> >                 break;
> >         }
> >         default:
> >                 retval = -EINVAL;
> >                 goto out2;
> >         }
> >
> >         if (retval == 0)
> >                 retval = nbytes;
> >  out2:
> >         mutex_unlock(&cgroup_mutex);
> >  out1:
> >         kfree(buffer);
> >         return retval;
> >  }

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Paul Menage]

On 10/24/07, Adrian Bunk <bunk@kernel.org> wrote:
>
> Two questions:
> - Is it really intended to perhaps change release_agent_path[] to have
>   less than PATH_MAX size?

I've got no intention to do so currently.

> - If yes, do you want to return -E2BIG for (nbytes >= PATH_MAX) or for
>   (nbytes >= sizeof(root->release_agent_path)) ?

I think E2BIG for the former for backwards compatabilty; the latter
could be either ENOSPC or E2BIG; i.e. both checks are useful - one to
stop us allocating more memory than is sensible, and one to stop us
overrunning the buffer; the fact that these two are the same size at
the moment is coincidence.

I guess ideally the first check would be for the max() of any of the
data structures that we expect to be able to write over; PATH_MAX was
just picked as a convenience.

Paul

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Paul Jackson]

Paul M wrote:
> I think I'd rather not make this change - if we later changed the size
> of release_agent_path[] this could silently fail. Can we get around
> the coverity checker somehow?

Perhaps we can simplify this check then, to:

  BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX));

Less runtime code.

This patch of Adrian highlighted a couple more opportunities
for code tweaking ... see my RFC patches, coming next from me.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Paul Menage]

On 10/24/07, Paul Jackson <pj@sgi.com> wrote:
> Paul M wrote:
> > I think I'd rather not make this change - if we later changed the size
> > of release_agent_path[] this could silently fail. Can we get around
> > the coverity checker somehow?
>
> Perhaps we can simplify this check then, to:
>
>   BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX));
>
> Less runtime code.

Sounds reasonable to me. Is there any kind of compile-time assert
macro in the kernel?

Paul

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Adrian Bunk]

On Thu, Oct 25, 2007 at 06:10:24PM -0700, Paul Menage wrote:
> On 10/24/07, Paul Jackson <pj@sgi.com> wrote:
> > Paul M wrote:
> > > I think I'd rather not make this change - if we later changed the size
> > > of release_agent_path[] this could silently fail. Can we get around
> > > the coverity checker somehow?
> >
> > Perhaps we can simplify this check then, to:
> >
> >   BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX));
> >
> > Less runtime code.
> 
> Sounds reasonable to me. Is there any kind of compile-time assert
> macro in the kernel?

BUILD_BUG_ON()

> Paul

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Paul Jackson]

Paul M wrote:
> Sounds reasonable to me. Is there any kind of compile-time assert
> macro in the kernel?

Check out the assembly code generated by:

    BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX));

(Hint: you can't find it ;)

It -is- compile time!

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Paul Menage]

On 10/25/07, Paul Jackson <pj@sgi.com> wrote:
> Paul M wrote:
> > Sounds reasonable to me. Is there any kind of compile-time assert
> > macro in the kernel?
>
> Check out the assembly code generated by:
>
>     BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX));
>
> (Hint: you can't find it ;)
>
> It -is- compile time!
>

Sounds like a good solution.

Paul

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Adrian Bunk]

On Thu, Oct 25, 2007 at 06:24:25PM -0700, Paul Jackson wrote:
> Paul M wrote:
> > Sounds reasonable to me. Is there any kind of compile-time assert
> > macro in the kernel?
> 
> Check out the assembly code generated by:
> 
>     BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX));
> 
> (Hint: you can't find it ;)
> 
> It -is- compile time!

But when the condition is fulfilled, you get a runtime error, not a 
compile error.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] kernel/cgroup.c: remove dead code

[Paul Jackson]

pj wrote:
> Check out the assembly code generated by:
>
>     BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX));
>
> (Hint: you can't find it ;)
>
> It -is- compile time!

To be clear, BUG_ON() in general is a runtime check.

But the compiler can optimize out constant expressions,
and code conditionally executed in the case of a constant
that can never be true.

Adrian wrote:
> > It -is- compile time!
> 
> But when the condition is fulfilled, you get a runtime error, not a 
> compile error.

Correct you are.  And when I advocated BUG_ON in this role, I did
two things:
 1) I was blissfully ignorant of BUILD_BUG_ON(), which would be
    better here, for the reasons you state, and
 2) I did a silent calculation in my head, noticing that if the
    constants ever changed so as to trigger this check, it would
    show up really quickly in testing, because it was on code path
    that would be hard to miss.  Because of this, the delay until
    runtime of this check was less of a disaster than it would
    have been on a rarely traveled code path.

In sum - Adrian is right - use BUILD_BUG_ON() here.

Thanks, Adrian.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401