[PATCH] quota: fix dquot_disable vs dquot_trasnfer race

[PATCH] quota: fix dquot_disable vs dquot_trasnfer race

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 125 bytes --]

Jan please take care of the patch attached. This race relatively easy
to reproduce so IMHO it is candidate for stable trees


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-quota-fix-dquot_disable-vs-dquot_trasnfer-race.patch --]
[-- Type: text/x-diff, Size: 1915 bytes --]

>From 1c438550a9aca7c3e209a492a9747954de571d38 Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@gmail.com>
Date: Sat, 9 Oct 2010 18:57:45 +0400
Subject: [PATCH] quota: fix dquot_disable vs dquot_trasnfer race

I've got following race:
dquot_disable                              dquot_transfer
                                            ->dqget()
					       sb_has_quota_active
dqopt->flags &= ~dquot_state_flag(f, cnt)      atomic_inc(dq->dq_count)
 ->drop_dquot_ref(sb, cnt);
    down_write(dqptr_sem)
    inode->i_dquot[cnt] = NULL              ->__dquot_transfer
invalidate_dquots(sb, cnt);		       down_write(&dqptr_sem)
  ->wait for dq_wait_unused		       inode->i_dquot = new_dquot
  /* wait forever */                            ^^^^New quota user^^^^^^

Inodes was already cleaned from quotas, and we can not allow new ones.
We have to recheck quota state under dqptr_sem and before assignment,
as we do it in dquot_initialize()

Signed-off-by: Dmitry Monakhov <dmonakhov@gmail.com>
---
 fs/quota/dquot.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 686c2b7..94fc2a0 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1759,6 +1759,9 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		if (!transfer_to[cnt])
 			continue;
+		/* Avoid races with quotaoff() */
+		if (!sb_has_quota_active(inode->i_sb, cnt))
+			continue;
 		transfer_from[cnt] = inode->i_dquot[cnt];
 		ret = check_idq(transfer_to[cnt], 1, warntype_to + cnt);
 		if (ret)
@@ -1777,6 +1780,8 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
 		 */
 		if (!transfer_to[cnt])
 			continue;
