| Message ID | 1555386991-8855-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] ARM: mvebu: at least report the kzalloc failure | expand |
On Tue, Apr 16, 2019 at 05:56:31AM +0200, Nicholas Mc Guire wrote: > Note that this will trigger a checkpatch WARNING > "WARNING: Possible unnecessary 'out of memory' message" > but comparing the oops with an without the one-line pr_err I would > argue that it makes sense to include it: Hi Nicholas It might be worth adding this as a comment, so that newbies don't submit patches removing the pr_err() because of the checkpatch warning. Andrew
On Tue, Apr 16, 2019 at 03:39:57PM +0200, Andrew Lunn wrote: > On Tue, Apr 16, 2019 at 05:56:31AM +0200, Nicholas Mc Guire wrote: > > > Note that this will trigger a checkpatch WARNING > > "WARNING: Possible unnecessary 'out of memory' message" > > but comparing the oops with an without the one-line pr_err I would > > argue that it makes sense to include it: > > Hi Nicholas > > It might be worth adding this as a comment, so that newbies don't > submit patches removing the pr_err() because of the checkpatch > warning. > hmm... I think if we start doing that we would make quite a mess of documentation in the kernel. Also note its a warning stating "possible unneceessary" - so I would not see the necessity. At most I would include a note on this in the commit message so that anyone checking the origin would see that this is intenttional - assuming that people modifying code would be using git blame to locate the origin of any code... thx! hofrat
Hi Nicholas, Nicholas Mc Guire <der.herr@hofr.at> writes: > On Tue, Apr 16, 2019 at 03:39:57PM +0200, Andrew Lunn wrote: >> On Tue, Apr 16, 2019 at 05:56:31AM +0200, Nicholas Mc Guire wrote: >> >> > Note that this will trigger a checkpatch WARNING >> > "WARNING: Possible unnecessary 'out of memory' message" >> > but comparing the oops with an without the one-line pr_err I would >> > argue that it makes sense to include it: >> >> Hi Nicholas >> >> It might be worth adding this as a comment, so that newbies don't >> submit patches removing the pr_err() because of the checkpatch >> warning. >> > hmm... I think if we start doing that we would make quite a mess of > documentation in the kernel. Also note its a warning stating "possible > unneceessary" - so I would not see the necessity. > > At most I would include a note on this in the commit message so that > anyone checking the origin would see that this is intenttional - assuming > that people modifying code would be using git blame to locate the > origin of any code... Don't bother to send a new version I don't attempt to take this patch. As you pointed it is very unlikely that we get an error so early during the boot for a very small amount of memory. If it happened then we have serious trouble and the message provided by the BUG() call will be more than enough. Thanks, Gregory > > thx! > hofrat > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Apr 17, 2019 at 02:07:44PM +0200, Gregory CLEMENT wrote: > Hi Nicholas, > > Nicholas Mc Guire <der.herr@hofr.at> writes: > > > On Tue, Apr 16, 2019 at 03:39:57PM +0200, Andrew Lunn wrote: > >> On Tue, Apr 16, 2019 at 05:56:31AM +0200, Nicholas Mc Guire wrote: > >> > >> > Note that this will trigger a checkpatch WARNING > >> > "WARNING: Possible unnecessary 'out of memory' message" > >> > but comparing the oops with an without the one-line pr_err I would > >> > argue that it makes sense to include it: > >> > >> Hi Nicholas > >> > >> It might be worth adding this as a comment, so that newbies don't > >> submit patches removing the pr_err() because of the checkpatch > >> warning. > >> > > hmm... I think if we start doing that we would make quite a mess of > > documentation in the kernel. Also note its a warning stating "possible > > unneceessary" - so I would not see the necessity. > > > > At most I would include a note on this in the commit message so that > > anyone checking the origin would see that this is intenttional - assuming > > that people modifying code would be using git blame to locate the > > origin of any code... > > Don't bother to send a new version I don't attempt to take this > patch. As you pointed it is very unlikely that we get an error so early > during the boot for a very small amount of memory. > > If it happened then we have serious trouble and the message provided by > the BUG() call will be more than enough. > yup - its a corner case - I'm trying to filter out those cases that are actually in __init function returning void - as those cases are, it seems, are generally cases where k{m,z}allocs will not have explicit checking. thx! hofrat
diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c index 0b10acd..df84cb6 100644 --- a/arch/arm/mach-mvebu/board-v7.c +++ b/arch/arm/mach-mvebu/board-v7.c @@ -128,6 +128,9 @@ static void __init i2c_quirk(void) struct property *new_compat; new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL); + if (!new_compat) + pr_err("new_compat allocation failure in %s()\n", + __func__); new_compat->name = kstrdup("compatible", GFP_KERNEL); new_compat->length = sizeof("marvell,mv78230-a0-i2c");
Although it is very unlikely that the allocation during init would fail any such failure should point to the original cause to allow easier understanding of the ensuing null-pointer dereference splat. Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> --- Problem located with experimental coccinelle script V2: Russell King <linux@armlinux.org.uk> pointed out that the use of WARN_ON() would result in a stack trace followed by the oops due to dereferencing of the NULL pointer and so make it even less likely that users would uncover the actual cause - so drop the WARN_ON() and use a short pr_err() message that points to the oops cause directly. Note that this will trigger a checkpatch WARNING "WARNING: Possible unnecessary 'out of memory' message" but comparing the oops with an without the one-line pr_err I would argue that it makes sense to include it: <snip> [ 8061.514840] shared page allocation failure in hello_init() [ 8113.563239] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000 [ 8113.563250] #PF error: [WRITE] [ 8113.563255] PGD 8000000129993067 P4D 8000000129993067 PUD 129992067 PMD 0 [ 8113.563267] Oops: 0002 [#1] SMP PTI [ 8113.563276] CPU: 2 PID: 2656 Comm: bash Tainted: G W O 5.0.0-rc3livepatchtest-next-20190123+ #4 [ 8113.563280] Hardware name: Quanta TWH/TWH, BIOS QU221 10/14/2011 [ 8113.563292] RIP: 0010:foo_store+0x3a/0x90 [hello_chardev] ... <snip> Patch was compile-tested: mvebu_v7_defconfig (implies MACH_MVEBU_ANY=y) (with some unrelated sparse warnings about missing syscalls) Patch is against 5.1-rc4 (localversion-next is 20190415) arch/arm/mach-mvebu/board-v7.c | 3 +++ 1 file changed, 3 insertions(+)