From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752843Ab2DCHsI (ORCPT ); Tue, 3 Apr 2012 03:48:08 -0400 Received: from out08.mta.xmission.com ([166.70.13.238]:34037 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751458Ab2DCHsG convert rfc822-to-8bit (ORCPT ); Tue, 3 Apr 2012 03:48:06 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Bruno =?utf-8?Q?Pr=C3=A9mont?= Cc: Ingo Molnar , Greg KH , Peter Zijlstra , linux-kernel@vger.kernel.org, Linus Torvalds References: <20120402162716.4c93bfd3@pluto.restena.lu> <20120402165036.2bc987ad@pluto.restena.lu> <20120402213440.49e9de74@neptune> <1333401898.2960.78.camel@laptop> <1333403193.2960.80.camel@laptop> <20120403060252.GA27084@gmail.com> <20120403081735.78ca3bb3@pluto.restena.lu> <20120403071543.GA17502@gmail.com> <20120403094107.4f641ed8@pluto.restena.lu> Date: Tue, 03 Apr 2012 00:51:48 -0700 In-Reply-To: <20120403094107.4f641ed8@pluto.restena.lu> ("Bruno =?utf-8?Q?Pr=C3=A9mont=22's?= message of "Tue, 3 Apr 2012 09:41:07 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-XM-SPF: eid=;;;mid=;;;hst=in02.mta.xmission.com;;;ip=98.207.153.68;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19gfsq9oYXJeT/qm9DO2yOnYDlGXSiogqA= X-SA-Exim-Connect-IP: 98.207.153.68 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * 0.1 XMSubLong Long Subject * 0.0 T_TM2_M_HEADER_IN_MSG BODY: T_TM2_M_HEADER_IN_MSG * -3.0 BAYES_00 BODY: Bayes spam probability is 0 to 1% * [score: 0.0000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa01 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_XMDrugObfuBody_12 obfuscated drug references * 0.1 XMSolicitRefs_0 Weightloss drug * 0.4 UNTRUSTED_Relay Comes from a non-trusted relay X-Spam-DCC: XMission; sa01 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: =?ISO-8859-1?Q?;Bruno Pr=c3=a9mont ?= X-Spam-Relay-Country: ** Subject: Re: [PATCH v2] Prevent crash on unset sysfs group attributes X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Fri, 06 Aug 2010 16:31:04 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Bruno Prémont writes: > Do not let the kernel crash when a device is registered with > sysfs while group attributes are not set (aka NULL). > > Warn about the offender with some information about the offending > device. > > This would warn instead of trying NULL pointer deref like: > BUG: unable to handle kernel NULL pointer dereference at > (null) IP: [] internal_create_group+0x83/0x1a0 > PGD 0 > Oops: 0000 [#1] SMP > CPU 0 > Modules linked in: > > Pid: 1, comm: swapper/0 Not tainted 3.4.0-rc1-x86_64 #3 HP ProLiant > DL360 G4 RIP: 0010:[] [] > internal_create_group+0x83/0x1a0 RSP: 0018:ffff88019485fd70 EFLAGS: > 00010202 RAX: 0000000000000001 RBX: 0000000000000000 RCX: > 0000000000000001 RDX: ffff880192e99908 RSI: ffff880192e99630 RDI: > ffffffff81a26c60 RBP: ffff88019485fdc0 R08: 0000000000000000 R09: > 0000000000000000 R10: ffff880192e99908 R11: 0000000000000000 R12: > ffffffff81a16a00 R13: ffff880192e99908 R14: ffffffff81a16900 R15: > 0000000000000000 FS: 0000000000000000(0000) GS:ffff88019bc00000(0000) > knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: > 000000008005003b CR2: 0000000000000000 CR3: 0000000001a0c000 CR4: > 00000000000007f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: > 0000000000000000 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: > 0000000000000400 Process swapper/0 (pid: 1, threadinfo > ffff88019485e000, task ffff880194878000) Stack: > ffff88019485fdd0 ffff880192da9d60 0000000000000000 ffff880192e99908 > ffff880192e995d8 0000000000000001 ffffffff81a16a00 ffff880192da9d60 > 0000000000000000 0000000000000000 ffff88019485fdd0 ffffffff811527be > Call Trace: > [] sysfs_create_group+0xe/0x10 > [] device_add_groups+0x46/0x80 > [] device_add+0x46d/0x6a0 > ... > > Signed-off-by: Bruno Prémont > --- > > On Tue, 3 Apr 2012 09:15:43 Ingo Molnar wrote: >> > > Greg, is this ok for you or should the check be moved out to >> > > calling internal_create_group()? >> > >> > Please put changes in internal_create_group where all of the >> > rest of the checks are. >> >> So you *do* agree that a check in a generic place is useful >> after all? ;-) >> >> > We should do something like: >> > if (!grp->attrs) { >> > WARN(1, "sysfs: idiot subsystem did not include attrs for group: %s/%s\n" >> > kobj->name, grp->name?"":grp->name); >> > return -EINVAL; >> > } >> > >> > As it stands your patch is horrible it leaves sysfs in an >> > inconsistent state. Creating the directory and leaving it >> > there. Not returning an error code. It looks like there are >> > all kinds of weird problems that removing the group or >> > updating the group could get into if we go with your patch. >> >> This is actually a sensible suggestion. Bruno, mind updating >> your patch to do something like this? Assuming Greg agrees with >> putting the check/warning there. > > Here come a respin with Eric's suggestion. > > In my case it would show the following result: > > [ 0.975025] ------------[ cut here ]------------ > [ 0.977507] WARNING: at /data/kernel/linux-2.6-git/fs/sysfs/group.c:72 internal_create_group+0x18c/0x1f0() > [ 0.981932] Hardware name: ProLiant DL360 G4 > [ 0.983916] sysfs: attrs not set by subsystem for group: cpu/ > [ 0.987030] Modules linked in: > [ 0.988664] Pid: 1, comm: swapper/0 Not tainted 3.4.0-rc1-x86_64 #6 > [ 0.991639] Call Trace: > [ 0.992911] [] warn_slowpath_common+0x7a/0xb0 > [ 0.995903] [] warn_slowpath_fmt+0x41/0x50 > [ 0.998593] [] internal_create_group+0x18c/0x1f0 > [ 1.001366] [] sysfs_create_group+0xe/0x10 > [ 1.003898] [] device_add_groups+0x46/0x80 > [ 1.006955] [] device_add+0x46d/0x6a0 > [ 1.009565] [] ? device_private_init+0x51/0x90 > [ 1.012806] [] ? utsname_sysctl_init+0x14/0x14 > [ 1.015866] [] pmu_dev_alloc+0x98/0xe0 > [ 1.018234] [] ? utsname_sysctl_init+0x14/0x14 > [ 1.021408] [] perf_event_sysfs_init+0x4b/0x9a > [ 1.025517] [] do_one_initcall+0x3d/0x170 > [ 1.029102] [] kernel_init+0x12d/0x1be > [ 1.031844] [] ? rdinit_setup+0x28/0x28 > [ 1.034571] [] kernel_thread_helper+0x4/0x10 > [ 1.037734] [] ? start_kernel+0x373/0x373 > [ 1.040790] [] ? gs_change+0xb/0xb > [ 1.043354] ---[ end trace 319c95c486d7d9cd ]--- > [ 1.045536] ------------[ cut here ]------------ > [ 1.047640] WARNING: at /data/kernel/linux-2.6-git/kernel/events/core.c:7144 perf_event_sysfs_init+0x70/0x9a() > [ 1.052595] Hardware name: ProLiant DL360 G4 > [ 1.055027] Failed to register pmu: cpu, reason -22 > [ 1.057720] Modules linked in: > [ 1.059284] Pid: 1, comm: swapper/0 Tainted: G W 3.4.0-rc1-x86_64 #6 > [ 1.062785] Call Trace: > [ 1.064094] [] warn_slowpath_common+0x7a/0xb0 > [ 1.067192] [] ? utsname_sysctl_init+0x14/0x14 > [ 1.070305] [] warn_slowpath_fmt+0x41/0x50 > [ 1.073129] [] ? pmu_dev_alloc+0xb4/0xe0 > [ 1.075771] [] perf_event_sysfs_init+0x70/0x9a > [ 1.078878] [] do_one_initcall+0x3d/0x170 > [ 1.081879] [] kernel_init+0x12d/0x1be > [ 1.084714] [] ? rdinit_setup+0x28/0x28 > [ 1.087754] [] kernel_thread_helper+0x4/0x10 > [ 1.090639] [] ? start_kernel+0x373/0x373 > [ 1.093372] [] ? gs_change+0xb/0xb > [ 1.096312] ---[ end trace 319c95c486d7d9ce ]--- That looks reasonable to me. Acked-by: "Eric W. Biederman" > fs/sysfs/group.c | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > index dd1701c..2df555c 100644 > --- a/fs/sysfs/group.c > +++ b/fs/sysfs/group.c > @@ -67,7 +67,11 @@ static int internal_create_group(struct kobject *kobj, int update, > /* Updates may happen before the object has been instantiated */ > if (unlikely(update && !kobj->sd)) > return -EINVAL; > - > + if (!grp->attrs) { > + WARN(1, "sysfs: attrs not set by subsystem for group: %s/%s\n", > + kobj->name, grp->name ? "" : grp->name); > + return -EINVAL; > + } > if (grp->name) { > error = sysfs_create_subdir(kobj, grp->name, &sd); > if (error)