[PATCH] fs: affs: fix a memroy leak in affs_remount

[PATCH] fs: affs: fix a memroy leak in affs_remount

[Navid Emamdoost]

In affs_remount if data is provided it is duplicated into new_opts. But
this is not actually used later! The allocated memory for new_opts is
only released if pare_options fail. This commit adds release for
new_opts.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 fs/affs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index cc463ae47c12..1d38fdbc5148 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -598,6 +598,8 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 	memcpy(sbi->s_volume, volume, 32);
 	spin_unlock(&sbi->symlink_lock);
 
+	kfree(new_opts);
+
 	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
 		return 0;
 
-- 
2.17.1

Re: [PATCH] fs: affs: fix a memory leak in affs_remount

[Markus Elfring]

* Please avoid typos in the commit message.

* I would prefer an other wording for the change description.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=97f9a3c4eee55b0178b518ae7114a6a53372913d#n151


> But this is not actually used later!

Can this information trigger the deletion of questionable source code
instead of adding a missing function call “kfree(new_opts)”?


How do you think about to add the tag “Fixes”?

Regards,
Markus

[PATCH v2] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

In affs_remount if data is provided it is duplicated into new_opts.
The allocated memory for new_opts is only released if pare_options fail.
The release for new_opts is added.

Fixes: c8f33d0bec99 ("affs: kstrdup() memory handling")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
Changes in v2:
	-- fix a type in title, and add fixes tag
---
 fs/affs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index cc463ae47c12..1d38fdbc5148 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -598,6 +598,8 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 	memcpy(sbi->s_volume, volume, 32);
 	spin_unlock(&sbi->symlink_lock);
 
+	kfree(new_opts);
+
 	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
 		return 0;
 
-- 
2.17.1

Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[Markus Elfring]

> The allocated memory for new_opts is only released if pare_options fail.

Can the following wording be nicer?

  The allocated memory for the buffer “new_opts” will be released
  only if a call of the function “parse_options” failed.


> The release for new_opts is added.

* How do you think about the change possibility to delete questionable
  source code here?

* Would you like to complete the data processing for corresponding options
  any more?


> 	-- fix a type in title, …

Please avoid typos also in your version comments.


> ---

I suggest to replace this second delimiter by a blank line.

Regards,
Markus

Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

Hi Markus, thanks for your suggestions for improving the quality of
the patch. At the moment I prefer first get a confirmation from
contributors about the leak and then work on any possible improvements
for the patch.

Thanks,
Navid.

On Tue, Oct 1, 2019 at 3:31 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > The allocated memory for new_opts is only released if pare_options fail.
>
> Can the following wording be nicer?
>
>   The allocated memory for the buffer “new_opts” will be released
>   only if a call of the function “parse_options” failed.
>
>
> > The release for new_opts is added.
>
> * How do you think about the change possibility to delete questionable
>   source code here?
>
> * Would you like to complete the data processing for corresponding options
>   any more?
>
>
> >       -- fix a type in title, …
>
> Please avoid typos also in your version comments.
>
>
> > ---
>
> I suggest to replace this second delimiter by a blank line.
>
> Regards,
> Markus



-- 
Navid.

Re: [v2] fs: affs: fix a memory leak in affs_remount

[Markus Elfring]

> Hi Markus, thanks for your suggestions for improving the quality of
> the patch. At the moment I prefer first get a confirmation from
> contributors about the leak and then work on any possible improvements
> for the patch.

Please fix this patch as soon as possible if you care for the correctness
of the provided information.

Regards,
Markus

Re: [v2] fs: affs: fix a memory leak in affs_remount

[Markus Elfring]

> Hi Markus, thanks for your suggestions for improving the quality of
> the patch. At the moment I prefer first get a confirmation from
> contributors about the leak and then work on any possible improvements
> for the patch.

Please fix this patch as soon as possible if you care for the correctness
of the provided information.

Regards,
Markus

Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[David Sterba]

