Messages in this thread | | | Subject | Re: [PATCH V4 2/9] jump_label: Add the jump_label_can_update_check() helper | From | Daniel Bristot de Oliveira <> | Date | Tue, 5 Feb 2019 14:50:39 +0100 |
| |
Hi Borislav!
On 2/5/19 8:22 AM, Borislav Petkov wrote: >> Subject: Re: [PATCH V4 2/9] jump_label: Add the jump_label_can_update_check() helper > > s/the/a/
ack!
> On Mon, Feb 04, 2019 at 08:58:55PM +0100, Daniel Bristot de Oliveira wrote: >> Move the check of if a jump_entry is valid to a function. > > s/of //
ack!
>> diff --git a/kernel/jump_label.c b/kernel/jump_label.c >> index 288d630da22d..456c0d7cbb5b 100644 >> --- a/kernel/jump_label.c >> +++ b/kernel/jump_label.c >> @@ -374,22 +374,32 @@ static enum jump_label_type jump_label_type(struct jump_entry *entry) >> return enabled ^ branch; >> } >> >> +bool jump_label_can_update_check(struct jump_entry *entry, bool init) > > static. > > Also, "jump_label_can_update" is sufficient for a name AFAICT.
sounds better indeed.
>> +{ >> + /* >> + * An entry->code of 0 indicates an entry which has been >> + * disabled because it was in an init text area. >> + */ >> + if (init || !jump_entry_is_init(entry)) { >> + if (!kernel_text_address(jump_entry_code(entry))) { >> + WARN_ONCE(1, "can't patch jump_label at %pS", >> + (void *)jump_entry_code(entry)); >> + return 0; >> + } >> + return 1; >> + } >> + return 0; > > Those should be bools which it returns, no? > > Also, I'd do the function this way, to make it more readable and not > have three returns back-to-back. :) > > /* > * An entry->code of 0 indicates an entry which has been disabled because it > * was in an init text area. > */ > bool jump_label_can_update(struct jump_entry *entry, bool init) > { > if (!init && jump_entry_is_init(entry)) > return false; > > if (WARN_ON_ONCE(!kernel_text_address(jump_entry_code(entry))), > "can't patch jump_label at %pS", (void *)jump_entry_code(entry)) > return false; > > return true; > } > > That second check could be even: > > if (WARN_ON_ONCE(!kernel_text_address(jump_entry_code(entry))), > "can't patch jump_label at %pS", (void *)jump_entry_code(entry)) > return false; > > but that's not more readable than above, I'd say.
Agreed!
> >> static void __jump_label_update(struct static_key *key, >> struct jump_entry *entry, >> struct jump_entry *stop, >> bool init) >> { >> for_each_label_entry(key, entry, stop) { >> - /* >> - * An entry->code of 0 indicates an entry which has been >> - * disabled because it was in an init text area. >> - */ >> - if (init || !jump_entry_is_init(entry)) { >> - if (kernel_text_address(jump_entry_code(entry))) >> - arch_jump_label_transform(entry, jump_label_type(entry)); >> - else >> - WARN_ONCE(1, "can't patch jump_label at %pS", >> - (void *)jump_entry_code(entry)); >> + if (jump_label_can_update_check(entry, init)) { >> + arch_jump_label_transform(entry, >> + jump_label_type(entry)); > > Yeah, let that one stick out.
I did not get this part...
Thanks!
-- Daniel
| |