lkml.org 
[lkml]   [2013]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2013-09-06 02:41    [W:0.050 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site