On Mon, Sep 30, 2019 at 04:01:10PM -0500, Navid Emamdoost wrote:
> In affs_remount if data is provided it is duplicated into new_opts.
> The allocated memory for new_opts is only released if pare_options fail.
> The release for new_opts is added.

A variable that is allocated and freed without use should ring a bell to
look closer at the code. There's a bit of history behind new_options,
originally there was save/replace options on the VFS layer so the 'data'
passed must not change (thus strdup), this got cleaned up in later
patches. But not completely.

There's no reason to do the strdup in cases where the filesystem does
not need to reuse the 'data' again, because strsep would modify it
directly.

So new_opts should be removed. 

Re: [PATCH v2] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

Thanks David,

On Wed, Oct 2, 2019 at 4:22 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Mon, Sep 30, 2019 at 04:01:10PM -0500, Navid Emamdoost wrote:
> > In affs_remount if data is provided it is duplicated into new_opts.
> > The allocated memory for new_opts is only released if pare_options fail.
> > The release for new_opts is added.
>
> A variable that is allocated and freed without use should ring a bell to
> look closer at the code. There's a bit of history behind new_options,
> originally there was save/replace options on the VFS layer so the 'data'
> passed must not change (thus strdup), this got cleaned up in later
> patches. But not completely.
>
> There's no reason to do the strdup in cases where the filesystem does
> not need to reuse the 'data' again, because strsep would modify it
> directly.
>
> So new_opts should be removed.

I will send a new patch with the unused variable removed.


-- 
Navid.

[PATCH v3] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

In affs_remount if data is provided it is duplicated into new_opts.
The allocated memory for new_opts is only released if pare_options fail.
But the variable is not used anywhere. So the new_opts should be
removed.

Fixes: c8f33d0bec99 ("affs: kstrdup() memory handling")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
Changes in v2:
	-- fix typo
Changes in v3:
	-- remove the call to kstrdup, as new_opts is not used anymore.
---
 fs/affs/super.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index cc463ae47c12..b6c6080d186c 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -565,10 +565,6 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 	char			 volume[32];
 	char			*prefix = NULL;
 
-	new_opts = kstrdup(data, GFP_KERNEL);
-	if (data && !new_opts)
-		return -ENOMEM;
-
 	pr_debug("%s(flags=0x%x,opts=\"%s\")\n", __func__, *flags, data);
 
 	sync_filesystem(sb);
@@ -579,7 +575,6 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 			   &blocksize, &prefix, volume,
 			   &mount_flags)) {
 		kfree(prefix);
-		kfree(new_opts);
 		return -EINVAL;
 	}
 
-- 
2.17.1

Re: [PATCH v3] fs: affs: fix a memory leak in affs_remount

[David Sterba]

On Wed, Oct 02, 2019 at 04:52:37PM -0500, Navid Emamdoost wrote:
> In affs_remount if data is provided it is duplicated into new_opts.
> The allocated memory for new_opts is only released if pare_options fail.
> But the variable is not used anywhere. So the new_opts should be
> removed.
> 
> Fixes: c8f33d0bec99 ("affs: kstrdup() memory handling")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> Changes in v2:
> 	-- fix typo
> Changes in v3:
> 	-- remove the call to kstrdup, as new_opts is not used anymore.

Added it to affs queue. There are still typos in the changelog and this
was pointed out, and v3 lacks the explanations I replied to v2.  I'll
fix it up.

Re: [PATCH] fs: affs: fix a memory leak in affs_remount

[Navid Emamdoost]

Fixed the issues in v2.

Thanks,
Navid.

On Mon, Sep 30, 2019 at 1:03 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> * Please avoid typos in the commit message.
>
> * I would prefer an other wording for the change description.
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=97f9a3c4eee55b0178b518ae7114a6a53372913d#n151
>
>
> > But this is not actually used later!
>
> Can this information trigger the deletion of questionable source code
> instead of adding a missing function call “kfree(new_opts)”?
>
>
> How do you think about to add the tag “Fixes”?
>
> Regards,
> Markus



-- 
Navid.