Re: [PATCH v2] fail dentry revalidation after namespace change

[Eric W. Biederman]

Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> writes:

>>  {
>>  	struct sysfs_dirent *sd;
>>  	int is_dir;
>> +	int type;
>>  
>>  	if (nd->flags & LOOKUP_RCU)
>>  		return -ECHILD;
>> @@ -326,6 +327,13 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)
>>  	if (strcmp(dentry->d_name.name, sd->s_name) != 0)
>>  		goto out_bad;
>>  
>> +	/* The sysfs dirent has been moved to a different namespace */
>> +	type = KOBJ_NS_TYPE_NONE;
>> +	if (sd->s_parent)
>> +		type = sysfs_ns_type(sd->s_parent);
>> +	if (type && (sysfs_info(dentry->d_sb)->ns[type] != sd->s_ns))
>
> eww, the code is assuming that KOBJ_NS_TYPE_NONE has a value of zero. 
> Don't do that; it smells bad.

Gag.  An incomplete change in idiom.

KOBJ_NS_TYPE_NONE is explicitly defined as 0 so that it can be used
this way, and every where else in fs/sysfs/dir.c uses this idiom.

Furthermore your change below takes one line of readable code and turns
it into something inappropriate to talk about in polite company.

If you want the code to be perfect type should be defined as
"enum kobj_ns_type type" instead of "int kobj_ns_type".

Of course the truly perfect solution is to rework the sysfs
code in a manner similar to proc, with magic internal symlinks
and multiple parallel tress for the different namespaces.
For the users of sysfs semantically there would be no changes
but in the implementation there would many fewer special cases
for namespaces.   The only special case would be reduced to
the internal sysfs symlink that lookup would have to know
about.

> @@ -329,10 +329,12 @@ static int sysfs_dentry_revalidate(struc
>  
>  	/* The sysfs dirent has been moved to a different namespace */
>  	type = KOBJ_NS_TYPE_NONE;
> -	if (sd->s_parent)
> +	if (sd->s_parent) {
>  		type = sysfs_ns_type(sd->s_parent);
> -	if (type && (sysfs_info(dentry->d_sb)->ns[type] != sd->s_ns))
> -		goto out_bad;
> +		if (type != KOBJ_NS_TYPE_NONE &&
> +				sysfs_info(dentry->d_sb)->ns[type] != sd->s_ns)
> +			goto out_bad;
> +	}

Pray tell in what parallel universe is that monstrosity above more
readable than the line it replaces?

Eric

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