ocfs2: the parent directory reference count should be increased only after the ocfs2_add_entry function is successfully called
diff mbox series

Message ID 27b32a04-d3f5-4a14-ec0e-11816c42c9a2@huawei.com
State New
Headers show
Series
  • ocfs2: the parent directory reference count should be increased only after the ocfs2_add_entry function is successfully called
Related show

Commit Message

wangjian March 15, 2020, 4:59 p.m. UTC
Under some conditions, the directory cannot be deleted.
The specific scenarios are as follows: (for example,
/mnt/ocfs2 is the mount point)

1. Create the /mnt/ocfs2/p_dir directory. At this time,
the i_nlink corresponding to the inode of
the /mnt/ocfs2/p_dir directory is equal to 2.

2. During the process of creating the
/mnt/ocfs2/p_dir/s_dir directory, if the call to the inc_nlink
function in ocfs2_mknod succeeds, the functions such as
ocfs2_init_acl, ocfs2_init_security_set, and ocfs2_dentry_attach_lock fail.
At this time, the i_nlink corresponding to the inode of the
/mnt/ocfs2/p_dir directory is equal to 3, but /mnt/ocfs2/p_dir/s_dir
is not added to the /mnt/ocfs2/p_dir directory entry.

3. Delete the /mnt/ocfs2/p_dir directory (rm -rf /mnt/ocfs2/p_dir).
At this time, it is found that the i_nlink corresponding to
the inode corresponding to the /mnt/ocfs2/p_dir directory is equal to 3.
Therefore, the /mnt/ocfs2/p_dir directory cannot be deleted.

Signed-off-by: Jian wang <wangjian161@huawei.com>
---
  ocfs2/namei.c | 24 +++++++++++++-----------
  1 file changed, 13 insertions(+), 11 deletions(-)

Comments

piaojun March 16, 2020, 3:56 a.m. UTC | #1
On 2020/3/16 0:59, wangjian wrote:
> Under some conditions, the directory cannot be deleted.
> The specific scenarios are as follows: (for example,
> /mnt/ocfs2 is the mount point)
> 
> 1. Create the /mnt/ocfs2/p_dir directory. At this time,
> the i_nlink corresponding to the inode of
> the /mnt/ocfs2/p_dir directory is equal to 2.
> 
> 2. During the process of creating the
> /mnt/ocfs2/p_dir/s_dir directory, if the call to the inc_nlink
> function in ocfs2_mknod succeeds, the functions such as
> ocfs2_init_acl, ocfs2_init_security_set, and ocfs2_dentry_attach_lock fail.
> At this time, the i_nlink corresponding to the inode of the
> /mnt/ocfs2/p_dir directory is equal to 3, but /mnt/ocfs2/p_dir/s_dir
> is not added to the /mnt/ocfs2/p_dir directory entry.
> 
> 3. Delete the /mnt/ocfs2/p_dir directory (rm -rf /mnt/ocfs2/p_dir).
> At this time, it is found that the i_nlink corresponding to
> the inode corresponding to the /mnt/ocfs2/p_dir directory is equal to 3.
> Therefore, the /mnt/ocfs2/p_dir directory cannot be deleted.
> 
> Signed-off-by: Jian wang <wangjian161@huawei.com>
> ---
>  ocfs2/namei.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/ocfs2/namei.c b/ocfs2/namei.c
> index 8ea51cf..19543b4 100644
> --- a/ocfs2/namei.c
> +++ b/ocfs2/namei.c
> @@ -388,17 +388,6 @@ static int ocfs2_mknod(struct inode *dir,
>  			mlog_errno(status);
>  			goto leave;
>  		}
> -
> -		status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
> -						 parent_fe_bh,
> -						 OCFS2_JOURNAL_ACCESS_WRITE);
> -		if (status < 0) {
> -			mlog_errno(status);
> -			goto leave;
> -		}
> -		ocfs2_add_links_count(dirfe, 1);
> -		ocfs2_journal_dirty(handle, parent_fe_bh);
> -		inc_nlink(dir);
>  	}
>  
>  	status = ocfs2_init_acl(handle, inode, dir, new_fe_bh, parent_fe_bh,
> @@ -440,6 +429,19 @@ static int ocfs2_mknod(struct inode *dir,
>  		goto leave;
>  	}
>  
> +	if (S_ISDIR(mode)) {
> +		status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
> +						 parent_fe_bh,
> +						 OCFS2_JOURNAL_ACCESS_WRITE);
> +		if (status < 0) {

I wonder if we should rollback the inode's new_fe_bh if getting failed
here?

Jun

> +			mlog_errno(status);
> +			goto leave;
> +		}
> +		ocfs2_add_links_count(dirfe, 1);
> +		ocfs2_journal_dirty(handle, parent_fe_bh);
> +		inc_nlink(dir);
> +	}
> +
>  	insert_inode_hash(inode);
>  	d_instantiate(dentry, inode);
>  	status = 0;
> -- 
> 1.8.3.1
> 
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>
Joseph Qi March 16, 2020, 4:46 a.m. UTC | #2
On 2020/3/16 00:59, wangjian wrote:
> Under some conditions, the directory cannot be deleted.
> The specific scenarios are as follows: (for example,
> /mnt/ocfs2 is the mount point)
> 
> 1. Create the /mnt/ocfs2/p_dir directory. At this time,
> the i_nlink corresponding to the inode of
> the /mnt/ocfs2/p_dir directory is equal to 2.
> 
> 2. During the process of creating the
> /mnt/ocfs2/p_dir/s_dir directory, if the call to the inc_nlink
> function in ocfs2_mknod succeeds, the functions such as
> ocfs2_init_acl, ocfs2_init_security_set, and ocfs2_dentry_attach_lock fail.
> At this time, the i_nlink corresponding to the inode of the
> /mnt/ocfs2/p_dir directory is equal to 3, but /mnt/ocfs2/p_dir/s_dir
> is not added to the /mnt/ocfs2/p_dir directory entry.
> 
> 3. Delete the /mnt/ocfs2/p_dir directory (rm -rf /mnt/ocfs2/p_dir).
> At this time, it is found that the i_nlink corresponding to
> the inode corresponding to the /mnt/ocfs2/p_dir directory is equal to 3.
> Therefore, the /mnt/ocfs2/p_dir directory cannot be deleted.
> 
> Signed-off-by: Jian wang <wangjian161@huawei.com>
> ---
>  ocfs2/namei.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/ocfs2/namei.c b/ocfs2/namei.c
> index 8ea51cf..19543b4 100644
> --- a/ocfs2/namei.c
> +++ b/ocfs2/namei.c
> @@ -388,17 +388,6 @@ static int ocfs2_mknod(struct inode *dir,
>              mlog_errno(status);
>              goto leave;
>          }
> -
> -        status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
> -                         parent_fe_bh,
> -                         OCFS2_JOURNAL_ACCESS_WRITE);
> -        if (status < 0) {
> -            mlog_errno(status);
> -            goto leave;
> -        }

