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

[Eric Biggers <ebiggers@xxxxxxxxxx>]

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