Btrfs devel - Jun 2013 - [PATCH] 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 <shhuiw@gmail.com>
---
  volumes.c |   16 +++++++++++++---
  1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/volumes.c b/volumes.c
index d6f81f8..257b740 100644
--- a/volumes.c
+++ b/volumes.c
@@ -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;
  }
-- 
1.7.10.4

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

On Tue, Jun 25, 2013 at 09:02:13PM +0800, Wang Sheng-Hui
wrote:
> +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.
> +	if (device->name)
> +		kfree(device->name);
> +	kfree(device);
> +}
david
--
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

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



On 06/25/2013 09:02 PM, Wang Sheng-Hui wrote:
> 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 <shhuiw@gmail.com>
> ---
>   volumes.c |   16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/volumes.c b/volumes.c
> index d6f81f8..257b740 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -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;
>   }
--
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

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

On 07/03/2013 01:01 PM, Anand Jain wrote:
>
>
>
>   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
>
>
>
> On 06/25/2013 09:02 PM, Wang Sheng-Hui wrote:
>> 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 <shhuiw@gmail.com>
>> ---
>>   volumes.c |   16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/volumes.c b/volumes.c
>> index d6f81f8..257b740 100644
>> --- a/volumes.c
>> +++ b/volumes.c
>> @@ -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;
>>   }
> --
> 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
--
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