kernel/taskstats.c: fix bogus nlmsg_free()

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 <[email protected]>

---

 kernel/taskstats.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

kernel/taskstats.c renamed from kernel/taskstats.c.old +0 -0

@@ -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]);

kernel/taskstats.c renamed from kernel/taskstats.c.old +2 -5

 		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;
 }

Quoted 14 lines · Adrian Bunk

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 <[email protected]>

---

 kernel/taskstats.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

kernel/taskstats.c renamed from kernel/taskstats.c.old +0 -0

...

kernel/taskstats.c

Quoted 4 lines

kernel/taskstats.c renamed from kernel/taskstats.c.old +1 -1

 		if (rc < 0)
-			goto err;
+			return rc;

We miss a fput_light() here

Quoted 5 lines

kernel/taskstats.c renamed from kernel/taskstats.c.old +0 -0

 		na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS,
 					sizeof(struct cgroupstats));
 		stats = nla_data(na);
 		memset(stats, 0, sizeof(*stats));

kernel/taskstats.c

Quoted 4 lines

kernel/taskstats.c renamed from kernel/taskstats.c.old +3 -0

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

kernel/taskstats.c +7 -7

@@ -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 <[email protected]>

---

 kernel/taskstats.c |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

eedd297ac36b5d92fc38b937677ba40dc6df1cd0

kernel/taskstats.c +15 -15

index 354e74b..07e86a8 100644
@@ -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;
 	}