Re: [PATCH] Prevent crash on missing sysfs attribute group

[Ingo Molnar]

* Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:

> Nacked-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> 
> Bruno Prémont <bonbons@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/