From patchwork Thu Oct 10 10:41:59 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Behrens X-Patchwork-Id: 3015241 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id E84D29F1E1 for ; Thu, 10 Oct 2013 10:42:44 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C0117202C7 for ; Thu, 10 Oct 2013 10:42:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9905A202F0 for ; Thu, 10 Oct 2013 10:42:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754246Ab3JJKmN (ORCPT ); Thu, 10 Oct 2013 06:42:13 -0400 Received: from mo-p00-ob.rzone.de ([81.169.146.162]:36689 "EHLO mo-p00-ob.rzone.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752748Ab3JJKmM (ORCPT ); Thu, 10 Oct 2013 06:42:12 -0400 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; t=1381401730; l=3516; s=domk; d=giantdisaster.de; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:References: Subject:CC:To:MIME-Version:From:Date:X-RZG-CLASS-ID:X-RZG-AUTH; bh=wSattxSku0hTxonx6Gphf+4u+6k=; b=dS2vMquFJqSGWS8p8SucCpCcDJa0QIaXt+1wVBDmbxTawnhutvsA6WF1kz/Qe63iaH0 PylL1Io9IoKEpmKNM9zqnUk/0HGumr0iqLgZNWN1z4A5i++FBg2GYHQKLfa3gL7evgErE mAp02VFdu2JIggBbAoS35ZokAK4GG+Ixyc4= X-RZG-AUTH: :IGUKYFjleetgZuRbHZjp6Ve7NzeE1efWuTR/wV06y353QgIuD5+acdRFtJ8MDHZp4u74mTVB5dOWYQ== X-RZG-CLASS-ID: mo00 Received: from [172.24.1.80] (yian-ho01.cronon.net [192.166.201.94]) by smtp.strato.de (RZmta 32.6 AUTH) with (TLSv1.0:DHE-RSA-AES256-SHA encrypted) ESMTPSA id D0157ap9AAMYyK ; Thu, 10 Oct 2013 12:42:07 +0200 (CEST) Message-ID: <52568477.3080008@giantdisaster.de> Date: Thu, 10 Oct 2013 12:41:59 +0200 From: Stefan Behrens User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.0 MIME-Version: 1.0 To: "Geyslan G. Bem" , chris.mason@fusionio.com CC: linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org, jbacik@fusionio.com, joe@perches.com, kernel-br@googlegroups.com Subject: Re: [PATCH v4] btrfs: Fix memory leakage in the tree-log.c References: <1381370495-13702-1-git-send-email-geyslan@gmail.com> In-Reply-To: <1381370495-13702-1-git-send-email-geyslan@gmail.com> Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 9 Oct 2013 23:01:35 -0300, Geyslan G. Bem wrote: > When 'dir' is NULL, after calling extref_get_fields(), add_inode_ref() > can be returning without freeing the 'name' pointer. > > Added kfree when necessary. > > Signed-off-by: Geyslan G. Bem > --- > fs/btrfs/tree-log.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c > index 79f057c..63c0b72 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -1169,8 +1169,11 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans, > */ > if (!dir) > dir = read_one_inode(root, parent_objectid); > - if (!dir) > + if (!dir) { > + if (!ret) > + kfree(name); > return -ENOENT; > + } > } else { > ret = ref_get_fields(eb, ref_ptr, &namelen, &name, > &ref_index); > There are many more places to fix up in this function. We lose up to two inodes ("if (!ret) return ret;" without the iput(dir), iput(inode)) and we can lose the memory for name ("if ... goto out;" and the out path does not contain the kfree(name)). I would prefer the approach with a label at the end of the function that handles all cleanup work and which is called from everywhere where a return is coded now. Like this (the code is completely untested since this is just a review comment): Could you rework your patch to do something similar to the code above? This approach is also documented in the file Documentation/CodingStyle in the kernel source tree in the section "Chapter 7: Centralized exiting of functions". --- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 964c583..40035db 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -1113,11 +1113,11 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans, struct extent_buffer *eb, int slot, struct btrfs_key *key) { - struct inode *dir; - struct inode *inode; + struct inode *dir = NULL; + struct inode *inode = NULL; unsigned long ref_ptr; unsigned long ref_end; - char *name; + char *name = NULL; int namelen; int ret; int search_done = 0; @@ -1150,13 +1150,15 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans, * care of the rest */ dir = read_one_inode(root, parent_objectid); - if (!dir) - return -ENOENT; + if (!dir) { + ret = -ENOENT; + goto out; + } inode = read_one_inode(root, inode_objectid); if (!inode) { - iput(dir); - return -EIO; + ret = -EIO; + goto out; } while (ref_ptr < ref_end) { @@ -1169,14 +1171,16 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans, */ if (!dir) dir = read_one_inode(root, parent_objectid); - if (!dir) - return -ENOENT; + if (!dir) { + ret = -ENOENT; + goto out; + } } else { ret = ref_get_fields(eb, ref_ptr, &namelen, &name, &ref_index); } if (ret) - return ret; + goto out; /* if we already have a perfect match, we're done */ if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode), @@ -1215,6 +1219,7 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans, ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen; kfree(name); + name = NULL; if (log_ref_ver) { iput(dir); dir = NULL; @@ -1225,6 +1230,7 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans, ret = overwrite_item(trans, root, path, eb, slot, key); out: btrfs_release_path(path); + kfree(name); iput(dir); iput(inode); return ret;