lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 24 Mar 2021 03:48:10 +0530 From: Shreeya Patel <shreeya.patel@...labora.com> To: Eric Biggers <ebiggers@...nel.org>, Gabriel Krisman Bertazi <krisman@...labora.com> Cc: tytso@....edu, adilger.kernel@...ger.ca, jaegeuk@...nel.org, chao@...nel.org, drosen@...gle.com, yuchao0@...wei.com, linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net, linux-fsdevel@...r.kernel.org, kernel@...labora.com, andre.almeida@...labora.com Subject: Re: [PATCH v3 5/5] fs: unicode: Add utf8 module and a unicode layer On 24/03/21 1:59 am, Eric Biggers wrote: > On Tue, Mar 23, 2021 at 03:51:44PM -0400, Gabriel Krisman Bertazi wrote: >>> -int unicode_validate(const struct unicode_map *um, const struct qstr *str) >>> -{ >>> - const struct utf8data *data = utf8nfdi(um->version); >>> - >>> - if (utf8nlen(data, str->name, str->len) < 0) >>> - return -1; >>> - return 0; >>> -} >>> +struct unicode_ops *utf8_ops; >>> +EXPORT_SYMBOL(utf8_ops); >>> + >>> +int _utf8_validate(const struct unicode_map *um, const struct qstr *str) >>> +{ >>> + return 0; >>> +} >>> -EXPORT_SYMBOL(unicode_validate); >> I think that any calls to the default static calls should return errors >> instead of succeeding without doing anything. >> >> In fact, are the default calls really necessary? If someone gets here, >> there is a bug elsewhere, so WARN_ON and maybe -EIO. >> >> int unicode_validate_default_static_call(...) >> { >> WARN_ON(1); >> return -EIO; >> } >> >> Or just have a NULL default, as I mentioned below, if that is possible. >> > [...] >>> +DEFINE_STATIC_CALL(utf8_validate, _utf8_validate); >>> +DEFINE_STATIC_CALL(utf8_strncmp, _utf8_strncmp); >>> +DEFINE_STATIC_CALL(utf8_strncasecmp, _utf8_strncasecmp); >>> +DEFINE_STATIC_CALL(utf8_strncasecmp_folded, _utf8_strncasecmp_folded); >>> +DEFINE_STATIC_CALL(utf8_normalize, _utf8_normalize); >>> +DEFINE_STATIC_CALL(utf8_casefold, _utf8_casefold); >>> +DEFINE_STATIC_CALL(utf8_casefold_hash, _utf8_casefold_hash); >>> +DEFINE_STATIC_CALL(utf8_load, _utf8_load); >>> +DEFINE_STATIC_CALL_NULL(utf8_unload, _utf8_unload); >>> +EXPORT_STATIC_CALL(utf8_strncmp); >>> +EXPORT_STATIC_CALL(utf8_strncasecmp); >>> +EXPORT_STATIC_CALL(utf8_strncasecmp_folded); >> I'm having a hard time understanding why some use >> DEFINE_STATIC_CALL_NULL, while other use DEFINE_STATIC_CALL. This new >> static call API is new to me :). None of this can be called if the >> module is not loaded anyway, so perhaps the default function can just be >> NULL, per the documentation of include/linux/static_call.h? >> >> Anyway, Aren't utf8_{validate,casefold,normalize} missing the >> equivalent EXPORT_STATIC_CALL? >> > The static_call API is fairly new to me too. But the intent of this patch seems > to be that none of the utf8 functions are called without the utf8 module loaded. > If they are called, it's a kernel bug. So there are two options for what to do > if it happens anyway: > > 1. call a "null" static call, which does nothing > > *or* > > 2. call a default function which does WARN_ON_ONCE() and returns an error if > possible. > > (or 3. don't use static calls and instead dereference a NULL utf8_ops like > previous versions of this patch did.) > > It shouldn't really matter which of these approaches you take, but please be > consistent and use the same one everywhere. > >> + void unicode_unregister(void) >> + { >> + spin_lock(&utf8ops_lock); >> + utf8_ops = NULL; >> + spin_unlock(&utf8ops_lock); >> + } >> + EXPORT_SYMBOL(unicode_unregister); > This should restore the static calls to their default values (either NULL or the > default functions, depending on what you decide). > > Also, it's weird to still have the utf8_ops structure when using static calls. > It seems it should be one way or the other: static calls *or* utf8_ops. > > The static calls could be exported, and the module could be responsible for > updating them. That would eliminate the need for utf8_ops. Hmmm yes, I think we are just using utf8_ops for getting the owner details which we can now remove and instead pass it as an argument while registering the module. Will make this change in v4. Thanks > > - Eric
Powered by blists - more mailing lists