Messages in this thread | | | Date | Tue, 3 Apr 2012 09:15:43 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] Prevent crash on missing sysfs attribute group |
| |
* Eric W. Biederman <ebiederm@xmission.com> wrote:
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com> > > Bruno Prémont <bonbons@linux-vserver.org> writes: > > > Prevent kernel from crashing when a device is being registered with sysfs > > but has no (aka NULL) group attributes, but warn about it so calling path > > can get fixed. > > The idea is reasonable but the implementation is horrible. > > >> Will do - but the underlying generic bug should be fixed as > >> well: we must not crash just because some attributes are missing > >> in a rarely used sub-driver ... > >> > >> We should WARN_ON(), etc. - but not crash. > > FIX perf to include sanity checks.
Huh, so put repeated, duplicated, inconsistently applied sanity checks into dozens of sysfs attribute using kernel subsystems?
Major FAIL, dude.
> Anything we do in sysfs is just pointless because perf was > clever and the offender did not show up in the backtrace. > > Right now perf is so bad we just waste everyone's time. > > > 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.
Eric's rant about putting sanit checks at every usage site is just crazy talk.
Thanks,
Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |