efivarfs: revert "fix memory leak in efivarfs_create()"

PATCH In review Ard Biesheuvel 4 participants 9 messages Nov 25, 2020 – Nov 27, 2020

Subsystems (3)

Stable branch EFI EFI variable filesystem

lore

Share & tools

lore

Ard Biesheuvel

author maintainer

2020-11-25 07:53 UTC #0 lore raw

PATCH 1/1 efivarfs: revert "fix memory leak in efivarfs_create()"

The memory leak addressed by commit fe5186cf12e3 is a false positive:
all allocations are recorded in a linked list, and freed when the filesystem is unmounted. This leads to double frees, and as reported by David, leads to crashes if SLUB is configured to self destruct when double frees occur.

So drop the redundant kfree() again, and instead, mark the offending pointer variable so the allocation is ignored by kmemleak.

Cc Vamshi K Sthambamkadi <[email protected]>

Fixes fe5186cf12e3 ("efivarfs: fix memory leak in efivarfs_create()")

Reported-by David Laight <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 fs/efivarfs/inode.c | 1 +
 fs/efivarfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

fs/efivarfs/inode.c +1 -0

index 96c0c86f3fff..38324427a2b3 100644
@@ -103,6 +103,7 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 	var->var.VariableName[i] = '\0';
 
 	inode->i_private = var;
+	kmemleak_ignore(var);
 
 	err = efivar_entry_add(var, &efivarfs_list);
 	if (err)

fs/efivarfs/super.c +0 -1

