* [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free()
@ 2007-10-24 16:25 Adrian Bunk
2007-10-24 17:34 ` Balbir Singh
0 siblings, 1 reply; 4+ messages in thread
From: Adrian Bunk @ 2007-10-24 16:25 UTC (permalink / raw)
To: Balbir Singh; +Cc: linux-kernel
We'd better not nlmsg_free on a pointer containing an undefined value
(and without having anything allocated)...
Spotted by the Coverity checker.
Signed-off-by: Adrian Bunk <bunk@kernel.org>
---
kernel/taskstats.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--- linux-2.6/kernel/taskstats.c.old 2007-10-23 19:01:07.000000000 +0200
+++ linux-2.6/kernel/taskstats.c 2007-10-23 19:21:54.000000000 +0200
@@ -383,67 +383,67 @@
static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
{
int rc = 0;
struct sk_buff *rep_skb;
struct cgroupstats *stats;
struct nlattr *na;
size_t size;
u32 fd;
struct file *file;
int fput_needed;
na = info->attrs[CGROUPSTATS_CMD_ATTR_FD];
if (!na)
return -EINVAL;
fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]);
file = fget_light(fd, &fput_needed);
if (file) {
size = nla_total_size(sizeof(struct cgroupstats));
rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb,
size);
if (rc < 0)
- goto err;
+ return rc;
na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS,
sizeof(struct cgroupstats));
stats = nla_data(na);
memset(stats, 0, sizeof(*stats));
rc = cgroupstats_build(stats, file->f_dentry);
+
+ fput_light(file, fput_needed);
+
if (rc < 0)
goto err;
- fput_light(file, fput_needed);
return send_reply(rep_skb, info->snd_pid);
+err:
+ nlmsg_free(rep_skb);
}
-err:
- if (file)
- fput_light(file, fput_needed);
- nlmsg_free(rep_skb);
return rc;
}
static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
{
int rc = 0;
struct sk_buff *rep_skb;
struct taskstats *stats;
size_t size;
cpumask_t mask;
rc = parse(info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK], &mask);
if (rc < 0)
return rc;
if (rc == 0)
return add_del_listener(info->snd_pid, &mask, REGISTER);
rc = parse(info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK], &mask);
if (rc < 0)
return rc;
if (rc == 0)
return add_del_listener(info->snd_pid, &mask, DEREGISTER);
/*
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free() 2007-10-24 16:25 [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free() Adrian Bunk @ 2007-10-24 17:34 ` Balbir Singh 2007-10-24 18:34 ` Adrian Bunk 0 siblings, 1 reply; 4+ messages in thread From: Balbir Singh @ 2007-10-24 17:34 UTC (permalink / raw) To: Adrian Bunk; +Cc: linux-kernel Adrian Bunk wrote: > We'd better not nlmsg_free on a pointer containing an undefined value > (and without having anything allocated)... > > Spotted by the Coverity checker. > > Signed-off-by: Adrian Bunk <bunk@kernel.org> > > --- > > kernel/taskstats.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > --- linux-2.6/kernel/taskstats.c.old 2007-10-23 19:01:07.000000000 +0200 > +++ linux-2.6/kernel/taskstats.c 2007-10-23 19:21:54.000000000 +0200 > @@ -383,67 +383,67 @@ > > static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > { > int rc = 0; > struct sk_buff *rep_skb; > struct cgroupstats *stats; > struct nlattr *na; > size_t size; > u32 fd; > struct file *file; > int fput_needed; > > na = info->attrs[CGROUPSTATS_CMD_ATTR_FD]; > if (!na) > return -EINVAL; > > fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]); > file = fget_light(fd, &fput_needed); > if (file) { > size = nla_total_size(sizeof(struct cgroupstats)); > > rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, > size); > if (rc < 0) > - goto err; > + return rc; > We miss a fput_light() here > na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, > sizeof(struct cgroupstats)); > stats = nla_data(na); > memset(stats, 0, sizeof(*stats)); > > rc = cgroupstats_build(stats, file->f_dentry); > + > + fput_light(file, fput_needed); > + I don't understand this movement, it makes code reading a bit odd too. rc is the result, but we check the result after fput_light? > if (rc < 0) > goto err; > > - fput_light(file, fput_needed); > return send_reply(rep_skb, info->snd_pid); > +err: > + nlmsg_free(rep_skb); > } > > -err: > - if (file) > - fput_light(file, fput_needed); > - nlmsg_free(rep_skb); > return rc; > } -- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL ^ permalink raw reply [flat|nested] 4+ messages in thread
* [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free() 2007-10-24 17:34 ` Balbir Singh @ 2007-10-24 18:34 ` Adrian Bunk 2007-10-25 11:30 ` Balbir Singh 0 siblings, 1 reply; 4+ messages in thread From: Adrian Bunk @ 2007-10-24 18:34 UTC (permalink / raw) To: Balbir Singh; +Cc: linux-kernel On Wed, Oct 24, 2007 at 11:04:34PM +0530, Balbir Singh wrote: > Adrian Bunk wrote: > > We'd better not nlmsg_free on a pointer containing an undefined value > > (and without having anything allocated)... > > > > Spotted by the Coverity checker. > > > > Signed-off-by: Adrian Bunk <bunk@kernel.org> > > > > --- > > > > kernel/taskstats.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > --- linux-2.6/kernel/taskstats.c.old 2007-10-23 19:01:07.000000000 +0200 > > +++ linux-2.6/kernel/taskstats.c 2007-10-23 19:21:54.000000000 +0200 >... > > if (rc < 0) > > - goto err; > > + return rc; > > > > We miss a fput_light() here Sorry, my fault. > > na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, > > sizeof(struct cgroupstats)); > > stats = nla_data(na); > > memset(stats, 0, sizeof(*stats)); > > > > rc = cgroupstats_build(stats, file->f_dentry); > > + > > + fput_light(file, fput_needed); > > + > > I don't understand this movement, it makes code reading a bit odd too. > rc is the result, but we check the result after fput_light? >... I considered it more odd to read rc = cgroupstats_build(stats, file->f_dentry); if (rc < 0) goto err; fput_light(file, fput_needed); return send_reply(rep_skb, info->snd_pid); err: fput_light(file, fput_needed); nlmsg_free(rep_skb); But that's just personal taste. Anyway, duplicating the same fput_light() call two or three times isn't nice. Originally I had the bigger patch below and considered it too big for only this fix, but now I think it might be appropriate. If you ignore whitespace when diff'ing, then all it does is: <-- snip --> @@ -398,7 +398,9 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]); file = fget_light(fd, &fput_needed); - if (file) { + if (!file) + return 0; + size = nla_total_size(sizeof(struct cgroupstats)); rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, @@ -412,17 +414,15 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) memset(stats, 0, sizeof(*stats)); rc = cgroupstats_build(stats, file->f_dentry); - if (rc < 0) + if (rc < 0) { + nlmsg_free(rep_skb); goto err; - - fput_light(file, fput_needed); - return send_reply(rep_skb, info->snd_pid); } + rc = send_reply(rep_skb, info->snd_pid); + err: - if (file) fput_light(file, fput_needed); - nlmsg_free(rep_skb); return rc; } <-- snip --> Is this patch OK for you? cu Adrian <-- snip --> We'd better not nlmsg_free on a pointer containing an undefined value (and without having anything allocated). Spotted by the Coverity checker. Signed-off-by: Adrian Bunk <bunk@kernel.org> --- kernel/taskstats.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) eedd297ac36b5d92fc38b937677ba40dc6df1cd0 diff --git a/kernel/taskstats.c b/kernel/taskstats.c index 354e74b..07e86a8 100644 --- a/kernel/taskstats.c +++ b/kernel/taskstats.c @@ -398,31 +398,31 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]); file = fget_light(fd, &fput_needed); - if (file) { - size = nla_total_size(sizeof(struct cgroupstats)); + if (!file) + return 0; - rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, - size); - if (rc < 0) - goto err; + size = nla_total_size(sizeof(struct cgroupstats)); - na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, - sizeof(struct cgroupstats)); - stats = nla_data(na); - memset(stats, 0, sizeof(*stats)); + rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, + size); + if (rc < 0) + goto err; - rc = cgroupstats_build(stats, file->f_dentry); - if (rc < 0) - goto err; + na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, + sizeof(struct cgroupstats)); + stats = nla_data(na); + memset(stats, 0, sizeof(*stats)); - fput_light(file, fput_needed); - return send_reply(rep_skb, info->snd_pid); + rc = cgroupstats_build(stats, file->f_dentry); + if (rc < 0) { + nlmsg_free(rep_skb); + goto err; } + rc = send_reply(rep_skb, info->snd_pid); + err: - if (file) - fput_light(file, fput_needed); - nlmsg_free(rep_skb); + fput_light(file, fput_needed); return rc; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free() 2007-10-24 18:34 ` Adrian Bunk @ 2007-10-25 11:30 ` Balbir Singh 0 siblings, 0 replies; 4+ messages in thread From: Balbir Singh @ 2007-10-25 11:30 UTC (permalink / raw) To: Adrian Bunk; +Cc: linux-kernel Adrian Bunk wrote: > I considered it more odd to read > > > rc = cgroupstats_build(stats, file->f_dentry); > if (rc < 0) > goto err; > > fput_light(file, fput_needed); > return send_reply(rep_skb, info->snd_pid); > err: > fput_light(file, fput_needed); > nlmsg_free(rep_skb); > I agree, but sometimes the alternative can be worse to read and understand > > But that's just personal taste. > > Anyway, duplicating the same fput_light() call two or three times isn't > nice. > > Originally I had the bigger patch below and considered it too big for > only this fix, but now I think it might be appropriate. > > If you ignore whitespace when diff'ing, then all it does is: > > <-- snip --> > > @@ -398,7 +398,9 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > > fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]); > file = fget_light(fd, &fput_needed); > - if (file) { > + if (!file) > + return 0; > + > size = nla_total_size(sizeof(struct cgroupstats)); > > rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, > @@ -412,17 +414,15 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > memset(stats, 0, sizeof(*stats)); > > rc = cgroupstats_build(stats, file->f_dentry); > - if (rc < 0) > + if (rc < 0) { > + nlmsg_free(rep_skb); > goto err; > - > - fput_light(file, fput_needed); > - return send_reply(rep_skb, info->snd_pid); > } > > + rc = send_reply(rep_skb, info->snd_pid); > + > err: > - if (file) > fput_light(file, fput_needed); > - nlmsg_free(rep_skb); > return rc; > } > > <-- snip --> > > > Is this patch OK for you? > > > cu > Adrian > > > <-- snip --> > > > We'd better not nlmsg_free on a pointer containing an undefined value > (and without having anything allocated). > > Spotted by the Coverity checker. > > Signed-off-by: Adrian Bunk <bunk@kernel.org> > > --- > > kernel/taskstats.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > > eedd297ac36b5d92fc38b937677ba40dc6df1cd0 > diff --git a/kernel/taskstats.c b/kernel/taskstats.c > index 354e74b..07e86a8 100644 > --- a/kernel/taskstats.c > +++ b/kernel/taskstats.c > @@ -398,31 +398,31 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > > fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]); > file = fget_light(fd, &fput_needed); > - if (file) { > - size = nla_total_size(sizeof(struct cgroupstats)); > + if (!file) > + return 0; > > - rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, > - size); > - if (rc < 0) > - goto err; > + size = nla_total_size(sizeof(struct cgroupstats)); > > - na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, > - sizeof(struct cgroupstats)); > - stats = nla_data(na); > - memset(stats, 0, sizeof(*stats)); > + rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, > + size); > + if (rc < 0) > + goto err; > > - rc = cgroupstats_build(stats, file->f_dentry); > - if (rc < 0) > - goto err; > + na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, > + sizeof(struct cgroupstats)); > + stats = nla_data(na); > + memset(stats, 0, sizeof(*stats)); > > - fput_light(file, fput_needed); > - return send_reply(rep_skb, info->snd_pid); > + rc = cgroupstats_build(stats, file->f_dentry); > + if (rc < 0) { > + nlmsg_free(rep_skb); > + goto err; > } > > + rc = send_reply(rep_skb, info->snd_pid); > + > err: > - if (file) > - fput_light(file, fput_needed); > - nlmsg_free(rep_skb); > + fput_light(file, fput_needed); > return rc; > } > > Looks good to me! Acked-by: Balbir Singh <balbir@linux.vnet.ibm> Yet to be tested, I'll try and test it soon. -- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-10-26 14:29 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-10-24 16:25 [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free() Adrian Bunk 2007-10-24 17:34 ` Balbir Singh 2007-10-24 18:34 ` Adrian Bunk 2007-10-25 11:30 ` Balbir Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox