Skip to contentSkip to conversation

btrfs-progs: avoid memory leak in btrfs_close_devices

PATCH Superseded Wang Sheng-Hui 3 participants 4 messages Jun 25, 2013 – Jul 3, 2013 Btrfs file system

lore
Share & tools
lore

Wang Sheng-Hui

author
#0 lore raw

PATCH 1/1 btrfs-progs: avoid memory leak in btrfs_close_devices

Three kind of structures need to be freed on close:
* All struct btrfs_device managed by fs_devices
* The name field for each struct btrfs_device
* The above items for seed_devices

Signed-off-by Wang Sheng-Hui <[email protected]>
---
 volumes.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
volumes.c +13 -3
index d6f81f8..257b740 100644
@@ -153,6 +153,16 @@ static int device_list_add(const char *path,
 	return 0;
 }
 
+static void btrfs_close_device(struct btrfs_device *device)
+{
+	close(device->fd);
+	device->fd = -1;
+	device->writeable = 0;
+	if (device->name)
+		kfree(device->name);
+	kfree(device);
+}
+
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_fs_devices *seed_devices;

@@ -161,17 +171,17 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 again:
 	list_for_each(cur, &fs_devices->devices) {
 		device = list_entry(cur, struct btrfs_device, dev_list);
-		close(device->fd);
-		device->fd = -1;
-		device->writeable = 0;
+		btrfs_close_device(device);
 	}
 
 	seed_devices = fs_devices->seed;
 	fs_devices->seed = NULL;
 	if (seed_devices) {
+		kfree(fs_devices);
 		fs_devices = seed_devices;
 		goto again;
 	}
+	kfree(fs_devices);
 
 	return 0;
 }
Signature · 1 line
1.7.10.4
#1 lore raw

