Re: [PATCH v6 4/4] fs: unicode: Add utf8 module and a unicode layer

[Eric Biggers]

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