lkml.org 
[lkml]   [2021]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 4/4] fs: unicode: Add utf8 module and a unicode layer
On Thu, Apr 01, 2021 at 02:37:51AM +0530, Shreeya Patel wrote:
> +# utf8data.h_shipped has a large database table which is an auto-generated
> +# decodification trie for the unicode normalization functions and it is not
> +# necessary to carry this large table in the kernel.
> +# Enabling UNICODE_UTF8 option will allow UTF-8 encoding to be built as a
> +# module and this module will be loaded by the unicode subsystem layer only
> +# when any filesystem needs it.
> +config UNICODE_UTF8
> + tristate "UTF-8 module"
> help
> Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding
> support.

Please update this help text to properly describe this option, especially the
consequences of setting it to 'm'.

> + select UNICODE

'select' should go before 'help'.

> struct unicode_map *unicode_load(const char *version)
> {
> + try_then_request_module(utf8mod, "utf8");
> + if (!utf8mod) {
> + pr_err("Failed to load UTF-8 module\n");
> + return ERR_PTR(-ENODEV);
> }
>
> + spin_lock(&utf8mod_lock);
> + if (!utf8mod || !try_module_get(utf8mod)) {
> + spin_unlock(&utf8mod_lock);
> + return ERR_PTR(-ENODEV);
> + }
> + spin_unlock(&utf8mod_lock);
> + return static_call(unicode_load_static_call)(version);
> }
> EXPORT_SYMBOL(unicode_load);
>
> void unicode_unload(struct unicode_map *um)
> {
> kfree(um);
> +
> + spin_lock(&utf8mod_lock);
> + if (utf8mod)
> + module_put(utf8mod);
> + spin_unlock(&utf8mod_lock);
> +
> }
> EXPORT_SYMBOL(unicode_unload);
>
> +void unicode_register(struct module *owner)
> +{
> + utf8mod = owner;
> +}
> +EXPORT_SYMBOL(unicode_register);
> +
> +void unicode_unregister(void)
> +{
> + spin_lock(&utf8mod_lock);
> + utf8mod = NULL;
> + spin_unlock(&utf8mod_lock);
> +}
> +EXPORT_SYMBOL(unicode_unregister);


This all looks very broken. First, when !CONFIG_MODULES, utf8mod will always be
NULL so unicode_load() will always fail.

Also, if the unicode_load_static_call() fails, a reference to the utf8mod will
be leaked.

Also, unicode_unload() can put a reference to the utf8mod that was never
acquired.

Also there is a data race on utf8mod because the accesses to it aren't properly
synchronized.

Please consider something like the following, which I think would address all
these bugs:

static bool utf8mod_get(void)
{
bool ret;

spin_lock(&utf8mod_lock);
ret = utf8mod_loaded && try_module_get(utf8mod);
spin_unlock(&utf8mod_lock);
return ret;
}

struct unicode_map *unicode_load(const char *version)
{
struct unicode_map *um;

if (!try_then_request_module(utf8mod_get(), "utf8")) {
pr_err("Failed to load UTF-8 module\n");
return ERR_PTR(-ENODEV);
}
um = static_call(unicode_load_static_call)(version);
if (IS_ERR(um))
module_put(utf8mod);
return um;
}
EXPORT_SYMBOL(unicode_load);

void unicode_unload(struct unicode_map *um)
{
if (um) {
kfree(um);
module_put(utf8mod);
}
}
EXPORT_SYMBOL(unicode_unload);

void unicode_register(struct module *owner)
{
spin_lock(&utf8mod_lock);
utf8mod = owner; /* note: will be NULL if !CONFIG_MODULES */
utf8mod_loaded = true;
spin_unlock(&utf8mod_lock);
}
EXPORT_SYMBOL(unicode_register);

void unicode_unregister(void)
{
spin_lock(&utf8mod_lock);
utf8mod = NULL;
utf8mod_loaded = false;
spin_unlock(&utf8mod_lock);
}
EXPORT_SYMBOL(unicode_unregister);

\
 
 \ /
  Last update: 2021-04-01 22:55    [W:0.058 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site