From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754701AbXJZO3t (ORCPT ); Fri, 26 Oct 2007 10:29:49 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751142AbXJZO3m (ORCPT ); Fri, 26 Oct 2007 10:29:42 -0400 Received: from E23SMTP02.au.ibm.com ([202.81.18.163]:41855 "EHLO e23smtp02.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751003AbXJZO3l (ORCPT ); Fri, 26 Oct 2007 10:29:41 -0400 Message-ID: <47207E5E.80305@linux.vnet.ibm.com> Date: Thu, 25 Oct 2007 17:00:38 +0530 From: Balbir Singh Reply-To: balbir@linux.vnet.ibm.com Organization: IBM User-Agent: Thunderbird 1.5.0.13 (X11/20070824) MIME-Version: 1.0 To: Adrian Bunk CC: linux-kernel@vger.kernel.org Subject: Re: [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free() References: <20071024162554.GC30533@stusta.de> <471F822A.2030904@linux.vnet.ibm.com> <20071024183457.GE30533@stusta.de> In-Reply-To: <20071024183457.GE30533@stusta.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > > 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 Yet to be tested, I'll try and test it soon. -- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL