* multiple softdeps
@ 2019-06-28 13:16 Harald Hoyer
2019-06-28 16:25 ` Lucas De Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Harald Hoyer @ 2019-06-28 13:16 UTC (permalink / raw)
To: linux-modules
Hi,
could you please enlighten me about kernel module softdeps?
$ modinfo cifs | grep soft
softdep: pre: ccm
softdep: pre: aead2
softdep: pre: sha512
softdep: pre: sha256
softdep: pre: cmac
softdep: pre: aes
softdep: pre: nls
softdep: pre: md5
softdep: pre: md4
softdep: pre: hmac
softdep: pre: ecb
softdep: pre: des
softdep: pre: arc4
$ grep cifs /lib/modules/$(uname -r)/modules.softdep
softdep cifs pre: ccm
softdep cifs pre: aead2
softdep cifs pre: sha512
softdep cifs pre: sha256
softdep cifs pre: cmac
softdep cifs pre: aes
softdep cifs pre: nls
softdep cifs pre: md5
softdep cifs pre: md4
softdep cifs pre: hmac
softdep cifs pre: ecb
softdep cifs pre: des
softdep cifs pre: arc4
But, calling kmod_module_get_softdeps() on the cifs module only returns one module in the pre list ("ccm").
Is my understanding about how softdeps work wrong, or is the cifs module misconfigured, or is libkmod buggy?
Please CC me, as I am not subscribed to the mailing list.
softdeps-test.c:
#include <stdio.h>
#include <libkmod.h>
#include <stdlib.h>
int main() {
int err;
struct kmod_ctx *ctx = NULL;
struct kmod_list *list = NULL;
struct kmod_list *modpre = NULL;
struct kmod_list *modpost = NULL;
struct kmod_list *itr, *l;
ctx = kmod_new(NULL, NULL);
err = kmod_module_new_from_lookup(ctx, "cifs", &list);
if (err < 0) {
perror("kmod_module_new_from_lookup");
return EXIT_FAILURE;
}
kmod_list_foreach(l, list) {
struct kmod_module *mod = NULL;
mod = kmod_module_get_module(l);
kmod_module_get_softdeps(mod, &modpre, &modpost);
kmod_list_foreach(itr, modpre) {
struct kmod_module *mod = NULL;
const char *path = NULL;
mod = kmod_module_get_module(itr);
path = kmod_module_get_path(mod);
puts(path);
}
}
}
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: multiple softdeps 2019-06-28 13:16 multiple softdeps Harald Hoyer @ 2019-06-28 16:25 ` Lucas De Marchi 2019-06-29 9:30 ` Jean Delvare 0 siblings, 1 reply; 5+ messages in thread From: Lucas De Marchi @ 2019-06-28 16:25 UTC (permalink / raw) To: Harald Hoyer; +Cc: linux-modules, Jean Delvare, Pavel Shilovsky, linux-cifs +cifs, +Jean, +Pavel On Fri, Jun 28, 2019 at 03:16:35PM +0200, Harald Hoyer wrote: >Hi, > >could you please enlighten me about kernel module softdeps? > >$ modinfo cifs | grep soft >softdep: pre: ccm >softdep: pre: aead2 >softdep: pre: sha512 >softdep: pre: sha256 >softdep: pre: cmac >softdep: pre: aes >softdep: pre: nls >softdep: pre: md5 >softdep: pre: md4 >softdep: pre: hmac >softdep: pre: ecb >softdep: pre: des >softdep: pre: arc4 > >$ grep cifs /lib/modules/$(uname -r)/modules.softdep >softdep cifs pre: ccm >softdep cifs pre: aead2 >softdep cifs pre: sha512 >softdep cifs pre: sha256 >softdep cifs pre: cmac >softdep cifs pre: aes >softdep cifs pre: nls >softdep cifs pre: md5 >softdep cifs pre: md4 >softdep cifs pre: hmac >softdep cifs pre: ecb >softdep cifs pre: des >softdep cifs pre: arc4 this is your bug. Multiple softdeps are not additive to the previous configuration, we never supported that. Commit b9be76d585d4 ("cifs: Add soft dependencies") added it for the wrong reasons actually. A sotfdep means kmod will actually load those dependencies before loading the module (or fail to load it if those dependencies don't exist). Besides the wrong commit message, if that is indeed what is desired, the fix would be: --------8<-------------- diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 65d9771e49f9..4f1f744ea3cd 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1591,18 +1591,6 @@ MODULE_DESCRIPTION ("VFS to access SMB3 servers e.g. Samba, Macs, Azure and Windows (and " "also older servers complying with the SNIA CIFS Specification)"); MODULE_VERSION(CIFS_VERSION); -MODULE_SOFTDEP("pre: arc4"); -MODULE_SOFTDEP("pre: des"); -MODULE_SOFTDEP("pre: ecb"); -MODULE_SOFTDEP("pre: hmac"); -MODULE_SOFTDEP("pre: md4"); -MODULE_SOFTDEP("pre: md5"); -MODULE_SOFTDEP("pre: nls"); -MODULE_SOFTDEP("pre: aes"); -MODULE_SOFTDEP("pre: cmac"); -MODULE_SOFTDEP("pre: sha256"); -MODULE_SOFTDEP("pre: sha512"); -MODULE_SOFTDEP("pre: aead2"); -MODULE_SOFTDEP("pre: ccm"); +MODULE_SOFTDEP("pre: arc4 des ecb hmac md4 md5 nls aes cmac sha256 sha512 aead2 ccm"); module_init(init_cifs) module_exit(exit_cifs) --------8<-------------- I guess we could actually implement additive deps, e.g. by using "pre+:" or something else. But I think the right thing to do for now is to apply something like above to cifs and propagate it to stable. Thanks for the report. Lucas De Marchi > >But, calling kmod_module_get_softdeps() on the cifs module only returns one module in the pre list ("ccm"). > >Is my understanding about how softdeps work wrong, or is the cifs module misconfigured, or is libkmod buggy? >Please CC me, as I am not subscribed to the mailing list. > > >softdeps-test.c: > >#include <stdio.h> >#include <libkmod.h> >#include <stdlib.h> > >int main() { > int err; > struct kmod_ctx *ctx = NULL; > struct kmod_list *list = NULL; > struct kmod_list *modpre = NULL; > struct kmod_list *modpost = NULL; > struct kmod_list *itr, *l; > > ctx = kmod_new(NULL, NULL); > > err = kmod_module_new_from_lookup(ctx, "cifs", &list); > if (err < 0) { > perror("kmod_module_new_from_lookup"); > return EXIT_FAILURE; > } > > kmod_list_foreach(l, list) { > struct kmod_module *mod = NULL; > mod = kmod_module_get_module(l); > > kmod_module_get_softdeps(mod, &modpre, &modpost); > > kmod_list_foreach(itr, modpre) { > struct kmod_module *mod = NULL; > const char *path = NULL; > mod = kmod_module_get_module(itr); > path = kmod_module_get_path(mod); > puts(path); > } > } >} ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: multiple softdeps 2019-06-28 16:25 ` Lucas De Marchi @ 2019-06-29 9:30 ` Jean Delvare 2019-06-30 5:57 ` Steve French 0 siblings, 1 reply; 5+ messages in thread From: Jean Delvare @ 2019-06-29 9:30 UTC (permalink / raw) To: Lucas De Marchi; +Cc: Harald Hoyer, linux-modules, Pavel Shilovsky, linux-cifs Hi Lucas, Harald, On Fri, 28 Jun 2019 09:25:17 -0700, Lucas De Marchi wrote: > +cifs, +Jean, +Pavel > > On Fri, Jun 28, 2019 at 03:16:35PM +0200, Harald Hoyer wrote: > >Hi, > > > >could you please enlighten me about kernel module softdeps? > > > >$ modinfo cifs | grep soft > >softdep: pre: ccm > >softdep: pre: aead2 > >softdep: pre: sha512 > >softdep: pre: sha256 > >softdep: pre: cmac > >softdep: pre: aes > >softdep: pre: nls > >softdep: pre: md5 > >softdep: pre: md4 > >softdep: pre: hmac > >softdep: pre: ecb > >softdep: pre: des > >softdep: pre: arc4 > > > >$ grep cifs /lib/modules/$(uname -r)/modules.softdep > >softdep cifs pre: ccm > >softdep cifs pre: aead2 > >softdep cifs pre: sha512 > >softdep cifs pre: sha256 > >softdep cifs pre: cmac > >softdep cifs pre: aes > >softdep cifs pre: nls > >softdep cifs pre: md5 > >softdep cifs pre: md4 > >softdep cifs pre: hmac > >softdep cifs pre: ecb > >softdep cifs pre: des > >softdep cifs pre: arc4 > > this is your bug. Multiple softdeps are not additive to the previous > configuration, we never supported that. Commit b9be76d585d4 ("cifs: Add That's my fault then, sorry about that. At the time I submitted the patch, there was no occurrence of module having multiple softdeps, so I didn't know how it should be done. It was some time ago and I don't remember the details though, and I have no trace of why I worked on this in the first place. Apparently I'm not the only one confused as I see driver pcengines-apuv2 has exactly the same problem. I'll send a patch for that one. If multiple softdep statements are not supported then shouldn't we prevent them from happening, either with a link-time check, or with a checkpatch test? I have no idea how to implement such a check though. > soft dependencies") added it for the wrong reasons actually. A sotfdep > means kmod will actually load those dependencies before loading the > module (or fail to load it if those dependencies don't exist). That's my definition of a regular (not soft) dependency. If failing soft pre dependencies are fatal, how do they differ from regular dependencies? Only because they are listed explicitly instead of being determined from symbol use? FWIW modprobe.d(5) mentions "optional modules" and insists that the main module is still usable without them, so my understanding was that a modprobe would be attempted on soft deps but a failure would be ignored. > Besides the wrong commit message, if that is indeed what is desired, the > fix would be: The commit message may sound wrong to you but the (fixed version of the patch) still solves the problem (including all potentially needed modules in initrd even when there are no hard dependencies) at least as a side effect. If you think that softdeps are not how the problem should be solved, then how do you think it should be solved? I understand that pre-loading all cipher and hash modules is not efficient, but we still need to ensure that they all make it to the initrd in case they are needed. Or we could decide that cifs in initrd is not supported? To be honest I'm not sure why people would do that. Samba mounts do not seem appropriate for system or home partitions anyway... So maybe I tried to solve a problem which nobody had in the first place. Can the CIFS people please share their views on that? That's also a question for Harald: Harald, what were you doing with the cifs driver which led you to discovering this bug? > --------8<-------------- > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index 65d9771e49f9..4f1f744ea3cd 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -1591,18 +1591,6 @@ MODULE_DESCRIPTION > ("VFS to access SMB3 servers e.g. Samba, Macs, Azure and Windows (and " > "also older servers complying with the SNIA CIFS Specification)"); > MODULE_VERSION(CIFS_VERSION); > -MODULE_SOFTDEP("pre: arc4"); > -MODULE_SOFTDEP("pre: des"); > -MODULE_SOFTDEP("pre: ecb"); > -MODULE_SOFTDEP("pre: hmac"); > -MODULE_SOFTDEP("pre: md4"); > -MODULE_SOFTDEP("pre: md5"); > -MODULE_SOFTDEP("pre: nls"); > -MODULE_SOFTDEP("pre: aes"); > -MODULE_SOFTDEP("pre: cmac"); > -MODULE_SOFTDEP("pre: sha256"); > -MODULE_SOFTDEP("pre: sha512"); > -MODULE_SOFTDEP("pre: aead2"); > -MODULE_SOFTDEP("pre: ccm"); > +MODULE_SOFTDEP("pre: arc4 des ecb hmac md4 md5 nls aes cmac sha256 sha512 aead2 ccm"); > module_init(init_cifs) > module_exit(exit_cifs) > --------8<-------------- > > I guess we could actually implement additive deps, e.g. by using > "pre+:" or something else. But I think the right thing to do for now is > to apply something like above to cifs and propagate it to stable. Patch looks sane, thanks. Reviewed-by: Jean Delvare <jdelvare@suse.de> Are you going to submit it or should I? Alternatively the initial patch could be reverted, depending on the answer to my question above. -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: multiple softdeps 2019-06-29 9:30 ` Jean Delvare @ 2019-06-30 5:57 ` Steve French 2019-07-01 17:33 ` Paulo Alcantara 0 siblings, 1 reply; 5+ messages in thread From: Steve French @ 2019-06-30 5:57 UTC (permalink / raw) To: Jean Delvare Cc: Lucas De Marchi, Harald Hoyer, linux-modules, Pavel Shilovsky, CIFS, Paulo Alcantara > Or we could decide that cifs in initrd is not supported? Paulo had been experimenting with booting from cifs.ko (over SMB3.11 mounts to Samba). Presumably since SMB3/SMB3.11 family of protocols is the most common network file system across a pretty broad variety of operating systems it is easier to imagine it being extended for special cases like booting the OS (it is already very feature rich compared to most network/cluster file system protocols and has exhaustive detailed documentation). My gut reaction is that if cifs.ko (SMB3.11 mounts) don't work with initrd ... we need to fix that even if it means minor protocol extensions, but Paulo might have more data. On Sat, Jun 29, 2019 at 4:31 AM Jean Delvare <jdelvare@suse.de> wrote: > > Hi Lucas, Harald, > > On Fri, 28 Jun 2019 09:25:17 -0700, Lucas De Marchi wrote: > > +cifs, +Jean, +Pavel > > > > On Fri, Jun 28, 2019 at 03:16:35PM +0200, Harald Hoyer wrote: > > >Hi, > > > > > >could you please enlighten me about kernel module softdeps? > > > > > >$ modinfo cifs | grep soft > > >softdep: pre: ccm > > >softdep: pre: aead2 > > >softdep: pre: sha512 > > >softdep: pre: sha256 > > >softdep: pre: cmac > > >softdep: pre: aes > > >softdep: pre: nls > > >softdep: pre: md5 > > >softdep: pre: md4 > > >softdep: pre: hmac > > >softdep: pre: ecb > > >softdep: pre: des > > >softdep: pre: arc4 > > > > > >$ grep cifs /lib/modules/$(uname -r)/modules.softdep > > >softdep cifs pre: ccm > > >softdep cifs pre: aead2 > > >softdep cifs pre: sha512 > > >softdep cifs pre: sha256 > > >softdep cifs pre: cmac > > >softdep cifs pre: aes > > >softdep cifs pre: nls > > >softdep cifs pre: md5 > > >softdep cifs pre: md4 > > >softdep cifs pre: hmac > > >softdep cifs pre: ecb > > >softdep cifs pre: des > > >softdep cifs pre: arc4 > > > > this is your bug. Multiple softdeps are not additive to the previous > > configuration, we never supported that. Commit b9be76d585d4 ("cifs: Add > > That's my fault then, sorry about that. At the time I submitted the > patch, there was no occurrence of module having multiple softdeps, so I > didn't know how it should be done. It was some time ago and I don't > remember the details though, and I have no trace of why I worked on > this in the first place. > > Apparently I'm not the only one confused as I see driver > pcengines-apuv2 has exactly the same problem. I'll send a patch for > that one. > > If multiple softdep statements are not supported then shouldn't we > prevent them from happening, either with a link-time check, or with a > checkpatch test? I have no idea how to implement such a check though. > > > soft dependencies") added it for the wrong reasons actually. A sotfdep > > means kmod will actually load those dependencies before loading the > > module (or fail to load it if those dependencies don't exist). > > That's my definition of a regular (not soft) dependency. If failing > soft pre dependencies are fatal, how do they differ from regular > dependencies? Only because they are listed explicitly instead of being > determined from symbol use? > > FWIW modprobe.d(5) mentions "optional modules" and insists that the > main module is still usable without them, so my understanding was that > a modprobe would be attempted on soft deps but a failure would be > ignored. > > > Besides the wrong commit message, if that is indeed what is desired, the > > fix would be: > > The commit message may sound wrong to you but the (fixed version of the > patch) still solves the problem (including all potentially needed > modules in initrd even when there are no hard dependencies) at least as > a side effect. If you think that softdeps are not how the problem > should be solved, then how do you think it should be solved? I > understand that pre-loading all cipher and hash modules is not > efficient, but we still need to ensure that they all make it to the > initrd in case they are needed. > > Or we could decide that cifs in initrd is not supported? To be honest > I'm not sure why people would do that. Samba mounts do not seem > appropriate for system or home partitions anyway... So maybe I tried to > solve a problem which nobody had in the first place. Can the CIFS > people please share their views on that? That's also a question for > Harald: Harald, what were you doing with the cifs driver which led you > to discovering this bug? > > > --------8<-------------- > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index 65d9771e49f9..4f1f744ea3cd 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -1591,18 +1591,6 @@ MODULE_DESCRIPTION > > ("VFS to access SMB3 servers e.g. Samba, Macs, Azure and Windows (and " > > "also older servers complying with the SNIA CIFS Specification)"); > > MODULE_VERSION(CIFS_VERSION); > > -MODULE_SOFTDEP("pre: arc4"); > > -MODULE_SOFTDEP("pre: des"); > > -MODULE_SOFTDEP("pre: ecb"); > > -MODULE_SOFTDEP("pre: hmac"); > > -MODULE_SOFTDEP("pre: md4"); > > -MODULE_SOFTDEP("pre: md5"); > > -MODULE_SOFTDEP("pre: nls"); > > -MODULE_SOFTDEP("pre: aes"); > > -MODULE_SOFTDEP("pre: cmac"); > > -MODULE_SOFTDEP("pre: sha256"); > > -MODULE_SOFTDEP("pre: sha512"); > > -MODULE_SOFTDEP("pre: aead2"); > > -MODULE_SOFTDEP("pre: ccm"); > > +MODULE_SOFTDEP("pre: arc4 des ecb hmac md4 md5 nls aes cmac sha256 sha512 aead2 ccm"); > > module_init(init_cifs) > > module_exit(exit_cifs) > > --------8<-------------- > > > > I guess we could actually implement additive deps, e.g. by using > > "pre+:" or something else. But I think the right thing to do for now is > > to apply something like above to cifs and propagate it to stable. > > Patch looks sane, thanks. > > Reviewed-by: Jean Delvare <jdelvare@suse.de> > > Are you going to submit it or should I? > > Alternatively the initial patch could be reverted, depending on the > answer to my question above. > > -- > Jean Delvare > SUSE L3 Support -- Thanks, Steve ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: multiple softdeps 2019-06-30 5:57 ` Steve French @ 2019-07-01 17:33 ` Paulo Alcantara 0 siblings, 0 replies; 5+ messages in thread From: Paulo Alcantara @ 2019-07-01 17:33 UTC (permalink / raw) To: Steve French, Jean Delvare Cc: Lucas De Marchi, Harald Hoyer, linux-modules, Pavel Shilovsky, CIFS [-- Attachment #1: Type: text/plain, Size: 996 bytes --] Steve French <smfrench@gmail.com> writes: >> Or we could decide that cifs in initrd is not supported? > > Paulo had been experimenting with booting from cifs.ko (over SMB3.11 > mounts to Samba). > > Presumably since SMB3/SMB3.11 family of protocols is the most common > network file system across a pretty broad variety of operating systems > it is easier to imagine it being extended for special cases like > booting the OS (it is already very feature rich compared to most > network/cluster file system protocols and has exhaustive detailed > documentation). > > My gut reaction is that if cifs.ko (SMB3.11 mounts) don't work with > initrd ... we need to fix that even if it means minor protocol > extensions, but Paulo might have more data. I was able to boot a Leap 15 system from cifs.ko with changes in [1], so IMHO, it would probably make sense to support cifs in initrd as well -- though I haven't tested it myself. Thanks, Paulo [1] https://git.paulo.ac/linux.git/commit/?h=smb-boot [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-01 17:44 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-28 13:16 multiple softdeps Harald Hoyer 2019-06-28 16:25 ` Lucas De Marchi 2019-06-29 9:30 ` Jean Delvare 2019-06-30 5:57 ` Steve French 2019-07-01 17:33 ` Paulo Alcantara
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).