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-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
