Some module notifiers; such as jump_label_module_notifier(),
tracepoint_module_notify(); can fail the MODULE_STATE_COMING callback
(due to -ENOMEM for example). However module.c:prepare_coming_module()
ignores all such errors, even though this function can already fail due
to klp_module_coming().
Therefore, propagate the notifier error and ensure we call the GOING
notifier when we do fail, to ensure cleanup for all notifiers that
didn't fail. Auditing all notifiers to make sure calling GOING without
COMING first is OK found no obvious problems with that, but it did find
a whole bunch of issues with return values, so clean those up too.
Cc: Josh Poimboeuf <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Miroslav Benes <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
drivers/oprofile/buffer_sync.c | 4 ++--
kernel/module.c | 9 +++++----
kernel/trace/bpf_trace.c | 8 ++++++--
kernel/trace/trace.c | 2 +-
kernel/trace/trace_events.c | 2 +-
kernel/trace/trace_printk.c | 4 ++--
kernel/tracepoint.c | 2 +-
7 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
index ac27f3d3fbb4..4f61b1b45e0d 100644
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -116,7 +116,7 @@ module_load_notify(struct notifier_block *self, unsigned long val, void *data)
{
#ifdef CONFIG_MODULES
if (val != MODULE_STATE_COMING)
- return 0;
+ return NOTIFY_DONE;
/* FIXME: should we process all CPU buffers ? */
mutex_lock(&buffer_mutex);
@@ -124,7 +124,7 @@ module_load_notify(struct notifier_block *self, unsigned long val, void *data)
add_event_entry(MODULE_LOADED_CODE);
mutex_unlock(&buffer_mutex);
#endif
- return 0;
+ return NOTIFY_OK;
}
diff --git a/kernel/module.c b/kernel/module.c
index 80c7c09584cf..0c4831f46380 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3638,9 +3638,10 @@ static int prepare_coming_module(struct module *mod)
if (err)
return err;
- blocking_notifier_call_chain(&module_notify_list,
- MODULE_STATE_COMING, mod);
- return 0;
+ ret = blocking_notifier_call_chain(&module_notify_list,
+ MODULE_STATE_COMING, mod);
+ ret = notifier_to_errno(ret);
+ return ret;
}
static int unknown_module_param_cb(char *param, char *val, const char *modname,
@@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
err = prepare_coming_module(mod);
if (err)
- goto bug_cleanup;
+ goto coming_cleanup;
/* Module is ready to execute: parsing args may do that. */
after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f92d6ad5e080..18afa75f5208 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1302,10 +1302,11 @@ static int bpf_event_notify(struct notifier_block *nb, unsigned long op,
{
struct bpf_trace_module *btm, *tmp;
struct module *mod = module;
+ int ret = 0;
if (mod->num_bpf_raw_events == 0 ||
(op != MODULE_STATE_COMING && op != MODULE_STATE_GOING))
- return 0;
+ goto out;
mutex_lock(&bpf_module_mutex);
@@ -1315,6 +1316,8 @@ static int bpf_event_notify(struct notifier_block *nb, unsigned long op,
if (btm) {
btm->module = module;
list_add(&btm->list, &bpf_trace_modules);
+ } else {
+ ret = -ENOMEM;
}
break;
case MODULE_STATE_GOING:
@@ -1330,7 +1333,8 @@ static int bpf_event_notify(struct notifier_block *nb, unsigned long op,
mutex_unlock(&bpf_module_mutex);
- return 0;
+out:
+ return notifier_from_errno(ret);
}
static struct notifier_block bpf_module_nb = {
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1c80521fd436..6757e7392f1a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8685,7 +8685,7 @@ static int trace_module_notify(struct notifier_block *self,
break;
}
- return 0;
+ return NOTIFY_OK;
}
static struct notifier_block trace_module_nb = {
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 0ce3db67f556..32098dbab72e 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2450,7 +2450,7 @@ static int trace_module_notify(struct notifier_block *self,
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
- return 0;
+ return NOTIFY_OK;
}
static struct notifier_block trace_module_nb = {
diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index c3fd849d4a8f..d5394e02163f 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -95,7 +95,7 @@ static int module_trace_bprintk_format_notify(struct notifier_block *self,
if (val == MODULE_STATE_COMING)
hold_module_trace_bprintk_format(start, end);
}
- return 0;
+ return NOTIFY_OK;
}
/*
@@ -173,7 +173,7 @@ __init static int
module_trace_bprintk_format_notify(struct notifier_block *self,
unsigned long val, void *data)
{
- return 0;
+ return NOTIFY_OK;
}
static inline const char **
find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index df3ade14ccbd..9ce0a510e6af 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -521,7 +521,7 @@ static int tracepoint_module_notify(struct notifier_block *self,
case MODULE_STATE_UNFORMED:
break;
}
- return ret;
+ return notifier_from_errno(ret);
}
static struct notifier_block tracepoint_module_nb = {
On Mon, 17 Jun 2019 11:03:35 +0200
Peter Zijlstra <[email protected]> wrote:
> Some module notifiers; such as jump_label_module_notifier(),
> tracepoint_module_notify(); can fail the MODULE_STATE_COMING callback
> (due to -ENOMEM for example). However module.c:prepare_coming_module()
> ignores all such errors, even though this function can already fail due
> to klp_module_coming().
>
> Therefore, propagate the notifier error and ensure we call the GOING
> notifier when we do fail, to ensure cleanup for all notifiers that
> didn't fail. Auditing all notifiers to make sure calling GOING without
> COMING first is OK found no obvious problems with that, but it did find
> a whole bunch of issues with return values, so clean those up too.
>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Jiri Kosina <[email protected]>
I think this patch should get a reviewed by from Josh or Jiri just to
take a look at the change in error paths wrt klp_module_coming() and
klp_module_going() updates.
But other than that.
Reviewed-by: Steven Rostedt (VMware) <[email protected]>
-- Steve
> Cc: Miroslav Benes <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> drivers/oprofile/buffer_sync.c | 4 ++--
> kernel/module.c | 9 +++++----
> kernel/trace/bpf_trace.c | 8 ++++++--
> kernel/trace/trace.c | 2 +-
> kernel/trace/trace_events.c | 2 +-
> kernel/trace/trace_printk.c | 4 ++--
> kernel/tracepoint.c | 2 +-
> 7 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
> index ac27f3d3fbb4..4f61b1b45e0d 100644
> --- a/drivers/oprofile/buffer_sync.c
> +++ b/drivers/oprofile/buffer_sync.c
> @@ -116,7 +116,7 @@ module_load_notify(struct notifier_block *self, unsigned long val, void *data)
> {
> #ifdef CONFIG_MODULES
> if (val != MODULE_STATE_COMING)
> - return 0;
> + return NOTIFY_DONE;
>
> /* FIXME: should we process all CPU buffers ? */
> mutex_lock(&buffer_mutex);
> @@ -124,7 +124,7 @@ module_load_notify(struct notifier_block *self, unsigned long val, void *data)
> add_event_entry(MODULE_LOADED_CODE);
> mutex_unlock(&buffer_mutex);
> #endif
> - return 0;
> + return NOTIFY_OK;
> }
>
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 80c7c09584cf..0c4831f46380 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3638,9 +3638,10 @@ static int prepare_coming_module(struct module *mod)
> if (err)
> return err;
>
> - blocking_notifier_call_chain(&module_notify_list,
> - MODULE_STATE_COMING, mod);
> - return 0;
> + ret = blocking_notifier_call_chain(&module_notify_list,
> + MODULE_STATE_COMING, mod);
> + ret = notifier_to_errno(ret);
> + return ret;
> }
>
> static int unknown_module_param_cb(char *param, char *val, const char *modname,
> @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>
> err = prepare_coming_module(mod);
> if (err)
> - goto bug_cleanup;
> + goto coming_cleanup;
>
> /* Module is ready to execute: parsing args may do that. */
> after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index f92d6ad5e080..18afa75f5208 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1302,10 +1302,11 @@ static int bpf_event_notify(struct notifier_block *nb, unsigned long op,
> {
> struct bpf_trace_module *btm, *tmp;
> struct module *mod = module;
> + int ret = 0;
>
> if (mod->num_bpf_raw_events == 0 ||
> (op != MODULE_STATE_COMING && op != MODULE_STATE_GOING))
> - return 0;
> + goto out;
>
> mutex_lock(&bpf_module_mutex);
>
> @@ -1315,6 +1316,8 @@ static int bpf_event_notify(struct notifier_block *nb, unsigned long op,
> if (btm) {
> btm->module = module;
> list_add(&btm->list, &bpf_trace_modules);
> + } else {
> + ret = -ENOMEM;
> }
> break;
> case MODULE_STATE_GOING:
> @@ -1330,7 +1333,8 @@ static int bpf_event_notify(struct notifier_block *nb, unsigned long op,
>
> mutex_unlock(&bpf_module_mutex);
>
> - return 0;
> +out:
> + return notifier_from_errno(ret);
> }
>
> static struct notifier_block bpf_module_nb = {
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 1c80521fd436..6757e7392f1a 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -8685,7 +8685,7 @@ static int trace_module_notify(struct notifier_block *self,
> break;
> }
>
> - return 0;
> + return NOTIFY_OK;
> }
>
> static struct notifier_block trace_module_nb = {
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 0ce3db67f556..32098dbab72e 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2450,7 +2450,7 @@ static int trace_module_notify(struct notifier_block *self,
> mutex_unlock(&trace_types_lock);
> mutex_unlock(&event_mutex);
>
> - return 0;
> + return NOTIFY_OK;
> }
>
> static struct notifier_block trace_module_nb = {
> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index c3fd849d4a8f..d5394e02163f 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -95,7 +95,7 @@ static int module_trace_bprintk_format_notify(struct notifier_block *self,
> if (val == MODULE_STATE_COMING)
> hold_module_trace_bprintk_format(start, end);
> }
> - return 0;
> + return NOTIFY_OK;
> }
>
> /*
> @@ -173,7 +173,7 @@ __init static int
> module_trace_bprintk_format_notify(struct notifier_block *self,
> unsigned long val, void *data)
> {
> - return 0;
> + return NOTIFY_OK;
> }
> static inline const char **
> find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index df3ade14ccbd..9ce0a510e6af 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -521,7 +521,7 @@ static int tracepoint_module_notify(struct notifier_block *self,
> case MODULE_STATE_UNFORMED:
> break;
> }
> - return ret;
> + return notifier_from_errno(ret);
> }
>
> static struct notifier_block tracepoint_module_nb = {
On Mon, 17 Jun 2019, Peter Zijlstra wrote:
>
> Some module notifiers; such as jump_label_module_notifier(),
> tracepoint_module_notify(); can fail the MODULE_STATE_COMING callback
> (due to -ENOMEM for example). However module.c:prepare_coming_module()
> ignores all such errors, even though this function can already fail due
> to klp_module_coming().
It does, but there is no change from the pre-prepare_coming_module()
times. Coming notifiers were called in complete_formation(), their return
values happily ignored and going notifiers not called to clean up even
before.
> Therefore, propagate the notifier error and ensure we call the GOING
> notifier when we do fail, to ensure cleanup for all notifiers that
> didn't fail. Auditing all notifiers to make sure calling GOING without
> COMING first is OK found no obvious problems with that, but it did find
> a whole bunch of issues with return values, so clean those up too.
Jessica, do you know why coming notifiers do not return errors without
this patch (or to be precise, blocking_notifier_call_chain() return value
is not taken into the account)? We have come across the issue couple of
times already and I think there was a reason, but I cannot remember
anything and the code does not help either.
Also the situation around the return values themselves is not completely
clear. If there is no NOTIFY_STOP_MASK set, only the return value of the
last notifier called is returned, so good that you checked, Peter.
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3638,9 +3638,10 @@ static int prepare_coming_module(struct module *mod)
> if (err)
> return err;
>
> - blocking_notifier_call_chain(&module_notify_list,
> - MODULE_STATE_COMING, mod);
> - return 0;
> + ret = blocking_notifier_call_chain(&module_notify_list,
> + MODULE_STATE_COMING, mod);
> + ret = notifier_to_errno(ret);
> + return ret;
> }
>
> static int unknown_module_param_cb(char *param, char *val, const char *modname,
> @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>
> err = prepare_coming_module(mod);
> if (err)
> - goto bug_cleanup;
> + goto coming_cleanup;
Not good. klp_module_going() is not prepared to be called without
klp_module_coming() succeeding. "Funny" things might happen.
Also destroy_params() might be called without parse_args() first now.
So it calls for more fine-grained error handling.
Miroslav
On Wed, Jun 19, 2019 at 01:12:12PM +0200, Miroslav Benes wrote:
> > @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >
> > err = prepare_coming_module(mod);
> > if (err)
> > - goto bug_cleanup;
> > + goto coming_cleanup;
>
> Not good. klp_module_going() is not prepared to be called without
> klp_module_coming() succeeding. "Funny" things might happen.
Bah, I did look at that but failed to spot it :/
> So it calls for more fine-grained error handling.
Another approach that I considered was trying to re-iterate the notifier
list up until the point we got, but that was fairly non-trivial and
needed changes to the notifier crud itself.
I'll try again.
On Wed, Jun 19, 2019 at 01:12:12PM +0200, Miroslav Benes wrote:
> On Mon, 17 Jun 2019, Peter Zijlstra wrote:
>
> >
> > Some module notifiers; such as jump_label_module_notifier(),
> > tracepoint_module_notify(); can fail the MODULE_STATE_COMING callback
> > (due to -ENOMEM for example). However module.c:prepare_coming_module()
> > ignores all such errors, even though this function can already fail due
> > to klp_module_coming().
>
> It does, but there is no change from the pre-prepare_coming_module()
> times. Coming notifiers were called in complete_formation(), their return
> values happily ignored and going notifiers not called to clean up even
> before.
Sure; but I didn't care to look before :-) The fact that it can
currently fail means most/all the unwinding is already there and we only
need to consider the additional cases.
On Wed, Jun 19, 2019 at 01:23:50PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 19, 2019 at 01:12:12PM +0200, Miroslav Benes wrote:
> > > @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > >
> > > err = prepare_coming_module(mod);
> > > if (err)
> > > - goto bug_cleanup;
> > > + goto coming_cleanup;
> >
> > Not good. klp_module_going() is not prepared to be called without
> > klp_module_coming() succeeding. "Funny" things might happen.
>
> Bah, I did look at that but failed to spot it :/
>
> > So it calls for more fine-grained error handling.
>
> Another approach that I considered was trying to re-iterate the notifier
> list up until the point we got, but that was fairly non-trivial and
> needed changes to the notifier crud itself.
>
> I'll try again.
How's something like so:
diff --git a/kernel/module.c b/kernel/module.c
index 80c7c09584cf..eba6560c89da 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3631,16 +3631,28 @@ static int complete_formation(struct module *mod, struct load_info *info)
static int prepare_coming_module(struct module *mod)
{
- int err;
+ struct blocking_notifier_head *nh = &module_notify_list;
+ int err, nr;
ftrace_module_enable(mod);
err = klp_module_coming(mod);
if (err)
return err;
- blocking_notifier_call_chain(&module_notify_list,
- MODULE_STATE_COMING, mod);
- return 0;
+ if (!rcu_access_pointer(nh->head))
+ return 0;
+
+ down_read(&nh->rwsem);
+ ret = notifier_call_chain(&nh->head, MODULE_STATE_COMING, mod, -1, &nr);
+ if (ret & NOTIFIER_STOP_MASK)
+ notifier_call_chain(&nh->head, MODULE_STATE_GOING, mod, nr, NULL);
+ up_read(&nh->rwsem);
+
+ err = notifier_to_err(err);
+ if (err)
+ klp_module_going(mod);
+
+ return err;
}
static int unknown_module_param_cb(char *param, char *val, const char *modname,
On Wed, Jun 19, 2019 at 01:33:24PM +0200, Peter Zijlstra wrote:
> How's something like so:
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 80c7c09584cf..eba6560c89da 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3631,16 +3631,28 @@ static int complete_formation(struct module *mod, struct load_info *info)
>
> static int prepare_coming_module(struct module *mod)
> {
> - int err;
> + struct blocking_notifier_head *nh = &module_notify_list;
> + int err, nr;
>
> ftrace_module_enable(mod);
> err = klp_module_coming(mod);
> if (err)
> return err;
>
> - blocking_notifier_call_chain(&module_notify_list,
> - MODULE_STATE_COMING, mod);
> - return 0;
> + if (!rcu_access_pointer(nh->head))
> + return 0;
> +
> + down_read(&nh->rwsem);
> + ret = notifier_call_chain(&nh->head, MODULE_STATE_COMING, mod, -1, &nr);
> + if (ret & NOTIFIER_STOP_MASK)
It compiles _lots_ better with s/ret/err/ on.
> + notifier_call_chain(&nh->head, MODULE_STATE_GOING, mod, nr, NULL);
> + up_read(&nh->rwsem);
> +
> + err = notifier_to_err(err);
> + if (err)
> + klp_module_going(mod);
> +
> + return err;
> }
>
> static int unknown_module_param_cb(char *param, char *val, const char *modname,
On Wed 2019-06-19 13:12:12, Miroslav Benes wrote:
> On Mon, 17 Jun 2019, Peter Zijlstra wrote:
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3638,9 +3638,10 @@ static int prepare_coming_module(struct module *mod)
> > if (err)
> > return err;
> >
> > - blocking_notifier_call_chain(&module_notify_list,
> > - MODULE_STATE_COMING, mod);
> > - return 0;
> > + ret = blocking_notifier_call_chain(&module_notify_list,
> > + MODULE_STATE_COMING, mod);
> > + ret = notifier_to_errno(ret);
> > + return ret;
> > }
> >
> > static int unknown_module_param_cb(char *param, char *val, const char *modname,
> > @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >
> > err = prepare_coming_module(mod);
> > if (err)
> > - goto bug_cleanup;
> > + goto coming_cleanup;
>
> Not good. klp_module_going() is not prepared to be called without
> klp_module_coming() succeeding. "Funny" things might happen.
We have discussed it with Miroslav. The best solution seems to
be to allow to call klp_module_going() even when
klp_module_comming() failed. Something like:
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index c4ce08f43bd6..42d72b62edb2 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1188,6 +1188,10 @@ void klp_module_going(struct module *mod)
return;
mutex_lock(&klp_mutex);
+
+ /* Nope when klp_module_comming failed */
+ if (!mod->klp_alive)
+ goto out;
/*
* Each module has to know that klp_module_going()
* has been called. We never know what module will
@@ -1196,7 +1200,7 @@ void klp_module_going(struct module *mod)
mod->klp_alive = false;
klp_cleanup_module_patches_limited(mod, NULL);
-
+out:
mutex_unlock(&klp_mutex);
}
Peter's patch actually allows to convert the klp hooks into
proper module notifiers. But we could do it later once this
patch is upstream or queued.
Best Regards,
Petr
On Wed, Jun 19, 2019 at 01:35:08PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 19, 2019 at 01:33:24PM +0200, Peter Zijlstra wrote:
> > How's something like so:
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 80c7c09584cf..eba6560c89da 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3631,16 +3631,28 @@ static int complete_formation(struct module *mod, struct load_info *info)
> >
> > static int prepare_coming_module(struct module *mod)
> > {
> > - int err;
> > + struct blocking_notifier_head *nh = &module_notify_list;
> > + int err, nr;
> >
> > ftrace_module_enable(mod);
> > err = klp_module_coming(mod);
> > if (err)
> > return err;
> >
> > - blocking_notifier_call_chain(&module_notify_list,
> > - MODULE_STATE_COMING, mod);
> > - return 0;
> > + if (!rcu_access_pointer(nh->head))
> > + return 0;
> > +
> > + down_read(&nh->rwsem);
> > + ret = notifier_call_chain(&nh->head, MODULE_STATE_COMING, mod, -1, &nr);
> > + if (ret & NOTIFIER_STOP_MASK)
>
> It compiles _lots_ better with s/ret/err/ on.
>
> > + notifier_call_chain(&nh->head, MODULE_STATE_GOING, mod, nr, NULL);
> > + up_read(&nh->rwsem);
> > +
> > + err = notifier_to_err(err);
> > + if (err)
> > + klp_module_going(mod);
> > +
> > + return err;
> > }
> >
> > static int unknown_module_param_cb(char *param, char *val, const char *modname,
Rafael, how is kernel/power/user.c snapshot_open() not broken (any any
other __pm_notifier_call_chain() user)?
afaict the __pm_notifier_call_chain() thing is broken in two ways:
- there can be a change to the notifier list between the PREPARE and
POST iteration
- the error value isn't put through notifier_to_errno().
+++ Miroslav Benes [19/06/19 13:12 +0200]:
>On Mon, 17 Jun 2019, Peter Zijlstra wrote:
>
>>
>> Some module notifiers; such as jump_label_module_notifier(),
>> tracepoint_module_notify(); can fail the MODULE_STATE_COMING callback
>> (due to -ENOMEM for example). However module.c:prepare_coming_module()
>> ignores all such errors, even though this function can already fail due
>> to klp_module_coming().
>
>It does, but there is no change from the pre-prepare_coming_module()
>times. Coming notifiers were called in complete_formation(), their return
>values happily ignored and going notifiers not called to clean up even
>before.
>
>> Therefore, propagate the notifier error and ensure we call the GOING
>> notifier when we do fail, to ensure cleanup for all notifiers that
>> didn't fail. Auditing all notifiers to make sure calling GOING without
>> COMING first is OK found no obvious problems with that, but it did find
>> a whole bunch of issues with return values, so clean those up too.
>
>Jessica, do you know why coming notifiers do not return errors without
>this patch (or to be precise, blocking_notifier_call_chain() return value
>is not taken into the account)? We have come across the issue couple of
>times already and I think there was a reason, but I cannot remember
>anything and the code does not help either.
I tried to do some digging but did not find a specific reason why the
return value is not taken into account. I don't think it was ever
considered. I traced it back to a commit in 2003 that introduced the
coming notifier (84486c2e135 "module load notification" in the history
repo), but even there the return value is ignored. After grepping
around it seems most usages of blocking_notifier_call_chain() just
ignore the return value.
>Also the situation around the return values themselves is not completely
>clear. If there is no NOTIFY_STOP_MASK set, only the return value of the
>last notifier called is returned, so good that you checked, Peter.
>
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3638,9 +3638,10 @@ static int prepare_coming_module(struct module *mod)
>> if (err)
>> return err;
>>
>> - blocking_notifier_call_chain(&module_notify_list,
>> - MODULE_STATE_COMING, mod);
>> - return 0;
>> + ret = blocking_notifier_call_chain(&module_notify_list,
>> + MODULE_STATE_COMING, mod);
>> + ret = notifier_to_errno(ret);
>> + return ret;
>> }
>>
>> static int unknown_module_param_cb(char *param, char *val, const char *modname,
>> @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>
>> err = prepare_coming_module(mod);
>> if (err)
>> - goto bug_cleanup;
>> + goto coming_cleanup;
>
>Not good. klp_module_going() is not prepared to be called without
>klp_module_coming() succeeding. "Funny" things might happen.
>
>Also destroy_params() might be called without parse_args() first now.
>
>So it calls for more fine-grained error handling.
I would not mind if prepare_coming_module() was taken apart to handle the more
fine-grained error handling. Maybe something like (untested and unreviewed):
diff --git a/kernel/module.c b/kernel/module.c
index c1517053e9d6..9e470f9ae0a5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3799,10 +3799,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
if (err)
goto ddebug_cleanup;
- err = prepare_coming_module(mod);
+ ftrace_module_enable(mod);
+ err = klp_module_coming(mod);
if (err)
goto bug_cleanup;
+ err = blocking_notifier_call_chain(&module_notify_list,
+ MODULE_STATE_COMING, mod);
+ if (err)
+ goto notifier_cleanup;
+
/* Module is ready to execute: parsing args may do that. */
after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
-32768, 32767, mod,
@@ -3837,8 +3843,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
sysfs_cleanup:
mod_sysfs_teardown(mod);
coming_cleanup:
- mod->state = MODULE_STATE_GOING;
destroy_params(mod->kp, mod->num_kp);
+notifier_cleanup:
+ mod->state = MODULE_STATE_GOING;
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_GOING, mod);
klp_module_going(mod);
But I think we could also still keep everything in prepare_coming_module() if
the klp hooks do get converted to notifiers.
Thanks,
Jessica
+++ Peter Zijlstra [19/06/19 13:23 +0200]:
>On Wed, Jun 19, 2019 at 01:12:12PM +0200, Miroslav Benes wrote:
>> > @@ -3780,7 +3781,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>> >
>> > err = prepare_coming_module(mod);
>> > if (err)
>> > - goto bug_cleanup;
>> > + goto coming_cleanup;
>>
>> Not good. klp_module_going() is not prepared to be called without
>> klp_module_coming() succeeding. "Funny" things might happen.
>
>Bah, I did look at that but failed to spot it :/
>
>> So it calls for more fine-grained error handling.
>
>Another approach that I considered was trying to re-iterate the notifier
>list up until the point we got, but that was fairly non-trivial and
>needed changes to the notifier crud itself.
>
>I'll try again.
Hm.. I would prefer if we didn't complicate the error handling too
much, especially since you mention it seems non-trivial, and it
doesn't look too nice. You also checked that calling the GOING without
the COMING notifiers should be safe, so I think we can keep things
simple. I tried to look at how other places in the kernel handle
blocking_notifier_call_chain() errors and the places that do look at
the error code (most invocations of blocking_notifier_call_chain()
seem to just ignore the return value) just call the opposing notifiers
(module "going" in our case) to cleanup. I also would not mind
breaking up prepare_coming_module() to refine the error handling, as I
mentioned in my other mail.
Thanks,
Jessica