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

[Waiman Long]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/