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

[Waiman Long <waiman.long@hp.com>]

On 09/05/2013 04:04 PM, Al Viro wrote:
> 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.

That is what I thought too. I am just not totally sure about it. So yes, 
I can scrap all these additional check.

As the internal dname buffer is at least 32 bytes, most dentries will 
use the internal buffer instead of allocating from kmem. IOW, the d_lock 
taking code path is unlikely to be used.

> 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.

OK, I will do that.

-Longman