diff mbox series

[2/2] quota: fix warning in dqgrab()

Message ID	20230527014018.47396-3-yebin10@huawei.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Ye Bin <yebin10@huawei.com> To: <jack@suse.com>, <linux-kernel@vger.kernel.org> CC: <yebin10@huawei.com> Subject: [PATCH 2/2] quota: fix warning in dqgrab() Date: Sat, 27 May 2023 09:40:18 +0800 Message-ID: <20230527014018.47396-3-yebin10@huawei.com> In-Reply-To: <20230527014018.47396-1-yebin10@huawei.com> References: <20230527014018.47396-1-yebin10@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII Precedence: bulk
Series	fix two issue about quota \| [0/2] fix two issue about quota [1/2] quota: fix null-ptr-deref in ext4_acquire_dquot() [2/2] quota: fix warning in dqgrab()

Commit Message

Ye Bin May 27, 2023, 1:40 a.m. UTC

  There's issue as follows when do fault injection:
WARNING: CPU: 1 PID: 14870 at include/linux/quotaops.h:51 dquot_disable+0x13b7/0x18c0
Modules linked in:
CPU: 1 PID: 14870 Comm: fsconfig Not tainted 6.3.0-next-20230505-00006-g5107a9c821af-dirty #541
RIP: 0010:dquot_disable+0x13b7/0x18c0
RSP: 0018:ffffc9000acc79e0 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff88825e41b980
RDX: 0000000000000000 RSI: ffff88825e41b980 RDI: 0000000000000002
RBP: ffff888179f68000 R08: ffffffff82087ca7 R09: 0000000000000000
R10: 0000000000000001 R11: ffffed102f3ed026 R12: ffff888179f68130
R13: ffff888179f68110 R14: dffffc0000000000 R15: ffff888179f68118
FS:  00007f450a073740(0000) GS:ffff88882fc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffe96f2efd8 CR3: 000000025c8ad000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 dquot_load_quota_sb+0xd53/0x1060
 dquot_resume+0x172/0x230
 ext4_reconfigure+0x1dc6/0x27b0
 reconfigure_super+0x515/0xa90
 __x64_sys_fsconfig+0xb19/0xd20
 do_syscall_64+0x39/0xb0
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Above issue may happens as follows:
ProcessA              ProcessB                    ProcessC
sys_fsconfig
  vfs_fsconfig_locked
   reconfigure_super
     ext4_remount
      dquot_suspend -> suspend all type quota

                 sys_fsconfig
                  vfs_fsconfig_locked
                    reconfigure_super
                     ext4_remount
                      dquot_resume
                       ret = dquot_load_quota_sb
                        add_dquot_ref
                                           do_open  -> open file O_RDWR
                                            vfs_open
                                             do_dentry_open
                                              get_write_access
                                               atomic_inc_unless_negative(&inode->i_writecount)
                                              ext4_file_open
                                               dquot_file_open
                                                dquot_initialize
                                                  __dquot_initialize
                                                   dqget
						    atomic_inc(&dquot->dq_count);

                          __dquot_initialize
                           __dquot_initialize
                            dqget
                             if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))
                               ext4_acquire_dquot
			        -> Return error DQ_ACTIVE_B flag isn't set
                         dquot_disable
			  invalidate_dquots
			   if (atomic_read(&dquot->dq_count))
	                    dqgrab
			     WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))
	                      -> Trigger warning

In the above scenario, 'dquot->dq_flags' has no DQ_ACTIVE_B is normal when
dqgrab().
So just remove 'WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))'
in dqgrab().

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 include/linux/quotaops.h | 1 -
 1 file changed, 1 deletion(-)

Comments

Jan Kara May 30, 2023, 10:15 a.m. UTC | #1

