[PATCH] btrfs: Fix memory leakage in the tree-log.c

[Geyslan G. Bem]

In some cases, add_inode_ref() is returning without freeing
the 'name' pointer.

Added bail out to explicitly call kfree when necessary.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
---
fs/btrfs/tree-log.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 79f057c..37d32c3 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1170,13 +1170,18 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
if (!dir)
dir = read_one_inode(root, parent_objectid);
if (!dir)
- return -ENOENT;
+ {
+ ret = -ENOENT;
+ goto bail;
+ }
} else {
ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
&ref_index);
}
if (ret)
- return ret;
+ {
+ goto bail;
+ }

/* if we already have a perfect match, we're done */
if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
@@ -1214,7 +1219,6 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
}

ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
- kfree(name);
if (log_ref_ver) {
iput(dir);
dir = NULL;
@@ -1227,6 +1231,9 @@ out:
btrfs_release_path(path);
iput(dir);
iput(inode);
+bail:
+ if (name)
+ kfree(name);
return ret;
}

--
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Felipe Pena]

Hi,

On Wed, Oct 9, 2013 at 8:13 PM, Geyslan G. Bem <gey...@gmail.com> wrote:
> In some cases, add_inode_ref() is returning without freeing
> the 'name' pointer.
>
> Added bail out to explicitly call kfree when necessary.
>
> Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
> ---
> fs/btrfs/tree-log.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 79f057c..37d32c3 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -1170,13 +1170,18 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
> if (!dir)
> dir = read_one_inode(root, parent_objectid);
> if (!dir)
> - return -ENOENT;
> + {
> + ret = -ENOENT;
> + goto bail;
> + }

No braces required here.

> } else {
> ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
> &ref_index);
> }
> if (ret)
> - return ret;
> + {
> + goto bail;
> + }
>

Ditto.

> /* if we already have a perfect match, we're done */
> if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
> @@ -1214,7 +1219,6 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
> }
>
> ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
> - kfree(name);
> if (log_ref_ver) {
> iput(dir);
> dir = NULL;
> @@ -1227,6 +1231,9 @@ out:
> btrfs_release_path(path);
> iput(dir);
> iput(inode);
> +bail:
> + if (name)
> + kfree(name);
> return ret;
> }
>
> --
> 1.8.4
>
> --

> Você está recebendo esta mensagem porque se inscreveu no grupo "Kernel Brasil" dos Grupos do Google.
> Para cancelar a inscrição neste grupo e parar de receber seus e-mails, envie um e-mail para kernel-br+...@googlegroups.com.
> Para postar neste grupo, envie um e-mail para kern...@googlegroups.com.
> Para ver esta discussão na web, acesse https://groups.google.com/d/msgid/kernel-br/1381360387-27535-1-git-send-email-geyslan%40gmail.com.
> Para obter mais opções, acesse https://groups.google.com/groups/opt_out.



--
Regards,
Felipe Pena

[Geyslan G. Bem]

In some cases, add_inode_ref() is returning without freeing
the 'name' pointer.

Added bail out to explicitly call kfree when necessary.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
---

fs/btrfs/tree-log.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 79f057c..727d4ff 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1170,13 +1170,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;
+ {
+ ret = -ENOENT;
+ goto bail;
+ }

} else {
ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
&ref_index);
}
if (ret)
- return ret;

+ goto bail;

/* if we already have a perfect match, we're done */
if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),

@@ -1214,7 +1217,6 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,

}

ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
- kfree(name);
if (log_ref_ver) {
iput(dir);
dir = NULL;

@@ -1227,6 +1229,9 @@ out:

btrfs_release_path(path);
iput(dir);
iput(inode);
+bail:
+ if (name)
+ kfree(name);
return ret;
}

--
1.8.4

--

[Geyslan Gregório Bem]

Felipe, thank you,

Sending v2.

Geyslan Gregório Bem
hackingbits.com


2013/10/9 Felipe Pena <feli...@gmail.com>:

[Geyslan G. Bem]

In some cases, add_inode_ref() is returning without freeing
the 'name' pointer.

Added bail out to explicitly call kfree when necessary.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
---

fs/btrfs/tree-log.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 79f057c..ad7cc5f 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1170,13 +1170,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;
+ {
+ ret = -ENOENT;
+ goto bail;
+ }

} else {
ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
&ref_index);
}
if (ret)
- return ret;

+ goto bail;

/* if we already have a perfect match, we're done */
if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),

@@ -1227,6 +1230,9 @@ out:

btrfs_release_path(path);
iput(dir);
iput(inode);
+bail:
+ if (name)
+ kfree(name);
return ret;
}

--
1.8.4

--

[Geyslan Gregório Bem]

Please,

Analyze [PATCH v3].

Regards,

Geyslan Gregório Bem
hackingbits.com


2013/10/9 Geyslan G. Bem <gey...@gmail.com>:

[Josef Bacik]

On Wed, Oct 09, 2013 at 08:40:30PM -0300, Geyslan G. Bem wrote:
> In some cases, add_inode_ref() is returning without freeing
> the 'name' pointer.
>
> Added bail out to explicitly call kfree when necessary.
>
> Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
> ---
> fs/btrfs/tree-log.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 79f057c..ad7cc5f 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -1170,13 +1170,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;
> + {
> + ret = -ENOENT;
> + goto bail;
> + }

Code formatting is

if () {
}

not

if ()
{

}


