| Message ID | 1587187200-13109-3-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] selftests: kmod: Use variable NAME in kmod_test_0001() | expand |
On Sat, Apr 18, 2020 at 01:19:59PM +0800, Tiezhu Yang wrote: > If module name is empty, it is better to return directly at the beginning > of request_module() without doing the needless call_modprobe() operation. > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > kernel/kmod.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/kernel/kmod.c b/kernel/kmod.c > index 3cd075c..5851444 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -28,6 +28,8 @@ > > #include <trace/events/module.h> > > +#define MODULE_NOT_FOUND 256 > + > /* > * Assuming: > * > @@ -144,6 +146,9 @@ int __request_module(bool wait, const char *fmt, ...) > if (ret >= MODULE_NAME_LEN) > return -ENAMETOOLONG; > > + if (strlen(module_name) == 0) > + return MODULE_NOT_FOUND; I'd rather we just use something standard like -EINVAL. What do we return if its not found? Then use that value. > + > ret = security_kernel_module_request(module_name); > if (ret) > return ret; > -- > 2.1.0 >
On Fri, Apr 17, 2020 at 11:45 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Sat, Apr 18, 2020 at 01:19:59PM +0800, Tiezhu Yang wrote: > > If module name is empty, it is better to return directly at the beginning > > of request_module() without doing the needless call_modprobe() operation. > > > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > > --- > > kernel/kmod.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/kernel/kmod.c b/kernel/kmod.c > > index 3cd075c..5851444 100644 > > --- a/kernel/kmod.c > > +++ b/kernel/kmod.c > > @@ -28,6 +28,8 @@ > > > > #include <trace/events/module.h> > > > > +#define MODULE_NOT_FOUND 256 > > + > > /* > > * Assuming: > > * > > @@ -144,6 +146,9 @@ int __request_module(bool wait, const char *fmt, ...) > > if (ret >= MODULE_NAME_LEN) > > return -ENAMETOOLONG; > > > > + if (strlen(module_name) == 0) > > + return MODULE_NOT_FOUND; > > I'd rather we just use something standard like -EINVAL. > What do we return if its not found? Then use that value. Also, are we testing for this condition yet? If not can we add one?
On 04/18/2020 01:48 PM, Luis Chamberlain wrote: > On Fri, Apr 17, 2020 at 11:45 PM Luis Chamberlain <mcgrof@kernel.org> wrote: >> On Sat, Apr 18, 2020 at 01:19:59PM +0800, Tiezhu Yang wrote: >>> If module name is empty, it is better to return directly at the beginning >>> of request_module() without doing the needless call_modprobe() operation. >>> >>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >>> --- >>> kernel/kmod.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/kernel/kmod.c b/kernel/kmod.c >>> index 3cd075c..5851444 100644 >>> --- a/kernel/kmod.c >>> +++ b/kernel/kmod.c >>> @@ -28,6 +28,8 @@ >>> >>> #include <trace/events/module.h> >>> >>> +#define MODULE_NOT_FOUND 256 >>> + >>> /* >>> * Assuming: >>> * >>> @@ -144,6 +146,9 @@ int __request_module(bool wait, const char *fmt, ...) >>> if (ret >= MODULE_NAME_LEN) >>> return -ENAMETOOLONG; >>> >>> + if (strlen(module_name) == 0) >>> + return MODULE_NOT_FOUND; >> I'd rather we just use something standard like -EINVAL. >> What do we return if its not found? Then use that value. > Also, are we testing for this condition yet? If not can we add one? Yes, kmod_test_0001_driver() in tools/testing/selftests/kmod/kmod.sh tests this case and expects result MODULE_NOT_FOUND which is 256.
On Sat, Apr 18, 2020 at 01:58:45PM +0800, Tiezhu Yang wrote: > On 04/18/2020 01:48 PM, Luis Chamberlain wrote: > > On Fri, Apr 17, 2020 at 11:45 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > On Sat, Apr 18, 2020 at 01:19:59PM +0800, Tiezhu Yang wrote: > > > > If module name is empty, it is better to return directly at the beginning > > > > of request_module() without doing the needless call_modprobe() operation. > > > > > > > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > > > > --- > > > > kernel/kmod.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/kernel/kmod.c b/kernel/kmod.c > > > > index 3cd075c..5851444 100644 > > > > --- a/kernel/kmod.c > > > > +++ b/kernel/kmod.c > > > > @@ -28,6 +28,8 @@ > > > > > > > > #include <trace/events/module.h> > > > > > > > > +#define MODULE_NOT_FOUND 256 > > > > + > > > > /* > > > > * Assuming: > > > > * > > > > @@ -144,6 +146,9 @@ int __request_module(bool wait, const char *fmt, ...) > > > > if (ret >= MODULE_NAME_LEN) > > > > return -ENAMETOOLONG; > > > > > > > > + if (strlen(module_name) == 0) > > > > + return MODULE_NOT_FOUND; > > > I'd rather we just use something standard like -EINVAL. > > > What do we return if its not found? Then use that value. > > Also, are we testing for this condition yet? If not can we add one? > > Yes, kmod_test_0001_driver() in tools/testing/selftests/kmod/kmod.sh tests > this case and expects result MODULE_NOT_FOUND which is 256. OK I see now I had put: errno_name_to_val() { case "$1" in # kmod calls modprobe and upon of a module not found # modprobe returns just 1... However in the # kernel we *sometimes* see 256... MODULE_NOT_FOUND) echo 256;; I found that through testing, however there was nothing set in stone, nothing documented. While you are at it, can you find the places where this is returned in the kernel code? We should clear this up and se things straight. We cannot change what we gave userspace already though. Luis
On 04/18/2020 03:19 PM, Luis Chamberlain wrote: > On Sat, Apr 18, 2020 at 01:58:45PM +0800, Tiezhu Yang wrote: >> On 04/18/2020 01:48 PM, Luis Chamberlain wrote: >>> On Fri, Apr 17, 2020 at 11:45 PM Luis Chamberlain <mcgrof@kernel.org> wrote: >>>> On Sat, Apr 18, 2020 at 01:19:59PM +0800, Tiezhu Yang wrote: >>>>> If module name is empty, it is better to return directly at the beginning >>>>> of request_module() without doing the needless call_modprobe() operation. >>>>> >>>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >>>>> --- >>>>> kernel/kmod.c | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/kernel/kmod.c b/kernel/kmod.c >>>>> index 3cd075c..5851444 100644 >>>>> --- a/kernel/kmod.c >>>>> +++ b/kernel/kmod.c >>>>> @@ -28,6 +28,8 @@ >>>>> >>>>> #include <trace/events/module.h> >>>>> >>>>> +#define MODULE_NOT_FOUND 256 >>>>> + >>>>> /* >>>>> * Assuming: >>>>> * >>>>> @@ -144,6 +146,9 @@ int __request_module(bool wait, const char *fmt, ...) >>>>> if (ret >= MODULE_NAME_LEN) >>>>> return -ENAMETOOLONG; >>>>> >>>>> + if (strlen(module_name) == 0) >>>>> + return MODULE_NOT_FOUND; >>>> I'd rather we just use something standard like -EINVAL. >>>> What do we return if its not found? Then use that value. >>> Also, are we testing for this condition yet? If not can we add one? >> Yes, kmod_test_0001_driver() in tools/testing/selftests/kmod/kmod.sh tests >> this case and expects result MODULE_NOT_FOUND which is 256. > OK I see now I had put: > > errno_name_to_val() > { > case "$1" in > # kmod calls modprobe and upon of a module not found > # modprobe returns just 1... However in the > # kernel we *sometimes* see 256... > MODULE_NOT_FOUND) > echo 256;; > > I found that through testing, however there was nothing set in stone, > nothing documented. While you are at it, can you find the places where > this is returned in the kernel code? We should clear this up and > se things straight. We cannot change what we gave userspace already > though. Call Trace: __request_module() | | call_modprobe() | | call_usermodehelper_exec() -- retval = sub_info->retval; | | call_usermodehelper_exec_work() | | call_usermodehelper_exec_sync() -- sub_info->retval = ret; | | --> call_usermodehelper_exec_async() --> do_execve() | kernel_wait4(pid, (int __user *)&ret, 0, NULL); __request_module() returns the exist status of child process, if module name is empty or non-exist, sub_info->retval is 256 after call kernel_wait4(). Should I add this analysis to the commit message? Thanks, Tiezhu Yang > > Luis
On 04/20/2020 12:08 PM, Tiezhu Yang wrote: > On 04/18/2020 03:19 PM, Luis Chamberlain wrote: >> On Sat, Apr 18, 2020 at 01:58:45PM +0800, Tiezhu Yang wrote: >>> On 04/18/2020 01:48 PM, Luis Chamberlain wrote: >>>> On Fri, Apr 17, 2020 at 11:45 PM Luis Chamberlain >>>> <mcgrof@kernel.org> wrote: >>>>> On Sat, Apr 18, 2020 at 01:19:59PM +0800, Tiezhu Yang wrote: >>>>>> If module name is empty, it is better to return directly at the >>>>>> beginning >>>>>> of request_module() without doing the needless call_modprobe() >>>>>> operation. >>>>>> >>>>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >>>>>> --- >>>>>> kernel/kmod.c | 5 +++++ >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/kernel/kmod.c b/kernel/kmod.c >>>>>> index 3cd075c..5851444 100644 >>>>>> --- a/kernel/kmod.c >>>>>> +++ b/kernel/kmod.c >>>>>> @@ -28,6 +28,8 @@ >>>>>> >>>>>> #include <trace/events/module.h> >>>>>> >>>>>> +#define MODULE_NOT_FOUND 256 >>>>>> + >>>>>> /* >>>>>> * Assuming: >>>>>> * >>>>>> @@ -144,6 +146,9 @@ int __request_module(bool wait, const char >>>>>> *fmt, ...) >>>>>> if (ret >= MODULE_NAME_LEN) >>>>>> return -ENAMETOOLONG; >>>>>> >>>>>> + if (strlen(module_name) == 0) >>>>>> + return MODULE_NOT_FOUND; >>>>> I'd rather we just use something standard like -EINVAL. >>>>> What do we return if its not found? Then use that value. >>>> Also, are we testing for this condition yet? If not can we add one? >>> Yes, kmod_test_0001_driver() in tools/testing/selftests/kmod/kmod.sh >>> tests >>> this case and expects result MODULE_NOT_FOUND which is 256. >> OK I see now I had put: >> >> errno_name_to_val() >> { >> case "$1" in >> # kmod calls modprobe and upon of a module not found >> # modprobe returns just 1... However in the >> # kernel we *sometimes* see 256... >> MODULE_NOT_FOUND) >> echo 256;; >> >> I found that through testing, however there was nothing set in stone, >> nothing documented. While you are at it, can you find the places where >> this is returned in the kernel code? We should clear this up and >> se things straight. We cannot change what we gave userspace already >> though. > > Call Trace: > > __request_module() > | > | > call_modprobe() > | > | > call_usermodehelper_exec() -- retval = sub_info->retval; > | > | > call_usermodehelper_exec_work() > | > | > call_usermodehelper_exec_sync() -- sub_info->retval = ret; > | > | --> call_usermodehelper_exec_async() --> do_execve() > | > kernel_wait4(pid, (int __user *)&ret, 0, NULL); > > __request_module() returns the exist status of child process, if > module name Sorry for the typo: exist status --> exit status > is empty or non-exist, sub_info->retval is 256 after call kernel_wait4(). > > Should I add this analysis to the commit message? > > Thanks, > Tiezhu Yang > >> >> Luis >
diff --git a/kernel/kmod.c b/kernel/kmod.c index 3cd075c..5851444 100644 --- a/kernel/kmod.c +++ b/kernel/kmod.c @@ -28,6 +28,8 @@ #include <trace/events/module.h> +#define MODULE_NOT_FOUND 256 + /* * Assuming: * @@ -144,6 +146,9 @@ int __request_module(bool wait, const char *fmt, ...) if (ret >= MODULE_NAME_LEN) return -ENAMETOOLONG; + if (strlen(module_name) == 0) + return MODULE_NOT_FOUND; + ret = security_kernel_module_request(module_name); if (ret) return ret;
If module name is empty, it is better to return directly at the beginning of request_module() without doing the needless call_modprobe() operation. Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- kernel/kmod.c | 5 +++++ 1 file changed, 5 insertions(+)