linux-kernel - Re: [PATCH 24/29] sysctl: Replace root_list with links between sysctl_table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 30 Jan 2012 19:10:04 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Lucian Adrian Grijincu <lucian.grijincu@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	netdev@...r.kernel.org,
	Damien Millescamps <damien.millescamps@...nd.com>
Subject: Re: [PATCH 24/29] sysctl: Replace root_list with links between sysctl_table_sets.

Lucian Adrian Grijincu <lucian.grijincu@...il.com> writes:

> Very nice way to handle netns specific files with links between sets!
>
> You did a much better job than I did at dealing with them.
>
> It took me a while to understand how the code works. I'll try to write
> something for Documentation/ because the inner workings are a bit
> intertwined.
>
> A few comments bellow.

Thank you for your review.

I'm going to see how many of your comments I can incorporate into the
code.

Since no significant bugs have been seen my plan is to use incremental
patches on top of the existing code, to address the small issues that
have been seen.  That way I don't have to retest yet again.

>>  void register_sysctl_root(struct ctl_table_root *root)
>>  {
>> -       spin_lock(&sysctl_lock);
>> -       list_add_tail(&root->root_list, &sysctl_table_root.root_list);
>> -       spin_unlock(&sysctl_lock);
>>  }
>
>
> This function is empty. Can be deleted (there are two callers in
> net/sysctl_net.c.

Agreed.  There might be something useful by registering roots.  Perhaps
initialize the default set.  So I have kept the function for the short
term.  register_sysctl_root makes sense as an interface.  We just don't
have anything for register_sysctl_root to do right now.

>> @@ -400,6 +373,7 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
>>        struct inode *inode;
>>        struct dentry *err = ERR_PTR(-ENOENT);
>>        struct ctl_dir *ctl_dir;
>> +       int ret;
>>
>>        if (IS_ERR(head))
>>                return ERR_CAST(head);
>> @@ -410,6 +384,11 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
>>        if (!p)
>>                goto out;
>>
>
>
> "sysctl_follow_link" implies that it will follow the link. I would
> pull out the check whether the header is a link or not. This wouldn't
> save much (a function call), but it would make the code easier to
> read:
>
> 	/* Get out quickly if not a link */
>         if (S_ISLNK(p->mode)) {
> 		ret = sysctl_follow_link(&h, &p, current->nsproxy);
> 		err = ERR_PTR(ret);
> 		if (ret)
> 		        goto out;
>         }

Good point, and obviousness is part of what I am aiming for.


>>  {
>> +       struct ctl_table_set *set = dir->header.set;
>>        struct ctl_dir *subdir, *new = NULL;
>>
>>        spin_lock(&sysctl_lock);
>> -       subdir = find_subdir(dir->header.set, dir, name, namelen);
>> -       if (!IS_ERR(subdir))
>> -               goto found;
>> -       if ((PTR_ERR(subdir) == -ENOENT) && set != dir->header.set)
>> -               subdir = find_subdir(set, dir, name, namelen);
>> +       subdir = find_subdir(dir, name, namelen);
>>        if (!IS_ERR(subdir))
>>                goto found;
>>        if (PTR_ERR(subdir) != -ENOENT)
>> @@ -817,13 +815,14 @@ static struct ctl_dir *get_subdir(struct ctl_table_set *set,
>>        if (!new)
>>                goto failed;
>>
>> -       subdir = find_subdir(set, dir, name, namelen);
>> +       subdir = find_subdir(dir, name, namelen);
>>        if (!IS_ERR(subdir))
>>                goto found;
>>        if (PTR_ERR(subdir) != -ENOENT)
>>                goto failed;
>
> I think you're returning the wrong error here. If we got to this point
> then subdir == ERR_PTR(-ENOENT).
> We want to create a new dir here even if one doesn't exist.
> So if we have an error in insert_header() we don't return that error,
> but ENOENT.
>
>>
>> -       insert_header(dir, &new->header);
>> +       if (insert_header(dir, &new->header))
>> +               goto failed;
>>        subdir = new;
>
> Something like this would fix it:
>
>         if (subdir = insert_header(dir, &new->header))
>
>
>
>>  found:
>>        subdir->header.nreg++;
>> @@ -841,6 +840,57 @@ failed:
>>        return subdir;
>>  }
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/