* [PATCH 1/2] module: merge repetitive strings in module_sig_check()
2020-10-13 20:32 [PATCH 0/2] module: some refactoring in module_sig_check() Sergey Shtylyov
@ 2020-10-13 20:34 ` Sergey Shtylyov
2020-10-13 20:35 ` [PATCH 2/2] module: unindent comments " Sergey Shtylyov
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2020-10-13 20:34 UTC (permalink / raw)
To: Jessica Yu, linux-kernel
The 'reason' variable in module_sig_check() points to 3 strings across
the *switch* statement, all needlessly starting with the same text. Let's
put as much of the starting text as we can into the pr_notice() call (this
includes some rewording of the 1st message) -- it saves 37 bytes of object
code (x86 gcc 10.2.1).
Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
---
kernel/module.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
Index: linux/kernel/module.c
===================================================================
--- linux.orig/kernel/module.c
+++ linux/kernel/module.c
@@ -2906,16 +2906,17 @@ static int module_sig_check(struct load_
* enforcing, certain errors are non-fatal.
*/
case -ENODATA:
- reason = "Loading of unsigned module";
+ reason = "no signature";
goto decide;
case -ENOPKG:
- reason = "Loading of module with unsupported crypto";
+ reason = "unsupported crypto";
goto decide;
case -ENOKEY:
- reason = "Loading of module with unavailable key";
+ reason = "unavailable key";
decide:
if (is_module_sig_enforced()) {
- pr_notice("%s: %s is rejected\n", info->name, reason);
+ pr_notice("%s: loading of module with %s is rejected\n",
+ info->name, reason);
return -EKEYREJECTED;
}
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 2/2] module: unindent comments in module_sig_check()
2020-10-13 20:32 [PATCH 0/2] module: some refactoring in module_sig_check() Sergey Shtylyov
2020-10-13 20:34 ` [PATCH 1/2] module: merge repetitive strings " Sergey Shtylyov
@ 2020-10-13 20:35 ` Sergey Shtylyov
2020-10-13 22:44 ` [PATCH 0/2] module: some refactoring " Joe Perches
2020-10-22 15:09 ` Jessica Yu
3 siblings, 0 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2020-10-13 20:35 UTC (permalink / raw)
To: Jessica Yu, linux-kernel
The way the comments in the *switch* statement in module_sig_check() are
indented, it may seem they refer to the statements above them, not below.
Align the comments with the *case* and *default* labels below them, fixing
the comment style and adding article/dash, while at it...
Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
---
kernel/module.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
Index: linux/kernel/module.c
===================================================================
--- linux.orig/kernel/module.c
+++ linux/kernel/module.c
@@ -2901,10 +2901,11 @@ static int module_sig_check(struct load_
info->sig_ok = true;
return 0;
- /* We don't permit modules to be loaded into trusted kernels
- * without a valid signature on them, but if we're not
- * enforcing, certain errors are non-fatal.
- */
+ /*
+ * We don't permit modules to be loaded into the trusted kernels
+ * without a valid signature on them, but if we're not enforcing,
+ * certain errors are non-fatal.
+ */
case -ENODATA:
reason = "no signature";
goto decide;
@@ -2922,10 +2923,10 @@ static int module_sig_check(struct load_
return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
- /* All other errors are fatal, including nomem, unparseable
- * signatures and signature check failures - even if signatures
- * aren't required.
- */
+ /*
+ * All other errors are fatal, including nomem, unparseable signatures
+ * and signature check failures -- even if signatures aren't required.
+ */
default:
return err;
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] module: some refactoring in module_sig_check()
2020-10-13 20:32 [PATCH 0/2] module: some refactoring in module_sig_check() Sergey Shtylyov
2020-10-13 20:34 ` [PATCH 1/2] module: merge repetitive strings " Sergey Shtylyov
2020-10-13 20:35 ` [PATCH 2/2] module: unindent comments " Sergey Shtylyov
@ 2020-10-13 22:44 ` Joe Perches
2020-10-14 8:18 ` Sergey Shtylyov
2020-10-22 15:09 ` Jessica Yu
3 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2020-10-13 22:44 UTC (permalink / raw)
To: Sergey Shtylyov, Jessica Yu, linux-kernel
On Tue, 2020-10-13 at 23:32 +0300, Sergey Shtylyov wrote:
> Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo.
> I'm doing some little refactoring in module_sig_check()...
>
> [1/2] module: merge repetitive strings in module_sig_check()
> [2/2] module: unindent comments in module_sig_check()
I think this code is rather cryptic and could be made clearer.
How about:
---
kernel/module.c | 51 ++++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index c075a18103fb..98b3af96a5bd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2884,46 +2884,47 @@ static int module_sig_check(struct load_info *info, int flags)
* Require flags == 0, as a module with version information
* removed is no longer the module that was signed
*/
- if (flags == 0 &&
- info->len > markerlen &&
- memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
+ if (flags == 0 && info->len > markerlen &&
+ !memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen)) {
/* We truncate the module to discard the signature */
info->len -= markerlen;
err = mod_verify_sig(mod, info);
+ if (!err) {
+ info->sig_ok = true;
+ return 0;
+ }
}
+ /*
+ * We don't permit modules to be loaded into trusted kernels
+ * without a valid signature on them, but if we're not enforcing,
+ * some errors are non-fatal.
+ */
switch (err) {
- case 0:
- info->sig_ok = true;
- return 0;
-
- /* We don't permit modules to be loaded into trusted kernels
- * without a valid signature on them, but if we're not
- * enforcing, certain errors are non-fatal.
- */
case -ENODATA:
- reason = "Loading of unsigned module";
- goto decide;
+ reason = "unsigned module";
+ break;
case -ENOPKG:
- reason = "Loading of module with unsupported crypto";
- goto decide;
+ reason = "module with unsupported crypto";
+ break;
case -ENOKEY:
- reason = "Loading of module with unavailable key";
- decide:
- if (is_module_sig_enforced()) {
- pr_notice("%s: %s is rejected\n", info->name, reason);
- return -EKEYREJECTED;
- }
-
- return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
-
+ reason = "module with unavailable key";
+ break;
+ default:
/* All other errors are fatal, including nomem, unparseable
* signatures and signature check failures - even if signatures
* aren't required.
*/
- default:
return err;
}
+
+ if (is_module_sig_enforced()) {
+ pr_notice("%s: loading of %s is rejected\n",
+ info->name, reason);
+ return -EKEYREJECTED;
+ }
+
+ return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
}
#else /* !CONFIG_MODULE_SIG */
static int module_sig_check(struct load_info *info, int flags)
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 0/2] module: some refactoring in module_sig_check()
2020-10-13 22:44 ` [PATCH 0/2] module: some refactoring " Joe Perches
@ 2020-10-14 8:18 ` Sergey Shtylyov
2020-10-14 8:35 ` Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Shtylyov @ 2020-10-14 8:18 UTC (permalink / raw)
To: Joe Perches, Jessica Yu, linux-kernel
Hello!
On 14.10.2020 1:44, Joe Perches wrote:
>> Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo.
>> I'm doing some little refactoring in module_sig_check()...
>>
>> [1/2] module: merge repetitive strings in module_sig_check()
>> [2/2] module: unindent comments in module_sig_check()
>
> I think this code is rather cryptic and could be made clearer.
>
> How about:
> ---
> kernel/module.c | 51 ++++++++++++++++++++++++++-------------------------
> 1 file changed, 26 insertions(+), 25 deletions(-)
Looks good. Do you want to post complete patch(es)? :-)
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/2] module: some refactoring in module_sig_check()
2020-10-14 8:18 ` Sergey Shtylyov
@ 2020-10-14 8:35 ` Joe Perches
2020-10-14 19:44 ` Sergey Shtylyov
0 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2020-10-14 8:35 UTC (permalink / raw)
To: Sergey Shtylyov, Jessica Yu, linux-kernel
On Wed, 2020-10-14 at 11:18 +0300, Sergey Shtylyov wrote:
> Hello!
>
> On 14.10.2020 1:44, Joe Perches wrote:
>
> > > Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo.
> > > I'm doing some little refactoring in module_sig_check()...
> > >
> > > [1/2] module: merge repetitive strings in module_sig_check()
> > > [2/2] module: unindent comments in module_sig_check()
> >
> > I think this code is rather cryptic and could be made clearer.
> >
> > How about:
> > ---
> > kernel/module.c | 51 ++++++++++++++++++++++++++-------------------------
> > 1 file changed, 26 insertions(+), 25 deletions(-)
>
> Looks good. Do you want to post complete patch(es)? :-)
I don't like posting actual patches on top of other people.
It's a complete and compilable diff, it's just unsigned.
If you want a Signed-off-by: here's one:
Signed-off-by: Joe Perches <joe@perches.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] module: some refactoring in module_sig_check()
2020-10-14 8:35 ` Joe Perches
@ 2020-10-14 19:44 ` Sergey Shtylyov
2020-10-14 20:48 ` Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Shtylyov @ 2020-10-14 19:44 UTC (permalink / raw)
To: Joe Perches, Jessica Yu, linux-kernel
On 10/14/20 11:35 AM, Joe Perches wrote:
[...]
>>>> Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo.
>>>> I'm doing some little refactoring in module_sig_check()...
>>>>
>>>> [1/2] module: merge repetitive strings in module_sig_check()
>>>> [2/2] module: unindent comments in module_sig_check()
>>>
>>> I think this code is rather cryptic and could be made clearer.
>>>
>>> How about:
>>> ---
>>> kernel/module.c | 51 ++++++++++++++++++++++++++-------------------------
>>> 1 file changed, 26 insertions(+), 25 deletions(-)
>>
>> Looks good. Do you want to post complete patch(es)? :-)
>
> I don't like posting actual patches on top of other people.
> It's a complete and compilable diff, it's just unsigned.
It does too many things simultaneously, I'll need to decompose it...
> If you want a Signed-off-by: here's one:
>
> Signed-off-by: Joe Perches <joe@perches.com>
I think I'll rather use Suggested-by: where appropriate...
MBR, Sergei
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] module: some refactoring in module_sig_check()
2020-10-14 19:44 ` Sergey Shtylyov
@ 2020-10-14 20:48 ` Joe Perches
0 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2020-10-14 20:48 UTC (permalink / raw)
To: Sergey Shtylyov, Jessica Yu, linux-kernel
On Wed, 2020-10-14 at 22:44 +0300, Sergey Shtylyov wrote:
> On 10/14/20 11:35 AM, Joe Perches wrote:
>
> [...]
> > > > > Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo.
> > > > > I'm doing some little refactoring in module_sig_check()...
> > > > >
> > > > > [1/2] module: merge repetitive strings in module_sig_check()
> > > > > [2/2] module: unindent comments in module_sig_check()
> > > >
> > > > I think this code is rather cryptic and could be made clearer.
> > > >
> > > > How about:
> > > > ---
> > > > kernel/module.c | 51 ++++++++++++++++++++++++++-------------------------
> > > > 1 file changed, 26 insertions(+), 25 deletions(-)
> > >
> > > Looks good. Do you want to post complete patch(es)? :-)
> >
> > I don't like posting actual patches on top of other people.
> > It's a complete and compilable diff, it's just unsigned.
>
> It does too many things simultaneously, I'll need to decompose it...
>
> > If you want a Signed-off-by: here's one:
> >
> > Signed-off-by: Joe Perches <joe@perches.com>
>
> I think I'll rather use Suggested-by: where appropriate...
I don't think it does too many things and I'm not
a big fan of micro-patches, but no worries, do what
you want to it.
I just thought the code poor and a bit unreadable
given the internal goto in a switch/case block.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] module: some refactoring in module_sig_check()
2020-10-13 20:32 [PATCH 0/2] module: some refactoring in module_sig_check() Sergey Shtylyov
` (2 preceding siblings ...)
2020-10-13 22:44 ` [PATCH 0/2] module: some refactoring " Joe Perches
@ 2020-10-22 15:09 ` Jessica Yu
2020-10-22 15:29 ` Sergey Shtylyov
3 siblings, 1 reply; 10+ messages in thread
From: Jessica Yu @ 2020-10-22 15:09 UTC (permalink / raw)
To: Sergey Shtylyov; +Cc: linux-kernel
+++ Sergey Shtylyov [13/10/20 23:32 +0300]:
>Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo.
>I'm doing some little refactoring in module_sig_check()...
>
>[1/2] module: merge repetitive strings in module_sig_check()
>[2/2] module: unindent comments in module_sig_check()
Hi Sergey,
I'm fine with these patches, but are you still planning on sending a
v2 based on Joe Perches' suggestions?
Thanks!
Jessica
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/2] module: some refactoring in module_sig_check()
2020-10-22 15:09 ` Jessica Yu
@ 2020-10-22 15:29 ` Sergey Shtylyov
0 siblings, 0 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2020-10-22 15:29 UTC (permalink / raw)
To: Jessica Yu; +Cc: linux-kernel
Hello!
On 10/22/20 6:09 PM, Jessica Yu wrote:
>> Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo.
>> I'm doing some little refactoring in module_sig_check()...
>>
>> [1/2] module: merge repetitive strings in module_sig_check()
>> [2/2] module: unindent comments in module_sig_check()
>
> Hi Sergey,
>
> I'm fine with these patches, but are you still planning on sending a
> v2 based on Joe Perches' suggestions?
Yes, I'm going to address his feedback, as soon as I have a time.
> Thanks!
>
> Jessica
MBR, Sergei
^ permalink raw reply [flat|nested] 10+ messages in thread