We can't simply move it down since we should get journal access before
modification.

Thanks,
Joseph 

> -        ocfs2_add_links_count(dirfe, 1);
> -        ocfs2_journal_dirty(handle, parent_fe_bh);
> -        inc_nlink(dir);
>      }
>  
>      status = ocfs2_init_acl(handle, inode, dir, new_fe_bh, parent_fe_bh,
> @@ -440,6 +429,19 @@ static int ocfs2_mknod(struct inode *dir,
>          goto leave;
>      }
>  
> +    if (S_ISDIR(mode)) {
> +        status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
> +                         parent_fe_bh,
> +                         OCFS2_JOURNAL_ACCESS_WRITE);
> +        if (status < 0) {
> +            mlog_errno(status);
> +            goto leave;
> +        }
> +        ocfs2_add_links_count(dirfe, 1);
> +        ocfs2_journal_dirty(handle, parent_fe_bh);
> +        inc_nlink(dir);
> +    }
> +
>      insert_inode_hash(inode);
>      d_instantiate(dentry, inode);
>      status = 0;
wangjian March 16, 2020, 7:27 a.m. UTC | #3
good suggestion. Can we just move the three statements 
ocfs2_add_links_count (dirfe, 1),
ocfs2_journal_dirty (handle, parent_fe_bh), inc_nlink (dir) to the 
following?
This seems to be no problem. Or reset the reference count back in case 
of failure?

Thanks,
Jian

