by Gabriel Krisman Bertazi

[permalink] [raw]

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

Shreeya Patel <[email protected]> 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

2021-03-24 07:23:44

by Shreeya Patel

[permalink] [raw]

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

On 19/03/21 2:35 am, Eric Biggers wrote:
> On Thu, Mar 18, 2021 at 07:03:05PM +0530, Shreeya Patel wrote:
>> +struct unicode_ops {
>> + struct module *owner;
>> + int (*validate)(const struct unicode_map *um, const struct qstr *str);
>> + int (*strncmp)(const struct unicode_map *um, const struct qstr *s1,
>> + const struct qstr *s2);
>> + int (*strncasecmp)(const struct unicode_map *um, const struct qstr *s1,
>> + const struct qstr *s2);
>> + int (*strncasecmp_folded)(const struct unicode_map *um, const struct qstr *cf,
>> + const struct qstr *s1);
>> + int (*normalize)(const struct unicode_map *um, const struct qstr *str,
>> + unsigned char *dest, size_t dlen);
>> + int (*casefold)(const struct unicode_map *um, const struct qstr *str,
>> + unsigned char *dest, size_t dlen);
>> + int (*casefold_hash)(const struct unicode_map *um, const void *salt,
>> + struct qstr *str);
>> + struct unicode_map* (*load)(const char *version);
>> +};
> Indirect calls are expensive these days, especially due to the Spectre
> mitigations. Would it make sense to use static calls
> (include/linux/static_call.h) instead for this?

Hi Eric,

I just sent a v3 with static_call implementation.

>
> - Eric
>