On Thu, Apr 17, 2014 at 8:40 PM, Lin Ming <minggr@xxxxxxxxx> wrote:
> On Thu, Apr 17, 2014 at 8:17 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>> On Fri, Apr 18, 2014 at 03:48:37AM +0100, Al Viro wrote:
>>> Crap... No, it's a bit trickier - we start with clearing all flags,
>>> so if we see the _intermediate_ d_flags and new d_inode, we'll sail
>>> past the check. Which would leave us with correct inode, but might
>>> give us a false negative in should_follow_link().
>>
>> Note that most of the places calling d_is_...() are protected by
>> the following: if we'd obtained dentry by __lookup_hash() and friends
>> *after* grabbing ->i_mutex on parent, we are fine - both positive-to-negative
>> and negative-to-positive are possible only with ->i_mutex held, so it
>> gives us a barrier.
>>
>> AFAICS, there are two more tricky places: walk_component() doing
>> if (!inode)
>> goto out_path_put;
>>
>> if (should_follow_link(path->dentry, follow)) {
>> with unpleasant consequences if the second test gives a false negative,
>> and similar for mountpoint_last().
>>
>> Basically, we'd concentrated on RCU races back then, and missed the possibility
>> of non-RCU-but-without-i_mutex ones. Proposed fix follows:
>
> Should be OK. I'll test it tomorrow.
Confirmed it's OK.
Reported-and-tested-by: Lin Ming <minggr@xxxxxxxxx>
Thanks.
>
> Thanks.
>
>>
>> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>> ---
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 40707d8..494a9def 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -1647,8 +1647,7 @@ static void __d_instantiate(struct dentry *dentry, struct inode *inode)
>> unsigned add_flags = d_flags_for_inode(inode);
>>
>> spin_lock(&dentry->d_lock);
>> - dentry->d_flags &= ~DCACHE_ENTRY_TYPE;
>> - dentry->d_flags |= add_flags;
>> + __d_set_type(dentry, add_flags);
>> if (inode)
>> hlist_add_head(&dentry->d_alias, &inode->i_dentry);
>> dentry->d_inode = inode;
>> diff --git a/fs/namei.c b/fs/namei.c
>> index c6157c8..8016827 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -1542,7 +1542,7 @@ static inline int walk_component(struct nameidata *nd, struct path *path,
>> inode = path->dentry->d_inode;
>> }
>> err = -ENOENT;
>> - if (!inode)
>> + if (!inode || d_is_negative(path->dentry))
>> goto out_path_put;
>>
>> if (should_follow_link(path->dentry, follow)) {
>> @@ -2249,7 +2249,7 @@ mountpoint_last(struct nameidata *nd, struct path *path)
>> mutex_unlock(&dir->d_inode->i_mutex);
>>
>> done:
>> - if (!dentry->d_inode) {
>> + if (!dentry->d_inode || d_is_negative(dentry)) {
>> error = -ENOENT;
>> dput(dentry);
>> goto out;
>> @@ -2994,7 +2994,7 @@ retry_lookup:
>> finish_lookup:
>> /* we _can_ be in RCU mode here */
>> error = -ENOENT;
>> - if (d_is_negative(path->dentry)) {
>> + if (!inode || d_is_negative(path->dentry)) {
>> path_to_nameidata(path, nd);
>> goto out;
>> }
--
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