On Sat 27-05-23 09:40:18, Ye Bin wrote:
> There's issue as follows when do fault injection:
> WARNING: CPU: 1 PID: 14870 at include/linux/quotaops.h:51 dquot_disable+0x13b7/0x18c0
> Modules linked in:
> CPU: 1 PID: 14870 Comm: fsconfig Not tainted 6.3.0-next-20230505-00006-g5107a9c821af-dirty #541
> RIP: 0010:dquot_disable+0x13b7/0x18c0
> RSP: 0018:ffffc9000acc79e0 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff88825e41b980
> RDX: 0000000000000000 RSI: ffff88825e41b980 RDI: 0000000000000002
> RBP: ffff888179f68000 R08: ffffffff82087ca7 R09: 0000000000000000
> R10: 0000000000000001 R11: ffffed102f3ed026 R12: ffff888179f68130
> R13: ffff888179f68110 R14: dffffc0000000000 R15: ffff888179f68118
> FS:  00007f450a073740(0000) GS:ffff88882fc00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007ffe96f2efd8 CR3: 000000025c8ad000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  dquot_load_quota_sb+0xd53/0x1060
>  dquot_resume+0x172/0x230
>  ext4_reconfigure+0x1dc6/0x27b0
>  reconfigure_super+0x515/0xa90
>  __x64_sys_fsconfig+0xb19/0xd20
>  do_syscall_64+0x39/0xb0
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Above issue may happens as follows:
> ProcessA              ProcessB                    ProcessC
> sys_fsconfig
>   vfs_fsconfig_locked
>    reconfigure_super
>      ext4_remount
>       dquot_suspend -> suspend all type quota
> 
>                  sys_fsconfig
>                   vfs_fsconfig_locked
>                     reconfigure_super
>                      ext4_remount
>                       dquot_resume
>                        ret = dquot_load_quota_sb
>                         add_dquot_ref
>                                            do_open  -> open file O_RDWR
>                                             vfs_open
>                                              do_dentry_open
>                                               get_write_access
>                                                atomic_inc_unless_negative(&inode->i_writecount)
>                                               ext4_file_open
>                                                dquot_file_open
>                                                 dquot_initialize
>                                                   __dquot_initialize
>                                                    dqget
> 						    atomic_inc(&dquot->dq_count);
> 
>                           __dquot_initialize
>                            __dquot_initialize
>                             dqget
>                              if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))
>                                ext4_acquire_dquot
> 			        -> Return error DQ_ACTIVE_B flag isn't set
>                          dquot_disable
> 			  invalidate_dquots
> 			   if (atomic_read(&dquot->dq_count))
> 	                    dqgrab
> 			     WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))
> 	                      -> Trigger warning
> 
> In the above scenario, 'dquot->dq_flags' has no DQ_ACTIVE_B is normal when
> dqgrab().
> So just remove 'WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))'
> in dqgrab().
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Thanks for the patch! Actually rather than deleting the assertion from
dqgrab() (which can be and is used by filesystems) I'd replace the dqgrab()
use in invalidate_dquots() with plain:

	atomic_inc(&dquot->dq_count);

								Honza

> ---
>  include/linux/quotaops.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> index 11a4becff3a9..cb5e4c11e503 100644
> --- a/include/linux/quotaops.h
> +++ b/include/linux/quotaops.h
> @@ -48,7 +48,6 @@ static inline struct dquot *dqgrab(struct dquot *dquot)
>  {
>  	/* Make sure someone else has active reference to dquot */
>  	WARN_ON_ONCE(!atomic_read(&dquot->dq_count));
> -	WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags));
>  	atomic_inc(&dquot->dq_count);
>  	return dquot;
>  }
> -- 
> 2.31.1
>

Ye Bin June 3, 2023, 2:57 a.m. UTC | #2