On 3/16/2020 12:46 PM, Joseph Qi wrote:
>
> On 2020/3/16 00:59, wangjian wrote:
>> Under some conditions, the directory cannot be deleted.
>> The specific scenarios are as follows: (for example,
>> /mnt/ocfs2 is the mount point)
>>
>> 1. Create the /mnt/ocfs2/p_dir directory. At this time,
>> the i_nlink corresponding to the inode of
>> the /mnt/ocfs2/p_dir directory is equal to 2.
>>
>> 2. During the process of creating the
>> /mnt/ocfs2/p_dir/s_dir directory, if the call to the inc_nlink
>> function in ocfs2_mknod succeeds, the functions such as
>> ocfs2_init_acl, ocfs2_init_security_set, and ocfs2_dentry_attach_lock fail.
>> At this time, the i_nlink corresponding to the inode of the
>> /mnt/ocfs2/p_dir directory is equal to 3, but /mnt/ocfs2/p_dir/s_dir
>> is not added to the /mnt/ocfs2/p_dir directory entry.
>>
>> 3. Delete the /mnt/ocfs2/p_dir directory (rm -rf /mnt/ocfs2/p_dir).
>> At this time, it is found that the i_nlink corresponding to
>> the inode corresponding to the /mnt/ocfs2/p_dir directory is equal to 3.
>> Therefore, the /mnt/ocfs2/p_dir directory cannot be deleted.
>>
>> Signed-off-by: Jian wang <wangjian161@huawei.com>
>> ---
>>   ocfs2/namei.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/ocfs2/namei.c b/ocfs2/namei.c
>> index 8ea51cf..19543b4 100644
>> --- a/ocfs2/namei.c
>> +++ b/ocfs2/namei.c
>> @@ -388,17 +388,6 @@ static int ocfs2_mknod(struct inode *dir,
>>               mlog_errno(status);
>>               goto leave;
>>           }
>> -
>> -        status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
>> -                         parent_fe_bh,
>> -                         OCFS2_JOURNAL_ACCESS_WRITE);
>> -        if (status < 0) {
>> -            mlog_errno(status);
>> -            goto leave;
>> -        }
> We can't simply move it down since we should get journal access before
> modification.
>
> Thanks,
> Joseph
>
>> -        ocfs2_add_links_count(dirfe, 1);
>> -        ocfs2_journal_dirty(handle, parent_fe_bh);
>> -        inc_nlink(dir);
>>       }
>>   
>>       status = ocfs2_init_acl(handle, inode, dir, new_fe_bh, parent_fe_bh,
>> @@ -440,6 +429,19 @@ static int ocfs2_mknod(struct inode *dir,
>>           goto leave;
>>       }
>>   
>> +    if (S_ISDIR(mode)) {
>> +        status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
>> +                         parent_fe_bh,
>> +                         OCFS2_JOURNAL_ACCESS_WRITE);
>> +        if (status < 0) {
>> +            mlog_errno(status);
>> +            goto leave;
>> +        }
>> +        ocfs2_add_links_count(dirfe, 1);
>> +        ocfs2_journal_dirty(handle, parent_fe_bh);
>> +        inc_nlink(dir);
>> +    }
>> +
>>       insert_inode_hash(inode);
>>       d_instantiate(dentry, inode);
>>       status = 0;
> .
>
Joseph Qi March 17, 2020, 12:58 a.m. UTC | #4
On 2020/3/16 15:27, wangjian wrote:
> good suggestion. Can we just move the three statements ocfs2_add_links_count (dirfe, 1),
> ocfs2_journal_dirty (handle, parent_fe_bh), inc_nlink (dir) to the following?
> This seems to be no problem. Or reset the reference count back in case of failure?
> 
IMO, rollback the link modification in error handling may be a better choice.

Thanks,
Joseph

Patch
diff mbox series

diff --git a/ocfs2/namei.c b/ocfs2/namei.c
index 8ea51cf..19543b4 100644
--- a/ocfs2/namei.c
+++ b/ocfs2/namei.c
@@ -388,17 +388,6 @@  static int ocfs2_mknod(struct inode *dir,
  			mlog_errno(status);
  			goto leave;
  		}
-
-		status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
-						 parent_fe_bh,
-						 OCFS2_JOURNAL_ACCESS_WRITE);
-		if (status < 0) {
-			mlog_errno(status);
-			goto leave;
-		}
-		ocfs2_add_links_count(dirfe, 1);
-		ocfs2_journal_dirty(handle, parent_fe_bh);
-		inc_nlink(dir);
  	}
  
  	status = ocfs2_init_acl(handle, inode, dir, new_fe_bh, parent_fe_bh,
@@ -440,6 +429,19 @@  static int ocfs2_mknod(struct inode *dir,
  		goto leave;
  	}
  
+	if (S_ISDIR(mode)) {
+		status = ocfs2_journal_access_di(handle, INODE_CACHE(dir),
+						 parent_fe_bh,
+						 OCFS2_JOURNAL_ACCESS_WRITE);
+		if (status < 0) {
+			mlog_errno(status);
+			goto leave;
+		}
+		ocfs2_add_links_count(dirfe, 1);
+		ocfs2_journal_dirty(handle, parent_fe_bh);
+		inc_nlink(dir);
+	}
+
  	insert_inode_hash(inode);
  	d_instantiate(dentry, inode);
  	status = 0;