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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