> } else {
> ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
> &ref_index);
> }
> if (ret)
> - return ret;
> + goto bail;
>
> /* if we already have a perfect match, we're done */
> if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
> @@ -1227,6 +1230,9 @@ out:
> btrfs_release_path(path);
> iput(dir);
> iput(inode);
> +bail:
> + if (name)
> + kfree(name);

kfree already does the if (name) part of this so this is redundant. Also if you
are going to do this you need to set name = NULL; after the kfree above it
otherwise we have a double free. Thanks,

Josef

[Geyslan Gregório Bem]

Josef,

Thank you. Sending v4.

Geyslan Gregório Bem
hackingbits.com


2013/10/9 Josef Bacik <jba...@fusionio.com>:

[Geyslan G. Bem]

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 <gey...@gmail.com>
---

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

--
1.8.4

[Stefan Behrens]

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

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;


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

[Geyslan G. Bem]

In add_inode_ref() function:

Initializes local pointers.

Reduces the logical condition with the __add_inode_ref() return
value by using only one 'goto out'.

Centralizes the exiting, ensuring the freeing of all used memory.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
---

fs/btrfs/tree-log.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 79f057c..beb41b0 100644

@@ -1169,14 +1171,17 @@ 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),

@@ -1196,12 +1201,12 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
parent_objectid,
ref_index, name, namelen,
&search_done);
- if (ret == 1) {
- ret = 0;
+
+ if (ret) {
+ if (ret == 1)
+ ret--;
goto out;
}
- if (ret)
- goto out;
}

/* insert our name */
@@ -1215,6 +1220,8 @@ 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 +1232,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;

--
1.8.4

[Stefan Behrens]

On Thu, 10 Oct 2013 19:11:22 -0300, Geyslan G. Bem wrote:
> In add_inode_ref() function:
>
> Initializes local pointers.
>
> Reduces the logical condition with the __add_inode_ref() return
> value by using only one 'goto out'.
>
> Centralizes the exiting, ensuring the freeing of all used memory.
>
> Signed-off-by: Geyslan G. Bem <gey...@gmail.com>

The patch looks correct to me, but there are some nitpicking style issues.

This additional empty line is not required, we would have two empty
lines in a row.

>
> /* if we already have a perfect match, we're done */
> if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
> @@ -1196,12 +1201,12 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
> parent_objectid,
> ref_index, name, namelen,
> &search_done);
> - if (ret == 1) {
> - ret = 0;
> +

This empty line is also not required. You know, this hurts on regular
80x24 terminals. Do you get more than 24 lines on your display?
I thought there was a rule for this in Documentation/CodingStyle, but
there isn't. Therefore it's up to you. But the two empty lines above are
definitely not wanted.

> + if (ret) {
> + if (ret == 1)
> + ret--;

I don't see a reason to change the "ret = 0" to a "ret--", this is not
an optimization and makes the code less readable.

> goto out;
> }
> - if (ret)
> - goto out;
> }
>
> /* insert our name */
> @@ -1215,6 +1220,8 @@ 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;
> +

Another empty line which is not required for the purpose of this patch.

> if (log_ref_ver) {
> iput(dir);
> dir = NULL;
> @@ -1225,6 +1232,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;
>


--

[Geyslan Gregório Bem]

2013/10/11 Stefan Behrens <sbeh...@giantdisaster.de>:

Ok, I'll remove it.

>
>
>>
>> /* if we already have a perfect match, we're done */
>> if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
>> @@ -1196,12 +1201,12 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
>> parent_objectid,
>> ref_index, name, namelen,
>> &search_done);
>> - if (ret == 1) {
>> - ret = 0;
>> +
>
> This empty line is also not required. You know, this hurts on regular
> 80x24 terminals. Do you get more than 24 lines on your display?
> I thought there was a rule for this in Documentation/CodingStyle, but
> there isn't. Therefore it's up to you. But the two empty lines above are
> definitely not wanted.
>

Yes, I get more than 24 lines. But no problem, I know about the Coding Style.
This issue will be fixed.

>
>> + if (ret) {
>> + if (ret == 1)
>> + ret--;
>
> I don't see a reason to change the "ret = 0" to a "ret--", this is not
> an optimization and makes the code less readable.
>

Really. Using -O2 the code is equal. I'll redo to "ret = 0;" with the
new conditional scope.

>
>> goto out;
>> }
>> - if (ret)
>> - goto out;
>> }
>>
>> /* insert our name */
>> @@ -1215,6 +1220,8 @@ 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;
>> +
>
> Another empty line which is not required for the purpose of this patch.

Ok, it'll be removed.

>
>
>> if (log_ref_ver) {
>> iput(dir);
>> dir = NULL;
>> @@ -1225,6 +1232,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;
>>
>
>

Thank you again.

Geyslan G. Bem

[Geyslan G. Bem]

In add_inode_ref() function:

Initializes local pointers.

Reduces the logical condition with the __add_inode_ref() return
value by using only one 'goto out'.

Centralizes the exiting, ensuring the freeing of all used memory.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>

---
fs/btrfs/tree-log.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 79f057c..61bb051 100644

@@ -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),

@@ -1196,12 +1200,11 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,

parent_objectid,
ref_index, name, namelen,
&search_done);
- if (ret == 1) {
- ret = 0;

+ if (ret) {
+ if (ret == 1)

+ ret = 0;

goto out;
}
- if (ret)
- goto out;
}

/* insert our name */

@@ -1215,6 +1218,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 +1229,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;

--
1.8.4