Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Saturday 28 February 2015 10:58 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 12:35 PM, Harish Jenny Kandiga Nagaraj
> wrote:
>> On Thursday 19 February 2015 07:32 PM, Harish Jenny Kandiga Nagaraj wrote:
>>> On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
>>>> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
>>> diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
>>> index 417f232..f6ffd3e 100644
>>> --- a/libkmod/libkmod-internal.h
>>> +++ b/libkmod/libkmod-internal.h
>>> @@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
>>> # endif
>>> #endif
>>>
>>> +/* Flags to kmod_builtin_status() */
>>> +enum kmod_builtin_status {
>>> +KMOD_BUILTIN_UNKNOWN = 0,
>>> +KMOD_BUILTIN_NO = 1,
>>> +KMOD_BUILTIN_YES = 2
>>> +};
>>> +
>>> void kmod_log(const struct kmod_ctx *ctx,
>>> int priority, const char *file, int line, const char *fn,
>>> const char *format, ...) __attribute__((format(printf, 6, 7)))
>>> __attribute__((nonnull(1, 3, 5)));
>>> @@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx
>>> *ctx, const char *name,
>>> int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char
>>> *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>> int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char
>>> *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>> int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char
>>> *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>> +int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const
>>> char *name)__attribute__((nonnull(1, 2)));
>>> int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char
>>> *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>> void kmod_set_modules_visited(struct kmod_ctx *ctx, bool visited)
>>> __attribute__((nonnull((1;
>>> void kmod_set_modules_required(struct kmod_ctx *ctx, bool required)
>>> __attribute__((nonnull((1;
>>> @@ -143,9 +151,9 @@ int kmod_module_parse_depline(struct kmod_module *mod,
>>> char *line) __attribute__
>>> void kmod_module_set_install_commands(struct kmod_module *mod, const char
>>> *cmd) __attribute__((nonnull(1)));
>>> void kmod_module_set_remove_commands(struct kmod_module *mod, const char
>>> *cmd) __attribute__((nonnull(1)));
>>> void kmod_module_set_visited(struct kmod_module *mod, bool visited)
>>> __attribute__((nonnull(1)));
>>> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>>> __attribute__((nonnull((1;
>>> +void kmod_module_set_builtin(struct kmod_module *mod, enum
>>> kmod_builtin_status builtin) __attribute__((nonnull((1;
>>> void kmod_module_set_required(struct kmod_module *mod, bool required)
>>> __attribute__((nonnull(1)));
>>> -
>>> +bool kmod_module_is_builtin(const struct kmod_module *mod);
>>>
>>> /* libkmod-file.c */
>>> struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char
>>> *filename) _must_check_ __attribute__((nonnull(1,2)));
>>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>>> index 19bb2ed..6b2f2f1 100644
>>> --- a/libkmod/libkmod-module.c
>>> +++ b/libkmod/libkmod-module.c
>>> @@ -98,7 +98,7 @@ struct kmod_module {
>>> * is set. There's nothing much useful one can do with such a
>>> * "module", except knowing it's builtin.
>>> */
>>> -bool builtin : 1;
>>> +enum kmod_builtin_status builtin;
>>> };
>>>
>>> static inline const char *path_join(const char *path, size_t prefixlen,
>>> @@ -210,7 +210,7 @@ void kmod_module_set_visited(struct kmod_module *mod,
>>> bool visited)
>>> mod->visited = visited;
>>> }
>>>
>>> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>>> +void kmod_module_set_builtin(struct kmod_module *mod, enum
>>> kmod_builtin_status builtin)
>>> {
>>> mod->builtin = builtin;
>>> }
>>> @@ -220,6 +220,15 @@ void kmod_module_set_required(struct kmod_module *mod,
>>> bool required)
>>> mod->required = required;
>>> }
>>>
>>> +bool
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Saturday 28 February 2015 10:58 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 12:35 PM, Harish Jenny Kandiga Nagaraj
harish_kand...@mentor.com wrote:
On Thursday 19 February 2015 07:32 PM, Harish Jenny Kandiga Nagaraj wrote:
On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 417f232..f6ffd3e 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
# endif
#endif
+/* Flags to kmod_builtin_status() */
+enum kmod_builtin_status {
+KMOD_BUILTIN_UNKNOWN = 0,
+KMOD_BUILTIN_NO = 1,
+KMOD_BUILTIN_YES = 2
+};
+
void kmod_log(const struct kmod_ctx *ctx,
int priority, const char *file, int line, const char *fn,
const char *format, ...) __attribute__((format(printf, 6, 7)))
__attribute__((nonnull(1, 3, 5)));
@@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx
*ctx, const char *name,
int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
+int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const
char *name)__attribute__((nonnull(1, 2)));
int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
void kmod_set_modules_visited(struct kmod_ctx *ctx, bool visited)
__attribute__((nonnull((1;
void kmod_set_modules_required(struct kmod_ctx *ctx, bool required)
__attribute__((nonnull((1;
@@ -143,9 +151,9 @@ int kmod_module_parse_depline(struct kmod_module *mod,
char *line) __attribute__
void kmod_module_set_install_commands(struct kmod_module *mod, const char
*cmd) __attribute__((nonnull(1)));
void kmod_module_set_remove_commands(struct kmod_module *mod, const char
*cmd) __attribute__((nonnull(1)));
void kmod_module_set_visited(struct kmod_module *mod, bool visited)
__attribute__((nonnull(1)));
-void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
__attribute__((nonnull((1;
+void kmod_module_set_builtin(struct kmod_module *mod, enum
kmod_builtin_status builtin) __attribute__((nonnull((1;
void kmod_module_set_required(struct kmod_module *mod, bool required)
__attribute__((nonnull(1)));
-
+bool kmod_module_is_builtin(const struct kmod_module *mod);
/* libkmod-file.c */
struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char
*filename) _must_check_ __attribute__((nonnull(1,2)));
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..6b2f2f1 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -98,7 +98,7 @@ struct kmod_module {
* is set. There's nothing much useful one can do with such a
* module, except knowing it's builtin.
*/
-bool builtin : 1;
+enum kmod_builtin_status builtin;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -210,7 +210,7 @@ void kmod_module_set_visited(struct kmod_module *mod,
bool visited)
mod-visited = visited;
}
-void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
+void kmod_module_set_builtin(struct kmod_module *mod, enum
kmod_builtin_status builtin)
{
mod-builtin = builtin;
}
@@ -220,6 +220,15 @@ void kmod_module_set_required(struct kmod_module *mod,
bool required)
mod-required = required;
}
+bool kmod_module_is_builtin(const struct kmod_module *mod)
+{
+int builtin = mod-builtin;
+
+if (builtin == KMOD_BUILTIN_UNKNOWN)
+builtin = kmod_just_lookup_alias_from_builtin_file(mod-ctx,
mod-name);
+
+return mod-builtin == KMOD_BUILTIN_YES;
+}
/*
* Memory layout with alias:
*
@@ -924,7 +933,7 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct
kmod_ctx *ctx,
module_is_blacklisted(mod))
continue;
-if ((filter_type KMOD_FILTER_BUILTIN) mod-builtin)
+if ((filter_type KMOD_FILTER_BUILTIN)
kmod_module_is_builtin(mod))
continue;
node = kmod_list_append(*output, mod);
@@ -1713,7 +1722,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const
struct kmod_module *mod)
if (mod == NULL)
return -ENOENT;
-if (mod-builtin)
+if (kmod_module_is_builtin(mod))
return KMOD_MODULE_BUILTIN;
pathlen = snprintf(path, sizeof(path),
diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 1a5a66b..d79bb12 100644
Re: Differences between builtins and modules
On Monday 23 February 2015 09:21 PM, Michal Marek wrote: > On 2015-02-23 15:30, Lucas De Marchi wrote: >> This could be particularly bad if in a kernel version an option was >> tristate and in a new version it changed to boolean. I'm not sure if >> this is common to happen in kernel. Any code that did "modprobe >> " would start to fail. > I think it's quite uncommon (*) and also the use case for loading > builtin modules is not that common. I can think of: > 1) building the initramfs, to determine which *.ko files need to be >copied to it. Since such tools are often updated for other reasons, >it's not a big deal. > 2) Hardcoded module names in things like softdep -- hopefully not that >common either, plus the kernel-provided soft dependencies can be >fixed together with the change. > > Until not so long ago, the kernel would return EINVAL if passed a > non-existent (renamed, removed) module option to init_module, yet there > were no attempts at preserving the module options for compatibility reasons. > > (*) I now did a quick search: > $ git log -p origin/master --no-merges -- '*/Kconfig*' | grep -C3 '^- > *tristate' | grep '^+ *bool' > + bool "Intel P state control" > + bool "Intel microcode patch loading support" > + bool "AMD microcode patch loading support" > +bool "STI text console" > + bool "Enable DDC2 Support" > + bool "Enable Console Acceleration" > > That's only 6 cases in the whole git history. Maybe there are a few more > hidden outside the three-line context as part of larger edits, but I'm > sure more modules have been *removed* entirely from the kernel over this > period. > > >> My questions are: >> 1) should we put *all* the "modules" in the builtin index? > You mean all *.o files that do not end up in some *.ko? That won't work, > because unlike module names, the names of object files are not global. > Plus, there was IIRC an idea to teach lsmod to print builtin modules -- > listing all *.o would make it rather useless. > > >> 2) should we actually check /sys/module/ to report a >> module as builtin or just stop doing that and rely solely in the >> index? Initially I'd like to do the opposite, but given the race in >> deciding this I'm favoring the index. > If the race between the creation of /sys/module/ and > /sys/module//initstate is inevitable, then I'm afraid we > have to rely on the index. > > Michal Can we add some flag like KMOD_PROBE_FORCE_DIRECTORY_CHECK = 0x00040, and pass it to kmod_module_get_initstate to make "modprobe --show-depends vt" to report as "builtin" ? Also if the use case for loading builtin modules is not that common ( Also don’t know if 'modprobe vt' command does the loading if not loaded) can we have the same flags be used after checking if it is .ko file or from .o file if required? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Differences between builtins and modules
On Monday 23 February 2015 09:21 PM, Michal Marek wrote: On 2015-02-23 15:30, Lucas De Marchi wrote: This could be particularly bad if in a kernel version an option was tristate and in a new version it changed to boolean. I'm not sure if this is common to happen in kernel. Any code that did modprobe module would start to fail. I think it's quite uncommon (*) and also the use case for loading builtin modules is not that common. I can think of: 1) building the initramfs, to determine which *.ko files need to be copied to it. Since such tools are often updated for other reasons, it's not a big deal. 2) Hardcoded module names in things like softdep -- hopefully not that common either, plus the kernel-provided soft dependencies can be fixed together with the change. Until not so long ago, the kernel would return EINVAL if passed a non-existent (renamed, removed) module option to init_module, yet there were no attempts at preserving the module options for compatibility reasons. (*) I now did a quick search: $ git log -p origin/master --no-merges -- '*/Kconfig*' | grep -C3 '^- *tristate' | grep '^+ *bool' + bool Intel P state control + bool Intel microcode patch loading support + bool AMD microcode patch loading support +bool STI text console + bool Enable DDC2 Support + bool Enable Console Acceleration That's only 6 cases in the whole git history. Maybe there are a few more hidden outside the three-line context as part of larger edits, but I'm sure more modules have been *removed* entirely from the kernel over this period. My questions are: 1) should we put *all* the modules in the builtin index? You mean all *.o files that do not end up in some *.ko? That won't work, because unlike module names, the names of object files are not global. Plus, there was IIRC an idea to teach lsmod to print builtin modules -- listing all *.o would make it rather useless. 2) should we actually check /sys/module/modulename to report a module as builtin or just stop doing that and rely solely in the index? Initially I'd like to do the opposite, but given the race in deciding this I'm favoring the index. If the race between the creation of /sys/module/modulename and /sys/module/modulename/initstate is inevitable, then I'm afraid we have to rely on the index. Michal Can we add some flag like KMOD_PROBE_FORCE_DIRECTORY_CHECK = 0x00040, and pass it to kmod_module_get_initstate to make modprobe --show-depends vt to report as builtin ? Also if the use case for loading builtin modules is not that common ( Also don’t know if 'modprobe vt' command does the loading if not loaded) can we have the same flags be used after checking if it is .ko file or from .o file if required? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 07:32 PM, Harish Jenny Kandiga Nagaraj wrote:
> On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
>> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
>> wrote:
>>> On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
>>>> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
>>>> wrote:
>>>>>> Harrish, in your patch if you just change the "return
>>>>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>>>>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works.
>>>>> The built-in modules are handled by looking at the modules.builtin index
>>>>> file. Is there any chance of returning KMOD_MODULE_COMING for builti-in
>>>>> modules? If it does not have any impact, then the fix should be fine.
>>>> well... you're not returning KMOD_MODULE_COMING for a builtin module.
>>>> Having the directory /sys/module/ and not the initstate could be
>>>> either that the module is builtin or that there's a race while loading
>>>> the module and it's in the coming state. However since we use the
>>>> index to decide if this module is builtin in the beginning of this
>>>> function, here it can only be the second case.
>>>>
>>>> However... mod->builtin in the beginning of this function is only set
>>>> if the module is created by a lookup rather than from name or from
>>>> path maybe here we need to actually fallback to the index rather
>>>> than the cached value, otherwise this test would fail (considering
>>>> "vt" is builtin):
>>>>
>>>> kmod_module_new_from_name(ctx, "vt", );
>>>> kmod_module_get_initstate(mod, );
>>>>
>>>
>>> something like this ?
>>>
>>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>>> index 19bb2ed..d424f3e 100644
>>> --- a/libkmod/libkmod-module.c
>>> +++ b/libkmod/libkmod-module.c
>>> @@ -99,6 +99,8 @@ struct kmod_module {
>>> * "module", except knowing it's builtin.
>>> */
>>> bool builtin : 1;
>>> +
>>> + bool lookup : 1;
>>> };
>>>
>>> static inline const char *path_join(const char *path, size_t prefixlen,
>>> @@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod,
>>> bool builtin)
>>> mod->builtin = builtin;
>>> }
>>>
>>> +void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
>>> +{
>>> + mod->lookup = lookup;
>>> +}
>>> +
>>> void kmod_module_set_required(struct kmod_module *mod, bool required)
>>> {
>>> mod->required = required;
>>> @@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const
>>> struct kmod_module *mod)
>>> struct stat st;
>>> path[pathlen - (sizeof("/initstate") - 1)] = '\0';
>>> if (stat(path, ) == 0 && S_ISDIR(st.st_mode))
>>> - return KMOD_MODULE_COMING;
>>> + return mod->lookup ? KMOD_MODULE_COMING :
>>> KMOD_MODULE_BUILTIN;
>> no. I guess this doesn't pass the proposed test:
>>
>> 1)
>> kmod_module_new_from_name(ctx, "vt", );
>> kmod_module_get_initstate(mod, );
>>
>> this must return builtin
>>
>> 2)
>> kmod_module_new_from_lookup(ctx, "vt", );
>> ... (get mod from list)
>> kmod_module_get_initstate(mod, );
>>
>> this must return builtin as well.
>>
>> I suggest you add a kmod_module_is_builtin() which does the lookup
>> (but doesn't increase the module refcount, i.e. doesn't call
>> new_module_()) iff it's not already done. For this you will need
>> to change mod->builtin to an enum: enum { _UNKNOWN, _NO,
>> _YES } then you do:
>>
>> bool kmod_module_is_builtin(mod) (don't export this function)
>> {
>> if (mod->_UNKNOWN) {
>> ... lookup in builtin index
>> }
>> return mod->builtin == _YES;
>> }
>>
>> then you change the users of mod->builtin.
>>
>
> something like this ?
>
>
> diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
> inde
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
> wrote:
>> On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
>>> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
>>> wrote:
>>>>> Harrish, in your patch if you just change the "return
>>>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>>>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works.
>>>> The built-in modules are handled by looking at the modules.builtin index
>>>> file. Is there any chance of returning KMOD_MODULE_COMING for builti-in
>>>> modules? If it does not have any impact, then the fix should be fine.
>>> well... you're not returning KMOD_MODULE_COMING for a builtin module.
>>> Having the directory /sys/module/ and not the initstate could be
>>> either that the module is builtin or that there's a race while loading
>>> the module and it's in the coming state. However since we use the
>>> index to decide if this module is builtin in the beginning of this
>>> function, here it can only be the second case.
>>>
>>> However... mod->builtin in the beginning of this function is only set
>>> if the module is created by a lookup rather than from name or from
>>> path maybe here we need to actually fallback to the index rather
>>> than the cached value, otherwise this test would fail (considering
>>> "vt" is builtin):
>>>
>>> kmod_module_new_from_name(ctx, "vt", );
>>> kmod_module_get_initstate(mod, );
>>>
>>
>>
>> something like this ?
>>
>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> index 19bb2ed..d424f3e 100644
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -99,6 +99,8 @@ struct kmod_module {
>> * "module", except knowing it's builtin.
>> */
>> bool builtin : 1;
>> +
>> + bool lookup : 1;
>> };
>>
>> static inline const char *path_join(const char *path, size_t prefixlen,
>> @@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod,
>> bool builtin)
>> mod->builtin = builtin;
>> }
>>
>> +void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
>> +{
>> + mod->lookup = lookup;
>> +}
>> +
>> void kmod_module_set_required(struct kmod_module *mod, bool required)
>> {
>> mod->required = required;
>> @@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct
>> kmod_module *mod)
>> struct stat st;
>> path[pathlen - (sizeof("/initstate") - 1)] = '\0';
>> if (stat(path, ) == 0 && S_ISDIR(st.st_mode))
>> - return KMOD_MODULE_COMING;
>> + return mod->lookup ? KMOD_MODULE_COMING :
>> KMOD_MODULE_BUILTIN;
> no. I guess this doesn't pass the proposed test:
>
> 1)
> kmod_module_new_from_name(ctx, "vt", );
> kmod_module_get_initstate(mod, );
>
> this must return builtin
>
> 2)
> kmod_module_new_from_lookup(ctx, "vt", );
> ... (get mod from list)
> kmod_module_get_initstate(mod, );
>
> this must return builtin as well.
>
> I suggest you add a kmod_module_is_builtin() which does the lookup
> (but doesn't increase the module refcount, i.e. doesn't call
> new_module_()) iff it's not already done. For this you will need
> to change mod->builtin to an enum: enum { _UNKNOWN, _NO,
> _YES } then you do:
>
> bool kmod_module_is_builtin(mod) (don't export this function)
> {
> if (mod->_UNKNOWN) {
> ... lookup in builtin index
> }
> return mod->builtin == _YES;
> }
>
> then you change the users of mod->builtin.
>
something like this ?
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 417f232..f6ffd3e 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
# endif
#endif
+/* Flags to kmod_builtin_status() */
+enum kmod_builtin_status {
+KMOD_BUILTIN_UNKNOWN = 0,
+KMOD_BUILTIN_NO = 1,
+KMOD_BUILTIN_YES = 2
+};
+
void kmod_log(const struct kmod_ctx *ctx,
int priority, const char *file, int line, const char *fn,
const char *format, ...) __attribute__((forma
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
> wrote:
>>> Harrish, in your patch if you just change the "return
>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works. The
>> built-in modules are handled by looking at the modules.builtin index file.
>> Is there any chance of returning KMOD_MODULE_COMING for builti-in modules?
>> If it does not have any impact, then the fix should be fine.
> well... you're not returning KMOD_MODULE_COMING for a builtin module.
> Having the directory /sys/module/ and not the initstate could be
> either that the module is builtin or that there's a race while loading
> the module and it's in the coming state. However since we use the
> index to decide if this module is builtin in the beginning of this
> function, here it can only be the second case.
>
> However... mod->builtin in the beginning of this function is only set
> if the module is created by a lookup rather than from name or from
> path maybe here we need to actually fallback to the index rather
> than the cached value, otherwise this test would fail (considering
> "vt" is builtin):
>
> kmod_module_new_from_name(ctx, "vt", );
> kmod_module_get_initstate(mod, );
>
something like this ?
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
* "module", except knowing it's builtin.
*/
bool builtin : 1;
+
+ bool lookup : 1;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool
builtin)
mod->builtin = builtin;
}
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+ mod->lookup = lookup;
+}
+
void kmod_module_set_required(struct kmod_module *mod, bool required)
{
mod->required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct
kmod_module *mod)
struct stat st;
path[pathlen - (sizeof("/initstate") - 1)] = '\0';
if (stat(path, ) == 0 && S_ISDIR(st.st_mode))
- return KMOD_MODULE_COMING;
+ return mod->lookup ? KMOD_MODULE_COMING :
KMOD_MODULE_BUILTIN;
}
DBG(mod->ctx, "could not open '%s': %s\n",
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3af16c7..43288b6 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -549,6 +549,7 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
if (lookup_only)
printf("%s\n", kmod_module_get_name(mod));
else {
+ kmod_module_set_lookup(mod,true);
err = kmod_module_probe_insert_module(mod, flags,
extra_options, NULL, NULL, show);
}
>> Do I need to send a separate patch ?
> I was hoping it would be a oneliner, but it isn't. If you are going to
> send a patch, please add the necessary checks for the builtin index.
> Otherwise I can take a look on this until the end of this week.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
> wrote:
>>> Harrish, in your patch if you just change the "return
>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works. The
>> built-in modules are handled by looking at the modules.builtin index file.
>> Is there any chance of returning KMOD_MODULE_COMING for builti-in modules?
>> If it does not have any impact, then the fix should be fine.
> well... you're not returning KMOD_MODULE_COMING for a builtin module.
> Having the directory /sys/module/ and not the initstate could be
> either that the module is builtin or that there's a race while loading
> the module and it's in the coming state. However since we use the
> index to decide if this module is builtin in the beginning of this
> function, here it can only be the second case.
>
> However... mod->builtin in the beginning of this function is only set
> if the module is created by a lookup rather than from name or from
> path maybe here we need to actually fallback to the index rather
> than the cached value, otherwise this test would fail (considering
> "vt" is builtin):
>
> kmod_module_new_from_name(ctx, "vt", );
> kmod_module_get_initstate(mod, );
>
something like this ?
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
* "module", except knowing it's builtin.
*/
bool builtin : 1;
+
+ bool lookup : 1;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool
builtin)
mod->builtin = builtin;
}
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+ mod->lookup = lookup;
+}
+
void kmod_module_set_required(struct kmod_module *mod, bool required)
{
mod->required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct
kmod_module *mod)
struct stat st;
path[pathlen - (sizeof("/initstate") - 1)] = '\0';
if (stat(path, ) == 0 && S_ISDIR(st.st_mode))
- return KMOD_MODULE_COMING;
+ return mod->lookup ? KMOD_MODULE_COMING :
KMOD_MODULE_BUILTIN;
}
DBG(mod->ctx, "could not open '%s': %s\n",
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3af16c7..43288b6 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -549,6 +549,7 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
if (lookup_only)
printf("%s\n", kmod_module_get_name(mod));
else {
+ kmod_module_set_lookup(mod,true);
err = kmod_module_probe_insert_module(mod, flags,
extra_options, NULL, NULL, show);
}
>> Do I need to send a separate patch ?
> I was hoping it would be a oneliner, but it isn't. If you are going to
> send a patch, please add the necessary checks for the builtin index.
> Otherwise I can take a look on this until the end of this week.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
harish_kand...@mentor.com wrote:
Harrish, in your patch if you just change the return
KMOD_MODULE_BUILTIN; to return KMOD_MODULE_COMING; does it work?
Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works. The
built-in modules are handled by looking at the modules.builtin index file.
Is there any chance of returning KMOD_MODULE_COMING for builti-in modules?
If it does not have any impact, then the fix should be fine.
well... you're not returning KMOD_MODULE_COMING for a builtin module.
Having the directory /sys/module/name and not the initstate could be
either that the module is builtin or that there's a race while loading
the module and it's in the coming state. However since we use the
index to decide if this module is builtin in the beginning of this
function, here it can only be the second case.
However... mod-builtin in the beginning of this function is only set
if the module is created by a lookup rather than from name or from
path maybe here we need to actually fallback to the index rather
than the cached value, otherwise this test would fail (considering
vt is builtin):
kmod_module_new_from_name(ctx, vt, mod);
kmod_module_get_initstate(mod, state);
something like this ?
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
* module, except knowing it's builtin.
*/
bool builtin : 1;
+
+ bool lookup : 1;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool
builtin)
mod-builtin = builtin;
}
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+ mod-lookup = lookup;
+}
+
void kmod_module_set_required(struct kmod_module *mod, bool required)
{
mod-required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct
kmod_module *mod)
struct stat st;
path[pathlen - (sizeof(/initstate) - 1)] = '\0';
if (stat(path, st) == 0 S_ISDIR(st.st_mode))
- return KMOD_MODULE_COMING;
+ return mod-lookup ? KMOD_MODULE_COMING :
KMOD_MODULE_BUILTIN;
}
DBG(mod-ctx, could not open '%s': %s\n,
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3af16c7..43288b6 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -549,6 +549,7 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
if (lookup_only)
printf(%s\n, kmod_module_get_name(mod));
else {
+ kmod_module_set_lookup(mod,true);
err = kmod_module_probe_insert_module(mod, flags,
extra_options, NULL, NULL, show);
}
Do I need to send a separate patch ?
I was hoping it would be a oneliner, but it isn't. If you are going to
send a patch, please add the necessary checks for the builtin index.
Otherwise I can take a look on this until the end of this week.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
harish_kand...@mentor.com wrote:
Harrish, in your patch if you just change the return
KMOD_MODULE_BUILTIN; to return KMOD_MODULE_COMING; does it work?
Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works. The
built-in modules are handled by looking at the modules.builtin index file.
Is there any chance of returning KMOD_MODULE_COMING for builti-in modules?
If it does not have any impact, then the fix should be fine.
well... you're not returning KMOD_MODULE_COMING for a builtin module.
Having the directory /sys/module/name and not the initstate could be
either that the module is builtin or that there's a race while loading
the module and it's in the coming state. However since we use the
index to decide if this module is builtin in the beginning of this
function, here it can only be the second case.
However... mod-builtin in the beginning of this function is only set
if the module is created by a lookup rather than from name or from
path maybe here we need to actually fallback to the index rather
than the cached value, otherwise this test would fail (considering
vt is builtin):
kmod_module_new_from_name(ctx, vt, mod);
kmod_module_get_initstate(mod, state);
something like this ?
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
* module, except knowing it's builtin.
*/
bool builtin : 1;
+
+ bool lookup : 1;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool
builtin)
mod-builtin = builtin;
}
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+ mod-lookup = lookup;
+}
+
void kmod_module_set_required(struct kmod_module *mod, bool required)
{
mod-required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct
kmod_module *mod)
struct stat st;
path[pathlen - (sizeof(/initstate) - 1)] = '\0';
if (stat(path, st) == 0 S_ISDIR(st.st_mode))
- return KMOD_MODULE_COMING;
+ return mod-lookup ? KMOD_MODULE_COMING :
KMOD_MODULE_BUILTIN;
}
DBG(mod-ctx, could not open '%s': %s\n,
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3af16c7..43288b6 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -549,6 +549,7 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
if (lookup_only)
printf(%s\n, kmod_module_get_name(mod));
else {
+ kmod_module_set_lookup(mod,true);
err = kmod_module_probe_insert_module(mod, flags,
extra_options, NULL, NULL, show);
}
Do I need to send a separate patch ?
I was hoping it would be a oneliner, but it isn't. If you are going to
send a patch, please add the necessary checks for the builtin index.
Otherwise I can take a look on this until the end of this week.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
harish_kand...@mentor.com wrote:
On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
harish_kand...@mentor.com wrote:
Harrish, in your patch if you just change the return
KMOD_MODULE_BUILTIN; to return KMOD_MODULE_COMING; does it work?
Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works.
The built-in modules are handled by looking at the modules.builtin index
file. Is there any chance of returning KMOD_MODULE_COMING for builti-in
modules? If it does not have any impact, then the fix should be fine.
well... you're not returning KMOD_MODULE_COMING for a builtin module.
Having the directory /sys/module/name and not the initstate could be
either that the module is builtin or that there's a race while loading
the module and it's in the coming state. However since we use the
index to decide if this module is builtin in the beginning of this
function, here it can only be the second case.
However... mod-builtin in the beginning of this function is only set
if the module is created by a lookup rather than from name or from
path maybe here we need to actually fallback to the index rather
than the cached value, otherwise this test would fail (considering
vt is builtin):
kmod_module_new_from_name(ctx, vt, mod);
kmod_module_get_initstate(mod, state);
something like this ?
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
* module, except knowing it's builtin.
*/
bool builtin : 1;
+
+ bool lookup : 1;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod,
bool builtin)
mod-builtin = builtin;
}
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+ mod-lookup = lookup;
+}
+
void kmod_module_set_required(struct kmod_module *mod, bool required)
{
mod-required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct
kmod_module *mod)
struct stat st;
path[pathlen - (sizeof(/initstate) - 1)] = '\0';
if (stat(path, st) == 0 S_ISDIR(st.st_mode))
- return KMOD_MODULE_COMING;
+ return mod-lookup ? KMOD_MODULE_COMING :
KMOD_MODULE_BUILTIN;
no. I guess this doesn't pass the proposed test:
1)
kmod_module_new_from_name(ctx, vt, mod);
kmod_module_get_initstate(mod, state);
this must return builtin
2)
kmod_module_new_from_lookup(ctx, vt, list);
... (get mod from list)
kmod_module_get_initstate(mod, state);
this must return builtin as well.
I suggest you add a kmod_module_is_builtin() which does the lookup
(but doesn't increase the module refcount, i.e. doesn't call
new_module_()) iff it's not already done. For this you will need
to change mod-builtin to an enum: enum { _UNKNOWN, _NO,
_YES } then you do:
bool kmod_module_is_builtin(mod) (don't export this function)
{
if (mod-_UNKNOWN) {
... lookup in builtin index
}
return mod-builtin == _YES;
}
then you change the users of mod-builtin.
something like this ?
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 417f232..f6ffd3e 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
# endif
#endif
+/* Flags to kmod_builtin_status() */
+enum kmod_builtin_status {
+KMOD_BUILTIN_UNKNOWN = 0,
+KMOD_BUILTIN_NO = 1,
+KMOD_BUILTIN_YES = 2
+};
+
void kmod_log(const struct kmod_ctx *ctx,
int priority, const char *file, int line, const char *fn,
const char *format, ...) __attribute__((format(printf, 6, 7)))
__attribute__((nonnull(1, 3, 5)));
@@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx *ctx,
const char *name,
int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char *name,
struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
+int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char
*name)__attribute__((nonnull(1, 2)));
int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char *name,
struct kmod_list **list) __attribute__((nonnull(1
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 07:32 PM, Harish Jenny Kandiga Nagaraj wrote:
On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
harish_kand...@mentor.com wrote:
On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
harish_kand...@mentor.com wrote:
Harrish, in your patch if you just change the return
KMOD_MODULE_BUILTIN; to return KMOD_MODULE_COMING; does it work?
Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works.
The built-in modules are handled by looking at the modules.builtin index
file. Is there any chance of returning KMOD_MODULE_COMING for builti-in
modules? If it does not have any impact, then the fix should be fine.
well... you're not returning KMOD_MODULE_COMING for a builtin module.
Having the directory /sys/module/name and not the initstate could be
either that the module is builtin or that there's a race while loading
the module and it's in the coming state. However since we use the
index to decide if this module is builtin in the beginning of this
function, here it can only be the second case.
However... mod-builtin in the beginning of this function is only set
if the module is created by a lookup rather than from name or from
path maybe here we need to actually fallback to the index rather
than the cached value, otherwise this test would fail (considering
vt is builtin):
kmod_module_new_from_name(ctx, vt, mod);
kmod_module_get_initstate(mod, state);
something like this ?
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
* module, except knowing it's builtin.
*/
bool builtin : 1;
+
+ bool lookup : 1;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod,
bool builtin)
mod-builtin = builtin;
}
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+ mod-lookup = lookup;
+}
+
void kmod_module_set_required(struct kmod_module *mod, bool required)
{
mod-required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const
struct kmod_module *mod)
struct stat st;
path[pathlen - (sizeof(/initstate) - 1)] = '\0';
if (stat(path, st) == 0 S_ISDIR(st.st_mode))
- return KMOD_MODULE_COMING;
+ return mod-lookup ? KMOD_MODULE_COMING :
KMOD_MODULE_BUILTIN;
no. I guess this doesn't pass the proposed test:
1)
kmod_module_new_from_name(ctx, vt, mod);
kmod_module_get_initstate(mod, state);
this must return builtin
2)
kmod_module_new_from_lookup(ctx, vt, list);
... (get mod from list)
kmod_module_get_initstate(mod, state);
this must return builtin as well.
I suggest you add a kmod_module_is_builtin() which does the lookup
(but doesn't increase the module refcount, i.e. doesn't call
new_module_()) iff it's not already done. For this you will need
to change mod-builtin to an enum: enum { _UNKNOWN, _NO,
_YES } then you do:
bool kmod_module_is_builtin(mod) (don't export this function)
{
if (mod-_UNKNOWN) {
... lookup in builtin index
}
return mod-builtin == _YES;
}
then you change the users of mod-builtin.
something like this ?
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 417f232..f6ffd3e 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
# endif
#endif
+/* Flags to kmod_builtin_status() */
+enum kmod_builtin_status {
+KMOD_BUILTIN_UNKNOWN = 0,
+KMOD_BUILTIN_NO = 1,
+KMOD_BUILTIN_YES = 2
+};
+
void kmod_log(const struct kmod_ctx *ctx,
int priority, const char *file, int line, const char *fn,
const char *format, ...) __attribute__((format(printf, 6, 7)))
__attribute__((nonnull(1, 3, 5)));
@@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx
*ctx, const char *name,
int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char
*name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
+int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const
char *name)__attribute__((nonnull(1, 2)));
int
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 06:49 AM, Lucas De Marchi wrote: > On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell wrote: >> Lucas De Marchi writes: >>> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell >>> wrote: >>> Yeah, I just thought (an wanted that) the attributes were being >>> created first and then hooked up in the sysfs tree under >>> /sys/module/. I.e. if the directory exists and there's no >>> initstate this is because it's a builtin module. I don't want to >>> wait/sleep on the file to appear because users of >>> kmod_module_get_initstate() may not tolerate this behavior. >>> >>> Looking up at the old module-init-tools, it used an ugly loop with >>> usleep() before trying to read the file again :-/ >>> >>> Can we change kernel side guaranteeing the initstate file appears >>> together with the directory? >> Greg? The core problem is that kmod looks for >> /sys/module//initstate; if it's not there, it assumes a builtin >> module. > Just to make it clear: > > We try to open /sys/module//initstate. If it fails we stat > /sys/module/ checking if it exists and is a directory. If it > does then we assume the module is builtin. > >> However, this is racy when a module is being inserted. Is there a way >> to create this sysfs file and dir atomically? > Greg, the question is still valid since it'd be nice to have this > guarantee and be able to correctly reply the state with whatever is in > initstate file, but... > > Rusty, thinking again if we fallback to "coming" instead of "builtin" > everything should be fine, no? Because the decision about builtin has > already been taken by looking at the modules.builtin index. If we > return "coming" here the second call to modprobe would call > init_module() again which would wait for the first one to complete (or > return EEXIST if it's already live) since we only shortcut the > init_module() call if the module is live or builtin > > what do you think? > > > Harrish, in your patch if you just change the "return > KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work? > Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine. Do I need to send a separate patch ? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Thursday 19 February 2015 06:49 AM, Lucas De Marchi wrote: On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell ru...@rustcorp.com.au wrote: Lucas De Marchi lucas.de.mar...@gmail.com writes: On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell ru...@rustcorp.com.au wrote: Yeah, I just thought (an wanted that) the attributes were being created first and then hooked up in the sysfs tree under /sys/module/modulename. I.e. if the directory exists and there's no initstate this is because it's a builtin module. I don't want to wait/sleep on the file to appear because users of kmod_module_get_initstate() may not tolerate this behavior. Looking up at the old module-init-tools, it used an ugly loop with usleep() before trying to read the file again :-/ Can we change kernel side guaranteeing the initstate file appears together with the directory? Greg? The core problem is that kmod looks for /sys/module/name/initstate; if it's not there, it assumes a builtin module. Just to make it clear: We try to open /sys/module/name/initstate. If it fails we stat /sys/module/name checking if it exists and is a directory. If it does then we assume the module is builtin. However, this is racy when a module is being inserted. Is there a way to create this sysfs file and dir atomically? Greg, the question is still valid since it'd be nice to have this guarantee and be able to correctly reply the state with whatever is in initstate file, but... Rusty, thinking again if we fallback to coming instead of builtin everything should be fine, no? Because the decision about builtin has already been taken by looking at the modules.builtin index. If we return coming here the second call to modprobe would call init_module() again which would wait for the first one to complete (or return EEXIST if it's already live) since we only shortcut the init_module() call if the module is live or builtin what do you think? Harrish, in your patch if you just change the return KMOD_MODULE_BUILTIN; to return KMOD_MODULE_COMING; does it work? Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine. Do I need to send a separate patch ? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Wednesday 18 February 2015 09:37 AM, Rusty Russell wrote:
> Lucas De Marchi writes:
>> On Tue, Feb 17, 2015 at 10:56 AM, Harish Jenny K N
>> wrote:
>>> usecase: two sd cards are being mounted in parallel at same time on
>>> dual core. example modules which are getting loaded is nls_cp437.
>>> While one module is being loaded , it starts creating sysfs files.
>>> meanwhile on other core, modprobe might return saying the module
>>> is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.
>> an {f,}init_module() call should not return until the sysfs files are
>> created and if there is /sys/module// there should be
>> /sys/module//initstate unless the module is builtin.
>>
>> Rusty, was there any changes in this area in the kernel recently?
> Not deliberately.
>
>> Or is the creation of /sys/module/ and
>> /sys/module//initstate not atomic?
> No, it's not atomic :( That makes it unreliable (it seemed like a good
> idea in 2006 I guess).
>
> Here's what happens from a kernel side:
>
> 1) Module is marked UNFORMED.
> 2) Check there's no module by same name already. If there is, and it's
>UNFORMED or COMING, we wait.
> 3) module is marked COMING
> 4) module parses arguments.
> 5) sysfs directory and attributes are created.
> 6) module's init is called.
> 7) module is marked LIVE.
These are the operations handled in kernel after syscall/init_module is called.
Here is the list of operations which happen before point number 1)
0a) __request_module/try_then_request_module gets called from kernel.
0b) call_modprobe gets called which calls usermode modprobe to see if module is
loaded.
0c) syscall init_module gets called from modprobe.
> So, the second init_module should be waiting.
>
> I tested this by putting a sleep in the nls_cp437 init, and watching
> what modprobe did. It works correctly.
Problem here is second init_module is not yet called. if it gets called , it is
handled properly. Adding a sleep in nls_cp437 is handled properly.
We need to add sleep in module_add_modinfo_attrs ( module.c ) while creating
sysfs files(sysfs_create_file) in order to reproduce the issue I mentioned.
>
> You are checking initstate, and getting caught in the race:
>
> 783 14:33:14.170205 open("/sys/module/nls_cp437/initstate",
> O_RDONLY|O_LARGEFILE|O_CLOEXEC)
>
> You should probably check initstate *after* loading fails. It's
> possible that it's unloaded in the meantime, but the race is pretty
> narrow and unloading is unusual.
Did not get the point of checking initstate after loading fails. However we
need to handle even unusual rare cases as well.
>
> Cheers,
> Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
On Wednesday 18 February 2015 09:37 AM, Rusty Russell wrote:
Lucas De Marchi lucas.de.mar...@gmail.com writes:
On Tue, Feb 17, 2015 at 10:56 AM, Harish Jenny K N
harish_kand...@mentor.com wrote:
usecase: two sd cards are being mounted in parallel at same time on
dual core. example modules which are getting loaded is nls_cp437.
While one module is being loaded , it starts creating sysfs files.
meanwhile on other core, modprobe might return saying the module
is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.
an {f,}init_module() call should not return until the sysfs files are
created and if there is /sys/module/module/ there should be
/sys/module/module/initstate unless the module is builtin.
Rusty, was there any changes in this area in the kernel recently?
Not deliberately.
Or is the creation of /sys/module/module and
/sys/module/module/initstate not atomic?
No, it's not atomic :( That makes it unreliable (it seemed like a good
idea in 2006 I guess).
Here's what happens from a kernel side:
1) Module is marked UNFORMED.
2) Check there's no module by same name already. If there is, and it's
UNFORMED or COMING, we wait.
3) module is marked COMING
4) module parses arguments.
5) sysfs directory and attributes are created.
6) module's init is called.
7) module is marked LIVE.
These are the operations handled in kernel after syscall/init_module is called.
Here is the list of operations which happen before point number 1)
0a) __request_module/try_then_request_module gets called from kernel.
0b) call_modprobe gets called which calls usermode modprobe to see if module is
loaded.
0c) syscall init_module gets called from modprobe.
So, the second init_module should be waiting.
I tested this by putting a sleep in the nls_cp437 init, and watching
what modprobe did. It works correctly.
Problem here is second init_module is not yet called. if it gets called , it is
handled properly. Adding a sleep in nls_cp437 is handled properly.
We need to add sleep in module_add_modinfo_attrs ( module.c ) while creating
sysfs files(sysfs_create_file) in order to reproduce the issue I mentioned.
You are checking initstate, and getting caught in the race:
783 14:33:14.170205 open(/sys/module/nls_cp437/initstate,
O_RDONLY|O_LARGEFILE|O_CLOEXEC)
You should probably check initstate *after* loading fails. It's
possible that it's unloaded in the meantime, but the race is pretty
narrow and unloading is unusual.
Did not get the point of checking initstate after loading fails. However we
need to handle even unusual rare cases as well.
Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
If that is the case , then subscriptions of netlink_sock should have been updated after netlink_remove or netlink_release. I don't see that happening. On Wednesday 03 September 2014 12:22 AM, David Miller wrote: > From: Harish Jenny Kandiga Nagaraj > Date: Tue, 2 Sep 2014 14:14:38 +0530 > >> In one of our random test runs we observed the crash mentioned in the >> previous mail. >> >> After debugging we found out that the call flow of the inline and static >> functions were >> netlink_release >> -netlink_remove >> -__sk_del_bind_node >> --__hlist_del >> >> *pprev was NULL in __hlist_del function while deleting >sk_bind_node >> hlist_node. Hence the patch was given. >> >> In netlink_remove function , first the sk_del_node_init function will be >> called. This internally calls __sk_del_node_init function. While deleting >> >sk_node hlist_node using __sk_del_node function there is a NULL check >> with sk_hashed function. >> >> Why there is no NULL check for *pprev while deleting >sk_bind_node ? > Because if ->subscriptions is non-zero, it must be on a list, and therefore > pprev must be non-NULL. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
In one of our random test runs we observed the crash mentioned in the previous mail. After debugging we found out that the call flow of the inline and static functions were netlink_release -netlink_remove -__sk_del_bind_node --__hlist_del *pprev was NULL in __hlist_del function while deleting >sk_bind_node hlist_node. Hence the patch was given. In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting >sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function. Why there is no NULL check for *pprev while deleting >sk_bind_node ? On Tuesday 02 September 2014 10:33 AM, David Miller wrote: > From: Harish Jenny K N > Date: Mon, 1 Sep 2014 12:38:29 +0530 > > Firstly, you really need to fix your outgoing email so that your email > address appears in your From: header properly. > >> From: Harish Jenny K N >> >> Unable to handle kernel NULL pointer dereference at virtual address >> >> (netlink_release+0x0/0x2a0) from [<8034e78c>] >> (sock_release+0x28/0xa4) >> (sock_release+0x0/0xa4) from [<8034e830>] (sock_close+0x28/0x34) >> (sock_close+0x0/0x34) from [<800f3490>] (__fput+0xf0/0x1ec) >> (__fput+0x0/0x1ec) from [<800f3634>] (fput+0x10/0x14) >> (fput+0x0/0x14) from [<80040a64>] (task_work_run+0xb8/0xd8) >> (task_work_run+0x0/0xd8) from [<800113a0>] >> (do_work_pending+0xb0/0xc4) >> (do_work_pending+0x0/0xc4) from [<8000d960>] (work_pending+0xc/0x20) >> Call flow of the inline and static functions >> netlink_release >> -netlink_remove >> -__sk_del_bind_node >> --__hlist_del >> >> Signed-off-by: Harish Jenny K N > This doesn't tell us anything about how this situation can be > arrived at. > > When subscriptions changes, we delete the node with the table lock > held if subscriptions goes to zero. We only try to delete the node > when subscriptions was zero. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
In one of our random test runs we observed the crash mentioned in the previous mail. After debugging we found out that the call flow of the inline and static functions were netlink_release -netlink_remove -__sk_del_bind_node --__hlist_del *pprev was NULL in __hlist_del function while deleting sk-sk_bind_node hlist_node. Hence the patch was given. In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting sk-sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function. Why there is no NULL check for *pprev while deleting sk-sk_bind_node ? On Tuesday 02 September 2014 10:33 AM, David Miller wrote: From: Harish Jenny K N Date: Mon, 1 Sep 2014 12:38:29 +0530 Firstly, you really need to fix your outgoing email so that your email address appears in your From: header properly. From: Harish Jenny K N harish_kand...@mentor.com Unable to handle kernel NULL pointer dereference at virtual address (netlink_release+0x0/0x2a0) from [8034e78c] (sock_release+0x28/0xa4) (sock_release+0x0/0xa4) from [8034e830] (sock_close+0x28/0x34) (sock_close+0x0/0x34) from [800f3490] (__fput+0xf0/0x1ec) (__fput+0x0/0x1ec) from [800f3634] (fput+0x10/0x14) (fput+0x0/0x14) from [80040a64] (task_work_run+0xb8/0xd8) (task_work_run+0x0/0xd8) from [800113a0] (do_work_pending+0xb0/0xc4) (do_work_pending+0x0/0xc4) from [8000d960] (work_pending+0xc/0x20) Call flow of the inline and static functions netlink_release -netlink_remove -__sk_del_bind_node --__hlist_del Signed-off-by: Harish Jenny K N harish_kand...@mentor.com This doesn't tell us anything about how this situation can be arrived at. When subscriptions changes, we delete the node with the table lock held if subscriptions goes to zero. We only try to delete the node when subscriptions was zero. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] netlink: Safer deletion of sk_bind_node
If that is the case , then subscriptions of netlink_sock should have been updated after netlink_remove or netlink_release. I don't see that happening. On Wednesday 03 September 2014 12:22 AM, David Miller wrote: From: Harish Jenny Kandiga Nagaraj harish_kand...@mentor.com Date: Tue, 2 Sep 2014 14:14:38 +0530 In one of our random test runs we observed the crash mentioned in the previous mail. After debugging we found out that the call flow of the inline and static functions were netlink_release -netlink_remove -__sk_del_bind_node --__hlist_del *pprev was NULL in __hlist_del function while deleting sk-sk_bind_node hlist_node. Hence the patch was given. In netlink_remove function , first the sk_del_node_init function will be called. This internally calls __sk_del_node_init function. While deleting sk-sk_node hlist_node using __sk_del_node function there is a NULL check with sk_hashed function. Why there is no NULL check for *pprev while deleting sk-sk_bind_node ? Because if -subscriptions is non-zero, it must be on a list, and therefore pprev must be non-NULL. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
