Message ID | 20210325000811.1379641-3-shreeya.patel@collabora.com |
---|---|
State | Superseded |
Headers | show |
Series | Make UTF-8 encoding loadable | expand |
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
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.
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 --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