Message ID | 20211108101924.15759-1-bp@alien8.de |
---|---|
Headers | show |
Series | notifiers: Return an error when callback is already registered | expand |
On Mon, Nov 08, 2021 at 11:19:24AM +0100, Borislav Petkov wrote: > From: Borislav Petkov <bp@suse.de> > > 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/YYkyUEqcsOwQMb1S@zn.tnic I guess I can add another indirection to notifier_chain_register() and avoid touching all the call sites.
On Mon, 8 Nov 2021 15:35:50 +0100 Borislav Petkov <bp@alien8.de> 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
From: Borislav Petkov <bp@suse.de> 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 <arnd@arndb.de> Cc: Ayush Sawal <ayush.sawal@chelsio.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Rohit Maheshwari <rohitm@chelsio.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Vinay Kumar Yadav <vinay.yadav@chelsio.com> Cc: alsa-devel@alsa-project.org Cc: bcm-kernel-feedback-list@broadcom.com Cc: intel-gfx@lists.freedesktop.org Cc: intel-gvt-dev@lists.freedesktop.org Cc: linux-alpha@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-clk@vger.kernel.org Cc: linux-crypto@vger.kernel.org Cc: linux-edac@vger.kernel.org Cc: linux-fbdev@vger.kernel.org Cc: linux-hyperv@vger.kernel.org Cc: linux-iio@vger.kernel.org Cc: linux-leds@vger.kernel.org Cc: linux-mips@vger.kernel.org Cc: linux-parisc@vger.kernel.org Cc: linux-pm@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-remoteproc@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org Cc: linux-s390@vger.kernel.org Cc: linux-scsi@vger.kernel.org Cc: linux-sh@vger.kernel.org Cc: linux-staging@lists.linux.dev Cc: linux-tegra@vger.kernel.org Cc: linux-um@lists.infradead.org Cc: linux-usb@vger.kernel.org Cc: linux-xtensa@linux-xtensa.org Cc: netdev@vger.kernel.org Cc: openipmi-developer@lists.sourceforge.net Cc: rcu@vger.kernel.org Cc: sparclinux@vger.kernel.org Cc: x86@kernel.org Cc: xen-devel@lists.xenproject.org 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(-)