linux-kernel - Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

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: <20080619221605.GE10888@mail.oracle.com>
Date:	Thu, 19 Jun 2008 15:16:05 -0700
From:	Joel Becker <Joel.Becker@...cle.com>
To:	Louis Rilling <louis.rilling@...labs.com>
Cc:	linux-kernel@...r.kernel.org, ocfs2-devel@....oracle.com
Subject: Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making
	rmdir() fail

On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:

	Ok, I can see some of why I hated this.

> +
>  		spin_lock(&configfs_dirent_lock);
> -		if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
> -			spin_unlock(&configfs_dirent_lock);
> -			config_item_put(item);
> -			kfree(sl);
> -			return -EPERM;
> -		}
> -		list_add(&sl->sl_list, &target_sd->s_links);
> +		/*
> +		 * Force rmdir() of parent_item to wait until we know if we
> +		 * succeed.
> +		 */
> +		parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
>  		spin_unlock(&configfs_dirent_lock);
> +
>  		ret = configfs_create_link(sl, parent_item->ci_dentry,
>  					   dentry);
> -		if (ret) {
> -			spin_lock(&configfs_dirent_lock);
> -			list_del_init(&sl->sl_list);
> +
> +		spin_lock(&configfs_dirent_lock);
> +		parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
> +
> +		if (ret || target_sd->s_type & CONFIGFS_USET_DROPPING) {

	Use parenthesis.  Actually, separate out the error cases.

> +			struct configfs_dirent *sd = NULL;
> +
> +			if (!ret) {
> +				sd = dentry->d_fsdata;
> +				list_del_init(&sd->s_sibling);
> +			}
>  			spin_unlock(&configfs_dirent_lock);
> +
> +			if (!ret) {
> +				configfs_drop_dentry(sd, dentry->d_parent);
> +				d_delete(dentry);
> +				configfs_put(sd);
> +			}

	This open-code of the VFS munging is ripe to break when the VFS
changes or other configfs changes happen.  The real issue is that you
are reimplementing the core of configfs_unlink().  Note how the core VFS
munging of configfs_rmdir() is separated out so that configfs_mkdir()
can also use it in the failure case?  Do the same with unlink() and it
will read much better ("if (DROPPING) configfs_delete_link()").  Call it
configfs_remove_link() or configfs_delete_link().

Joel

-- 

Life's Little Instruction Book #337

	"Reread your favorite book."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@...cle.com
Phone: (650) 506-8127
--
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/