Messages in this thread | | | Date | Thu, 05 Sep 2013 16:43:14 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v2 1/1] dcache: Translating dentry into pathname without taking rename_lock |
| |
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
| |