From: Eric Biggers <ebiggers@kernel.org>
To: Shreeya Patel <shreeya.patel@collabora.com>
Cc: tytso@mit.edu, adilger.kernel@dilger.ca, jaegeuk@kernel.org,
chao@kernel.org, krisman@collabora.com, drosen@google.com,
yuchao0@huawei.com, linux-ext4@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, kernel@collabora.com,
andre.almeida@collabora.com
Subject: Re: [PATCH v6 4/4] fs: unicode: Add utf8 module and a unicode layer
Date: Thu, 1 Apr 2021 13:53:27 -0700 [thread overview]
Message-ID: <YGYyxzLu2gmO5CCk@gmail.com> (raw)
In-Reply-To: <20210331210751.281645-5-shreeya.patel@collabora.com>
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);
prev parent reply other threads:[~2021-04-01 20:54 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-31 21:07 [PATCH v6 0/4] Make UTF-8 encoding loadable Shreeya Patel
2021-03-31 21:07 ` [PATCH v6 1/4] fs: unicode: Use strscpy() instead of strncpy() Shreeya Patel
2021-03-31 21:07 ` [PATCH v6 2/4] fs: unicode: Rename function names from utf8 to unicode Shreeya Patel
2021-03-31 21:07 ` [PATCH v6 3/4] fs: unicode: Rename utf8-core file to unicode-core Shreeya Patel
2021-03-31 21:07 ` [PATCH v6 4/4] fs: unicode: Add utf8 module and a unicode layer Shreeya Patel
2021-04-01 20:53 ` Eric Biggers [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YGYyxzLu2gmO5CCk@gmail.com \
--to=ebiggers@kernel.org \
--cc=adilger.kernel@dilger.ca \
--cc=andre.almeida@collabora.com \
--cc=chao@kernel.org \
--cc=drosen@google.com \
--cc=jaegeuk@kernel.org \
--cc=kernel@collabora.com \
--cc=krisman@collabora.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=shreeya.patel@collabora.com \
--cc=tytso@mit.edu \
--cc=yuchao0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).