* Fwd: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) [not found] <f8e3f271-82df-165f-63f1-6df73ba3d59c@gmail.com> @ 2020-04-01 21:22 ` Heiner Kallweit 2020-04-08 21:37 ` Heiner Kallweit [not found] ` <20200409000200.2qsqcbrzcztk6gmu@ldmartin-desk1> 2 siblings, 0 replies; 6+ messages in thread From: Heiner Kallweit @ 2020-04-01 21:22 UTC (permalink / raw) To: linux-modules [-- Attachment #1: Type: text/plain, Size: 1968 bytes --] Mistakenly sent it to linux-module instead of linux-modules. -------- Forwarded Message -------- Subject: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) Date: Wed, 1 Apr 2020 23:20:20 +0200 From: Heiner Kallweit <hkallweit1@gmail.com> To: Lucas De Marchi <lucas.demarchi@intel.com>, Jessica Yu <jeyu@kernel.org> CC: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-module@vger.kernel.org, netdev@vger.kernel.org <netdev@vger.kernel.org> Currently we have no way to express a hard dependency that is not a symbol-based dependency (symbol defined in module A is used in module B). Use case: Network driver ND uses callbacks in the dedicated PHY driver DP for the integrated PHY (namely read_page() and write_page() in struct phy_driver). If DP can't be loaded (e.g. because ND is in initramfs but DP is not), then phylib will use the generic PHY driver GP. GP doesn't implement certain callbacks that are needed by ND, therefore ND's probe has to bail out with an error once it detects that DP is not loaded. We have this problem with driver r8169 having such a dependency on PHY driver realtek. Some distributions have tools for configuring initramfs that consider hard dependencies based on depmod output. Means so far somebody can add r8169.ko to initramfs, and neither human being nor machine will have an idea that realtek.ko needs to be added too. Attached patch set (two patches for kmod, one for the kernel) allows to express this hard dependency of ND from DP. depmod will read this dependency information and treat it like a symbol-based dependency. As a result tools e.g. populating initramfs can consider the dependency and place DP in initramfs if ND is in initramfs. On my system the patch set does the trick when adding following line to r8169_main.c: MODULE_HARDDEP("realtek"); I'm interested in your opinion on the patches, and whether you maybe have a better idea how to solve the problem. Heiner [-- Attachment #2: 0001-depmod-add-helper-mod_add_dep_unique.patch --] [-- Type: text/plain, Size: 1584 bytes --] From 290e7dee9f6043d677f08dc06e612e13ee0d2d83 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit <hkallweit1@gmail.com> Date: Tue, 31 Mar 2020 23:02:47 +0200 Subject: [PATCH 1/2] depmod: add helper mod_add_dep_unique Create new helper mod_add_dep_unique(), next patch in this series will also make use of it. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- tools/depmod.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/tools/depmod.c b/tools/depmod.c index 875e314..5419d4d 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -907,23 +907,35 @@ static void mod_free(struct mod *mod) free(mod); } -static int mod_add_dependency(struct mod *mod, struct symbol *sym) +static int mod_add_dep_unique(struct mod *mod, struct mod *dep) { int err; - DBG("%s depends on %s %s\n", mod->path, sym->name, - sym->owner != NULL ? sym->owner->path : "(unknown)"); - - if (sym->owner == NULL) + if (dep == NULL) return 0; - err = array_append_unique(&mod->deps, sym->owner); + err = array_append_unique(&mod->deps, dep); if (err == -EEXIST) return 0; if (err < 0) return err; - sym->owner->users++; + dep->users++; + + return 1; +} + +static int mod_add_dependency(struct mod *mod, struct symbol *sym) +{ + int err; + + DBG("%s depends on %s %s\n", mod->path, sym->name, + sym->owner != NULL ? sym->owner->path : "(unknown)"); + + err = mod_add_dep_unique(mod, sym->owner); + if (err <= 0) + return err; + SHOW("%s needs \"%s\": %s\n", mod->path, sym->name, sym->owner->path); return 0; } -- 2.26.0 [-- Attachment #3: 0001-module-add-MODULE_HARDDEP.patch --] [-- Type: text/plain, Size: 1707 bytes --] From b12fa0d85b21d84cdf4509c5048c67e17914eb28 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit <hkallweit1@gmail.com> Date: Mon, 30 Mar 2020 17:12:44 +0200 Subject: [PATCH] module: add MODULE_HARDDEP Currently we have no way to express a hard dependency that is not a symbol-based dependency (symbol defined in module A is used in module B). Use case: Network driver ND uses callbacks in the dedicated PHY driver DP for the integrated PHY. If DP can't be loaded (e.g. because ND is in initramfs but DP is not), then phylib will load the generic PHY driver GP. GP doesn't implement certain callbacks that are used by ND, therefore ND's probe has to bail out with an error once it detects that DP is not loaded. This patch allows to express this hard dependency of ND from DP. depmod will read this dependency information and treat it like a symbol-based dependency. As a result tools e.g. populating initramfs can consider the dependency and place DP in initramfs if ND is in initramfs. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- include/linux/module.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/linux/module.h b/include/linux/module.h index 1ad393e62..f38d4107f 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -169,6 +169,11 @@ extern void cleanup_module(void); */ #define MODULE_SOFTDEP(_softdep) MODULE_INFO(softdep, _softdep) +/* Hard module dependencies that are not code dependencies + * Example: MODULE_HARDDEP("module-foo module-bar") + */ +#define MODULE_HARDDEP(_harddep) MODULE_INFO(harddep, _harddep) + /* * MODULE_FILE is used for generating modules.builtin * So, make it no-op when this is being built as a module -- 2.26.0 [-- Attachment #4: 0002-depmod-add-depmod_load_harddeps.patch --] [-- Type: text/plain, Size: 2033 bytes --] From af3a25833a160e029441eaf5a93f7c8625544296 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit <hkallweit1@gmail.com> Date: Wed, 1 Apr 2020 22:42:55 +0200 Subject: [PATCH 2/2] depmod: add depmod_load_harddeps Load explicitly declared hard dependency information from modules and add it to the symbol-derived dependencies. This will allow depmod-based tools to consider hard dependencies that are not code dependencies. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- tools/depmod.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tools/depmod.c b/tools/depmod.c index 5419d4d..5771dc9 100644 --- a/tools/depmod.c +++ b/tools/depmod.c @@ -1522,6 +1522,41 @@ static struct symbol *depmod_symbol_find(const struct depmod *depmod, return hash_find(depmod->symbols, name); } +static void depmod_load_harddeps(struct depmod *depmod, struct mod *mod) +{ + + struct kmod_list *l; + + kmod_list_foreach(l, mod->info_list) { + const char *key = kmod_module_info_get_key(l); + const char *dep_name; + struct mod *dep; + char *value; + + if (!streq(key, "harddep")) + continue; + + value = strdup(kmod_module_info_get_value(l)); + if (value == NULL) + return; + + dep_name = strtok(value, " \t"); + + while (dep_name) { + dep = hash_find(depmod->modules_by_name, dep_name); + if (dep) + mod_add_dep_unique(mod, dep); + else + WRN("harddep: %s: unknown dependency %s\n", + mod->modname, dep_name); + + dep_name = strtok(NULL, " \t"); + } + + free(value); + } +} + static int depmod_load_modules(struct depmod *depmod) { struct mod **itr, **itr_end; @@ -1569,6 +1604,9 @@ static int depmod_load_module_dependencies(struct depmod *depmod, struct mod *mo struct kmod_list *l; DBG("do dependencies of %s\n", mod->path); + + depmod_load_harddeps(depmod, mod); + kmod_list_foreach(l, mod->dep_sym_list) { const char *name = kmod_module_dependency_symbol_get_symbol(l); uint64_t crc = kmod_module_dependency_symbol_get_crc(l); -- 2.26.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) [not found] <f8e3f271-82df-165f-63f1-6df73ba3d59c@gmail.com> 2020-04-01 21:22 ` Fwd: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) Heiner Kallweit @ 2020-04-08 21:37 ` Heiner Kallweit [not found] ` <20200409000200.2qsqcbrzcztk6gmu@ldmartin-desk1> 2 siblings, 0 replies; 6+ messages in thread From: Heiner Kallweit @ 2020-04-08 21:37 UTC (permalink / raw) To: Lucas De Marchi, Jessica Yu Cc: Linux Kernel Mailing List, linux-modules, netdev On 01.04.2020 23:20, Heiner Kallweit wrote: > Currently we have no way to express a hard dependency that is not > a symbol-based dependency (symbol defined in module A is used in > module B). Use case: > Network driver ND uses callbacks in the dedicated PHY driver DP > for the integrated PHY (namely read_page() and write_page() in > struct phy_driver). If DP can't be loaded (e.g. because ND is in > initramfs but DP is not), then phylib will use the generic > PHY driver GP. GP doesn't implement certain callbacks that are > needed by ND, therefore ND's probe has to bail out with an error > once it detects that DP is not loaded. > We have this problem with driver r8169 having such a dependency > on PHY driver realtek. Some distributions have tools for > configuring initramfs that consider hard dependencies based on > depmod output. Means so far somebody can add r8169.ko to initramfs, > and neither human being nor machine will have an idea that > realtek.ko needs to be added too. > > Attached patch set (two patches for kmod, one for the kernel) > allows to express this hard dependency of ND from DP. depmod will > read this dependency information and treat it like a symbol-based > dependency. As a result tools e.g. populating initramfs can > consider the dependency and place DP in initramfs if ND is in > initramfs. On my system the patch set does the trick when > adding following line to r8169_main.c: > MODULE_HARDDEP("realtek"); > > I'm interested in your opinion on the patches, and whether you > maybe have a better idea how to solve the problem. > > Heiner > Any feedback? Thanks, Heiner ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20200409000200.2qsqcbrzcztk6gmu@ldmartin-desk1>]
* Re: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) [not found] ` <20200409000200.2qsqcbrzcztk6gmu@ldmartin-desk1> @ 2020-04-09 22:25 ` Heiner Kallweit 2020-04-14 16:09 ` Jessica Yu 0 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2020-04-09 22:25 UTC (permalink / raw) To: Lucas De Marchi Cc: Jessica Yu, Linux Kernel Mailing List, linux-modules, netdev On 09.04.2020 02:02, Lucas De Marchi wrote: > On Wed, Apr 01, 2020 at 11:20:20PM +0200, Heiner Kallweit wrote: >> Currently we have no way to express a hard dependency that is not >> a symbol-based dependency (symbol defined in module A is used in >> module B). Use case: >> Network driver ND uses callbacks in the dedicated PHY driver DP >> for the integrated PHY (namely read_page() and write_page() in >> struct phy_driver). If DP can't be loaded (e.g. because ND is in >> initramfs but DP is not), then phylib will use the generic >> PHY driver GP. GP doesn't implement certain callbacks that are >> needed by ND, therefore ND's probe has to bail out with an error >> once it detects that DP is not loaded. >> We have this problem with driver r8169 having such a dependency >> on PHY driver realtek. Some distributions have tools for >> configuring initramfs that consider hard dependencies based on >> depmod output. Means so far somebody can add r8169.ko to initramfs, >> and neither human being nor machine will have an idea that >> realtek.ko needs to be added too. > > Could you expand on why softdep doesn't solve this problem > with MODULE_SOFTDEP() > > initramfs tools can already read it and modules can already expose them > (they end up in /lib/modules/$(uname -r)/modules.softdep and modprobe > makes use of them) > Thanks for the feedback. I was under the impression that initramfs-tools is affected, but you're right, it considers softdeps. Therefore I checked the error reports again, and indeed they are about Gentoo's "genkernel" tool only. See here: https://bugzilla.kernel.org/show_bug.cgi?id=204343#c15 If most kernel/initramfs tools consider softdeps, then I don't see a need for the proposed change. But well, everything is good for something, and I learnt something about the structure of kmod. Sorry for the noise. > Lucas De Marchi > Heiner >> >> Attached patch set (two patches for kmod, one for the kernel) >> allows to express this hard dependency of ND from DP. depmod will >> read this dependency information and treat it like a symbol-based >> dependency. As a result tools e.g. populating initramfs can >> consider the dependency and place DP in initramfs if ND is in >> initramfs. On my system the patch set does the trick when >> adding following line to r8169_main.c: >> MODULE_HARDDEP("realtek"); >> >> I'm interested in your opinion on the patches, and whether you >> maybe have a better idea how to solve the problem. >> >> Heiner > >> From 290e7dee9f6043d677f08dc06e612e13ee0d2d83 Mon Sep 17 00:00:00 2001 >> From: Heiner Kallweit <hkallweit1@gmail.com> >> Date: Tue, 31 Mar 2020 23:02:47 +0200 >> Subject: [PATCH 1/2] depmod: add helper mod_add_dep_unique >> >> Create new helper mod_add_dep_unique(), next patch in this series will >> also make use of it. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> tools/depmod.c | 26 +++++++++++++++++++------- >> 1 file changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/tools/depmod.c b/tools/depmod.c >> index 875e314..5419d4d 100644 >> --- a/tools/depmod.c >> +++ b/tools/depmod.c >> @@ -907,23 +907,35 @@ static void mod_free(struct mod *mod) >> free(mod); >> } >> >> -static int mod_add_dependency(struct mod *mod, struct symbol *sym) >> +static int mod_add_dep_unique(struct mod *mod, struct mod *dep) >> { >> int err; >> >> - DBG("%s depends on %s %s\n", mod->path, sym->name, >> - sym->owner != NULL ? sym->owner->path : "(unknown)"); >> - >> - if (sym->owner == NULL) >> + if (dep == NULL) >> return 0; >> >> - err = array_append_unique(&mod->deps, sym->owner); >> + err = array_append_unique(&mod->deps, dep); >> if (err == -EEXIST) >> return 0; >> if (err < 0) >> return err; >> >> - sym->owner->users++; >> + dep->users++; >> + >> + return 1; >> +} >> + >> +static int mod_add_dependency(struct mod *mod, struct symbol *sym) >> +{ >> + int err; >> + >> + DBG("%s depends on %s %s\n", mod->path, sym->name, >> + sym->owner != NULL ? sym->owner->path : "(unknown)"); >> + >> + err = mod_add_dep_unique(mod, sym->owner); >> + if (err <= 0) >> + return err; >> + >> SHOW("%s needs \"%s\": %s\n", mod->path, sym->name, sym->owner->path); >> return 0; >> } >> -- >> 2.26.0 >> > >> From b12fa0d85b21d84cdf4509c5048c67e17914eb28 Mon Sep 17 00:00:00 2001 >> From: Heiner Kallweit <hkallweit1@gmail.com> >> Date: Mon, 30 Mar 2020 17:12:44 +0200 >> Subject: [PATCH] module: add MODULE_HARDDEP >> >> Currently we have no way to express a hard dependency that is not a >> symbol-based dependency (symbol defined in module A is used in >> module B). Use case: >> Network driver ND uses callbacks in the dedicated PHY driver DP >> for the integrated PHY. If DP can't be loaded (e.g. because ND >> is in initramfs but DP is not), then phylib will load the generic >> PHY driver GP. GP doesn't implement certain callbacks that are >> used by ND, therefore ND's probe has to bail out with an error >> once it detects that DP is not loaded. >> This patch allows to express this hard dependency of ND from DP. >> depmod will read this dependency information and treat it like >> a symbol-based dependency. As a result tools e.g. populating >> initramfs can consider the dependency and place DP in initramfs >> if ND is in initramfs. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> include/linux/module.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/include/linux/module.h b/include/linux/module.h >> index 1ad393e62..f38d4107f 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -169,6 +169,11 @@ extern void cleanup_module(void); >> */ >> #define MODULE_SOFTDEP(_softdep) MODULE_INFO(softdep, _softdep) >> >> +/* Hard module dependencies that are not code dependencies >> + * Example: MODULE_HARDDEP("module-foo module-bar") >> + */ >> +#define MODULE_HARDDEP(_harddep) MODULE_INFO(harddep, _harddep) >> + >> /* >> * MODULE_FILE is used for generating modules.builtin >> * So, make it no-op when this is being built as a module >> -- >> 2.26.0 >> > >> From af3a25833a160e029441eaf5a93f7c8625544296 Mon Sep 17 00:00:00 2001 >> From: Heiner Kallweit <hkallweit1@gmail.com> >> Date: Wed, 1 Apr 2020 22:42:55 +0200 >> Subject: [PATCH 2/2] depmod: add depmod_load_harddeps >> >> Load explicitly declared hard dependency information from modules and >> add it to the symbol-derived dependencies. This will allow >> depmod-based tools to consider hard dependencies that are not code >> dependencies. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> tools/depmod.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/tools/depmod.c b/tools/depmod.c >> index 5419d4d..5771dc9 100644 >> --- a/tools/depmod.c >> +++ b/tools/depmod.c >> @@ -1522,6 +1522,41 @@ static struct symbol *depmod_symbol_find(const struct depmod *depmod, >> return hash_find(depmod->symbols, name); >> } >> >> +static void depmod_load_harddeps(struct depmod *depmod, struct mod *mod) >> +{ >> + >> + struct kmod_list *l; >> + >> + kmod_list_foreach(l, mod->info_list) { >> + const char *key = kmod_module_info_get_key(l); >> + const char *dep_name; >> + struct mod *dep; >> + char *value; >> + >> + if (!streq(key, "harddep")) >> + continue; >> + >> + value = strdup(kmod_module_info_get_value(l)); >> + if (value == NULL) >> + return; >> + >> + dep_name = strtok(value, " \t"); >> + >> + while (dep_name) { >> + dep = hash_find(depmod->modules_by_name, dep_name); >> + if (dep) >> + mod_add_dep_unique(mod, dep); >> + else >> + WRN("harddep: %s: unknown dependency %s\n", >> + mod->modname, dep_name); >> + >> + dep_name = strtok(NULL, " \t"); >> + } >> + >> + free(value); >> + } >> +} >> + >> static int depmod_load_modules(struct depmod *depmod) >> { >> struct mod **itr, **itr_end; >> @@ -1569,6 +1604,9 @@ static int depmod_load_module_dependencies(struct depmod *depmod, struct mod *mo >> struct kmod_list *l; >> >> DBG("do dependencies of %s\n", mod->path); >> + >> + depmod_load_harddeps(depmod, mod); >> + >> kmod_list_foreach(l, mod->dep_sym_list) { >> const char *name = kmod_module_dependency_symbol_get_symbol(l); >> uint64_t crc = kmod_module_dependency_symbol_get_crc(l); >> -- >> 2.26.0 >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) 2020-04-09 22:25 ` Heiner Kallweit @ 2020-04-14 16:09 ` Jessica Yu 2020-04-14 16:20 ` Heiner Kallweit 0 siblings, 1 reply; 6+ messages in thread From: Jessica Yu @ 2020-04-14 16:09 UTC (permalink / raw) To: Heiner Kallweit Cc: Lucas De Marchi, Petr Mladek, Linux Kernel Mailing List, linux-modules, netdev, live-patching +++ Heiner Kallweit [10/04/20 00:25 +0200]: >On 09.04.2020 02:02, Lucas De Marchi wrote: >> On Wed, Apr 01, 2020 at 11:20:20PM +0200, Heiner Kallweit wrote: >>> Currently we have no way to express a hard dependency that is not >>> a symbol-based dependency (symbol defined in module A is used in >>> module B). Use case: >>> Network driver ND uses callbacks in the dedicated PHY driver DP >>> for the integrated PHY (namely read_page() and write_page() in >>> struct phy_driver). If DP can't be loaded (e.g. because ND is in >>> initramfs but DP is not), then phylib will use the generic >>> PHY driver GP. GP doesn't implement certain callbacks that are >>> needed by ND, therefore ND's probe has to bail out with an error >>> once it detects that DP is not loaded. >>> We have this problem with driver r8169 having such a dependency >>> on PHY driver realtek. Some distributions have tools for >>> configuring initramfs that consider hard dependencies based on >>> depmod output. Means so far somebody can add r8169.ko to initramfs, >>> and neither human being nor machine will have an idea that >>> realtek.ko needs to be added too. >> >> Could you expand on why softdep doesn't solve this problem >> with MODULE_SOFTDEP() >> >> initramfs tools can already read it and modules can already expose them >> (they end up in /lib/modules/$(uname -r)/modules.softdep and modprobe >> makes use of them) >> >Thanks for the feedback. I was under the impression that initramfs-tools >is affected, but you're right, it considers softdeps. >Therefore I checked the error reports again, and indeed they are about >Gentoo's "genkernel" tool only. See here: >https://bugzilla.kernel.org/show_bug.cgi?id=204343#c15 > >If most kernel/initramfs tools consider softdeps, then I don't see >a need for the proposed change. But well, everything is good for >something, and I learnt something about the structure of kmod. >Sorry for the noise. Well, I wouldn't really call it noise :) I think there *could* be cases out there where a establishing a non-symbol-based hard dependency would be beneficial. In the bug you linked, I think one could hypothetically run into the same oops if the realtek module fails to load for whatever reason, no? Since realtek is only a soft dependency of r8169, modprobe would consider realtek optional and would still try to load r8169 even if realtek had failed to load previously. Then wouldn't the same problem (described in the bugzilla) arise? Maybe a hard dependency could possibly come in handy in this case, because a softdep unfortunately implies that r8169 can work without realtek loaded. Another potential usecase - I think livepatch folks (CC'd) have contemplated establishing some type of hard dependency between patch module and the target module before. I have only briefly skimmed Petr's POC [1] and it looks like this patch module dependency is accomplished through a request_module() call during load_module() (specifically, in klp_module_coming()) so that the patch module gets loaded before the target module finishes loading. I initially thought this dependency could be expressed through MODULE_HARDDEP(target_module) in the patch module source code, but it looks like livepatch might require something more fine-grained, since the current POC tries to load the patch module before the target module runs its init(). In addition, this method wouldn't prevent the target module from loading if the patch module fails to load.. In any case, I appreciate the RFC and don't really have any gripes against having an analogous MODULE_HARDDEP() macro. If more similar cases crop up then it may be worth seriously pursuing, and then we'll at least have this RFC to start with :) Thanks, Jessica ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) 2020-04-14 16:09 ` Jessica Yu @ 2020-04-14 16:20 ` Heiner Kallweit 2020-04-15 9:16 ` Jessica Yu 0 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2020-04-14 16:20 UTC (permalink / raw) To: Jessica Yu Cc: Lucas De Marchi, Petr Mladek, Linux Kernel Mailing List, linux-modules, netdev, live-patching On 14.04.2020 18:09, Jessica Yu wrote: > +++ Heiner Kallweit [10/04/20 00:25 +0200]: >> On 09.04.2020 02:02, Lucas De Marchi wrote: >>> On Wed, Apr 01, 2020 at 11:20:20PM +0200, Heiner Kallweit wrote: >>>> Currently we have no way to express a hard dependency that is not >>>> a symbol-based dependency (symbol defined in module A is used in >>>> module B). Use case: >>>> Network driver ND uses callbacks in the dedicated PHY driver DP >>>> for the integrated PHY (namely read_page() and write_page() in >>>> struct phy_driver). If DP can't be loaded (e.g. because ND is in >>>> initramfs but DP is not), then phylib will use the generic >>>> PHY driver GP. GP doesn't implement certain callbacks that are >>>> needed by ND, therefore ND's probe has to bail out with an error >>>> once it detects that DP is not loaded. >>>> We have this problem with driver r8169 having such a dependency >>>> on PHY driver realtek. Some distributions have tools for >>>> configuring initramfs that consider hard dependencies based on >>>> depmod output. Means so far somebody can add r8169.ko to initramfs, >>>> and neither human being nor machine will have an idea that >>>> realtek.ko needs to be added too. >>> >>> Could you expand on why softdep doesn't solve this problem >>> with MODULE_SOFTDEP() >>> >>> initramfs tools can already read it and modules can already expose them >>> (they end up in /lib/modules/$(uname -r)/modules.softdep and modprobe >>> makes use of them) >>> >> Thanks for the feedback. I was under the impression that initramfs-tools >> is affected, but you're right, it considers softdeps. >> Therefore I checked the error reports again, and indeed they are about >> Gentoo's "genkernel" tool only. See here: >> https://bugzilla.kernel.org/show_bug.cgi?id=204343#c15 >> >> If most kernel/initramfs tools consider softdeps, then I don't see >> a need for the proposed change. But well, everything is good for >> something, and I learnt something about the structure of kmod. >> Sorry for the noise. > > Well, I wouldn't really call it noise :) I think there *could* be > cases out there where a establishing a non-symbol-based hard > dependency would be beneficial. > Thanks for the encouraging words ;) > In the bug you linked, I think one could hypothetically run into the > same oops if the realtek module fails to load for whatever reason, no? Basically yes. Just that it wouldn't be an oops any longer, r8169 would detect the missing dedicated PHY driver and bail out in probe(). > Since realtek is only a soft dependency of r8169, modprobe would > consider realtek optional and would still try to load r8169 even if > realtek had failed to load previously. Then wouldn't the same problem > (described in the bugzilla) arise? Maybe a hard dependency could > possibly come in handy in this case, because a softdep unfortunately > implies that r8169 can work without realtek loaded. > Right. Even though kmod treats a softdep more or less like a harddep with regard to module loading, it's called "soft" for a reason. Relying on a softdep to satisfy a hard dependency doesn't seem to be the ideal solution. > Another potential usecase - I think livepatch folks (CC'd) have > contemplated establishing some type of hard dependency between patch > module and the target module before. I have only briefly skimmed > Petr's POC [1] and it looks like this patch module dependency is > accomplished through a request_module() call during load_module() > (specifically, in klp_module_coming()) so that the patch module gets > loaded before the target module finishes loading. > > I initially thought this dependency could be expressed through > MODULE_HARDDEP(target_module) in the patch module source code, but it > looks like livepatch might require something more fine-grained, since > the current POC tries to load the patch module before the target > module runs its init(). In addition, this method wouldn't prevent the > target module from loading if the patch module fails to load.. > > In any case, I appreciate the RFC and don't really have any gripes > against having an analogous MODULE_HARDDEP() macro. If more similar > cases crop up then it may be worth seriously pursuing, and then we'll > at least have this RFC to start with :) > > Thanks, > > Jessica Heiner ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) 2020-04-14 16:20 ` Heiner Kallweit @ 2020-04-15 9:16 ` Jessica Yu 0 siblings, 0 replies; 6+ messages in thread From: Jessica Yu @ 2020-04-15 9:16 UTC (permalink / raw) To: Heiner Kallweit Cc: Lucas De Marchi, Petr Mladek, Linux Kernel Mailing List, linux-modules, netdev, live-patching +++ Heiner Kallweit [14/04/20 18:20 +0200]: >On 14.04.2020 18:09, Jessica Yu wrote: >> +++ Heiner Kallweit [10/04/20 00:25 +0200]: >>> On 09.04.2020 02:02, Lucas De Marchi wrote: >>>> On Wed, Apr 01, 2020 at 11:20:20PM +0200, Heiner Kallweit wrote: >>>>> Currently we have no way to express a hard dependency that is not >>>>> a symbol-based dependency (symbol defined in module A is used in >>>>> module B). Use case: >>>>> Network driver ND uses callbacks in the dedicated PHY driver DP >>>>> for the integrated PHY (namely read_page() and write_page() in >>>>> struct phy_driver). If DP can't be loaded (e.g. because ND is in >>>>> initramfs but DP is not), then phylib will use the generic >>>>> PHY driver GP. GP doesn't implement certain callbacks that are >>>>> needed by ND, therefore ND's probe has to bail out with an error >>>>> once it detects that DP is not loaded. >>>>> We have this problem with driver r8169 having such a dependency >>>>> on PHY driver realtek. Some distributions have tools for >>>>> configuring initramfs that consider hard dependencies based on >>>>> depmod output. Means so far somebody can add r8169.ko to initramfs, >>>>> and neither human being nor machine will have an idea that >>>>> realtek.ko needs to be added too. >>>> >>>> Could you expand on why softdep doesn't solve this problem >>>> with MODULE_SOFTDEP() >>>> >>>> initramfs tools can already read it and modules can already expose them >>>> (they end up in /lib/modules/$(uname -r)/modules.softdep and modprobe >>>> makes use of them) >>>> >>> Thanks for the feedback. I was under the impression that initramfs-tools >>> is affected, but you're right, it considers softdeps. >>> Therefore I checked the error reports again, and indeed they are about >>> Gentoo's "genkernel" tool only. See here: >>> https://bugzilla.kernel.org/show_bug.cgi?id=204343#c15 >>> >>> If most kernel/initramfs tools consider softdeps, then I don't see >>> a need for the proposed change. But well, everything is good for >>> something, and I learnt something about the structure of kmod. >>> Sorry for the noise. >> >> Well, I wouldn't really call it noise :) I think there *could* be >> cases out there where a establishing a non-symbol-based hard >> dependency would be beneficial. >> >Thanks for the encouraging words ;) > >> In the bug you linked, I think one could hypothetically run into the >> same oops if the realtek module fails to load for whatever reason, no? > >Basically yes. Just that it wouldn't be an oops any longer, r8169 >would detect the missing dedicated PHY driver and bail out in probe(). > >> Since realtek is only a soft dependency of r8169, modprobe would >> consider realtek optional and would still try to load r8169 even if >> realtek had failed to load previously. Then wouldn't the same problem >> (described in the bugzilla) arise? Maybe a hard dependency could >> possibly come in handy in this case, because a softdep unfortunately >> implies that r8169 can work without realtek loaded. >> >Right. Even though kmod treats a softdep more or less like a harddep >with regard to module loading, it's called "soft" for a reason. >Relying on a softdep to satisfy a hard dependency doesn't seem >to be the ideal solution. Hm, I wonder how many other drivers do this. It may be worth auditing all current MODULE_SOFTDEP() users to see if there are others also using it to mean harddep (i.e., it's actually a non-optional dependency) rather than softdep. Something to add on my todo list. Jessica ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-15 9:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <f8e3f271-82df-165f-63f1-6df73ba3d59c@gmail.com> 2020-04-01 21:22 ` Fwd: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek) Heiner Kallweit 2020-04-08 21:37 ` Heiner Kallweit [not found] ` <20200409000200.2qsqcbrzcztk6gmu@ldmartin-desk1> 2020-04-09 22:25 ` Heiner Kallweit 2020-04-14 16:09 ` Jessica Yu 2020-04-14 16:20 ` Heiner Kallweit 2020-04-15 9:16 ` Jessica Yu
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).