* [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