Re: [PATCH 04/24] sysfs: Normalize removing sysfs directories.

[Eric W. Biederman]

Tejun Heo <tj@xxxxxxxxxx> writes:

> Hello,
>
> Eric W. Biederman wrote:
>> @@ -732,12 +732,28 @@ const struct inode_operations sysfs_dir_inode_operations = {
>>  	.setattr	= sysfs_setattr,
>>  };
>>  
>> -static void remove_dir(struct sysfs_dirent *sd)
>> +static void remove_dir(struct sysfs_dirent *dir_sd)
>>  {
>>  	struct sysfs_addrm_cxt acxt;
>>  
>> -	sysfs_addrm_start(&acxt, sd->s_parent);
>> -	sysfs_remove_one(&acxt, sd);
>> +	pr_debug("sysfs %s: removing dir\n", dir_sd->s_name);
>> +
>> +	/* Removing non-empty directories is not valid complain! */
> 						     ^^^
> 						  missing . or ,
>
>> +static struct sysfs_dirent *get_dirent_to_remove(struct sysfs_dirent *dir_sd)
>> +{
>> +	struct sysfs_dirent *sd;
>> +
>> +	mutex_lock(&sysfs_mutex);
>> +	for (sd = dir_sd->s_dir.children; sd; sd = sd->s_sibling) {
>> +		/* Directories might be owned by someone else
>> +		 * making recursive directory removal unsafe.
>> +		 */
>> +		if (sysfs_type(sd) == SYSFS_DIR)
>> +			continue;
>> +		break;
>> +	}
>> +	sysfs_get(sd);
>> +	mutex_unlock(&sysfs_mutex);
>> +
>> +	return sd;
>> +}
>>  
>>  static void __sysfs_remove_dir(struct sysfs_dirent *dir_sd)
>>  {
>>  	struct sysfs_addrm_cxt acxt;
>> -	struct sysfs_dirent **pos;
>> -
>> -	if (!dir_sd)
>> -		return;
>> -
>> -	pr_debug("sysfs %s: removing dir\n", dir_sd->s_name);
>> -	sysfs_addrm_start(&acxt, dir_sd);
>> -	pos = &dir_sd->s_dir.children;
>> -	while (*pos) {
>> -		struct sysfs_dirent *sd = *pos;
>> +	struct sysfs_dirent *sd;
>>  
>> -		if (sysfs_type(sd) != SYSFS_DIR)
>> -			sysfs_remove_one(&acxt, sd);
>> -		else
>> -			pos = &(*pos)->s_sibling;
>> +	/* Remove children that we think are safe */
>> +	while ((sd = get_dirent_to_remove(dir_sd))) {
>> +		sysfs_addrm_start(&acxt, sd->s_parent);
>> +		sysfs_remove_one(&acxt, sd);
>> +		sysfs_addrm_finish(&acxt);
>> +		sysfs_put(sd);
>>  	}
>> -	sysfs_addrm_finish(&acxt);
>
> Ummm... Null @dir_sd handling is being removed, which could be fine
> but please do it in a separate patch or at least mention it in the
> patch description.

Agreed.  That should be documented.  I took a look and it appears we
are completely protected by the kobj->state_in_sysfs flag.

  Also, I'm quite uncomfortable with these things
> being done in non-atomic manner.  It can be made to work but things
> like this can lead to subtle race conditions and with the kind of
> layering we put on top of sysfs (kobject, driver model, driver
> midlayers and so on), it isn't all that easy to verify what's going
> on, so NACK for this one.

Total nonsense.

Mucking about with sysfs after we start deleting a directory is a bug.
At worst my change makes a buggy race slightly less deterministic.

I am not ready to consider keeping the current unnecessary atomic
removal step.  That unnecessary atomicity makes the following patches
more difficult, and requires a lot of unnecessary retesting.

What do you think the extra unnecessary atomicity helps protect?

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/