On 2023/5/30 18:15, Jan Kara wrote:
> On Sat 27-05-23 09:40:18, Ye Bin wrote:
>> There's issue as follows when do fault injection:
>> WARNING: CPU: 1 PID: 14870 at include/linux/quotaops.h:51 dquot_disable+0x13b7/0x18c0
>> Modules linked in:
>> CPU: 1 PID: 14870 Comm: fsconfig Not tainted 6.3.0-next-20230505-00006-g5107a9c821af-dirty #541
>> RIP: 0010:dquot_disable+0x13b7/0x18c0
>> RSP: 0018:ffffc9000acc79e0 EFLAGS: 00010246
>> RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff88825e41b980
>> RDX: 0000000000000000 RSI: ffff88825e41b980 RDI: 0000000000000002
>> RBP: ffff888179f68000 R08: ffffffff82087ca7 R09: 0000000000000000
>> R10: 0000000000000001 R11: ffffed102f3ed026 R12: ffff888179f68130
>> R13: ffff888179f68110 R14: dffffc0000000000 R15: ffff888179f68118
>> FS:  00007f450a073740(0000) GS:ffff88882fc00000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007ffe96f2efd8 CR3: 000000025c8ad000 CR4: 00000000000006e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>>   <TASK>
>>   dquot_load_quota_sb+0xd53/0x1060
>>   dquot_resume+0x172/0x230
>>   ext4_reconfigure+0x1dc6/0x27b0
>>   reconfigure_super+0x515/0xa90
>>   __x64_sys_fsconfig+0xb19/0xd20
>>   do_syscall_64+0x39/0xb0
>>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Above issue may happens as follows:
>> ProcessA              ProcessB                    ProcessC
>> sys_fsconfig
>>    vfs_fsconfig_locked
>>     reconfigure_super
>>       ext4_remount
>>        dquot_suspend -> suspend all type quota
>>
>>                   sys_fsconfig
>>                    vfs_fsconfig_locked
>>                      reconfigure_super
>>                       ext4_remount
>>                        dquot_resume
>>                         ret = dquot_load_quota_sb
>>                          add_dquot_ref
>>                                             do_open  -> open file O_RDWR
>>                                              vfs_open
>>                                               do_dentry_open
>>                                                get_write_access
>>                                                 atomic_inc_unless_negative(&inode->i_writecount)
>>                                                ext4_file_open
>>                                                 dquot_file_open
>>                                                  dquot_initialize
>>                                                    __dquot_initialize
>>                                                     dqget
>> 						    atomic_inc(&dquot->dq_count);
>>
>>                            __dquot_initialize
>>                             __dquot_initialize
>>                              dqget
>>                               if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))
>>                                 ext4_acquire_dquot
>> 			        -> Return error DQ_ACTIVE_B flag isn't set
>>                           dquot_disable
>> 			  invalidate_dquots
>> 			   if (atomic_read(&dquot->dq_count))
>> 	                    dqgrab
>> 			     WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))
>> 	                      -> Trigger warning
>>
>> In the above scenario, 'dquot->dq_flags' has no DQ_ACTIVE_B is normal when
>> dqgrab().
>> So just remove 'WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags))'
>> in dqgrab().
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Thanks for the patch! Actually rather than deleting the assertion from
> dqgrab() (which can be and is used by filesystems) I'd replace the dqgrab()
> use in invalidate_dquots() with plain:
>
> 	atomic_inc(&dquot->dq_count);
>
> 								Honza
This is indeed a good idea, I will send a new version.
>> ---
>>   include/linux/quotaops.h | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
>> index 11a4becff3a9..cb5e4c11e503 100644
>> --- a/include/linux/quotaops.h
>> +++ b/include/linux/quotaops.h
>> @@ -48,7 +48,6 @@ static inline struct dquot *dqgrab(struct dquot *dquot)
>>   {
>>   	/* Make sure someone else has active reference to dquot */
>>   	WARN_ON_ONCE(!atomic_read(&dquot->dq_count));
>> -	WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags));
>>   	atomic_inc(&dquot->dq_count);
>>   	return dquot;
>>   }
>> -- 
>> 2.31.1
>>

diff mbox series

Patch

diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index 11a4becff3a9..cb5e4c11e503 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -48,7 +48,6 @@  static inline struct dquot *dqgrab(struct dquot *dquot)
 {
 	/* Make sure someone else has active reference to dquot */
 	WARN_ON_ONCE(!atomic_read(&dquot->dq_count));
-	WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags));
 	atomic_inc(&dquot->dq_count);
 	return dquot;
 }