volumes.c static int device_list_add(const char *path, · line 160

Quoted 5 lines · Wang Sheng-Hui
volumes.c +5 -0
+static void btrfs_close_device(struct btrfs_device *device)
+{
+	close(device->fd);
+	device->fd = -1;
+	device->writeable = 0;

It's not necessary to set these variables when we're freeing the structure immediatelly.

volumes.c static int device_list_add(const char *path, · line 164

Quoted 4 lines
volumes.c +4 -0
+	if (device->name)
+		kfree(device->name);
+	kfree(device);
+}

david

#2 lore raw
In reply to Wang Sheng-Hui

further, you need to free device->label as well.
----
static int device_list_add(const char *path,
struct btrfs_super_block *disk_super,
u64 devid, struct btrfs_fs_devices **fs_devices_ret)
{
::

                device->label = kstrdup(disk_super->label, GFP_NOFS);

----

disk_super->label is never null when disk_super is not null since its inline allocation. and kstrdup does len = strlen(s) + 1; which looks like device->label is never NULL, but I havn't traced down kmalloc_track_caller until to its end

-----
22 char *kstrdup(const char *s, gfp_t gfp)
23 {
24 size_t len;
25 char *buf;
26
27 if (!s)
28 return NULL;
29

 30         len = strlen(s) + 1;
 31         buf = kmalloc_track_caller(len, gfp);
 32         if (buf)

33 memcpy(buf, s, len);
34 return buf;
35 }
----------

Thanks, Anand

Quoted 53 lines · Wang Sheng-Hui

Three kind of structures need to be freed on close:
* All struct btrfs_device managed by fs_devices
* The name field for each struct btrfs_device
* The above items for seed_devices

Signed-off-by Wang Sheng-Hui <[email protected]>
---
  volumes.c |   16 +++++++++++++---
  1 file changed, 13 insertions(+), 3 deletions(-)
volumes.c +13 -3
index d6f81f8..257b740 100644
@@ -153,6 +153,16 @@ static int device_list_add(const char *path,
      return 0;
  }
 
+static void btrfs_close_device(struct btrfs_device *device)
+{
+    close(device->fd);
+    device->fd = -1;
+    device->writeable = 0;
+    if (device->name)
+        kfree(device->name);
+    kfree(device);
+}
+
  int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
  {
      struct btrfs_fs_devices *seed_devices;

@@ -161,17 +171,17 @@ int btrfs_close_devices(struct btrfs_fs_devices
 *fs_devices)
  again:
      list_for_each(cur, &fs_devices->devices) {
          device = list_entry(cur, struct btrfs_device, dev_list);
-        close(device->fd);
-        device->fd = -1;
-        device->writeable = 0;
+        btrfs_close_device(device);
      }
 
      seed_devices = fs_devices->seed;
      fs_devices->seed = NULL;
      if (seed_devices) {
+        kfree(fs_devices);
          fs_devices = seed_devices;
          goto again;
      }
+    kfree(fs_devices);
 
      return 0;

  }
#3 lore raw

Sorry for multiple emails, however looking closely it appears
this will make btrfs_close_devices should be the last thing in the thread, which means thread can not use the list after calling btrfs_close_devices(). That would confuse.

Further not all threads using device_list_add() would call btrfs_open_devices() for eg cmd_show(), so there will still be memory leak since you can't call btrfs_close_devices() here.

So since we have device_list_add() its better to have its undo part as a separate function and not something to do within close.

Further, below patch which I submitted provided a way to delete a fsid+devices from the list. But just noticed that it missed the bug which you are addressing here and it should check if device is closed before releasing the list item.

[PATCH 09/13] btrfs-progs: function to release a specific fsid from the list

I can revamp this patch to the bug here, based feedback(s). (my new patch-set doesn't have to call device_list_fini() any more, so this patch is kind of void now).

Thanks, Anand

Quoted 98 lines · Anand Jain

further, you need to free device->label as well.
----
static int device_list_add(const char *path,
struct btrfs_super_block *disk_super, u64 devid, struct btrfs_fs_devices **fs_devices_ret)
{
::

                 device->label = kstrdup(disk_super->label, GFP_NOFS);

----

disk_super->label is never null when disk_super is not null since its inline allocation. and kstrdup does len = strlen(s) + 1; which looks like device->label is never NULL, but I havn't traced down kmalloc_track_caller until to its end

-----
22 char *kstrdup(const char *s, gfp_t gfp)
23 {
24 size_t len;
25 char *buf;
26
27 if (!s)
28 return NULL;
29

  30         len = strlen(s) + 1;
  31         buf = kmalloc_track_caller(len, gfp);
  32         if (buf)

33 memcpy(buf, s, len);
34 return buf;
35 }
----------

Thanks, Anand

Quoted 52 lines · Wang Sheng-Hui

Three kind of structures need to be freed on close:
* All struct btrfs_device managed by fs_devices
* The name field for each struct btrfs_device
* The above items for seed_devices

Signed-off-by Wang Sheng-Hui <[email protected]>
---
  volumes.c |   16 +++++++++++++---
  1 file changed, 13 insertions(+), 3 deletions(-)
volumes.c +13 -3
index d6f81f8..257b740 100644
@@ -153,6 +153,16 @@ static int device_list_add(const char *path,
      return 0;
  }
 
+static void btrfs_close_device(struct btrfs_device *device)
+{
+    close(device->fd);
+    device->fd = -1;
+    device->writeable = 0;
+    if (device->name)
+        kfree(device->name);
+    kfree(device);
+}
+
  int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
  {
      struct btrfs_fs_devices *seed_devices;

@@ -161,17 +171,17 @@ int btrfs_close_devices(struct btrfs_fs_devices
 *fs_devices)
  again:
      list_for_each(cur, &fs_devices->devices) {
          device = list_entry(cur, struct btrfs_device, dev_list);
-        close(device->fd);
-        device->fd = -1;
-        device->writeable = 0;
+        btrfs_close_device(device);
      }
 
      seed_devices = fs_devices->seed;
      fs_devices->seed = NULL;
      if (seed_devices) {
+        kfree(fs_devices);
          fs_devices = seed_devices;
          goto again;
      }
+    kfree(fs_devices);
 
      return 0;
Quoted 1 line
  }

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html