Re: [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free()

[Balbir Singh <balbir@linux.vnet.ibm.com>]

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