index f943fd0b0699..15880a68faad 100644
@@ -21,7 +21,6 @@ LIST_HEAD(efivarfs_list);
 static void efivarfs_evict_inode(struct inode *inode)
 {
 	clear_inode(inode);
-	kfree(inode->i_private);
 }
 
 static const struct super_operations efivarfs_ops = {

Signature · 1 line

2.17.1

Oleksandr Natalenko

2020-11-25 08:05 UTC #1 lore raw

Hello.

Quoted 10 lines · Ard Biesheuvel

The memory leak addressed by commit fe5186cf12e3 is a false positive:
all allocations are recorded in a linked list, and freed when the filesystem is unmounted. This leads to double frees, and as reported by David, leads to crashes if SLUB is configured to self destruct when double frees occur.
So drop the redundant kfree() again, and instead, mark the offending pointer variable so the allocation is ignored by kmemleak.
Cc Vamshi K Sthambamkadi <[email protected]>

Should also have:

Cc <[email protected]> # v5.9

Quoted 33 lines

Fixes fe5186cf12e3 ("efivarfs: fix memory leak in efivarfs_create()")

Reported-by David Laight <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 fs/efivarfs/inode.c | 1 +
 fs/efivarfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

fs/efivarfs/inode.c +1 -0

index 96c0c86f3fff..38324427a2b3 100644
@@ -103,6 +103,7 @@ static int efivarfs_create(struct inode *dir,
 struct dentry *dentry,
 	var->var.VariableName[i] = '\0';
 
 	inode->i_private = var;
+	kmemleak_ignore(var);
 
 	err = efivar_entry_add(var, &efivarfs_list);

fs/efivarfs/inode.c +0 -0

 	if (err)

fs/efivarfs/super.c +0 -1

index f943fd0b0699..15880a68faad 100644
@@ -21,7 +21,6 @@ LIST_HEAD(efivarfs_list);
 static void efivarfs_evict_inode(struct inode *inode)
 {
 	clear_inode(inode);
-	kfree(inode->i_private);
 }
 
 static const struct super_operations efivarfs_ops = {

Signature · 1 line

  Oleksandr Natalenko (post-factum)

Ard Biesheuvel

author maintainer

2020-11-25 08:05 UTC #2 lore raw

Quoted 10 lines · Oleksandr Natalenko

Hello.
Quoted 6 lines · Ard Biesheuvel
The memory leak addressed by commit fe5186cf12e3 is a false positive:
all allocations are recorded in a linked list, and freed when the filesystem is unmounted. This leads to double frees, and as reported by David, leads to crashes if SLUB is configured to self destruct when double frees occur.

Quoted 9 lines

Quoted 4 lines
So drop the redundant kfree() again, and instead, mark the offending pointer variable so the allocation is ignored by kmemleak.
Cc Vamshi K Sthambamkadi <[email protected]>
Should also have:
Cc <[email protected]> # v5.9

No it should not. The fixes tag should be sufficient.

Quoted 36 lines

Quoted 26 lines
Fixes fe5186cf12e3 ("efivarfs: fix memory leak in efivarfs_create()")
Reported-by David Laight <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 fs/efivarfs/inode.c | 1 +
 fs/efivarfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
fs/efivarfs/inode.c +1 -0
index 96c0c86f3fff..38324427a2b3 100644
@@ -103,6 +103,7 @@ static int efivarfs_create(struct inode *dir,
 struct dentry *dentry,
      var->var.VariableName[i] = '\0';
 
      inode->i_private = var;
+     kmemleak_ignore(var);
 
      err = efivar_entry_add(var, &efivarfs_list);
if (err)
fs/efivarfs/super.c +0 -0
index f943fd0b0699..15880a68faad 100644
fs/efivarfs/super.c LIST_HEAD(efivarfs_list); · line 26
Quoted 7 lines
fs/efivarfs/super.c +0 -1
@@ -21,7 +21,6 @@ LIST_HEAD(efivarfs_list);
 static void efivarfs_evict_inode(struct inode *inode)
 {
      clear_inode(inode);
-     kfree(inode->i_private);
 }
 
 static const struct super_operations efivarfs_ops = {
--
Oleksandr Natalenko (post-factum)

Greg Kroah-Hartman

maintainer

2020-11-25 08:24 UTC #3 lore raw

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Oleksandr Natalenko
Hello.
Quoted 9 lines · Ard Biesheuvel
The memory leak addressed by commit fe5186cf12e3 is a false positive:
all allocations are recorded in a linked list, and freed when the filesystem is unmounted. This leads to double frees, and as reported by David, leads to crashes if SLUB is configured to self destruct when double frees occur.
So drop the redundant kfree() again, and instead, mark the offending pointer variable so the allocation is ignored by kmemleak.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Cc Vamshi K Sthambamkadi <[email protected]>
Should also have:
Cc <[email protected]> # v5.9
No it should not. The fixes tag should be sufficient.

No, "Fixes:" does not ever mean "I want this patch to go to a stable tree". It might happen, it might not, if you REALLY know this should go to a stable tree, please follow the directions for what we have been doing for 15+ years now, as documented in:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Hint, use "cc: [email protected]" if you know you want it applied, otherwise it's just a best-guess-effort on our part.

thanks,

greg k-h

Oleksandr Natalenko

2020-11-25 10:27 UTC #4 lore raw

In reply to Ard Biesheuvel

Quoted 23 lines · Ard Biesheuvel

The memory leak addressed by commit fe5186cf12e3 is a false positive:
all allocations are recorded in a linked list, and freed when the filesystem is unmounted. This leads to double frees, and as reported by David, leads to crashes if SLUB is configured to self destruct when double frees occur.
So drop the redundant kfree() again, and instead, mark the offending pointer variable so the allocation is ignored by kmemleak.
Cc Vamshi K Sthambamkadi <[email protected]>
Fixes fe5186cf12e3 ("efivarfs: fix memory leak in efivarfs_create()")
Reported-by David Laight <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 fs/efivarfs/inode.c | 1 +
 fs/efivarfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
fs/efivarfs/inode.c +0 -0
index 96c0c86f3fff..38324427a2b3 100644

fs/efivarfs/inode.c static int efivarfs_create(struct inode *dir, · line 107

Quoted 5 lines

fs/efivarfs/inode.c +1 -0

@@ -103,6 +103,7 @@ static int efivarfs_create(struct inode *dir,
 struct dentry *dentry,
 	var->var.VariableName[i] = '\0';
 
 	inode->i_private = var;
+	kmemleak_ignore(var);

Do we need to do this as well:

#include <linux/kmemleak.h>

Because otherwise for 5.9 I get:

[  148s] fs/efivarfs/inode.c: In function 'efivarfs_create':
[  148s] fs/efivarfs/inode.c:106:2: error: implicit declaration of function 'kmemleak_ignore' [-Werror=implicit-function-declaration]
[  148s]   106 |  kmemleak_ignore(var);
[  148s]       |  ^~~~~~~~~~~~~~~

Quoted 15 lines

fs/efivarfs/inode.c +0 -0

 	err = efivar_entry_add(var, &efivarfs_list);
 	if (err)

fs/efivarfs/super.c +0 -1

index f943fd0b0699..15880a68faad 100644
@@ -21,7 +21,6 @@ LIST_HEAD(efivarfs_list);
 static void efivarfs_evict_inode(struct inode *inode)
 {
 	clear_inode(inode);
-	kfree(inode->i_private);
 }
 
 static const struct super_operations efivarfs_ops = {

Signature · 1 line

  Oleksandr Natalenko (post-factum)

Ard Biesheuvel

author maintainer

2020-11-25 10:28 UTC #5 lore raw

Quoted 37 lines · Oleksandr Natalenko

Quoted 23 lines · Ard Biesheuvel
The memory leak addressed by commit fe5186cf12e3 is a false positive:
all allocations are recorded in a linked list, and freed when the filesystem is unmounted. This leads to double frees, and as reported by David, leads to crashes if SLUB is configured to self destruct when double frees occur.
So drop the redundant kfree() again, and instead, mark the offending pointer variable so the allocation is ignored by kmemleak.
Cc Vamshi K Sthambamkadi <[email protected]>
Fixes fe5186cf12e3 ("efivarfs: fix memory leak in efivarfs_create()")
Reported-by David Laight <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 fs/efivarfs/inode.c | 1 +
 fs/efivarfs/super.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
fs/efivarfs/inode.c +0 -0
index 96c0c86f3fff..38324427a2b3 100644
fs/efivarfs/inode.c static int efivarfs_create(struct inode *dir, · line 107
Quoted 5 lines
fs/efivarfs/inode.c +1 -0
@@ -103,6 +103,7 @@ static int efivarfs_create(struct inode *dir,
 struct dentry *dentry,
      var->var.VariableName[i] = '\0';
 
      inode->i_private = var;
+     kmemleak_ignore(var);
Do we need to do this as well:
#include <linux/kmemleak.h>
?

Quoted 8 lines

Because otherwise for 5.9 I get:
[ 148s] fs/efivarfs/inode.c: In function 'efivarfs_create':
[ 148s] fs/efivarfs/inode.c:106:2: error: implicit declaration of function 'kmemleak_ignore' [-Werror=implicit-function-declaration]
[  148s]   106 |  kmemleak_ignore(var);
[  148s]       |  ^~~~~~~~~~~~~~~

Ah yes, thanks for the report. I will add the include to the patch.

Quoted 18 lines

Quoted 8 lines
fs/efivarfs/inode.c +0 -0
      err = efivar_entry_add(var, &efivarfs_list);
      if (err)
fs/efivarfs/super.c +0 -0
index f943fd0b0699..15880a68faad 100644
fs/efivarfs/super.c LIST_HEAD(efivarfs_list); · line 26
Quoted 7 lines
fs/efivarfs/super.c +0 -1
@@ -21,7 +21,6 @@ LIST_HEAD(efivarfs_list);
 static void efivarfs_evict_inode(struct inode *inode)
 {
      clear_inode(inode);
-     kfree(inode->i_private);
 }
 
 static const struct super_operations efivarfs_ops = {
--
Oleksandr Natalenko (post-factum)

Jonathon Fernyhough

2020-11-27 16:56 UTC #6 lore raw

Quoted 4 lines · Ard Biesheuvel

Quoted 2 lines · Oleksandr Natalenko
On 25.11.2020 08:53, Ard Biesheuvel wrote:

--snip--

Quoted 9 lines

Quoted 9 lines
Do we need to do this as well:
#include <linux/kmemleak.h>
?
Because otherwise for 5.9 I get:

Quoted 10 lines

Quoted 6 lines
[ 148s] fs/efivarfs/inode.c: In function 'efivarfs_create':
[ 148s] fs/efivarfs/inode.c:106:2: error: implicit declaration of function 'kmemleak_ignore' [-Werror=implicit-function-declaration]
[  148s]   106 |  kmemleak_ignore(var);
[  148s]       |  ^~~~~~~~~~~~~~~
Ah yes, thanks for the report. I will add the include to the patch.

Is this necessary if CONFIG_DEBUG_KMEMLEAK is not enabled in the kernel config? e.g. should there be an #ifdef CONFIG_DEBUG_KMEMLEAK somewhere in there?

Ard Biesheuvel

author maintainer

2020-11-27 16:59 UTC #7 lore raw

Quoted 23 lines · Jonathon Fernyhough

Quoted 4 lines · Ard Biesheuvel
Quoted 2 lines · Oleksandr Natalenko
On 25.11.2020 08:53, Ard Biesheuvel wrote:
--snip--
Quoted 16 lines
Quoted 15 lines
Do we need to do this as well:
#include <linux/kmemleak.h>
?
Because otherwise for 5.9 I get:
[ 148s] fs/efivarfs/inode.c: In function 'efivarfs_create':
[ 148s] fs/efivarfs/inode.c:106:2: error: implicit declaration of function 'kmemleak_ignore' [-Werror=implicit-function-declaration]
[  148s]   106 |  kmemleak_ignore(var);
[  148s]       |  ^~~~~~~~~~~~~~~

Quoted 8 lines

Quoted 3 lines
Ah yes, thanks for the report. I will add the include to the patch.
Is this necessary if CONFIG_DEBUG_KMEMLEAK is not enabled in the kernel config? e.g. should there be an #ifdef CONFIG_DEBUG_KMEMLEAK somewhere in there?

We typically define these helpers unconditionally, and sort out the differences in the header file. In this case, we have

static inline void kmemleak_ignore(const void *ptr) {
}

in include/linux/kmemleak.h if CONFIG_DEBUG_KMEMLEAK is not set.

This makes the calling code much cleaner.

Oleksandr Natalenko

2020-11-27 16:59 UTC #8 lore raw

In reply to Jonathon Fernyhough

Hi.

Quoted 22 lines · Jonathon Fernyhough

Quoted 4 lines · Ard Biesheuvel
Quoted 2 lines · Oleksandr Natalenko
On 25.11.2020 08:53, Ard Biesheuvel wrote:
--snip--
Quoted 16 lines
Quoted 15 lines
Do we need to do this as well:
#include <linux/kmemleak.h>
?
Because otherwise for 5.9 I get:
[ 148s] fs/efivarfs/inode.c: In function 'efivarfs_create':
[ 148s] fs/efivarfs/inode.c:106:2: error: implicit declaration of function 'kmemleak_ignore' [-Werror=implicit-function-declaration]
[  148s]   106 |  kmemleak_ignore(var);
[  148s]       |  ^~~~~~~~~~~~~~~

Quoted 7 lines

Quoted 3 lines
Ah yes, thanks for the report. I will add the include to the patch.
Is this necessary if CONFIG_DEBUG_KMEMLEAK is not enabled in the kernel config? e.g. should there be an #ifdef CONFIG_DEBUG_KMEMLEAK somewhere in there?

kmemleak_ignore() is a noop if CONFIG_DEBUG_KMEMLEAK is not set. See include/linux/kmemleak.h. Thus no extra condition is needed here.

Signature · 1 line

  Oleksandr Natalenko (post-factum)