lkml.org 
[lkml]   [2012]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH] Prevent crash on missing sysfs attribute group

    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
    --
    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/

    \
     
     \ /
      Last update: 2012-04-03 09:11    [W:3.592 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site