diff mbox series

[v4,2/5] fs: Check if utf8 encoding is loaded before calling utf8_unload()

Message ID 20210325000811.1379641-3-shreeya.patel@collabora.com (mailing list archive)
State New, archived
Headers show
Series Make UTF-8 encoding loadable | expand

Commit Message

Shreeya Patel March 25, 2021, 12:08 a.m. UTC 
utf8_unload is being called if CONFIG_UNICODE is enabled.
The ifdef block doesn't check if utf8 encoding has been loaded
or not before calling the utf8_unload() function.
This is not the expected behavior since it would sometimes lead
to unloading utf8 even before loading it.
Hence, add a condition which will check if sb->encoding is NOT NULL
before calling the utf8_unload().

Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.com>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 fs/ext4/super.c | 6 ++++--
 fs/f2fs/super.c | 9 ++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

Comments

Eric Biggers March 25, 2021, 7:21 p.m. UTC | #1 
On Thu, Mar 25, 2021 at 05:38:08AM +0530, Shreeya Patel wrote:
> utf8_unload is being called if CONFIG_UNICODE is enabled.
> The ifdef block doesn't check if utf8 encoding has been loaded
> or not before calling the utf8_unload() function.
> This is not the expected behavior since it would sometimes lead
> to unloading utf8 even before loading it.
> Hence, add a condition which will check if sb->encoding is NOT NULL
> before calling the utf8_unload().
> 
> Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>  fs/ext4/super.c | 6 ++++--
>  fs/f2fs/super.c | 9 ++++++---
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ad34a37278cd..e438d14f9a87 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1259,7 +1259,8 @@ static void ext4_put_super(struct super_block *sb)
>  	fs_put_dax(sbi->s_daxdev);
>  	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
>  #ifdef CONFIG_UNICODE
> -	utf8_unload(sb->s_encoding);
> +	if (sb->s_encoding)
> +		utf8_unload(sb->s_encoding);
>  #endif
>  	kfree(sbi);
>  }


What's the benefit of this change?  utf8_unload is a no-op when passed a NULL
pointer; why not keep it that way?

- Eric
Gabriel Krisman Bertazi March 25, 2021, 7:31 p.m. UTC | #2 
Eric Biggers <ebiggers@kernel.org> writes:

> On Thu, Mar 25, 2021 at 05:38:08AM +0530, Shreeya Patel wrote:
>> utf8_unload is being called if CONFIG_UNICODE is enabled.
>> The ifdef block doesn't check if utf8 encoding has been loaded
>> or not before calling the utf8_unload() function.
>> This is not the expected behavior since it would sometimes lead
>> to unloading utf8 even before loading it.
>> Hence, add a condition which will check if sb->encoding is NOT NULL
>> before calling the utf8_unload().
>> 
>> Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>> ---
>>  fs/ext4/super.c | 6 ++++--
>>  fs/f2fs/super.c | 9 ++++++---
>>  2 files changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index ad34a37278cd..e438d14f9a87 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1259,7 +1259,8 @@ static void ext4_put_super(struct super_block *sb)
>>  	fs_put_dax(sbi->s_daxdev);
>>  	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
>>  #ifdef CONFIG_UNICODE
>> -	utf8_unload(sb->s_encoding);
>> +	if (sb->s_encoding)
>> +		utf8_unload(sb->s_encoding);
>>  #endif
>>  	kfree(sbi);
>>  }
>
>
> What's the benefit of this change?  utf8_unload is a no-op when passed a NULL
> pointer; why not keep it that way?

For the record, it no longer is a no-op after patch 5 of this series.
Honestly, I prefer making it explicitly at the caller that we are not
entering the function, like the patch does, instead of returning from it
immediately.  Makes it more readable, IMO.
Eric Biggers March 25, 2021, 7:43 p.m. UTC | #3 
On Thu, Mar 25, 2021 at 03:31:42PM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > On Thu, Mar 25, 2021 at 05:38:08AM +0530, Shreeya Patel wrote:
> >> utf8_unload is being called if CONFIG_UNICODE is enabled.
> >> The ifdef block doesn't check if utf8 encoding has been loaded
> >> or not before calling the utf8_unload() function.
> >> This is not the expected behavior since it would sometimes lead
> >> to unloading utf8 even before loading it.
> >> Hence, add a condition which will check if sb->encoding is NOT NULL
> >> before calling the utf8_unload().
> >> 
> >> Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> >> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> >> ---
> >>  fs/ext4/super.c | 6 ++++--
> >>  fs/f2fs/super.c | 9 ++++++---
> >>  2 files changed, 10 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> >> index ad34a37278cd..e438d14f9a87 100644
> >> --- a/fs/ext4/super.c
> >> +++ b/fs/ext4/super.c
> >> @@ -1259,7 +1259,8 @@ static void ext4_put_super(struct super_block *sb)
> >>  	fs_put_dax(sbi->s_daxdev);
> >>  	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
> >>  #ifdef CONFIG_UNICODE
> >> -	utf8_unload(sb->s_encoding);
> >> +	if (sb->s_encoding)
> >> +		utf8_unload(sb->s_encoding);
> >>  #endif
> >>  	kfree(sbi);
> >>  }
> >
> >
> > What's the benefit of this change?  utf8_unload is a no-op when passed a NULL
> > pointer; why not keep it that way?
> 
> For the record, it no longer is a no-op after patch 5 of this series.
> Honestly, I prefer making it explicitly at the caller that we are not
> entering the function, like the patch does, instead of returning from it
> immediately.  Makes it more readable, IMO.
> 

I don't think making all the callers do the NULL check is more readable.  It's
conventional for free-like functions to accept NULL pointers.  See for example
every other function in the code snippet above -- fs_put_dax(),
fscrypt_free_dummy_policy(), and kfree().

This seems more like an issue with patch 5; it shouldn't be dropping the NULL
check from unicode_unload().

- Eric
diff mbox series

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ad34a37278cd..e438d14f9a87 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1259,7 +1259,8 @@  static void ext4_put_super(struct super_block *sb)
 	fs_put_dax(sbi->s_daxdev);
 	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
+	if (sb->s_encoding)
+		utf8_unload(sb->s_encoding);
 #endif
 	kfree(sbi);
 }
@@ -5165,7 +5166,8 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		crypto_free_shash(sbi->s_chksum_driver);
 
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
+	if (sb->s_encoding)
+		utf8_unload(sb->s_encoding);
 #endif
 
 #ifdef CONFIG_QUOTA
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7069793752f1..0a04983c294e 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1430,7 +1430,8 @@  static void f2fs_put_super(struct super_block *sb)
 	for (i = 0; i < NR_PAGE_TYPE; i++)
 		kvfree(sbi->write_io[i]);
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
+	if (sb->s_encoding)
+		utf8_unload(sb->s_encoding);
 #endif
 	kfree(sbi);
 }
@@ -4073,8 +4074,10 @@  static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		kvfree(sbi->write_io[i]);
 
 #ifdef CONFIG_UNICODE
-	utf8_unload(sb->s_encoding);
-	sb->s_encoding = NULL;
+	if (sb->s_encoding) {
+		utf8_unload(sb->s_encoding);
+		sb->s_encoding = NULL;
+	}
 #endif
 free_options:
 #ifdef CONFIG_QUOTA