+		if (!sb_has_quota_active(inode->i_sb, cnt))
+			continue;
 
 		/* Due to IO error we might not have transfer_from[] structure */
 		if (transfer_from[cnt]) {
-- 
1.6.6.1

Re: [PATCH] quota: fix dquot_disable vs dquot_trasnfer race

[Dmitry]

On Sat, 09 Oct 2010 19:24:56 +0400, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
> Jan please take care of the patch attached. This race relatively easy
> to reproduce so IMHO it is candidate for stable trees
OOps, please excuse me, this patch was prepared against my local tree
which has not "Pass back references to put" logic at the end.
Please ignore this patch. I'll send correct one soon.
> 
> From 1c438550a9aca7c3e209a492a9747954de571d38 Mon Sep 17 00:00:00 2001
> From: Dmitry Monakhov <dmonakhov@gmail.com>
> Date: Sat, 9 Oct 2010 18:57:45 +0400
> Subject: [PATCH] quota: fix dquot_disable vs dquot_trasnfer race
> 
> I've got following race:
> dquot_disable                              dquot_transfer
>                                             ->dqget()
> 					       sb_has_quota_active
> dqopt->flags &= ~dquot_state_flag(f, cnt)      atomic_inc(dq->dq_count)
>  ->drop_dquot_ref(sb, cnt);
>     down_write(dqptr_sem)
>     inode->i_dquot[cnt] = NULL              ->__dquot_transfer
> invalidate_dquots(sb, cnt);		       down_write(&dqptr_sem)
>   ->wait for dq_wait_unused		       inode->i_dquot = new_dquot
>   /* wait forever */                            ^^^^New quota user^^^^^^
> 
> Inodes was already cleaned from quotas, and we can not allow new ones.
> We have to recheck quota state under dqptr_sem and before assignment,
> as we do it in dquot_initialize()
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@gmail.com>
> ---
>  fs/quota/dquot.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 686c2b7..94fc2a0 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -1759,6 +1759,9 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
>  		if (!transfer_to[cnt])
>  			continue;
> +		/* Avoid races with quotaoff() */
> +		if (!sb_has_quota_active(inode->i_sb, cnt))
> +			continue;
>  		transfer_from[cnt] = inode->i_dquot[cnt];
>  		ret = check_idq(transfer_to[cnt], 1, warntype_to + cnt);
>  		if (ret)
> @@ -1777,6 +1780,8 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
>  		 */
>  		if (!transfer_to[cnt])
>  			continue;
> +		if (!sb_has_quota_active(inode->i_sb, cnt))
> +			continue;
>  
>  		/* Due to IO error we might not have transfer_from[] structure */
>  		if (transfer_from[cnt]) {
> -- 
> 1.6.6.1
> 

[PATCH] quota: fix dquot_disable vs dquot_trasnfer race v2

[Dmitry]

I've got following lockup:
dquot_disable                              dquot_transfer
                                            ->dqget()
					       sb_has_quota_active
dqopt->flags &= ~dquot_state_flag(f, cnt)      atomic_inc(dq->dq_count)
 ->drop_dquot_ref(sb, cnt);
    down_write(dqptr_sem)
    inode->i_dquot[cnt] = NULL              ->__dquot_transfer
invalidate_dquots(sb, cnt);		       down_write(&dqptr_sem)
  ->wait for dq_wait_unused		       inode->i_dquot = new_dquot
  /* wait forever */                            ^^^^New quota user^^^^^^

Inodes was already cleaned from quotas, and we can not allow new ones.
We have to recheck quota state under dqptr_sem and before assignment,
as we do it in dquot_initialize()

Signed-off-by: Dmitry Monakhov <dmonakhov@gmail.com>
---
 fs/quota/dquot.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 686c2b7..5b262d3 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1736,6 +1736,7 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
 	qsize_t rsv_space = 0;
 	struct dquot *transfer_from[MAXQUOTAS] = {};
 	int cnt, ret = 0;
+	char is_valid[MAXQUOTAS] = {};
 	char warntype_to[MAXQUOTAS];
 	char warntype_from_inodes[MAXQUOTAS], warntype_from_space[MAXQUOTAS];
 
@@ -1757,8 +1758,15 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
 	space = cur_space + rsv_space;
 	/* Build the transfer_from list and check the limits */
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
+		/*
+		 * Skip changes for same uid or gid or for turned off quota-type.
+		 */
 		if (!transfer_to[cnt])
 			continue;
+		/* Avoid races with quotaoff() */
+		if (!sb_has_quota_active(inode->i_sb, cnt))
+			continue;
+		is_valid[cnt] = 1;
 		transfer_from[cnt] = inode->i_dquot[cnt];
 		ret = check_idq(transfer_to[cnt], 1, warntype_to + cnt);
 		if (ret)
@@ -1772,12 +1780,8 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
 	 * Finally perform the needed transfer from transfer_from to transfer_to
 	 */
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
-		/*
-		 * Skip changes for same uid or gid or for turned off quota-type.
-		 */
-		if (!transfer_to[cnt])
+		if (!is_valid[cnt])
 			continue;
-
 		/* Due to IO error we might not have transfer_from[] structure */
 		if (transfer_from[cnt]) {
 			warntype_from_inodes[cnt] =
@@ -1803,7 +1807,9 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
 	mark_all_dquot_dirty(transfer_to);
 	/* Pass back references to put */
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
-		transfer_to[cnt] = transfer_from[cnt];
+		if (is_valid[cnt])
+			transfer_to[cnt] = transfer_from[cnt];
+
 warn:
 	flush_warnings(transfer_to, warntype_to);
 	flush_warnings(transfer_from, warntype_from_inodes);
-- 
1.6.6.1

Re: [PATCH] quota: fix dquot_disable vs dquot_trasnfer race v2

[Jan Kara]

On Sat 09-10-10 23:15:30, Dmitry wrote:
> 
> I've got following lockup:
> dquot_disable                              dquot_transfer
>                                             ->dqget()
> 					       sb_has_quota_active
> dqopt->flags &= ~dquot_state_flag(f, cnt)      atomic_inc(dq->dq_count)
>  ->drop_dquot_ref(sb, cnt);
>     down_write(dqptr_sem)
>     inode->i_dquot[cnt] = NULL              ->__dquot_transfer
> invalidate_dquots(sb, cnt);		       down_write(&dqptr_sem)
>   ->wait for dq_wait_unused		       inode->i_dquot = new_dquot
>   /* wait forever */                            ^^^^New quota user^^^^^^
> 
> Inodes was already cleaned from quotas, and we can not allow new ones.
> We have to recheck quota state under dqptr_sem and before assignment,
> as we do it in dquot_initialize()
  Thanks. Merged into my tree.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR