Now that notifiers got unbroken; use the proper interface to handle
notifier errors and propagate them.
There were already MODULE_STATE_COMING notifiers that failed; notably:
- jump_label_module_notifier()
- tracepoint_module_notify()
- bpf_event_notify()
By propagating this error, we fix those users.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Cc: [email protected]
---
kernel/module.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3751,9 +3751,13 @@ static int prepare_coming_module(struct
if (err)
return err;
- blocking_notifier_call_chain(&module_notify_list,
- MODULE_STATE_COMING, mod);
- return 0;
+ err = blocking_notifier_call_chain_robust(&module_notify_list,
+ MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
+ err = notifier_to_errno(err);
+ if (err)
+ klp_module_going(mod);
+
+ return err;
}
static int unknown_module_param_cb(char *param, char *val, const char *modname,
+++ Peter Zijlstra [24/03/20 14:56 +0100]:
>Now that notifiers got unbroken; use the proper interface to handle
>notifier errors and propagate them.
>
>There were already MODULE_STATE_COMING notifiers that failed; notably:
>
> - jump_label_module_notifier()
> - tracepoint_module_notify()
> - bpf_event_notify()
>
>By propagating this error, we fix those users.
>
>Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>Cc: [email protected]
>---
> kernel/module.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3751,9 +3751,13 @@ static int prepare_coming_module(struct
> if (err)
> return err;
>
>- blocking_notifier_call_chain(&module_notify_list,
>- MODULE_STATE_COMING, mod);
>- return 0;
>+ err = blocking_notifier_call_chain_robust(&module_notify_list,
>+ MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
>+ err = notifier_to_errno(err);
>+ if (err)
>+ klp_module_going(mod);
>+
>+ return err;
> }
>
> static int unknown_module_param_cb(char *param, char *val, const char *modname,
>
This looks fine to me - klp_module_going() is only called after
successful klp_module_coming(), and klp_module_going() is fine with
mod->state still being MODULE_STATE_COMING here. Would be good to have
livepatch folks double check. Which reminds me - Miroslav had pointed
out in the past that if there is an error when calling the COMING
notifiers, the GOING notifiers will be called while the mod->state is
still MODULE_STATE_COMING. I've briefly looked through all the module
notifiers and it looks like nobody is looking at mod->state directly
at least.
Acked-by: Jessica Yu <[email protected]>
On Wed, Mar 25, 2020 at 06:35:22PM +0100, Jessica Yu wrote:
> +++ Peter Zijlstra [24/03/20 14:56 +0100]:
> > Now that notifiers got unbroken; use the proper interface to handle
> > notifier errors and propagate them.
> >
> > There were already MODULE_STATE_COMING notifiers that failed; notably:
> >
> > - jump_label_module_notifier()
> > - tracepoint_module_notify()
> > - bpf_event_notify()
> >
> > By propagating this error, we fix those users.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Cc: [email protected]
> > ---
> > kernel/module.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3751,9 +3751,13 @@ static int prepare_coming_module(struct
> > if (err)
> > return err;
> >
> > - blocking_notifier_call_chain(&module_notify_list,
> > - MODULE_STATE_COMING, mod);
> > - return 0;
> > + err = blocking_notifier_call_chain_robust(&module_notify_list,
> > + MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
> > + err = notifier_to_errno(err);
> > + if (err)
> > + klp_module_going(mod);
> > +
> > + return err;
> > }
> >
> > static int unknown_module_param_cb(char *param, char *val, const char *modname,
> >
>
> This looks fine to me - klp_module_going() is only called after
> successful klp_module_coming(), and klp_module_going() is fine with
> mod->state still being MODULE_STATE_COMING here. Would be good to have
> livepatch folks double check. Which reminds me - Miroslav had pointed
> out in the past that if there is an error when calling the COMING
> notifiers, the GOING notifiers will be called while the mod->state is
> still MODULE_STATE_COMING. I've briefly looked through all the module
> notifiers and it looks like nobody is looking at mod->state directly
> at least.
>
> Acked-by: Jessica Yu <[email protected]>
Looks good to me. klp_module_going() is already called in other
load_module() error scenarios so this should be fine from a livepatch
standpoint.
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
On Wed, 25 Mar 2020, Jessica Yu wrote:
> +++ Peter Zijlstra [24/03/20 14:56 +0100]:
> >Now that notifiers got unbroken; use the proper interface to handle
> >notifier errors and propagate them.
> >
> >There were already MODULE_STATE_COMING notifiers that failed; notably:
> >
> > - jump_label_module_notifier()
> > - tracepoint_module_notify()
> > - bpf_event_notify()
> >
> >By propagating this error, we fix those users.
> >
> >Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> >Cc: [email protected]
> >---
> > kernel/module.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -3751,9 +3751,13 @@ static int prepare_coming_module(struct
> > if (err)
> > return err;
> >
> >- blocking_notifier_call_chain(&module_notify_list,
> >- MODULE_STATE_COMING, mod);
> >- return 0;
> >+ err = blocking_notifier_call_chain_robust(&module_notify_list,
> >+ MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
> >+ err = notifier_to_errno(err);
> >+ if (err)
> >+ klp_module_going(mod);
> >+
> >+ return err;
> > }
> >
> > static int unknown_module_param_cb(char *param, char *val, const char
> > *modname,
> >
>
> This looks fine to me - klp_module_going() is only called after
> successful klp_module_coming(), and klp_module_going() is fine with
> mod->state still being MODULE_STATE_COMING here. Would be good to have
> livepatch folks double check.
Yes, it is ok.
> Which reminds me - Miroslav had pointed
> out in the past that if there is an error when calling the COMING
> notifiers, the GOING notifiers will be called while the mod->state is
> still MODULE_STATE_COMING. I've briefly looked through all the module
> notifiers and it looks like nobody is looking at mod->state directly
> at least.
Thanks for double-checking. I triple-checked and yes, it should be fine.
All module notifiers check the value from the function parameter and not
mod->state directly.
Reviewed-by: Miroslav Benes <[email protected]>
M