From: Borislav Petkov <[email protected]>
Hi all,
this is a huge patchset for something which is really trivial - it
changes the notifier registration routines to return an error value
if a notifier callback is already present on the respective list of
callbacks. For more details scroll to the last patch.
Everything before it is converting the callers to check the return value
of the registration routines and issue a warning, instead of the WARN()
notifier_chain_register() does now.
Before the last patch has been applied, though, that checking is a
NOP which would make the application of those patches trivial - every
maintainer can pick a patch at her/his discretion - only the last one
enables the build warnings and that one will be queued only after the
preceding patches have all been merged so that there are no build
warnings.
Due to the sheer volume of the patches, I have addressed the respective
patch and the last one, which enables the warning, with addressees for
each maintained area so as not to spam people unnecessarily.
If people prefer I carry some through tip, instead, I'll gladly do so -
your call.
And, if you think the warning messages need to be more precise, feel
free to adjust them before committing.
Thanks!
Cc: Arnd Bergmann <[email protected]>
Cc: Ayush Sawal <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Rohit Maheshwari <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Vinay Kumar Yadav <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Borislav Petkov (42):
x86: Check notifier registration return value
xen/x86: Check notifier registration return value
impi: Check notifier registration return value
clk: renesas: Check notifier registration return value
dca: Check notifier registration return value
firmware: Check notifier registration return value
drm/i915: Check notifier registration return value
Drivers: hv: vmbus: Check notifier registration return value
iio: proximity: cros_ec: Check notifier registration return value
leds: trigger: Check notifier registration return value
misc: Check notifier registration return value
ethernet: chelsio: Check notifier registration return value
power: reset: Check notifier registration return value
remoteproc: Check notifier registration return value
scsi: target: Check notifier registration return value
USB: Check notifier registration return value
drivers: video: Check notifier registration return value
drivers/xen: Check notifier registration return value
kernel/hung_task: Check notifier registration return value
rcu: Check notifier registration return value
tracing: Check notifier registration return value
net: fib_notifier: Check notifier registration return value
ASoC: soc-jack: Check notifier registration return value
staging: olpc_dcon: Check notifier registration return value
arch/um: Check notifier registration return value
alpha: Check notifier registration return value
bus: brcmstb_gisb: Check notifier registration return value
soc: bcm: brcmstb: pm: pm-arm: Check notifier registration return
value
arm64: Check notifier registration return value
soc/tegra: Check notifier registration return value
parisc: Check notifier registration return value
macintosh/adb: Check notifier registration return value
mips: Check notifier registration return value
powerpc: Check notifier registration return value
sh: Check notifier registration return value
s390: Check notifier registration return value
sparc: Check notifier registration return value
xtensa: Check notifier registration return value
crypto: ccree - check notifier registration return value
EDAC/altera: Check notifier registration return value
power: supply: ab8500: Check notifier registration return value
notifier: Return an error when callback is already registered
arch/alpha/kernel/setup.c | 5 +--
arch/arm64/kernel/setup.c | 6 ++--
arch/mips/kernel/relocate.c | 6 ++--
arch/mips/sgi-ip22/ip22-reset.c | 4 ++-
arch/mips/sgi-ip32/ip32-reset.c | 4 ++-
arch/parisc/kernel/pdc_chassis.c | 5 +--
arch/powerpc/kernel/setup-common.c | 12 ++++---
arch/s390/kernel/ipl.c | 4 ++-
arch/s390/kvm/kvm-s390.c | 7 ++--
arch/sh/kernel/cpu/sh4a/setup-sh7724.c | 11 +++---
arch/sparc/kernel/sstate.c | 6 ++--
arch/um/drivers/mconsole_kern.c | 6 ++--
arch/um/kernel/um_arch.c | 5 +--
arch/x86/kernel/cpu/mce/core.c | 3 +-
arch/x86/kernel/cpu/mce/dev-mcelog.c | 3 +-
arch/x86/kernel/setup.c | 7 ++--
arch/x86/xen/enlighten.c | 4 ++-
arch/xtensa/platforms/iss/setup.c | 3 +-
drivers/bus/brcmstb_gisb.c | 6 ++--
drivers/char/ipmi/ipmi_msghandler.c | 3 +-
drivers/clk/renesas/clk-div6.c | 4 ++-
drivers/clk/renesas/rcar-cpg-lib.c | 4 ++-
drivers/crypto/ccree/cc_fips.c | 4 ++-
drivers/dca/dca-core.c | 3 +-
drivers/edac/altera_edac.c | 6 ++--
drivers/firmware/arm_scmi/notify.c | 3 +-
drivers/firmware/google/gsmi.c | 6 ++--
drivers/gpu/drm/i915/gvt/scheduler.c | 6 ++--
drivers/hv/vmbus_drv.c | 4 +--
.../iio/proximity/cros_ec_mkbp_proximity.c | 3 +-
drivers/leds/trigger/ledtrig-activity.c | 6 ++--
drivers/leds/trigger/ledtrig-heartbeat.c | 6 ++--
drivers/leds/trigger/ledtrig-panic.c | 4 +--
drivers/macintosh/adbhid.c | 4 +--
drivers/misc/ibmasm/heartbeat.c | 3 +-
drivers/misc/pvpanic/pvpanic.c | 3 +-
.../chelsio/inline_crypto/chtls/chtls_main.c | 5 ++-
drivers/parisc/power.c | 5 +--
drivers/power/reset/ltc2952-poweroff.c | 6 ++--
drivers/power/supply/ab8500_charger.c | 8 ++---
drivers/remoteproc/qcom_common.c | 3 +-
drivers/remoteproc/qcom_sysmon.c | 4 ++-
drivers/remoteproc/remoteproc_core.c | 4 ++-
drivers/s390/char/con3215.c | 5 ++-
drivers/s390/char/con3270.c | 5 ++-
drivers/s390/char/sclp_con.c | 4 ++-
drivers/s390/char/sclp_vt220.c | 4 ++-
drivers/s390/char/zcore.c | 4 ++-
drivers/soc/bcm/brcmstb/pm/pm-arm.c | 5 +--
drivers/soc/tegra/ari-tegra186.c | 7 ++--
drivers/staging/olpc_dcon/olpc_dcon.c | 4 ++-
drivers/target/tcm_fc/tfc_conf.c | 4 ++-
drivers/usb/core/notify.c | 3 +-
drivers/video/console/dummycon.c | 3 +-
drivers/video/fbdev/hyperv_fb.c | 5 +--
drivers/xen/manage.c | 3 +-
drivers/xen/xenbus/xenbus_probe.c | 8 +++--
include/linux/notifier.h | 8 ++---
kernel/hung_task.c | 3 +-
kernel/notifier.c | 36 ++++++++++---------
kernel/rcu/tree_stall.h | 4 ++-
kernel/trace/trace.c | 4 +--
net/core/fib_notifier.c | 4 ++-
sound/soc/soc-jack.c | 3 +-
64 files changed, 222 insertions(+), 118 deletions(-)
--
2.29.2
On Mon, Nov 08, 2021 at 11:19:24AM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> Hi all,
>
> this is a huge patchset for something which is really trivial - it
> changes the notifier registration routines to return an error value
> if a notifier callback is already present on the respective list of
> callbacks. For more details scroll to the last patch.
>
> Everything before it is converting the callers to check the return value
> of the registration routines and issue a warning, instead of the WARN()
> notifier_chain_register() does now.
What reason is there for moving the check into the callers? It seems
like pointless churn. Why not add the error return code, change the
WARN to pr_warn, and leave the callers as they are? Wouldn't that end
up having exactly the same effect?
For that matter, what sort of remedial action can a caller take if the
return code is -EEXIST? Is there any point in forcing callers to check
the return code if they can't do anything about it?
> Before the last patch has been applied, though, that checking is a
> NOP which would make the application of those patches trivial - every
> maintainer can pick a patch at her/his discretion - only the last one
> enables the build warnings and that one will be queued only after the
> preceding patches have all been merged so that there are no build
> warnings.
Why should there be _any_ build warnings? The real problem occurs when
a notifier callback is added twice, not when a caller fails to check the
return code. Double-registration is not the sort of thing that can be
detected at build time.
Alan Stern
> Due to the sheer volume of the patches, I have addressed the respective
> patch and the last one, which enables the warning, with addressees for
> each maintained area so as not to spam people unnecessarily.
>
> If people prefer I carry some through tip, instead, I'll gladly do so -
> your call.
>
> And, if you think the warning messages need to be more precise, feel
> free to adjust them before committing.
>
> Thanks!
On Mon, Nov 08, 2021 at 09:17:03AM -0500, Alan Stern wrote:
> What reason is there for moving the check into the callers? It seems
> like pointless churn. Why not add the error return code, change the
> WARN to pr_warn, and leave the callers as they are? Wouldn't that end
> up having exactly the same effect?
>
> For that matter, what sort of remedial action can a caller take if the
> return code is -EEXIST? Is there any point in forcing callers to check
> the return code if they can't do anything about it?
See my reply to Geert from just now:
https://lore.kernel.org/r/[email protected]
I guess I can add another indirection to notifier_chain_register() and
avoid touching all the call sites.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Nov 08, 2021 at 03:24:39PM +0100, Borislav Petkov wrote:
> I guess I can add another indirection to notifier_chain_register() and
> avoid touching all the call sites.
IOW, something like this below.
This way I won't have to touch all the callsites and the registration
routines would still return a proper value instead of returning 0
unconditionally.
---
diff --git a/kernel/notifier.c b/kernel/notifier.c
index b8251dc0bc0f..04f08b2ef17f 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -19,14 +19,12 @@ BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);
* are layered on top of these, with appropriate locking added.
*/
-static int notifier_chain_register(struct notifier_block **nl,
- struct notifier_block *n)
+static int __notifier_chain_register(struct notifier_block **nl,
+ 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);
@@ -36,6 +34,18 @@ static int notifier_chain_register(struct notifier_block **nl,
return 0;
}
+static int notifier_chain_register(struct notifier_block **nl,
+ struct notifier_block *n)
+{
+ int ret = __notifier_chain_register(nl, n);
+
+ if (ret == -EEXIST)
+ WARN(1, "double register of notifier callback %ps detected",
+ n->notifier_call);
+
+ return ret;
+}
+
static int notifier_chain_unregister(struct notifier_block **nl,
struct notifier_block *n)
{
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, 8 Nov 2021 15:35:50 +0100
Borislav Petkov <[email protected]> wrote:
> On Mon, Nov 08, 2021 at 03:24:39PM +0100, Borislav Petkov wrote:
> > I guess I can add another indirection to notifier_chain_register() and
> > avoid touching all the call sites.
>
> IOW, something like this below.
>
> This way I won't have to touch all the callsites and the registration
> routines would still return a proper value instead of returning 0
> unconditionally.
I prefer this method.
Question, how often does this warning trigger? Is it common to see in
development?
-- Steve
On Mon, Nov 08, 2021 at 11:23:13AM -0500, Steven Rostedt wrote:
> Question, how often does this warning trigger? Is it common to see in
> development?
Yeah, haven't seen it myself yet.
But we hashed it out over IRC. :-)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette