| Message ID | 20211108101157.15189-43-bp@alien8.de |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Borislav, On Mon, Nov 8, 2021 at 11:13 AM Borislav Petkov <bp@alien8.de> wrote: > From: Borislav Petkov <bp@suse.de> > > The notifier registration routine doesn't return a proper error value > when a callback has already been registered, leading people to track > whether that registration has happened at the call site: > > https://lore.kernel.org/amd-gfx/20210512013058.6827-1-mukul.joshi@amd.com/ > > Which is unnecessary. > > Return -EEXIST to signal that case so that callers can act accordingly. > Enforce callers to check the return value, leading to loud screaming > during build: > > arch/x86/kernel/cpu/mce/core.c: In function ‘mce_register_decode_chain’: > arch/x86/kernel/cpu/mce/core.c:167:2: error: ignoring return value of \ > ‘blocking_notifier_chain_register’, declared with attribute warn_unused_result [-Werror=unused-result] > blocking_notifier_chain_register(&x86_mce_decoder_chain, nb); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Drop the WARN too, while at it. > > Suggested-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Borislav Petkov <bp@suse.de> Thanks for your patch! > --- a/include/linux/notifier.h > +++ b/include/linux/notifier.h > @@ -141,13 +141,13 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh); > > #ifdef __KERNEL__ > > -extern int atomic_notifier_chain_register(struct atomic_notifier_head *nh, > +extern int __must_check atomic_notifier_chain_register(struct atomic_notifier_head *nh, > struct notifier_block *nb); > -extern int blocking_notifier_chain_register(struct blocking_notifier_head *nh, > +extern int __must_check blocking_notifier_chain_register(struct blocking_notifier_head *nh, > struct notifier_block *nb); > -extern int raw_notifier_chain_register(struct raw_notifier_head *nh, > +extern int __must_check raw_notifier_chain_register(struct raw_notifier_head *nh, > struct notifier_block *nb); > -extern int srcu_notifier_chain_register(struct srcu_notifier_head *nh, > +extern int __must_check srcu_notifier_chain_register(struct srcu_notifier_head *nh, > struct notifier_block *nb); I think the addition of __must_check is overkill, leading to the addition of useless error checks and message printing. Many callers call this where it cannot fail, and where nothing can be done in the very unlikely event that the call would ever start to fail. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Nov 08, 2021 at 03:07:03PM +0100, Geert Uytterhoeven wrote: > I think the addition of __must_check is overkill, leading to the > addition of useless error checks and message printing. See the WARN in notifier_chain_register() - it will already do "message printing". > Many callers call this where it cannot fail, and where nothing can > be done in the very unlikely event that the call would ever start to > fail. This is an attempt to remove this WARN() hack in notifier_chain_register() and have the function return a proper error value instead of this "Currently always returns zero." which is bad design. Some of the registration functions around the tree check that retval and some don't. So if "it cannot fail" those registration either should not return a value or callers should check that return value - what we have now doesn't make a whole lot of sense. Oh, and then fixing this should avoid stuff like: + if (notifier_registered == false) { + mce_register_decode_chain(&amdgpu_bad_page_nb); + notifier_registered = true; + } from propagating in the code. The other idea I have is to add another indirection in notifier_chain_register() which will check the *proper* return value of a lower level __notifier_chain_register() and then issue that warning. That would definitely avoid touch so many call sites. Bottom line is: what we have now needs cleaning up. Thx.
Hi Borislav, On Mon, Nov 8, 2021 at 3:21 PM Borislav Petkov <bp@alien8.de> wrote: > On Mon, Nov 08, 2021 at 03:07:03PM +0100, Geert Uytterhoeven wrote: > > I think the addition of __must_check is overkill, leading to the > > addition of useless error checks and message printing. > > See the WARN in notifier_chain_register() - it will already do "message > printing". I mean the addition of useless error checks and message printing _to the callers_. > > Many callers call this where it cannot fail, and where nothing can > > be done in the very unlikely event that the call would ever start to > > fail. > > This is an attempt to remove this WARN() hack in > notifier_chain_register() and have the function return a proper error > value instead of this "Currently always returns zero." which is bad > design. > > Some of the registration functions around the tree check that retval and > some don't. So if "it cannot fail" those registration either should not > return a value or callers should check that return value - what we have > now doesn't make a whole lot of sense. With __must_check callers are required to check, even if they know it cannot fail. > Oh, and then fixing this should avoid stuff like: > > + if (notifier_registered == false) { > + mce_register_decode_chain(&amdgpu_bad_page_nb); > + notifier_registered = true; > + } > > from propagating in the code. That's unrelated to the addition of __must_check. I'm not against returning proper errors codes. I'm against forcing callers to check things that cannot fail and to add individual error printing to each and every caller. Note that in other areas, we are moving in the other direction, to a centralized printing of error messages, cfr. e.g. commit 7723f4c5ecdb8d83 ("driver core: platform: Add an error message to platform_get_irq*()"). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Nov 08, 2021 at 04:25:47PM +0100, Geert Uytterhoeven wrote: > I'm not against returning proper errors codes. I'm against forcing > callers to check things that cannot fail and to add individual error > printing to each and every caller. If you're against checking things at the callers, then the registration function should be void. IOW, those APIs are not optimally designed atm. > Note that in other areas, we are moving in the other direction, > to a centralized printing of error messages, cfr. e.g. commit > 7723f4c5ecdb8d83 ("driver core: platform: Add an error message to > platform_get_irq*()"). Yes, thus my other idea to add a lower level __notifier_chain_register() to do the checking. I'll see if I can convert those notifier registration functions to return void, in the process. But let's see what the others think first. Thanks for taking the time.
Hi Borislav, On Mon, Nov 8, 2021 at 4:59 PM Borislav Petkov <bp@alien8.de> wrote: > On Mon, Nov 08, 2021 at 04:25:47PM +0100, Geert Uytterhoeven wrote: > > I'm not against returning proper errors codes. I'm against forcing > > callers to check things that cannot fail and to add individual error > > printing to each and every caller. > > If you're against checking things at the callers, then the registration > function should be void. IOW, those APIs are not optimally designed atm. Returning void is the other extreme ;-) There are 3 levels (ignoring BUG_ON()/panic () inside the callee): 1. Return void: no one can check success or failure, 2. Return an error code: up to the caller to decide, 3. Return a __must_check error code: every caller must check. I'm in favor of 2, as there are several places where it cannot fail. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Nov 08, 2021 at 05:12:16PM +0100, Geert Uytterhoeven wrote: > Returning void is the other extreme ;-) > > There are 3 levels (ignoring BUG_ON()/panic () inside the callee): > 1. Return void: no one can check success or failure, > 2. Return an error code: up to the caller to decide, > 3. Return a __must_check error code: every caller must check. > > I'm in favor of 2, as there are several places where it cannot fail. Makes sense to me. I'll do that in the next iteration. Thx.
On Mon, Nov 08, 2021 at 05:21:45PM +0100, Borislav Petkov wrote: > On Mon, Nov 08, 2021 at 05:12:16PM +0100, Geert Uytterhoeven wrote: > > Returning void is the other extreme ;-) > > > > There are 3 levels (ignoring BUG_ON()/panic () inside the callee): > > 1. Return void: no one can check success or failure, > > 2. Return an error code: up to the caller to decide, > > 3. Return a __must_check error code: every caller must check. > > > > I'm in favor of 2, as there are several places where it cannot fail. > > Makes sense to me. I'll do that in the next iteration. Is there really any reason for returning an error code? For example, is it anticipated that at some point in the future these registration calls might fail? Currently, the only reason for failing to register a notifier callback is because the callback is already registered. In a sense this isn't even an actual failure -- after the registration returns the callback _will_ still be registered. So if the call can never really fail, why bother with a return code? Especially since the caller can't do anything with such a code value. Given the current state of affairs, I vote in favor of 1 (plus a WARN or something similar to generate a stack dump in the callee, since double registration really is a bug). Alan Stern
On Mon, Nov 08, 2021 at 03:59:26PM -0500, Alan Stern wrote: > Is there really any reason for returning an error code? For example, is > it anticipated that at some point in the future these registration calls > might fail? > > Currently, the only reason for failing... Right, I believe with not making it return void we're leaving the door open for some, *hypothetical* future return values if we decide we need to return them too, at some point. Yes, I can't think of another fact to state besides that the callback was already registered or return success but who knows what we wanna do in the future... And so if we change them all to void now, I think it'll be a lot more churn to switch back to returning a non-void value and having the callers who choose to handle that value, do so again. So, long story short, keeping the retval - albeit not very useful right now - is probably easier. I hope I'm making some sense here.
diff --git a/include/linux/notifier.h b/include/linux/notifier.h index 87069b8459af..45cc5a8d0fd8 100644 --- a/include/linux/notifier.h +++ b/include/linux/notifier.h @@ -141,13 +141,13 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh); #ifdef __KERNEL__ -extern int atomic_notifier_chain_register(struct atomic_notifier_head *nh, +extern int __must_check atomic_notifier_chain_register(struct atomic_notifier_head *nh, struct notifier_block *nb); -extern int blocking_notifier_chain_register(struct blocking_notifier_head *nh, +extern int __must_check blocking_notifier_chain_register(struct blocking_notifier_head *nh, struct notifier_block *nb); -extern int raw_notifier_chain_register(struct raw_notifier_head *nh, +extern int __must_check raw_notifier_chain_register(struct raw_notifier_head *nh, struct notifier_block *nb); -extern int srcu_notifier_chain_register(struct srcu_notifier_head *nh, +extern int __must_check srcu_notifier_chain_register(struct srcu_notifier_head *nh, struct notifier_block *nb); extern int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh, diff --git a/kernel/notifier.c b/kernel/notifier.c index b8251dc0bc0f..451ef3f73ad2 100644 --- a/kernel/notifier.c +++ b/kernel/notifier.c @@ -20,13 +20,11 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list); */ static int notifier_chain_register(struct notifier_block **nl, - struct notifier_block *n) + struct notifier_block *n) { while ((*nl) != NULL) { - if (unlikely((*nl) == n)) { - WARN(1, "double register detected"); - return 0; - } + if (unlikely((*nl) == n)) + return -EEXIST; if (n->priority > (*nl)->priority) break; nl = &((*nl)->next); @@ -134,10 +132,11 @@ static int notifier_call_chain_robust(struct notifier_block **nl, * * Adds a notifier to an atomic notifier chain. * - * Currently always returns zero. + * Returns 0 on success, %-EEXIST on error. */ -int atomic_notifier_chain_register(struct atomic_notifier_head *nh, - struct notifier_block *n) +int __must_check +atomic_notifier_chain_register(struct atomic_notifier_head *nh, + struct notifier_block *n) { unsigned long flags; int ret; @@ -216,10 +215,11 @@ NOKPROBE_SYMBOL(atomic_notifier_call_chain); * Adds a notifier to a blocking notifier chain. * Must be called in process context. * - * Currently always returns zero. + * Returns 0 on success, %-EEXIST on error. */ -int blocking_notifier_chain_register(struct blocking_notifier_head *nh, - struct notifier_block *n) +int __must_check +blocking_notifier_chain_register(struct blocking_notifier_head *nh, + struct notifier_block *n) { int ret; @@ -335,10 +335,11 @@ EXPORT_SYMBOL_GPL(blocking_notifier_call_chain); * Adds a notifier to a raw notifier chain. * All locking must be provided by the caller. * - * Currently always returns zero. + * Returns 0 on success, %-EEXIST on error. */ -int raw_notifier_chain_register(struct raw_notifier_head *nh, - struct notifier_block *n) +int __must_check +raw_notifier_chain_register(struct raw_notifier_head *nh, + struct notifier_block *n) { return notifier_chain_register(&nh->head, n); } @@ -406,10 +407,11 @@ EXPORT_SYMBOL_GPL(raw_notifier_call_chain); * Adds a notifier to an SRCU notifier chain. * Must be called in process context. * - * Currently always returns zero. + * Returns 0 on success, %-EEXIST on error. */ -int srcu_notifier_chain_register(struct srcu_notifier_head *nh, - struct notifier_block *n) +int __must_check +srcu_notifier_chain_register(struct srcu_notifier_head *nh, + struct notifier_block *n) { int ret;