kernel/cgroup.c: remove dead code
PATCH 1/1 kernel/cgroup.c: remove dead code
This patch removes dead code spotted by the Coverity checker (look at the "(nbytes >= PATH_MAX)" check).
---
kernel/cgroup.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
kernel/cgroup.c renamed from kernel/cgroup.c.old +8 -10
@@ -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);
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
Quoted 113 lines · Adrian Bunk
This patch removes dead code spotted by the Coverity checker (look at the "(nbytes >=
PATH_MAX)" check).Signed-off-by Adrian Bunk <[email protected]>---
kernel/cgroup.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)kernel/cgroup.c 2007-10-23 18:39:15.000000000 +0200 renamed from kernel/cgroup.c.old 2007-10-23 18:37:43.000000000 +0200 +8
-10@@ -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);
Quoted 3 lines · 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?
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)) ?
Quoted 81 lines
Paul
Quoted 78 lines · Adrian Bunk
This patch removes dead code spotted by the Coverity checker (look at the "(nbytes >=
PATH_MAX)" check).Signed-off-by Adrian Bunk <[email protected]>---
kernel/cgroup.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)kernel/cgroup.c 2007-10-23 18:39:15.000000000 +0200 renamed from kernel/cgroup.c.old 2007-10-23 18:37:43.000000000 +0200 +8
-10@@ -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; }
Quoted 8 lines
kernel/cgroup.c 2007-10-23 18:39:15.000000000 +0200 static ssize_t cgroup_common_file_write( · line 1382
Quoted 8 lines
kernel/cgroup.c 2007-10-23 18:39:15.000000000 +0200 renamed from kernel/cgroup.c.old 2007-10-23 18:37:43.000000000 +0200 +0
-0if (retval == 0) retval = nbytes; out2: mutex_unlock(&cgroup_mutex); out1: kfree(buffer); return retval; }
cu
Adrian
Signature · 4 lines
"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 SeedQuoted 4 lines · Adrian Bunk
Two questions:
- Is it really intended to perhaps change release_agent_path[] to have less thanPATH_MAXsize?
I've got no intention to do so currently.
Quoted 2 lines
- If yes, do you want to return
-E2BIGfor (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
Quoted 3 lines · Paul M
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.
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401Quoted 10 lines · Paul Jackson
Quoted 3 lines · Paul M
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
Quoted 6 lines · Paul Menage
Quoted 5 lines · Paul Jackson
Quoted 3 lines · Paul M
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?
Quoted 8 lines
Quoted 5 lines
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()
Quoted 1 line
Paul
cu
Adrian
Signature · 4 lines
"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 SeedQuoted 2 lines · Paul M
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!
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401Quoted 4 lines · Paul Jackson
Quoted 2 lines · Paul M
Sounds reasonable to me. Is there any kind of compile-time assert macro in the kernel?
Quoted 8 lines
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
Quoted 7 lines · pj
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.
Quoted 4 lines · Adrian
Quoted 1 line
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.
Signature · 3 lines
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401Quoted 11 lines · Paul Jackson
Quoted 2 lines · Paul M
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
Signature · 4 lines
"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