[PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
@ 2005-06-16 22:51 ` Jun'ichi Nomura
  0 siblings, 0 replies; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-16 22:51 UTC (permalink / raw)
  To: Alasdair Kergon, device-mapper development; +Cc: linux-kernel

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

Hello,

Attached patch fixes the another bug in dm-raid1.c that
the dirty region may stay in or be moved to clean list
and freed while in use.

It happens as follows:

   CPU0                                   CPU1
   ------------------------------------------------------------------------------
   rh_dec()
     if (atomic_dec_and_test(pending))
        <the region is still marked dirty>
                                          rh_inc()
                                            if the region is clean
                                               mark the region dirty
                                               and remove from clean list
        mark the region clean
        and move to clean list
                                                  atomic_inc(pending)

At this stage, the region is in clean list and
will be mistakenly reclaimed by rh_update_states() later.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>


[-- Attachment #2: dm-raid1-race2.patch --]
[-- Type: text/x-patch, Size: 1071 bytes --]

--- kernel/drivers/md/dm-raid1.c.orig	2005-06-16 07:13:50.610325768 -0400
+++ kernel/drivers/md/dm-raid1.c	2005-06-16 10:34:12.510719112 -0400
@@ -375,16 +380,18 @@ static void rh_inc(struct region_hash *r
 
 	read_lock(&rh->hash_lock);
 	reg = __rh_find(rh, region);
+
+	atomic_inc(&reg->pending);
+
+	spin_lock_irq(&rh->region_lock);
 	if (reg->state == RH_CLEAN) {
 		rh->log->type->mark_region(rh->log, reg->key);
 
-		spin_lock_irq(&rh->region_lock);
 		reg->state = RH_DIRTY;
 		list_del_init(&reg->list);	/* take off the clean list */
-		spin_unlock_irq(&rh->region_lock);
 	}
+	spin_unlock_irq(&rh->region_lock);
 
-	atomic_inc(&reg->pending);
 	read_unlock(&rh->hash_lock);
 }
 
@@ -408,6 +414,10 @@ static void rh_dec(struct region_hash *r
 
 	if (atomic_dec_and_test(&reg->pending)) {
 		spin_lock_irqsave(&rh->region_lock, flags);
+		if (atomic_read(&reg->pending)) { /* check race */
+			spin_unlock_irqrestore(&rh->region_lock, flags);
+			return;
+		}
 		if (reg->state == RH_RECOVERING) {
 			list_add_tail(&reg->list, &rh->quiesced_regions);
 		} else {

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
@ 2005-06-16 22:51 ` Jun'ichi Nomura
  0 siblings, 0 replies; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-16 22:51 UTC (permalink / raw)
  To: Alasdair Kergon, device-mapper development; +Cc: linux-kernel

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

Hello,

Attached patch fixes the another bug in dm-raid1.c that
the dirty region may stay in or be moved to clean list
and freed while in use.

It happens as follows:

   CPU0                                   CPU1
   ------------------------------------------------------------------------------
   rh_dec()
     if (atomic_dec_and_test(pending))
        <the region is still marked dirty>
                                          rh_inc()
                                            if the region is clean
                                               mark the region dirty
                                               and remove from clean list
        mark the region clean
        and move to clean list
                                                  atomic_inc(pending)

At this stage, the region is in clean list and
will be mistakenly reclaimed by rh_update_states() later.

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>


[-- Attachment #2: dm-raid1-race2.patch --]
[-- Type: text/x-patch, Size: 1071 bytes --]

--- kernel/drivers/md/dm-raid1.c.orig	2005-06-16 07:13:50.610325768 -0400
+++ kernel/drivers/md/dm-raid1.c	2005-06-16 10:34:12.510719112 -0400
@@ -375,16 +380,18 @@ static void rh_inc(struct region_hash *r
 
 	read_lock(&rh->hash_lock);
 	reg = __rh_find(rh, region);
+
+	atomic_inc(&reg->pending);
+
+	spin_lock_irq(&rh->region_lock);
 	if (reg->state == RH_CLEAN) {
 		rh->log->type->mark_region(rh->log, reg->key);
 
-		spin_lock_irq(&rh->region_lock);
 		reg->state = RH_DIRTY;
 		list_del_init(&reg->list);	/* take off the clean list */
-		spin_unlock_irq(&rh->region_lock);
 	}
+	spin_unlock_irq(&rh->region_lock);
 
-	atomic_inc(&reg->pending);
 	read_unlock(&rh->hash_lock);
 }
 
@@ -408,6 +414,10 @@ static void rh_dec(struct region_hash *r
 
 	if (atomic_dec_and_test(&reg->pending)) {
 		spin_lock_irqsave(&rh->region_lock, flags);
+		if (atomic_read(&reg->pending)) { /* check race */
+			spin_unlock_irqrestore(&rh->region_lock, flags);
+			return;
+		}
 		if (reg->state == RH_RECOVERING) {
 			list_add_tail(&reg->list, &rh->quiesced_regions);
 		} else {

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
  2005-06-16 22:51 ` Jun'ichi Nomura
  (?)
@ 2005-06-24 15:19 ` Jonathan E Brassow
  2005-06-24 15:45   ` Jun'ichi Nomura
  -1 siblings, 1 reply; 5+ messages in thread
From: Jonathan E Brassow @ 2005-06-24 15:19 UTC (permalink / raw)
  To: device-mapper development; +Cc: Nomura Jun'ichi

could this be solved by doing your patch in rh_dec and just moving the  
atomic_inc in rh_inc?  The reason I ask is that the mark_region log  
call can block.

  brassow
On Jun 16, 2005, at 5:51 PM, Jun'ichi Nomura wrote:

> Hello,
>
> Attached patch fixes the another bug in dm-raid1.c that
> the dirty region may stay in or be moved to clean list
> and freed while in use.
>
> It happens as follows:
>
>    CPU0                                   CPU1
>     
> ----------------------------------------------------------------------- 
> -------
>    rh_dec()
>      if (atomic_dec_and_test(pending))
>         <the region is still marked dirty>
>                                           rh_inc()
>                                             if the region is clean
>                                                mark the region dirty
>                                                and remove from clean  
> list
>         mark the region clean
>         and move to clean list
>                                                   atomic_inc(pending)
>
> At this stage, the region is in clean list and
> will be mistakenly reclaimed by rh_update_states() later.
>
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>
> --- kernel/drivers/md/dm-raid1.c.orig	2005-06-16 07:13:50.610325768  
> -0400
> +++ kernel/drivers/md/dm-raid1.c	2005-06-16 10:34:12.510719112 -0400
> @@ -375,16 +380,18 @@ static void rh_inc(struct region_hash *r
>
>  	read_lock(&rh->hash_lock);
>  	reg = __rh_find(rh, region);
> +
> +	atomic_inc(&reg->pending);
> +
> +	spin_lock_irq(&rh->region_lock);
>  	if (reg->state == RH_CLEAN) {
>  		rh->log->type->mark_region(rh->log, reg->key);
>
> -		spin_lock_irq(&rh->region_lock);
>  		reg->state = RH_DIRTY;
>  		list_del_init(&reg->list);	/* take off the clean list */
> -		spin_unlock_irq(&rh->region_lock);
>  	}
> +	spin_unlock_irq(&rh->region_lock);
>
> -	atomic_inc(&reg->pending);
>  	read_unlock(&rh->hash_lock);
>  }
>
> @@ -408,6 +414,10 @@ static void rh_dec(struct region_hash *r
>
>  	if (atomic_dec_and_test(&reg->pending)) {
>  		spin_lock_irqsave(&rh->region_lock, flags);
> +		if (atomic_read(&reg->pending)) { /* check race */
> +			spin_unlock_irqrestore(&rh->region_lock, flags);
> +			return;
> +		}
>  		if (reg->state == RH_RECOVERING) {
>  			list_add_tail(&reg->list, &rh->quiesced_regions);
>  		} else {
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
  2005-06-24 15:19 ` Jonathan E Brassow
@ 2005-06-24 15:45   ` Jun'ichi Nomura
  2005-06-27 17:34     ` Jun'ichi Nomura
  0 siblings, 1 reply; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-24 15:45 UTC (permalink / raw)
  To: Jonathan Brassow; +Cc: device-mapper development

Hi Jon,

Jonathan E Brassow wrote:
 > could this be solved by doing your patch in rh_dec and just moving the
 > atomic_inc in rh_inc?  The reason I ask is that the mark_region log
 > call can block.

No.
Unless they are serialized, it's possible that rh_inc() will see the
state RH_DIRTY, while rh_dec change it to RH_CLEAN.
As a result, the region which has I/O in-flight may be freed.

Is it reasonable to call mark_region() unconditionally?
Then we can call it outside of the lock.

 >>    CPU0                                   CPU1
 >>
 >> -----------------------------------------------------------------------
 >> -------
 >>    rh_dec()
 >>      if (atomic_dec_and_test(pending))
 >>         <the region is still marked dirty>
            if (atomic_read(pending)==0)
 >>                                           rh_inc()
 >>                                             atomic_inc(pending)
 >>                                             if the region is clean
 >>                                                mark the region dirty
 >>                                                and remove from clean list
                                                else do nothing
 >>         mark the region clean
 >>         and move to clean list

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
  2005-06-24 15:45   ` Jun'ichi Nomura
@ 2005-06-27 17:34     ` Jun'ichi Nomura
  0 siblings, 0 replies; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-27 17:34 UTC (permalink / raw)
  To: device-mapper development

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

Hello,

I revised the patch based on comments from Jon.
Attached patch should work on the version of mark_region which may block.

Thanks,

Jun'ichi Nomura wrote:
> Jonathan E Brassow wrote:
>  > could this be solved by doing your patch in rh_dec and just moving the
>  > atomic_inc in rh_inc?  The reason I ask is that the mark_region log
>  > call can block.
> 
> No.
> Unless they are serialized, it's possible that rh_inc() will see the
> state RH_DIRTY, while rh_dec change it to RH_CLEAN.
> As a result, the region which has I/O in-flight may be freed.
> 
> Is it reasonable to call mark_region() unconditionally?
> Then we can call it outside of the lock.
> 
>  >>    CPU0                                   CPU1
>  >>
>  >> -----------------------------------------------------------------------
>  >> -------
>  >>    rh_dec()
>  >>      if (atomic_dec_and_test(pending))
>  >>         <the region is still marked dirty>
>            if (atomic_read(pending)==0)
>  >>                                           rh_inc()
>  >>                                             atomic_inc(pending)
>  >>                                             if the region is clean
>  >>                                                mark the region dirty
>  >>                                                and remove from clean 
> list
>                                                else do nothing
>  >>         mark the region clean
>  >>         and move to clean list

[-- Attachment #2: dm-raid1-race2.new.patch --]
[-- Type: text/x-patch, Size: 976 bytes --]

diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -375,6 +375,9 @@ static void rh_inc(struct region_hash *r
 
 	read_lock(&rh->hash_lock);
 	reg = __rh_find(rh, region);
+	spin_lock_irq(&rh->region_lock);
+	atomic_inc(&reg->pending);
+	spin_unlock_irq(&rh->region_lock);
 	if (reg->state == RH_CLEAN) {
 		rh->log->type->mark_region(rh->log, reg->key);
 
@@ -384,7 +387,6 @@ static void rh_inc(struct region_hash *r
 		spin_unlock_irq(&rh->region_lock);
 	}
 
-	atomic_inc(&reg->pending);
 	read_unlock(&rh->hash_lock);
 }
 
@@ -408,6 +410,10 @@ static void rh_dec(struct region_hash *r
 
 	if (atomic_dec_and_test(&reg->pending)) {
 		spin_lock_irqsave(&rh->region_lock, flags);
+		if (atomic_read(&reg->pending)) { /* check race */
+			spin_unlock_irqrestore(&rh->region_lock, flags);
+			return;
+		}
 		if (reg->state == RH_RECOVERING) {
 			list_add_tail(&reg->list, &rh->quiesced_regions);
 		} else {

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2005-06-27 17:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-06-16 22:51 [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c Jun'ichi Nomura
2005-06-16 22:51 ` Jun'ichi Nomura
2005-06-24 15:19 ` Jonathan E Brassow
2005-06-24 15:45   ` Jun'ichi Nomura
2005-06-27 17:34     ` Jun'ichi Nomura

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.