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

[ebiederm@xmission.com (Eric W. Biederman)]

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.

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.

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.

Eric
> ---
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index dd1701c..0040ff2 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -32,7 +32,8 @@ static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
>  	struct attribute *const* attr;
>  	int error = 0, i;
>  
> -	for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> +	WARN_ON(!grp->attrs);
> +	for (i = 0, attr = grp->attrs; attr && *attr && !error; i++, attr++) {
>  		umode_t mode = 0;
>  
>  		/* in update mode, we're changing the permissions or