linux-kernel - Re: [PATCH 04/20] sysfs: Handle the general case of removing of directories with subdirectories

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <4A14F356.3030501@kernel.org>
Date:	Thu, 21 May 2009 15:23:18 +0900
From:	Tejun Heo <tj@...nel.org>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	linux-kernel@...r.kernel.org,
	Cornelia Huck <cornelia.huck@...ibm.com>,
	linux-fsdevel@...r.kernel.org,
	"Eric W. Biederman" <ebiederm@...stanetworks.com>
Subject: Re: [PATCH 04/20] sysfs: Handle the general case of removing of directories
 with subdirectories

Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@...ssion.com>
> 
> Modify sysfs to properly remove directories containing attributes and
> subdirectories.  The code is relatively simple and means we don't have
> to worry about what might use this logic.
> 
> In a quick survey I have only found /sys/dev/char and /sys/dev/block that are
> removing non-enmpty directories today (and they are exclusively filled with symlinks).
> So only removing empty directories does not appear to be an option.
> 
> I don't hold sysfs_mutex across the entire operation as that is unneeded
> for coherence at the sysfs level and some level of coordination is expected
> at the upper layers.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@...stanetworks.com>
...
> -void sysfs_remove_subdir(struct sysfs_dirent *sd)
> -{
> -	remove_dir(sd);
> +	struct sysfs_dirent *sd = dir_sd;
> +	mutex_lock(&sysfs_mutex);
> +	while ((sysfs_type(sd) == SYSFS_DIR) && sd->s_dir.children)
> +		sd = sd->s_dir.children;
> +	if (sd != dir_sd)
> +		sysfs_get(sd);
> +	else
> +		sd = NULL;
> +	mutex_unlock(&sysfs_mutex);
> +	return sd;
>  }

Some blank lines wouldn't hurt, especially after local variable
declaration.

> -static void __sysfs_remove_dir(struct sysfs_dirent *dir_sd)
> +static void remove_dir(struct sysfs_dirent *dir_sd)
>  {
>  	struct sysfs_addrm_cxt acxt;
> -	struct sysfs_dirent **pos;
> -
> -	if (!dir_sd)
> -		return;
> +	struct sysfs_dirent *sd;
>  
>  	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;
>  
> -		if (sysfs_type(sd) != SYSFS_DIR)
> -			sysfs_remove_one(&acxt, sd);
> -		else
> -			pos = &(*pos)->s_sibling;
> +	while ((sd = sysfs_get_one(dir_sd))) {
> +		sysfs_addrm_start(&acxt, sd->s_parent);
> +		sysfs_remove_one(&acxt, sd);
> +		sysfs_addrm_finish(&acxt);
> +		sysfs_put(sd);
>  	}
> +	sysfs_addrm_start(&acxt, dir_sd->s_parent);
> +	sysfs_remove_one(&acxt, dir_sd);
>  	sysfs_addrm_finish(&acxt);
> +}

I agree we should be heading this way but what happens to attributes
or directories living below the subdirectories?  If it's gonna handle
recursive case, I think it better do it properly.  I had patches of
similar effect.

 http://article.gmane.org/gmane.linux.kernel/582151
 http://article.gmane.org/gmane.linux.kernel/582155

The patchset didn't really go anywhere but the recursive atomic
removal should be usable.

Thanks.

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