Re: [PATCH v2 1/1] dcache: Translating dentry into pathname without taking rename_lock

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Sep 05, 2013 at 02:55:16PM -0400, Waiman Long wrote:
> +	const char *dname = ACCESS_ONCE(dentry->d_name.name);
> +	u32 dlen = dentry->d_name.len;
> +	int error;
> +
> +	if (likely(dname == (const char *)dentry->d_iname)) {
> +		/*
> +		 * Internal dname, the string memory is valid as long
> +		 * as its length is not over the limit.
> +		 */
> +		if (unlikely(dlen > sizeof(dentry->d_iname)))
> +			return -EINVAL;
> +	} else if (!dname)
> +		return -EINVAL;
Can't happen.
> +	else {
> +		const char *ptr;
> +		u32 len;
> +
> +		/*
> +		 * External dname, need to fetch name pointer and length
> +		 * again under d_lock to get a consistent set and avoid
> +		 * racing with d_move() which will take d_lock before
> +		 * acting on the dentries.
> +		 */
> +		spin_lock(&dentry->d_lock);
> +		dname = dentry->d_name.name;
> +		dlen  = dentry->d_name.len;
> +		spin_unlock(&dentry->d_lock);
> +
> +		if (unlikely(!dname || !dlen))
> +			return -EINVAL;
Can't happen.

> +		/*
> +		 * As the length and the content of the string may not be
> +		 * valid, need to scan the string and return EINVAL if there
> +		 * is embedded null byte within the length of the string.
> +		 */
> +		for (ptr = dname, len = dlen; len; len--, ptr++) {
> +			if (*ptr == '\0')
> +				return -EINVAL;

Egads...  First of all, this is completely pointless - if you've grabbed
->d_name.name and ->d_name.len under ->d_lock, you don't *need* that crap.
At all.  The whole point of that exercise is to avoid taking ->d_lock;
_that_ is where the "read byte by byte until you hit NUL" comes from.
And if you do that, you can bloody well just go ahead and store them in
the target array *as* *you* *go*.  No reason to bother with memcpy()
afterwards.

Damnit, just grab len and name (no ->d_lock, etc.).  Check if you've got
enough space in the buffer, treat "not enough" as an overflow.  Then
proceed to copy the damn thing over there (starting at *buffer -= len)
byte by byte, stopping when you've copied len bytes *or* when the byte you've
got happens to be NUL.  Don't bother with EINVAL, etc. - just return to
caller and let rename_lock logics take care of the races.  That's it - nothing
more is needed.