From: Gabriel Krisman Bertazi <krisman@collabora.com>
To: Shreeya Patel <shreeya.patel@collabora.com>
Cc: jaegeuk@kernel.org, yuchao0@huawei.com, tytso@mit.edu,
adilger.kernel@dilger.ca, drosen@google.com, ebiggers@google.com,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
kernel@collabora.com, andre.almeida@collabora.com
Subject: Re: [PATCH v2 4/4] fs: unicode: Add utf8 module and a unicode layer
Date: Fri, 19 Mar 2021 09:34:44 -0400 [thread overview]
Message-ID: <877dm3i7wr.fsf@collabora.com> (raw)
In-Reply-To: <fcd0f413-0ae9-db25-0e0d-8d48e24f3ce6@collabora.com> (Shreeya Patel's message of "Fri, 19 Mar 2021 15:56:59 +0530")
Shreeya Patel <shreeya.patel@collabora.com> writes:
> On 19/03/21 1:27 am, Gabriel Krisman Bertazi wrote:
>> Maybe, the if leg should be:
>>
>> if (!utf8_ops || !try_module_get(utf8_ops->owner)
>> return ERR_PTR(-ENODEV)
>>
>> But this is still racy, since you are not protecting utf8_ops before
>> acquiring the reference. If you race with module removal here, a
>> NULL ptr dereference can still occur. See below.
>
>
> If module is removed before reaching this step, then unicode_unregister
> function would make utf8_ops NULL. So the first condition of if will be true
> and it will return error so how can we have a NULL ptr dereference
> then?
Hi Shreeya,
As we discussed offline, it can happen if the module is deregistered
after checking utf8_ops and before doing the try_module_get.
>>> }
>>> -EXPORT_SYMBOL(unicode_normalize);
>>> +EXPORT_SYMBOL(unicode_load);
>>> -static int unicode_parse_version(const char *version, unsigned int
>>> *maj,
>>> - unsigned int *min, unsigned int *rev)
>>> +void unicode_unload(struct unicode_map *um)
>>> {
>>> - substring_t args[3];
>>> - char version_string[12];
>>> - static const struct match_token token[] = {
>>> - {1, "%d.%d.%d"},
>>> - {0, NULL}
>>> - };
>>> -
>>> - strscpy(version_string, version, sizeof(version_string));
>>> -
>>> - if (match_token(version_string, token, args) != 1)
>>> - return -EINVAL;
>>> + if (utf8_ops)
>>> + module_put(utf8_ops->owner);
>>>
>> How can we have a unicode_map to free if utf8_ops is NULL? that seems
>> to be an invalid use of API, which suggests a bug elsewhere
>> in the kernel. maybe this should read like this:
>>
>> void unicode_unload(struct unicode_map *um)
>> {
>> if (WARN_ON(!utf8_ops))
>> return;
>>
>> module_put(utf8_ops->owner);
>> kfree(um);
>> }
>
>
> The reason for adding the check if(utf8_ops) is that some of the filesystem
> calls the unicode_unload function even before calling the unicode_load
> function.
> if we try to decrement the reference without even having the
> reference. ( i.e. not loading the module )
> it would result in kernel panic.
> fs/ext4/super.c
> fs/f2fs/super.c
> Both the above files call the unicode_unload function if CONFIG_UNICODE
> is enabled.
> Not sure if this is an odd behavior or expected.
Those seem to be error paths, where the mount fails before we get a
chance to load the unicode map. I suggest we fix the callers to avoid
calling the unicode API unnecessarily.
--
Gabriel Krisman Bertazi
next prev parent reply other threads:[~2021-03-19 13:35 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-18 13:33 [PATCH v2 0/4] Make UTF-8 encoding loadable Shreeya Patel
2021-03-18 13:33 ` [PATCH v2 1/4] fs: unicode: Rename function names from utf8 to unicode Shreeya Patel
2021-03-18 13:33 ` [PATCH v2 2/4] fs: unicode: Rename utf8-core file to unicode-core Shreeya Patel
2021-03-18 13:33 ` [PATCH v2 3/4] fs: unicode: Use strscpy() instead of strncpy() Shreeya Patel
2021-03-18 14:13 ` Shreeya Patel
2021-03-18 15:40 ` David Laight
2021-03-18 21:03 ` Eric Biggers
2021-03-19 10:32 ` Shreeya Patel
2021-03-18 13:33 ` [PATCH v2 4/4] fs: unicode: Add utf8 module and a unicode layer Shreeya Patel
2021-03-18 19:57 ` Gabriel Krisman Bertazi
2021-03-19 10:26 ` Shreeya Patel
2021-03-19 13:34 ` Gabriel Krisman Bertazi [this message]
2021-03-18 21:05 ` Eric Biggers
2021-03-23 19:20 ` Shreeya Patel
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=877dm3i7wr.fsf@collabora.com \
--to=krisman@collabora.com \
--cc=adilger.kernel@dilger.ca \
--cc=andre.almeida@collabora.com \
--cc=drosen@google.com \
--cc=ebiggers@google.com \
--cc=jaegeuk@kernel.org \
--cc=kernel@collabora.com \
--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 NNTP newsgroup(s).