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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux