[PATCH] block: make sure last_lookup set as NULL after part deleted

[PATCH] block: make sure last_lookup set as NULL after part deleted

[Yufen Yu]

When delete partition executes concurrently with IOs issue,
it may cause use-after-free on part in disk_map_sector_rcu()
as following:

blk_account_io_start(req1)  delete_partition  blk_account_io_start(req2)

rcu_read_lock()
disk_map_sector_rcu
part = rcu_dereference(ptbl->part[4])
                           rcu_assign_pointer(ptbl->part[4], NULL);
                           rcu_assign_pointer(ptbl->last_lookup, NULL);
rcu_assign_pointer(ptbl->last_lookup, part);

                           hd_struct_kill(part)
!hd_struct_try_get
  part = &rq->rq_disk->part0;
rcu_read_unlock()
                           __delete_partition
                           call_rcu
                                            rcu_read_lock
                                            disk_map_sector_rcu
                                            part = rcu_dereference(ptbl->last_lookup);

                           delete_partition_work_fn
                           free(part)
                                            hd_struct_try_get(part)
                                            BUG_ON use-after-free

req1 try to get 'ptbl->part[4]', while the part is beening
deleted. Although the delete_partition() try to set last_lookup
as NULL, req1 can overwrite it as 'part[4]' again.

After calling call_rcu() and free() for the part, req2 can
get the part by last_lookup, resulting in use after free.

In fact, this bug has been reported by syzbot:
    https://lkml.org/lkml/2019/1/4/357

We fix the bug by rereading ptbl->part[i] after setting
last_lookup. If part[i] is NULL, that means the part will
be freed, we need to clear last_lookup.

Then we can make sure last_lookup have been set as NULL
before call_rcu(). Thus, others can not get the freed part.

Fixes: a6f23657d307 ("block: add one-hit cache for disk partition lookup")
Reported-by: Zheng Chuan <zhengchuan@huawei.com>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 block/genhd.c             | 16 ++++++++++++++++
 block/partition-generic.c |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index ff6268970ddc..39fa8999905f 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
 		part = rcu_dereference(ptbl->part[i]);
 
 		if (part && sector_in_part(part, sector)) {
+			/*
+			 * After delete_partition() trying to delete this
+			 * part and setting last_lookup as 'NULL', we can
+			 * overwrite it here. Then, others may use the freed
+			 * part by reading last_lookup.
+			 *
+			 * To avoiding the use-after-free, we reread the
+			 * value of ptbl->part[i] here. If it has been set
+			 * as 'NULL', that means the part will be freed.
+			 * And we need to clear last_lookup also.
+			 */
 			rcu_assign_pointer(ptbl->last_lookup, part);
+			part = rcu_dereference(ptbl->part[i]);
+			if (part == NULL) {
+				rcu_assign_pointer(ptbl->last_lookup, NULL);
+				break;
+			}
 			return part;
 		}
 	}
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1d20c9cf213f..1e0065ed6f02 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
 		return;
 
 	rcu_assign_pointer(ptbl->part[partno], NULL);
+	/*
+	 * Without the memory barrier, disk_map_sector_rcu()
+	 * may read the old value after overwriting the
+	 * last_lookup. Then it can not clear last_lookup,
+	 * which may cause use-after-free.
+	 */
+	smp_mb();
 	rcu_assign_pointer(ptbl->last_lookup, NULL);
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));
-- 
2.17.2

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Hou Tao]

Hi,

On 2019/12/31 19:09, Yufen Yu wrote:
> When delete partition executes concurrently with IOs issue,
> it may cause use-after-free on part in disk_map_sector_rcu()
> as following:
snip

> 
> diff --git a/block/genhd.c b/block/genhd.c
> index ff6268970ddc..39fa8999905f 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>  		part = rcu_dereference(ptbl->part[i]);
>  
>  		if (part && sector_in_part(part, sector)) {
snip

>  			rcu_assign_pointer(ptbl->last_lookup, part);
> +			part = rcu_dereference(ptbl->part[i]);
> +			if (part == NULL) {
> +				rcu_assign_pointer(ptbl->last_lookup, NULL);
> +				break;
> +			}
>  			return part;
>  		}
>  	}

Not ensure whether the re-read can handle the following case or not:

process A                                 process B                          process C

disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():

rcu_read_lock

  // need to iterate partition table
  part[i] != NULL   (1)                   part[i] = NULL (2)
                                          smp_mb()
                                          last_lookup = NULL (3)
                                          call_rcu()  (4)
    last_lookup = part[i] (5)


                                                                             rcu_read_lock()
                                                                             read last_lookup return part[i] (6)
                                                                             sector_in_part() is OK (7)
                                                                             return part[i] (8)

  part[i] == NULL (9)
      last_lookup = NULL (10)
  rcu_read_unlock() (11)
                                           one RCU grace period completes
                                           __delete_partition() (12)
                                           free hd_partition (13)
                                                                             // use-after-free
                                                                             hd_struct_try_get(part[i])  (14)

* the number in the parenthesis is the sequence of events.

Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.

If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
(the following patch is only compile tested):

diff --git a/block/genhd.c b/block/genhd.c
index 6e8543ca6912..179e0056fae1 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
 		part = rcu_dereference(ptbl->part[i]);

 		if (part && sector_in_part(part, sector)) {
-			rcu_assign_pointer(ptbl->last_lookup, part);
+			struct hd_struct *old;
+
+			if (!hd_struct_try_get(part))
+				break;
+
+			old = xchg(&ptbl->last_lookup, part);
+			if (old)
+				hd_struct_put(old);
 			return part;
 		}
 	}
@@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
 	rcu_assign_pointer(disk->part_tbl, new_ptbl);

 	if (old_ptbl) {
-		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
+		struct hd_struct *part;
+
+		part = xchg(&old_ptbl->last_lookup, NULL);
+		if (part)
+			hd_struct_put(part);
 		kfree_rcu(old_ptbl, rcu_head);
 	}
 }
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 98d60a59b843..441c1c591c04 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
 		return;

 	rcu_assign_pointer(ptbl->part[partno], NULL);
-	rcu_assign_pointer(ptbl->last_lookup, NULL);
+	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
+		hd_struct_put(part);
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));

-- 
2.22.0

Regards,
Tao


> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 1d20c9cf213f..1e0065ed6f02 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
>  		return;
>  
>  	rcu_assign_pointer(ptbl->part[partno], NULL);
> +	/*
> +	 * Without the memory barrier, disk_map_sector_rcu()
> +	 * may read the old value after overwriting the
> +	 * last_lookup. Then it can not clear last_lookup,
> +	 * which may cause use-after-free.
> +	 */
> +	smp_mb();
>  	rcu_assign_pointer(ptbl->last_lookup, NULL);
>  	kobject_put(part->holder_dir);
>  	device_del(part_to_dev(part));
> 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Paul E. McKenney]

On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
> Hi,
> 
> On 2019/12/31 19:09, Yufen Yu wrote:
> > When delete partition executes concurrently with IOs issue,
> > it may cause use-after-free on part in disk_map_sector_rcu()
> > as following:
> snip
> 
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index ff6268970ddc..39fa8999905f 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >  		part = rcu_dereference(ptbl->part[i]);
> >  
> >  		if (part && sector_in_part(part, sector)) {
> snip
> 
> >  			rcu_assign_pointer(ptbl->last_lookup, part);
> > +			part = rcu_dereference(ptbl->part[i]);
> > +			if (part == NULL) {
> > +				rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +				break;
> > +			}
> >  			return part;
> >  		}
> >  	}
> 
> Not ensure whether the re-read can handle the following case or not:
> 
> process A                                 process B                          process C
> 
> disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
> 
> rcu_read_lock
> 
>   // need to iterate partition table
>   part[i] != NULL   (1)                   part[i] = NULL (2)
>                                           smp_mb()
>                                           last_lookup = NULL (3)
>                                           call_rcu()  (4)
>     last_lookup = part[i] (5)
> 
> 
>                                                                              rcu_read_lock()
>                                                                              read last_lookup return part[i] (6)
>                                                                              sector_in_part() is OK (7)
>                                                                              return part[i] (8)

Just for the record...

Use of RCU needs to ensure that readers cannot access the to-be-freed
structure -before- invoking call_rcu().  Which does look to happen here
with the "last_lookup = NULL".  But in addition, the callback needs to
get access to the to-be-freed structure via some sideband (usually the
structure passed to call_rcu()), not from the reader-accessible structure.

Or am I misinterpreting this sequence of events?

							Thanx, Paul

>   part[i] == NULL (9)
>       last_lookup = NULL (10)
>   rcu_read_unlock() (11)
>                                            one RCU grace period completes
>                                            __delete_partition() (12)
>                                            free hd_partition (13)
>                                                                              // use-after-free
>                                                                              hd_struct_try_get(part[i])  (14)
> 
> * the number in the parenthesis is the sequence of events.
> 
> Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
> 
> If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
> (the following patch is only compile tested):
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 6e8543ca6912..179e0056fae1 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>  		part = rcu_dereference(ptbl->part[i]);
> 
>  		if (part && sector_in_part(part, sector)) {
> -			rcu_assign_pointer(ptbl->last_lookup, part);
> +			struct hd_struct *old;
> +
> +			if (!hd_struct_try_get(part))
> +				break;
> +
> +			old = xchg(&ptbl->last_lookup, part);
> +			if (old)
> +				hd_struct_put(old);
>  			return part;
>  		}
>  	}
> @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
>  	rcu_assign_pointer(disk->part_tbl, new_ptbl);
> 
>  	if (old_ptbl) {
> -		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
> +		struct hd_struct *part;
> +
> +		part = xchg(&old_ptbl->last_lookup, NULL);
> +		if (part)
> +			hd_struct_put(part);
>  		kfree_rcu(old_ptbl, rcu_head);
>  	}
>  }
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 98d60a59b843..441c1c591c04 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
>  		return;
> 
>  	rcu_assign_pointer(ptbl->part[partno], NULL);
> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
> +		hd_struct_put(part);
>  	kobject_put(part->holder_dir);
>  	device_del(part_to_dev(part));
> 
> -- 
> 2.22.0
> 
> Regards,
> Tao
> 
> 
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 1d20c9cf213f..1e0065ed6f02 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
> >  		return;
> >  
> >  	rcu_assign_pointer(ptbl->part[partno], NULL);
> > +	/*
> > +	 * Without the memory barrier, disk_map_sector_rcu()
> > +	 * may read the old value after overwriting the
> > +	 * last_lookup. Then it can not clear last_lookup,
> > +	 * which may cause use-after-free.
> > +	 */
> > +	smp_mb();
> >  	rcu_assign_pointer(ptbl->last_lookup, NULL);
> >  	kobject_put(part->holder_dir);
> >  	device_del(part_to_dev(part));
> > 
> 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[htbegin]

Hi Paul,

Paul E. McKenney <paulmck@kernel.org> 于2020年1月1日周三 上午7:13写道：
>
> On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
> > Hi,
> >
> > On 2019/12/31 19:09, Yufen Yu wrote:
> > > When delete partition executes concurrently with IOs issue,
> > > it may cause use-after-free on part in disk_map_sector_rcu()
> > > as following:
> > snip
> >
> > >
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index ff6268970ddc..39fa8999905f 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> > >             part = rcu_dereference(ptbl->part[i]);
> > >
> > >             if (part && sector_in_part(part, sector)) {
> > snip
> >
> > >                     rcu_assign_pointer(ptbl->last_lookup, part);
> > > +                   part = rcu_dereference(ptbl->part[i]);
> > > +                   if (part == NULL) {
> > > +                           rcu_assign_pointer(ptbl->last_lookup, NULL);
> > > +                           break;
> > > +                   }
> > >                     return part;
> > >             }
> > >     }
> >
> > Not ensure whether the re-read can handle the following case or not:
> >
> > process A                                 process B                          process C
> >
> > disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
> >
> > rcu_read_lock
> >
> >   // need to iterate partition table
> >   part[i] != NULL   (1)                   part[i] = NULL (2)
> >                                           smp_mb()
> >                                           last_lookup = NULL (3)
> >                                           call_rcu()  (4)
> >     last_lookup = part[i] (5)
> >
> >
> >                                                                              rcu_read_lock()
> >                                                                              read last_lookup return part[i] (6)
> >                                                                              sector_in_part() is OK (7)
> >                                                                              return part[i] (8)
>
> Just for the record...
>
> Use of RCU needs to ensure that readers cannot access the to-be-freed
> structure -before- invoking call_rcu().  Which does look to happen here
> with the "last_lookup = NULL".

But the last_lookup pointer may be reassigned a to-be-freed pointer (namely
part[i]) as show in process A, and I think the RCU read section starts after
the call of call_rcu() may read the reassigned pointer and use it, right ?

> But in addition, the callback needs to
> get access to the to-be-freed structure via some sideband (usually the
> structure passed to call_rcu()), not from the reader-accessible structure.
>
Er, could you clarify the meaning of "reader-accessible structure" ? Do you
mean "reader-writable structure" like last_lookup ? In the case, the callback
does access the to-be-freed struct by the embedded "rcu_work".

> Or am I misinterpreting this sequence of events?
>
Ah, I over simplify the procedures happened in process B. Does the following
diagram make it clearer ?

process A                           process B                     process C

disk_map_sector_rcu():              delete_partition():
disk_map_sector_rcu():

rcu_read_lock

  // iterate part table
  part[i] != NULL   (1)
                                    deleted = part[i]
                                    part[i] = NULL (2)
                                    smp_mb()
                                    last_lookup = NULL (3)

                                    percpu_ref_kill(deleted)
                                      // last ref
                                      __delete_partition (4)
                                        queue_rcu_work
    // re-assign last_lookup
    last_lookup = part[i] (5)



rcu_read_lock()
                                                                  //
hit last_lookup
                                                                  read
last_lookup returns part[i] (6)

sector_in_part() is OK (7)
                                                                  will
return part[i] (8)

rcu_read_unlock()

  // found part[i] is NULL
  // reassign last_lookup
  last_lookup = NULL
  will return part0

  rcu_read_unlock
                                    // RCU callback
                                    delete_partition_work_fn
                                      free deleted
                                                                   //
use-after-free ?

hd_struct_try_get
Regards,
Tao


>                                                         Thanx, Paul
>
> >   part[i] == NULL (9)
> >       last_lookup = NULL (10)
> >   rcu_read_unlock() (11)
> >                                            one RCU grace period completes
> >                                            __delete_partition() (12)
> >                                            free hd_partition (13)
> >                                                                              // use-after-free
> >                                                                              hd_struct_try_get(part[i])  (14)
> >
> > * the number in the parenthesis is the sequence of events.
> >
> > Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
> >
> > If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
> > (the following patch is only compile tested):
> >
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 6e8543ca6912..179e0056fae1 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >               part = rcu_dereference(ptbl->part[i]);
> >
> >               if (part && sector_in_part(part, sector)) {
> > -                     rcu_assign_pointer(ptbl->last_lookup, part);
> > +                     struct hd_struct *old;
> > +
> > +                     if (!hd_struct_try_get(part))
> > +                             break;
> > +
> > +                     old = xchg(&ptbl->last_lookup, part);
> > +                     if (old)
> > +                             hd_struct_put(old);
> >                       return part;
> >               }
> >       }
> > @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
> >       rcu_assign_pointer(disk->part_tbl, new_ptbl);
> >
> >       if (old_ptbl) {
> > -             rcu_assign_pointer(old_ptbl->last_lookup, NULL);
> > +             struct hd_struct *part;
> > +
> > +             part = xchg(&old_ptbl->last_lookup, NULL);
> > +             if (part)
> > +                     hd_struct_put(part);
> >               kfree_rcu(old_ptbl, rcu_head);
> >       }
> >  }
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 98d60a59b843..441c1c591c04 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
> >               return;
> >
> >       rcu_assign_pointer(ptbl->part[partno], NULL);
> > -     rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +     if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
> > +             hd_struct_put(part);
> >       kobject_put(part->holder_dir);
> >       device_del(part_to_dev(part));
> >
> > --
> > 2.22.0
> >
> > Regards,
> > Tao
> >
> >
> > > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > > index 1d20c9cf213f..1e0065ed6f02 100644
> > > --- a/block/partition-generic.c
> > > +++ b/block/partition-generic.c
> > > @@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
> > >             return;
> > >
> > >     rcu_assign_pointer(ptbl->part[partno], NULL);
> > > +   /*
> > > +    * Without the memory barrier, disk_map_sector_rcu()
> > > +    * may read the old value after overwriting the
> > > +    * last_lookup. Then it can not clear last_lookup,
> > > +    * which may cause use-after-free.
> > > +    */
> > > +   smp_mb();
> > >     rcu_assign_pointer(ptbl->last_lookup, NULL);
> > >     kobject_put(part->holder_dir);
> > >     device_del(part_to_dev(part));
> > >
> >

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[htbegin]

Sorry, it seems that i miss-formatted the diagram, so I resend it here:

process A                    process B            process C

disk_map_sector_rcu():       delete_partition():  disk_map_sector_rcu():

rcu_read_lock

  // iterate part table
  part[i] != NULL  (1)

                             deleted = part[i]
                             part[i] = NULL (2)
                             smp_mb()
                             last_lookup = NULL (3)

                             percpu_ref_kill(deleted)
                               // last ref
                               __delete_partition (4)
                                 queue_rcu_work

  // re-assign last_lookup
  last_lookup = part[i] (5)


                                                  rcu_read_lock()
                                                  // is it possible ?
                                                  last_lookup is part[i] (6)
                                                  sector_in_part() is OK (7)
                                                  will return part[i] (8)
                                                  rcu_read_unlock()

  // found part[i] is NULL
  // reassign last_lookup
  last_lookup = NULL
  will return part0
  rcu_read_unlock

                               // RCU callback
                               delete_partition_work_fn
                                 free deleted
                                                       // use-after-free ?
                                                       hd_struct_try_get

On Wed, Jan 1, 2020 at 10:33 AM htbegin <hotforest@gmail.com> wrote:
>
> Hi Paul,
>
> Paul E. McKenney <paulmck@kernel.org> 于2020年1月1日周三 上午7:13写道：
> >
> > On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
> > > Hi,
> > >
> > > On 2019/12/31 19:09, Yufen Yu wrote:
> > > > When delete partition executes concurrently with IOs issue,
> > > > it may cause use-after-free on part in disk_map_sector_rcu()
> > > > as following:
> > > snip
> > >
> > > >
> > > > diff --git a/block/genhd.c b/block/genhd.c
> > > > index ff6268970ddc..39fa8999905f 100644
> > > > --- a/block/genhd.c
> > > > +++ b/block/genhd.c
> > > > @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> > > >             part = rcu_dereference(ptbl->part[i]);
> > > >
> > > >             if (part && sector_in_part(part, sector)) {
> > > snip
> > >
> > > >                     rcu_assign_pointer(ptbl->last_lookup, part);
> > > > +                   part = rcu_dereference(ptbl->part[i]);
> > > > +                   if (part == NULL) {
> > > > +                           rcu_assign_pointer(ptbl->last_lookup, NULL);
> > > > +                           break;
> > > > +                   }
> > > >                     return part;
> > > >             }
> > > >     }
> > >
> > > Not ensure whether the re-read can handle the following case or not:
> > >
> > > process A                                 process B                          process C
> > >
> > > disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
> > >
> > > rcu_read_lock
> > >
> > >   // need to iterate partition table
> > >   part[i] != NULL   (1)                   part[i] = NULL (2)
> > >                                           smp_mb()
> > >                                           last_lookup = NULL (3)
> > >                                           call_rcu()  (4)
> > >     last_lookup = part[i] (5)
> > >
> > >
> > >                                                                              rcu_read_lock()
> > >                                                                              read last_lookup return part[i] (6)
> > >                                                                              sector_in_part() is OK (7)
> > >                                                                              return part[i] (8)
> >
> > Just for the record...
> >
> > Use of RCU needs to ensure that readers cannot access the to-be-freed
> > structure -before- invoking call_rcu().  Which does look to happen here
> > with the "last_lookup = NULL".
>
> But the last_lookup pointer may be reassigned a to-be-freed pointer (namely
> part[i]) as show in process A, and I think the RCU read section starts after
> the call of call_rcu() may read the reassigned pointer and use it, right ?
>
> > But in addition, the callback needs to
> > get access to the to-be-freed structure via some sideband (usually the
> > structure passed to call_rcu()), not from the reader-accessible structure.
> >
> Er, could you clarify the meaning of "reader-accessible structure" ? Do you
> mean "reader-writable structure" like last_lookup ? In the case, the callback
> does access the to-be-freed struct by the embedded "rcu_work".
>
> > Or am I misinterpreting this sequence of events?
> >
> Ah, I over simplify the procedures happened in process B. Does the following
> diagram make it clearer ?
>
> process A                           process B                     process C
>
> disk_map_sector_rcu():              delete_partition():
> disk_map_sector_rcu():
>
> rcu_read_lock
>
>   // iterate part table
>   part[i] != NULL   (1)
>                                     deleted = part[i]
>                                     part[i] = NULL (2)
>                                     smp_mb()
>                                     last_lookup = NULL (3)
>
>                                     percpu_ref_kill(deleted)
>                                       // last ref
>                                       __delete_partition (4)
>                                         queue_rcu_work
>     // re-assign last_lookup
>     last_lookup = part[i] (5)
>
>
>
> rcu_read_lock()
>                                                                   //
> hit last_lookup
>                                                                   read
> last_lookup returns part[i] (6)
>
> sector_in_part() is OK (7)
>                                                                   will
> return part[i] (8)
>
> rcu_read_unlock()
>
>   // found part[i] is NULL
>   // reassign last_lookup
>   last_lookup = NULL
>   will return part0
>
>   rcu_read_unlock
>                                     // RCU callback
>                                     delete_partition_work_fn
>                                       free deleted
>                                                                    //
> use-after-free ?
>
> hd_struct_try_get
> Regards,
> Tao
>
>
> >                                                         Thanx, Paul
> >
> > >   part[i] == NULL (9)
> > >       last_lookup = NULL (10)
> > >   rcu_read_unlock() (11)
> > >                                            one RCU grace period completes
> > >                                            __delete_partition() (12)
> > >                                            free hd_partition (13)
> > >                                                                              // use-after-free
> > >                                                                              hd_struct_try_get(part[i])  (14)
> > >
> > > * the number in the parenthesis is the sequence of events.
> > >
> > > Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
> > >
> > > If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
> > > (the following patch is only compile tested):
> > >
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index 6e8543ca6912..179e0056fae1 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> > >               part = rcu_dereference(ptbl->part[i]);
> > >
> > >               if (part && sector_in_part(part, sector)) {
> > > -                     rcu_assign_pointer(ptbl->last_lookup, part);
> > > +                     struct hd_struct *old;
> > > +
> > > +                     if (!hd_struct_try_get(part))
> > > +                             break;
> > > +
> > > +                     old = xchg(&ptbl->last_lookup, part);
> > > +                     if (old)
> > > +                             hd_struct_put(old);
> > >                       return part;
> > >               }
> > >       }
> > > @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
> > >       rcu_assign_pointer(disk->part_tbl, new_ptbl);
> > >
> > >       if (old_ptbl) {
> > > -             rcu_assign_pointer(old_ptbl->last_lookup, NULL);
> > > +             struct hd_struct *part;
> > > +
> > > +             part = xchg(&old_ptbl->last_lookup, NULL);
> > > +             if (part)
> > > +                     hd_struct_put(part);
> > >               kfree_rcu(old_ptbl, rcu_head);
> > >       }
> > >  }
> > > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > > index 98d60a59b843..441c1c591c04 100644
> > > --- a/block/partition-generic.c
> > > +++ b/block/partition-generic.c
> > > @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
> > >               return;
> > >
> > >       rcu_assign_pointer(ptbl->part[partno], NULL);
> > > -     rcu_assign_pointer(ptbl->last_lookup, NULL);
> > > +     if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
> > > +             hd_struct_put(part);
> > >       kobject_put(part->holder_dir);
> > >       device_del(part_to_dev(part));
> > >
> > > --
> > > 2.22.0
> > >
> > > Regards,
> > > Tao
> > >
> > >
> > > > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > > > index 1d20c9cf213f..1e0065ed6f02 100644
> > > > --- a/block/partition-generic.c
> > > > +++ b/block/partition-generic.c
> > > > @@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
> > > >             return;
> > > >
> > > >     rcu_assign_pointer(ptbl->part[partno], NULL);
> > > > +   /*
> > > > +    * Without the memory barrier, disk_map_sector_rcu()
> > > > +    * may read the old value after overwriting the
> > > > +    * last_lookup. Then it can not clear last_lookup,
> > > > +    * which may cause use-after-free.
> > > > +    */
> > > > +   smp_mb();
> > > >     rcu_assign_pointer(ptbl->last_lookup, NULL);
> > > >     kobject_put(part->holder_dir);
> > > >     device_del(part_to_dev(part));
> > > >
> > >

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Joel Fernandes]

On Tue, Dec 31, 2019 at 03:11:58PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
> > Hi,
> > 
> > On 2019/12/31 19:09, Yufen Yu wrote:
> > > When delete partition executes concurrently with IOs issue,
> > > it may cause use-after-free on part in disk_map_sector_rcu()
> > > as following:
> > snip
> > 
> > > 
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index ff6268970ddc..39fa8999905f 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> > >  		part = rcu_dereference(ptbl->part[i]);
> > >  
> > >  		if (part && sector_in_part(part, sector)) {
> > snip
> > 
> > >  			rcu_assign_pointer(ptbl->last_lookup, part);
> > > +			part = rcu_dereference(ptbl->part[i]);
> > > +			if (part == NULL) {
> > > +				rcu_assign_pointer(ptbl->last_lookup, NULL);
> > > +				break;
> > > +			}
> > >  			return part;
> > >  		}
> > >  	}
> > 
> > Not ensure whether the re-read can handle the following case or not:
> > 
> > process A                                 process B                          process C
> > 
> > disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
> > 
> > rcu_read_lock
> > 
> >   // need to iterate partition table
> >   part[i] != NULL   (1)                   part[i] = NULL (2)
> >                                           smp_mb()
> >                                           last_lookup = NULL (3)
> >                                           call_rcu()  (4)
> >     last_lookup = part[i] (5)
> > 
> > 
> >                                                                              rcu_read_lock()
> >                                                                              read last_lookup return part[i] (6)
> >                                                                              sector_in_part() is OK (7)
> >                                                                              return part[i] (8)
> 
> Just for the record...
> 
> Use of RCU needs to ensure that readers cannot access the to-be-freed
> structure -before- invoking call_rcu().  Which does look to happen here
> with the "last_lookup = NULL".  But in addition, the callback needs to
> get access to the to-be-freed structure via some sideband (usually the
> structure passed to call_rcu()), not from the reader-accessible structure.
> 
> Or am I misinterpreting this sequence of events?

If I understand correctly, the issue described above is there are 2 threads
setting last_lookup pointer simultaneously, one of them is NULLing it and
waiting for a GP before freeing it (process B above), while the other is
assigning to it concurrently after it was just NULLed (process A). Meanwhile
process C starts a reader section *after* the GP by process B already started
and accesses the reassigned pointer causing use-after-free.

Did I miss something?

I believe the fix is what Tao already posted which is to use refcounts so
that the destructor does not free it while references are already held. Is
that what the final fix is going to be? That other thread is pretty long so I
lost track a bit..

thanks,

 - Joel



> 							Thanx, Paul
> 
> >   part[i] == NULL (9)
> >       last_lookup = NULL (10)
> >   rcu_read_unlock() (11)
> >                                            one RCU grace period completes
> >                                            __delete_partition() (12)
> >                                            free hd_partition (13)
> >                                                                              // use-after-free
> >                                                                              hd_struct_try_get(part[i])  (14)
> > 
> > * the number in the parenthesis is the sequence of events.
> > 
> > Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
> > 
> > If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
> > (the following patch is only compile tested):
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 6e8543ca6912..179e0056fae1 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >  		part = rcu_dereference(ptbl->part[i]);
> > 
> >  		if (part && sector_in_part(part, sector)) {
> > -			rcu_assign_pointer(ptbl->last_lookup, part);
> > +			struct hd_struct *old;
> > +
> > +			if (!hd_struct_try_get(part))
> > +				break;
> > +
> > +			old = xchg(&ptbl->last_lookup, part);
> > +			if (old)
> > +				hd_struct_put(old);
> >  			return part;
> >  		}
> >  	}
> > @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
> >  	rcu_assign_pointer(disk->part_tbl, new_ptbl);
> > 
> >  	if (old_ptbl) {
> > -		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
> > +		struct hd_struct *part;
> > +
> > +		part = xchg(&old_ptbl->last_lookup, NULL);
> > +		if (part)
> > +			hd_struct_put(part);
> >  		kfree_rcu(old_ptbl, rcu_head);
> >  	}
> >  }
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 98d60a59b843..441c1c591c04 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
> >  		return;
> > 
> >  	rcu_assign_pointer(ptbl->part[partno], NULL);
> > -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
> > +		hd_struct_put(part);
> >  	kobject_put(part->holder_dir);
> >  	device_del(part_to_dev(part));
> > 
> > -- 
> > 2.22.0
> > 
> > Regards,
> > Tao
> > 
> > 
> > > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > > index 1d20c9cf213f..1e0065ed6f02 100644
> > > --- a/block/partition-generic.c
> > > +++ b/block/partition-generic.c
> > > @@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
> > >  		return;
> > >  
> > >  	rcu_assign_pointer(ptbl->part[partno], NULL);
> > > +	/*
> > > +	 * Without the memory barrier, disk_map_sector_rcu()
> > > +	 * may read the old value after overwriting the
> > > +	 * last_lookup. Then it can not clear last_lookup,
> > > +	 * which may cause use-after-free.
> > > +	 */
> > > +	smp_mb();
> > >  	rcu_assign_pointer(ptbl->last_lookup, NULL);
> > >  	kobject_put(part->holder_dir);
> > >  	device_del(part_to_dev(part));
> > > 
> > 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Hou Tao]

Hi Joel,

On 2020/1/4 7:45, Joel Fernandes wrote:
> On Tue, Dec 31, 2019 at 03:11:58PM -0800, Paul E. McKenney wrote:
>> On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
>>> Hi,
>>>
>>> On 2019/12/31 19:09, Yufen Yu wrote:
>>>> When delete partition executes concurrently with IOs issue,
>>>> it may cause use-after-free on part in disk_map_sector_rcu()
>>>> as following:
>>> snip
>>>
>>>>
>>>> diff --git a/block/genhd.c b/block/genhd.c
>>>> index ff6268970ddc..39fa8999905f 100644
>>>> --- a/block/genhd.c
>>>> +++ b/block/genhd.c
>>>> @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>>>>  		part = rcu_dereference(ptbl->part[i]);
>>>>  
>>>>  		if (part && sector_in_part(part, sector)) {
>>> snip
>>>
>>>>  			rcu_assign_pointer(ptbl->last_lookup, part);
>>>> +			part = rcu_dereference(ptbl->part[i]);
>>>> +			if (part == NULL) {
>>>> +				rcu_assign_pointer(ptbl->last_lookup, NULL);
>>>> +				break;
>>>> +			}
>>>>  			return part;
>>>>  		}
>>>>  	}
>>>
>>> Not ensure whether the re-read can handle the following case or not:
>>>
>>> process A                                 process B                          process C
>>>
>>> disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
>>>
>>> rcu_read_lock
>>>
>>>   // need to iterate partition table
>>>   part[i] != NULL   (1)                   part[i] = NULL (2)
>>>                                           smp_mb()
>>>                                           last_lookup = NULL (3)
>>>                                           call_rcu()  (4)
>>>     last_lookup = part[i] (5)
>>>
>>>
>>>                                                                              rcu_read_lock()
>>>                                                                              read last_lookup return part[i] (6)
>>>                                                                              sector_in_part() is OK (7)
>>>                                                                              return part[i] (8)
>>
>> Just for the record...
>>
>> Use of RCU needs to ensure that readers cannot access the to-be-freed
>> structure -before- invoking call_rcu().  Which does look to happen here
>> with the "last_lookup = NULL".  But in addition, the callback needs to
>> get access to the to-be-freed structure via some sideband (usually the
>> structure passed to call_rcu()), not from the reader-accessible structure.
>>
>> Or am I misinterpreting this sequence of events?
> 
> If I understand correctly, the issue described above is there are 2 threads
> setting last_lookup pointer simultaneously, one of them is NULLing it and
> waiting for a GP before freeing it (process B above), while the other is
> assigning to it concurrently after it was just NULLed (process A). Meanwhile
> process C starts a reader section *after* the GP by process B already started
> and accesses the reassigned pointer causing use-after-free.
> 
> Did I miss something?
> 
No. It's exactly the same as you have summarized. And thanks for that.

> I believe the fix is what Tao already posted which is to use refcounts so
> that the destructor does not free it while references are already held. Is
> that what the final fix is going to be? That other thread is pretty long so I
> lost track a bit..
> We are just trying to find a better solution (e.g. more readable or understandable).

Regards,
Tao

> thanks,
> 
>  - Joel
> 
> 
> 
>> 							Thanx, Paul
>>
>>>   part[i] == NULL (9)
>>>       last_lookup = NULL (10)
>>>   rcu_read_unlock() (11)
>>>                                            one RCU grace period completes
>>>                                            __delete_partition() (12)
>>>                                            free hd_partition (13)
>>>                                                                              // use-after-free
>>>                                                                              hd_struct_try_get(part[i])  (14)
>>>
>>> * the number in the parenthesis is the sequence of events.
>>>
>>> Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
>>>
>>> If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
>>> (the following patch is only compile tested):
>>>
>>> diff --git a/block/genhd.c b/block/genhd.c
>>> index 6e8543ca6912..179e0056fae1 100644
>>> --- a/block/genhd.c
>>> +++ b/block/genhd.c
>>> @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>>>  		part = rcu_dereference(ptbl->part[i]);
>>>
>>>  		if (part && sector_in_part(part, sector)) {
>>> -			rcu_assign_pointer(ptbl->last_lookup, part);
>>> +			struct hd_struct *old;
>>> +
>>> +			if (!hd_struct_try_get(part))
>>> +				break;
>>> +
>>> +			old = xchg(&ptbl->last_lookup, part);
>>> +			if (old)
>>> +				hd_struct_put(old);
>>>  			return part;
>>>  		}
>>>  	}
>>> @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
>>>  	rcu_assign_pointer(disk->part_tbl, new_ptbl);
>>>
>>>  	if (old_ptbl) {
>>> -		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
>>> +		struct hd_struct *part;
>>> +
>>> +		part = xchg(&old_ptbl->last_lookup, NULL);
>>> +		if (part)
>>> +			hd_struct_put(part);
>>>  		kfree_rcu(old_ptbl, rcu_head);
>>>  	}
>>>  }
>>> diff --git a/block/partition-generic.c b/block/partition-generic.c
>>> index 98d60a59b843..441c1c591c04 100644
>>> --- a/block/partition-generic.c
>>> +++ b/block/partition-generic.c
>>> @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
>>>  		return;
>>>
>>>  	rcu_assign_pointer(ptbl->part[partno], NULL);
>>> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
>>> +	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
>>> +		hd_struct_put(part);
>>>  	kobject_put(part->holder_dir);
>>>  	device_del(part_to_dev(part));
>>>
>>> -- 
>>> 2.22.0
>>>
>>> Regards,
>>> Tao
>>>
>>>
>>>> diff --git a/block/partition-generic.c b/block/partition-generic.c
>>>> index 1d20c9cf213f..1e0065ed6f02 100644
>>>> --- a/block/partition-generic.c
>>>> +++ b/block/partition-generic.c
>>>> @@ -284,6 +284,13 @@ void delete_partition(struct gendisk *disk, int partno)
>>>>  		return;
>>>>  
>>>>  	rcu_assign_pointer(ptbl->part[partno], NULL);
>>>> +	/*
>>>> +	 * Without the memory barrier, disk_map_sector_rcu()
>>>> +	 * may read the old value after overwriting the
>>>> +	 * last_lookup. Then it can not clear last_lookup,
>>>> +	 * which may cause use-after-free.
>>>> +	 */
>>>> +	smp_mb();
>>>>  	rcu_assign_pointer(ptbl->last_lookup, NULL);
>>>>  	kobject_put(part->holder_dir);
>>>>  	device_del(part_to_dev(part));
>>>>
>>>
> 
> .
> 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Ming Lei]

On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
> Hi,
> 
> On 2019/12/31 19:09, Yufen Yu wrote:
> > When delete partition executes concurrently with IOs issue,
> > it may cause use-after-free on part in disk_map_sector_rcu()
> > as following:
> snip
> 
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index ff6268970ddc..39fa8999905f 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >  		part = rcu_dereference(ptbl->part[i]);
> >  
> >  		if (part && sector_in_part(part, sector)) {
> snip
> 
> >  			rcu_assign_pointer(ptbl->last_lookup, part);
> > +			part = rcu_dereference(ptbl->part[i]);
> > +			if (part == NULL) {
> > +				rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +				break;
> > +			}
> >  			return part;
> >  		}
> >  	}
> 
> Not ensure whether the re-read can handle the following case or not:
> 
> process A                                 process B                          process C
> 
> disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
> 
> rcu_read_lock
> 
>   // need to iterate partition table
>   part[i] != NULL   (1)                   part[i] = NULL (2)
>                                           smp_mb()
>                                           last_lookup = NULL (3)
>                                           call_rcu()  (4)
>     last_lookup = part[i] (5)
> 
> 
>                                                                              rcu_read_lock()
>                                                                              read last_lookup return part[i] (6)
>                                                                              sector_in_part() is OK (7)
>                                                                              return part[i] (8)
> 
>   part[i] == NULL (9)
>       last_lookup = NULL (10)
>   rcu_read_unlock() (11)
>                                            one RCU grace period completes
>                                            __delete_partition() (12)
>                                            free hd_partition (13)
>                                                                              // use-after-free
>                                                                              hd_struct_try_get(part[i])  (14)
> 
> * the number in the parenthesis is the sequence of events.
> 
> Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
> 
> If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
> (the following patch is only compile tested):
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 6e8543ca6912..179e0056fae1 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>  		part = rcu_dereference(ptbl->part[i]);
> 
>  		if (part && sector_in_part(part, sector)) {
> -			rcu_assign_pointer(ptbl->last_lookup, part);
> +			struct hd_struct *old;
> +
> +			if (!hd_struct_try_get(part))
> +				break;
> +
> +			old = xchg(&ptbl->last_lookup, part);
> +			if (old)
> +				hd_struct_put(old);
>  			return part;
>  		}
>  	}
> @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
>  	rcu_assign_pointer(disk->part_tbl, new_ptbl);
> 
>  	if (old_ptbl) {
> -		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
> +		struct hd_struct *part;
> +
> +		part = xchg(&old_ptbl->last_lookup, NULL);
> +		if (part)
> +			hd_struct_put(part);
>  		kfree_rcu(old_ptbl, rcu_head);
>  	}
>  }
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 98d60a59b843..441c1c591c04 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
>  		return;
> 
>  	rcu_assign_pointer(ptbl->part[partno], NULL);
> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
> +		hd_struct_put(part);
>  	kobject_put(part->holder_dir);
>  	device_del(part_to_dev(part));

IMO this approach looks good.

Given partition is actually protected by percpu-refcount now, I guess the
RCU annotation for referencing ->part[partno] and ->last_lookup may not
be necessary, together with the part->rcu_work.


Thanks,
Ming

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Hou Tao]

Hi,

On 2020/1/2 9:23, Ming Lei wrote:
> On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
>> Hi,
>>
>> On 2019/12/31 19:09, Yufen Yu wrote:
>>> When delete partition executes concurrently with IOs issue,
>>> it may cause use-after-free on part in disk_map_sector_rcu()
>>> as following:
>> snip
>>
>>>
>>> diff --git a/block/genhd.c b/block/genhd.c
>>> index ff6268970ddc..39fa8999905f 100644
>>> --- a/block/genhd.c
>>> +++ b/block/genhd.c
>>> @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>>>  		part = rcu_dereference(ptbl->part[i]);
>>>  
>>>  		if (part && sector_in_part(part, sector)) {
>> snip
>>
>>>  			rcu_assign_pointer(ptbl->last_lookup, part);
>>> +			part = rcu_dereference(ptbl->part[i]);
>>> +			if (part == NULL) {
>>> +				rcu_assign_pointer(ptbl->last_lookup, NULL);
>>> +				break;
>>> +			}
>>>  			return part;
>>>  		}
>>>  	}
>>
>> Not ensure whether the re-read can handle the following case or not:
>>
We have written a similar test case for the following case and found out that
process C still may got the freed hd_struct pointer from process A. So
the re-read will not resolve the problem.

>> process A                                 process B                          process C
>>
>> disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
>>
>> rcu_read_lock
>>
>>   // need to iterate partition table
>>   part[i] != NULL   (1)                   part[i] = NULL (2)
>>                                           smp_mb()
>>                                           last_lookup = NULL (3)
>>                                           call_rcu()  (4)
>>     last_lookup = part[i] (5)
>>
>>
>>                                                                              rcu_read_lock()
>>                                                                              read last_lookup return part[i] (6)
>>                                                                              sector_in_part() is OK (7)
>>                                                                              return part[i] (8)
>>
>>   part[i] == NULL (9)
>>       last_lookup = NULL (10)
>>   rcu_read_unlock() (11)
>>                                            one RCU grace period completes
>>                                            __delete_partition() (12)
>>                                            free hd_partition (13)
>>                                                                              // use-after-free
>>                                                                              hd_struct_try_get(part[i])  (14)
>>
>> * the number in the parenthesis is the sequence of events.
>>



>> Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
>>
>> If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
>> (the following patch is only compile tested):
>>
>> diff --git a/block/genhd.c b/block/genhd.c
>> index 6e8543ca6912..179e0056fae1 100644
>> --- a/block/genhd.c
>> +++ b/block/genhd.c
>> @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>>  		part = rcu_dereference(ptbl->part[i]);
>>
>>  		if (part && sector_in_part(part, sector)) {
>> -			rcu_assign_pointer(ptbl->last_lookup, part);
>> +			struct hd_struct *old;
>> +
>> +			if (!hd_struct_try_get(part))
>> +				break;
>> +
>> +			old = xchg(&ptbl->last_lookup, part);
>> +			if (old)
>> +				hd_struct_put(old);
>>  			return part;
>>  		}
>>  	}
>> @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
>>  	rcu_assign_pointer(disk->part_tbl, new_ptbl);
>>
>>  	if (old_ptbl) {
>> -		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
>> +		struct hd_struct *part;
>> +
>> +		part = xchg(&old_ptbl->last_lookup, NULL);
>> +		if (part)
>> +			hd_struct_put(part);
>>  		kfree_rcu(old_ptbl, rcu_head);
>>  	}
>>  }
>> diff --git a/block/partition-generic.c b/block/partition-generic.c
>> index 98d60a59b843..441c1c591c04 100644
>> --- a/block/partition-generic.c
>> +++ b/block/partition-generic.c
>> @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
>>  		return;
>>
>>  	rcu_assign_pointer(ptbl->part[partno], NULL);
>> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
>> +	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
>> +		hd_struct_put(part);
>>  	kobject_put(part->holder_dir);
>>  	device_del(part_to_dev(part));
> 
> IMO this approach looks good.
>
Not sure about the overhead when there are concurrent IOs on different partitions,
we will measure that.

We have got a seemingly better solution: caching the index of last_lookup in tbl->part[]
instead of caching the pointer itself, so we can ensure the validity of returned pointer
by ensuring it's not NULL in tbl->part[] as does when last_lookup is NULL or 0.

> Given partition is actually protected by percpu-refcount now, I guess the
> RCU annotation for referencing ->part[partno] and ->last_lookup may not
> be necessary, together with the part->rcu_work.
> 
So we will depends on the invocation of of call_rcu() on __percpu_ref_switch_mode() to
ensure the RCU readers will find part[i] is NULL before trying to increasing
the atomic ref-counter of part[i], right ?

Regards,
Tao

> 
> Thanks,
> Ming
> 
> 
> .
> 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Ming Lei]

On Fri, Jan 03, 2020 at 11:06:25AM +0800, Hou Tao wrote:
> Hi,
> 
> On 2020/1/2 9:23, Ming Lei wrote:
> > On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
> >> Hi,
> >>
> >> On 2019/12/31 19:09, Yufen Yu wrote:
> >>> When delete partition executes concurrently with IOs issue,
> >>> it may cause use-after-free on part in disk_map_sector_rcu()
> >>> as following:
> >> snip
> >>
> >>>
> >>> diff --git a/block/genhd.c b/block/genhd.c
> >>> index ff6268970ddc..39fa8999905f 100644
> >>> --- a/block/genhd.c
> >>> +++ b/block/genhd.c
> >>> @@ -293,7 +293,23 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >>>  		part = rcu_dereference(ptbl->part[i]);
> >>>  
> >>>  		if (part && sector_in_part(part, sector)) {
> >> snip
> >>
> >>>  			rcu_assign_pointer(ptbl->last_lookup, part);
> >>> +			part = rcu_dereference(ptbl->part[i]);
> >>> +			if (part == NULL) {
> >>> +				rcu_assign_pointer(ptbl->last_lookup, NULL);
> >>> +				break;
> >>> +			}
> >>>  			return part;
> >>>  		}
> >>>  	}
> >>
> >> Not ensure whether the re-read can handle the following case or not:
> >>
> We have written a similar test case for the following case and found out that
> process C still may got the freed hd_struct pointer from process A. So
> the re-read will not resolve the problem.
> 
> >> process A                                 process B                          process C
> >>
> >> disk_map_sector_rcu():                    delete_partition():               disk_map_sector_rcu():
> >>
> >> rcu_read_lock
> >>
> >>   // need to iterate partition table
> >>   part[i] != NULL   (1)                   part[i] = NULL (2)
> >>                                           smp_mb()
> >>                                           last_lookup = NULL (3)
> >>                                           call_rcu()  (4)
> >>     last_lookup = part[i] (5)
> >>
> >>
> >>                                                                              rcu_read_lock()
> >>                                                                              read last_lookup return part[i] (6)
> >>                                                                              sector_in_part() is OK (7)
> >>                                                                              return part[i] (8)
> >>
> >>   part[i] == NULL (9)
> >>       last_lookup = NULL (10)
> >>   rcu_read_unlock() (11)
> >>                                            one RCU grace period completes
> >>                                            __delete_partition() (12)
> >>                                            free hd_partition (13)
> >>                                                                              // use-after-free
> >>                                                                              hd_struct_try_get(part[i])  (14)
> >>
> >> * the number in the parenthesis is the sequence of events.
> >>
> 
> 
> 
> >> Maybe RCU experts can shed some light on this problem, so cc +paulmck@kernel.org, +joel@joelfernandes.org and +RCU maillist.
> >>
> >> If the above case is possible, maybe we can fix the problem by pinning last_lookup through increasing its ref-count
> >> (the following patch is only compile tested):
> >>
> >> diff --git a/block/genhd.c b/block/genhd.c
> >> index 6e8543ca6912..179e0056fae1 100644
> >> --- a/block/genhd.c
> >> +++ b/block/genhd.c
> >> @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >>  		part = rcu_dereference(ptbl->part[i]);
> >>
> >>  		if (part && sector_in_part(part, sector)) {
> >> -			rcu_assign_pointer(ptbl->last_lookup, part);
> >> +			struct hd_struct *old;
> >> +
> >> +			if (!hd_struct_try_get(part))
> >> +				break;
> >> +
> >> +			old = xchg(&ptbl->last_lookup, part);
> >> +			if (old)
> >> +				hd_struct_put(old);
> >>  			return part;
> >>  		}
> >>  	}
> >> @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
> >>  	rcu_assign_pointer(disk->part_tbl, new_ptbl);
> >>
> >>  	if (old_ptbl) {
> >> -		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
> >> +		struct hd_struct *part;
> >> +
> >> +		part = xchg(&old_ptbl->last_lookup, NULL);
> >> +		if (part)
> >> +			hd_struct_put(part);
> >>  		kfree_rcu(old_ptbl, rcu_head);
> >>  	}
> >>  }
> >> diff --git a/block/partition-generic.c b/block/partition-generic.c
> >> index 98d60a59b843..441c1c591c04 100644
> >> --- a/block/partition-generic.c
> >> +++ b/block/partition-generic.c
> >> @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
> >>  		return;
> >>
> >>  	rcu_assign_pointer(ptbl->part[partno], NULL);
> >> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> >> +	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
> >> +		hd_struct_put(part);
> >>  	kobject_put(part->holder_dir);
> >>  	device_del(part_to_dev(part));
> > 
> > IMO this approach looks good.
> >
> Not sure about the overhead when there are concurrent IOs on different partitions,
> we will measure that.
> 
> We have got a seemingly better solution: caching the index of last_lookup in tbl->part[]
> instead of caching the pointer itself, so we can ensure the validity of returned pointer
> by ensuring it's not NULL in tbl->part[] as does when last_lookup is NULL or 0.

Thinking of the problem further, looks we don't need to hold ref for
.last_lookup.

What we need is to make sure the partition's ref is increased just
before assigning .last_lookup, so how about something like the following?

diff --git a/block/blk-core.c b/block/blk-core.c
index 089e890ab208..79599f5fd5b7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
 		part_stat_inc(part, merges[rw]);
 	} else {
 		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
-		if (!hd_struct_try_get(part)) {
-			/*
-			 * The partition is already being removed,
-			 * the request will be accounted on the disk only
-			 *
-			 * We take a reference on disk->part0 although that
-			 * partition will never be deleted, so we can treat
-			 * it as any other partition.
-			 */
-			part = &rq->rq_disk->part0;
-			hd_struct_get(part);
-		}
 		part_inc_in_flight(rq->q, part, rw);
 		rq->part = part;
 	}
diff --git a/block/genhd.c b/block/genhd.c
index ff6268970ddc..21f4a9b8d24d 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -286,17 +286,24 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
 	ptbl = rcu_dereference(disk->part_tbl);
 
 	part = rcu_dereference(ptbl->last_lookup);
-	if (part && sector_in_part(part, sector))
+	if (part && sector_in_part(part, sector)) {
+		if (!hd_struct_try_get(part))
+			goto exit;
 		return part;
+	}
 
 	for (i = 1; i < ptbl->len; i++) {
 		part = rcu_dereference(ptbl->part[i]);
 
 		if (part && sector_in_part(part, sector)) {
+                       if (!hd_struct_try_get(part))
+                               goto exit;
 			rcu_assign_pointer(ptbl->last_lookup, part);
 			return part;
 		}
 	}
+ exit:
+	hd_struct_get(&disk->part0);
 	return &disk->part0;
 }
 EXPORT_SYMBOL_GPL(disk_map_sector_rcu);


> 
> > Given partition is actually protected by percpu-refcount now, I guess the
> > RCU annotation for referencing ->part[partno] and ->last_lookup may not
> > be necessary, together with the part->rcu_work.
> > 
> So we will depends on the invocation of of call_rcu() on __percpu_ref_switch_mode() to
> ensure the RCU readers will find part[i] is NULL before trying to increasing
> the atomic ref-counter of part[i], right ?

Yeah.

Thanks,
Ming

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Hou Tao]

Hi,

On 2020/1/3 12:18, Ming Lei wrote:
> On Fri, Jan 03, 2020 at 11:06:25AM +0800, Hou Tao wrote:
>> Hi,
>>
>> On 2020/1/2 9:23, Ming Lei wrote:
>>> On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
>>>> Hi,
>>>>
snip

>> We have got a seemingly better solution: caching the index of last_lookup in tbl->part[]
>> instead of caching the pointer itself, so we can ensure the validity of returned pointer
>> by ensuring it's not NULL in tbl->part[] as does when last_lookup is NULL or 0.
> 
> Thinking of the problem further, looks we don't need to hold ref for
> .last_lookup.
> 
> What we need is to make sure the partition's ref is increased just
> before assigning .last_lookup, so how about something like the following?
> 
The approach will work for the above case, but it will not work for the following case:

when blk_account_io_done() releases the last ref-counter of last_lookup and calls call_rcu(),
and then a RCU read gets the to-be-freed hd-struct.

blk_account_io_done
  rcu_read_lock()
  // the last ref of last_lookup
  hd_struct_put()
    call_rcu

                              rcu_read_lock
                              read last_lookup

    free()
                              // use-after-free ?
                              hd_struct_try_get

Regards,
Tao

> diff --git a/block/blk-core.c b/block/blk-core.c
> index 089e890ab208..79599f5fd5b7 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
>  		part_stat_inc(part, merges[rw]);
>  	} else {
>  		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
> -		if (!hd_struct_try_get(part)) {
> -			/*
> -			 * The partition is already being removed,
> -			 * the request will be accounted on the disk only
> -			 *
> -			 * We take a reference on disk->part0 although that
> -			 * partition will never be deleted, so we can treat
> -			 * it as any other partition.
> -			 */
> -			part = &rq->rq_disk->part0;
> -			hd_struct_get(part);
> -		}
>  		part_inc_in_flight(rq->q, part, rw);
>  		rq->part = part;
>  	}
> diff --git a/block/genhd.c b/block/genhd.c
> index ff6268970ddc..21f4a9b8d24d 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -286,17 +286,24 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>  	ptbl = rcu_dereference(disk->part_tbl);
>  
>  	part = rcu_dereference(ptbl->last_lookup);
> -	if (part && sector_in_part(part, sector))
> +	if (part && sector_in_part(part, sector)) {
> +		if (!hd_struct_try_get(part))
> +			goto exit;
>  		return part;
> +	}
>  
>  	for (i = 1; i < ptbl->len; i++) {
>  		part = rcu_dereference(ptbl->part[i]);
>  
>  		if (part && sector_in_part(part, sector)) {
> +                       if (!hd_struct_try_get(part))
> +                               goto exit;
>  			rcu_assign_pointer(ptbl->last_lookup, part);
>  			return part;
>  		}
>  	}
> + exit:
> +	hd_struct_get(&disk->part0);
>  	return &disk->part0;
>  }
>  EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
> 
> 
>>
>>> Given partition is actually protected by percpu-refcount now, I guess the
>>> RCU annotation for referencing ->part[partno] and ->last_lookup may not
>>> be necessary, together with the part->rcu_work.
>>>
>> So we will depends on the invocation of of call_rcu() on __percpu_ref_switch_mode() to
>> ensure the RCU readers will find part[i] is NULL before trying to increasing
>> the atomic ref-counter of part[i], right ?
> 
> Yeah.
> 
> Thanks,
> Ming
> 
> 
> .
> 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Ming Lei]

On Fri, Jan 03, 2020 at 03:35:42PM +0800, Hou Tao wrote:
> Hi,
> 
> On 2020/1/3 12:18, Ming Lei wrote:
> > On Fri, Jan 03, 2020 at 11:06:25AM +0800, Hou Tao wrote:
> >> Hi,
> >>
> >> On 2020/1/2 9:23, Ming Lei wrote:
> >>> On Tue, Dec 31, 2019 at 10:55:47PM +0800, Hou Tao wrote:
> >>>> Hi,
> >>>>
> snip
> 
> >> We have got a seemingly better solution: caching the index of last_lookup in tbl->part[]
> >> instead of caching the pointer itself, so we can ensure the validity of returned pointer
> >> by ensuring it's not NULL in tbl->part[] as does when last_lookup is NULL or 0.
> > 
> > Thinking of the problem further, looks we don't need to hold ref for
> > .last_lookup.
> > 
> > What we need is to make sure the partition's ref is increased just
> > before assigning .last_lookup, so how about something like the following?
> > 
> The approach will work for the above case, but it will not work for the following case:
> 
> when blk_account_io_done() releases the last ref-counter of last_lookup and calls call_rcu(),
> and then a RCU read gets the to-be-freed hd-struct.
> 
> blk_account_io_done
>   rcu_read_lock()
>   // the last ref of last_lookup
>   hd_struct_put()
>     call_rcu
> 
>                               rcu_read_lock
>                               read last_lookup
> 
>     free()
>                               // use-after-free ?
>                               hd_struct_try_get

We may avoid that by clearing partition pointer after killing the
partition, how about the following change?

diff --git a/block/blk-core.c b/block/blk-core.c
index 089e890ab208..79599f5fd5b7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
 		part_stat_inc(part, merges[rw]);
 	} else {
 		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
-		if (!hd_struct_try_get(part)) {
-			/*
-			 * The partition is already being removed,
-			 * the request will be accounted on the disk only
-			 *
-			 * We take a reference on disk->part0 although that
-			 * partition will never be deleted, so we can treat
-			 * it as any other partition.
-			 */
-			part = &rq->rq_disk->part0;
-			hd_struct_get(part);
-		}
 		part_inc_in_flight(rq->q, part, rw);
 		rq->part = part;
 	}
diff --git a/block/genhd.c b/block/genhd.c
index ff6268970ddc..e3dec90b1f43 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
 	ptbl = rcu_dereference(disk->part_tbl);
 
 	part = rcu_dereference(ptbl->last_lookup);
-	if (part && sector_in_part(part, sector))
+	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
 		return part;
 
 	for (i = 1; i < ptbl->len; i++) {
 		part = rcu_dereference(ptbl->part[i]);
 
 		if (part && sector_in_part(part, sector)) {
+                       if (!hd_struct_try_get(part))
+                               goto exit;
 			rcu_assign_pointer(ptbl->last_lookup, part);
 			return part;
 		}
 	}
+ exit:
+	hd_struct_get(&disk->part0);
 	return &disk->part0;
 }
 EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1d20c9cf213f..9ef6c13d5650 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -283,8 +283,8 @@ void delete_partition(struct gendisk *disk, int partno)
 	if (!part)
 		return;
 
-	rcu_assign_pointer(ptbl->part[partno], NULL);
-	rcu_assign_pointer(ptbl->last_lookup, NULL);
+	get_device(disk_to_dev(disk));
+
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));
 
@@ -296,6 +296,15 @@ void delete_partition(struct gendisk *disk, int partno)
 	 */
 	blk_invalidate_devt(part_devt(part));
 	hd_struct_kill(part);
+
+	/*
+	 * clear partition pointers after this partition is killed, then
+	 * IO path can't re-assign ->last_lookup any more
+	 */
+	rcu_assign_pointer(ptbl->part[partno], NULL);
+	rcu_assign_pointer(ptbl->last_lookup, NULL);
+
+	put_device(disk_to_dev(disk));
 }
 
 static ssize_t whole_disk_show(struct device *dev,

Thanks,
Ming

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Yufen Yu]

Hi, Ming

On 2020/1/3 16:17, Ming Lei wrote:
> 
> We may avoid that by clearing partition pointer after killing the
> partition, how about the following change?
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 089e890ab208..79599f5fd5b7 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
>   		part_stat_inc(part, merges[rw]);
>   	} else {
>   		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
> -		if (!hd_struct_try_get(part)) {
> -			/*
> -			 * The partition is already being removed,
> -			 * the request will be accounted on the disk only
> -			 *
> -			 * We take a reference on disk->part0 although that
> -			 * partition will never be deleted, so we can treat
> -			 * it as any other partition.
> -			 */
> -			part = &rq->rq_disk->part0;
> -			hd_struct_get(part);
> -		}
>   		part_inc_in_flight(rq->q, part, rw);
>   		rq->part = part;
>   	}
> diff --git a/block/genhd.c b/block/genhd.c
> index ff6268970ddc..e3dec90b1f43 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>   	ptbl = rcu_dereference(disk->part_tbl);
>   
>   	part = rcu_dereference(ptbl->last_lookup);
> -	if (part && sector_in_part(part, sector))
> +	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
>   		return part;
>   
>   	for (i = 1; i < ptbl->len; i++) {
>   		part = rcu_dereference(ptbl->part[i]);
>   
>   		if (part && sector_in_part(part, sector)) {
> +                       if (!hd_struct_try_get(part))
> +                               goto exit;
>   			rcu_assign_pointer(ptbl->last_lookup, part);
>   			return part;
>   		}
>   	}
> + exit:
> +	hd_struct_get(&disk->part0);
>   	return &disk->part0;
>   }
>   EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 1d20c9cf213f..9ef6c13d5650 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -283,8 +283,8 @@ void delete_partition(struct gendisk *disk, int partno)
>   	if (!part)
>   		return;
>   
> -	rcu_assign_pointer(ptbl->part[partno], NULL);
> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +	get_device(disk_to_dev(disk));
> +
>   	kobject_put(part->holder_dir);
>   	device_del(part_to_dev(part));
>   
> @@ -296,6 +296,15 @@ void delete_partition(struct gendisk *disk, int partno)
>   	 */
>   	blk_invalidate_devt(part_devt(part));
>   	hd_struct_kill(part);
> +
> +	/*
> +	 * clear partition pointers after this partition is killed, then
> +	 * IO path can't re-assign ->last_lookup any more
> +	 */
> +	rcu_assign_pointer(ptbl->part[partno], NULL);
> +	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +
> +	put_device(disk_to_dev(disk));
>   }
>   

This change may cannot solve follow case:

disk_map_sector_rcu     delete_partition  disk_map_sector_rcu
hd_struct_try_get(part)
                         hd_struct_kill
                         last_lookup = NULL;
last_lookup = part


call_rcu
                                            read last_lookup

free()
                                            //use-after-free
                                            sector_in_part(part, sector)

There is an interval between getting part and setting last_lookup
in disk_map_sector_rcu(). If we kill the part and clear last_lookup
at that interval, last_lookup will be re-assign again, which can cause
use-after-free for readers after call_rcu.

Thanks
Yufen

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Ming Lei]

Hello Yufen,

On Fri, Jan 03, 2020 at 08:03:54PM +0800, Yufen Yu wrote:
> Hi, Ming
> 
> On 2020/1/3 16:17, Ming Lei wrote:
> > 
> > We may avoid that by clearing partition pointer after killing the
> > partition, how about the following change?
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 089e890ab208..79599f5fd5b7 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
> >   		part_stat_inc(part, merges[rw]);
> >   	} else {
> >   		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
> > -		if (!hd_struct_try_get(part)) {
> > -			/*
> > -			 * The partition is already being removed,
> > -			 * the request will be accounted on the disk only
> > -			 *
> > -			 * We take a reference on disk->part0 although that
> > -			 * partition will never be deleted, so we can treat
> > -			 * it as any other partition.
> > -			 */
> > -			part = &rq->rq_disk->part0;
> > -			hd_struct_get(part);
> > -		}
> >   		part_inc_in_flight(rq->q, part, rw);
> >   		rq->part = part;
> >   	}
> > diff --git a/block/genhd.c b/block/genhd.c
> > index ff6268970ddc..e3dec90b1f43 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >   	ptbl = rcu_dereference(disk->part_tbl);
> >   	part = rcu_dereference(ptbl->last_lookup);
> > -	if (part && sector_in_part(part, sector))
> > +	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
> >   		return part;
> >   	for (i = 1; i < ptbl->len; i++) {
> >   		part = rcu_dereference(ptbl->part[i]);
> >   		if (part && sector_in_part(part, sector)) {
> > +                       if (!hd_struct_try_get(part))
> > +                               goto exit;
> >   			rcu_assign_pointer(ptbl->last_lookup, part);
> >   			return part;
> >   		}
> >   	}
> > + exit:
> > +	hd_struct_get(&disk->part0);
> >   	return &disk->part0;
> >   }
> >   EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 1d20c9cf213f..9ef6c13d5650 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -283,8 +283,8 @@ void delete_partition(struct gendisk *disk, int partno)
> >   	if (!part)
> >   		return;
> > -	rcu_assign_pointer(ptbl->part[partno], NULL);
> > -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +	get_device(disk_to_dev(disk));
> > +
> >   	kobject_put(part->holder_dir);
> >   	device_del(part_to_dev(part));
> > @@ -296,6 +296,15 @@ void delete_partition(struct gendisk *disk, int partno)
> >   	 */
> >   	blk_invalidate_devt(part_devt(part));
> >   	hd_struct_kill(part);
> > +
> > +	/*
> > +	 * clear partition pointers after this partition is killed, then
> > +	 * IO path can't re-assign ->last_lookup any more
> > +	 */
> > +	rcu_assign_pointer(ptbl->part[partno], NULL);
> > +	rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +
> > +	put_device(disk_to_dev(disk));
> >   }
> 
> This change may cannot solve follow case:
> 
> disk_map_sector_rcu     delete_partition  disk_map_sector_rcu
> hd_struct_try_get(part)
>                         hd_struct_kill
>                         last_lookup = NULL;
> last_lookup = part
> 
> 
> call_rcu
>                                            read last_lookup
> 
> free()
>                                            //use-after-free
>                                            sector_in_part(part, sector)
> 
> There is an interval between getting part and setting last_lookup
> in disk_map_sector_rcu(). If we kill the part and clear last_lookup
> at that interval, last_lookup will be re-assign again, which can cause
> use-after-free for readers after call_rcu.

OK, we still can move clearing .last_lookup into __delete_partition(),
at that time all IO path can observe the partition percpu-refcount killed.

Also the rcu work fn is run after one RCU grace period, at that time,
the NULL .last_lookup becomes visible in all IO path too.

diff --git a/block/blk-core.c b/block/blk-core.c
index 089e890ab208..79599f5fd5b7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
 		part_stat_inc(part, merges[rw]);
 	} else {
 		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
-		if (!hd_struct_try_get(part)) {
-			/*
-			 * The partition is already being removed,
-			 * the request will be accounted on the disk only
-			 *
-			 * We take a reference on disk->part0 although that
-			 * partition will never be deleted, so we can treat
-			 * it as any other partition.
-			 */
-			part = &rq->rq_disk->part0;
-			hd_struct_get(part);
-		}
 		part_inc_in_flight(rq->q, part, rw);
 		rq->part = part;
 	}
diff --git a/block/genhd.c b/block/genhd.c
index ff6268970ddc..e3dec90b1f43 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
 	ptbl = rcu_dereference(disk->part_tbl);
 
 	part = rcu_dereference(ptbl->last_lookup);
-	if (part && sector_in_part(part, sector))
+	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
 		return part;
 
 	for (i = 1; i < ptbl->len; i++) {
 		part = rcu_dereference(ptbl->part[i]);
 
 		if (part && sector_in_part(part, sector)) {
+                       if (!hd_struct_try_get(part))
+                               goto exit;
 			rcu_assign_pointer(ptbl->last_lookup, part);
 			return part;
 		}
 	}
+ exit:
+	hd_struct_get(&disk->part0);
 	return &disk->part0;
 }
 EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 1d20c9cf213f..1739f750dbf2 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -262,6 +262,12 @@ static void delete_partition_work_fn(struct work_struct *work)
 void __delete_partition(struct percpu_ref *ref)
 {
 	struct hd_struct *part = container_of(ref, struct hd_struct, ref);
+	struct disk_part_tbl *ptbl =
+		rcu_dereference_protected(part->disk->part_tbl, 1);
+
+	rcu_assign_pointer(ptbl->last_lookup, NULL);
+	put_device(disk_to_dev(part->disk));
+
 	INIT_RCU_WORK(&part->rcu_work, delete_partition_work_fn);
 	queue_rcu_work(system_wq, &part->rcu_work);
 }
@@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
 	if (!part)
 		return;
 
+	get_device(disk_to_dev(disk));
 	rcu_assign_pointer(ptbl->part[partno], NULL);
-	rcu_assign_pointer(ptbl->last_lookup, NULL);
+
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));
 
@@ -349,6 +356,7 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
 	p->nr_sects = len;
 	p->partno = partno;
 	p->policy = get_disk_ro(disk);
+	p->disk = disk;
 
 	if (info) {
 		struct partition_meta_info *pinfo = alloc_part_info(disk);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 8bb63027e4d6..66660ec5e8ee 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -129,6 +129,7 @@ struct hd_struct {
 #else
 	struct disk_stats dkstats;
 #endif
+	struct gendisk *disk;
 	struct percpu_ref ref;
 	struct rcu_work rcu_work;
 };


Thanks,
Ming

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Yufen Yu]

Hi, Ming

On 2020/1/3 23:16, Ming Lei wrote:
> Hello Yufen,
> 
> OK, we still can move clearing .last_lookup into __delete_partition(),
> at that time all IO path can observe the partition percpu-refcount killed.
> 
> Also the rcu work fn is run after one RCU grace period, at that time,
> the NULL .last_lookup becomes visible in all IO path too.
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 089e890ab208..79599f5fd5b7 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
>   		part_stat_inc(part, merges[rw]);
>   	} else {
>   		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
> -		if (!hd_struct_try_get(part)) {
> -			/*
> -			 * The partition is already being removed,
> -			 * the request will be accounted on the disk only
> -			 *
> -			 * We take a reference on disk->part0 although that
> -			 * partition will never be deleted, so we can treat
> -			 * it as any other partition.
> -			 */
> -			part = &rq->rq_disk->part0;
> -			hd_struct_get(part);
> -		}
>   		part_inc_in_flight(rq->q, part, rw);
>   		rq->part = part;
>   	}
> diff --git a/block/genhd.c b/block/genhd.c
> index ff6268970ddc..e3dec90b1f43 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>   	ptbl = rcu_dereference(disk->part_tbl);
>   
>   	part = rcu_dereference(ptbl->last_lookup);
> -	if (part && sector_in_part(part, sector))
> +	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
>   		return part;
>   
>   	for (i = 1; i < ptbl->len; i++) {
>   		part = rcu_dereference(ptbl->part[i]);
>   
>   		if (part && sector_in_part(part, sector)) {
> +                       if (!hd_struct_try_get(part))
> +                               goto exit;
>   			rcu_assign_pointer(ptbl->last_lookup, part);
>   			return part;
>   		}
>   	}
> + exit:
> +	hd_struct_get(&disk->part0);
>   	return &disk->part0;
>   }
>   EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 1d20c9cf213f..1739f750dbf2 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -262,6 +262,12 @@ static void delete_partition_work_fn(struct work_struct *work)
>   void __delete_partition(struct percpu_ref *ref)
>   {
>   	struct hd_struct *part = container_of(ref, struct hd_struct, ref);
> +	struct disk_part_tbl *ptbl =
> +		rcu_dereference_protected(part->disk->part_tbl, 1);
> +
> +	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +	put_device(disk_to_dev(part->disk));
> +
>   	INIT_RCU_WORK(&part->rcu_work, delete_partition_work_fn);
>   	queue_rcu_work(system_wq, &part->rcu_work);
>   }
> @@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
>   	if (!part)
>   		return;
>   
> +	get_device(disk_to_dev(disk));
>   	rcu_assign_pointer(ptbl->part[partno], NULL);
> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +
>   	kobject_put(part->holder_dir);
>   	device_del(part_to_dev(part));
>   
> @@ -349,6 +356,7 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
>   	p->nr_sects = len;
>   	p->partno = partno;
>   	p->policy = get_disk_ro(disk);
> +	p->disk = disk;
>   
>   	if (info) {
>   		struct partition_meta_info *pinfo = alloc_part_info(disk);
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 8bb63027e4d6..66660ec5e8ee 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -129,6 +129,7 @@ struct hd_struct {
>   #else
>   	struct disk_stats dkstats;
>   #endif
> +	struct gendisk *disk;
>   	struct percpu_ref ref;
>   	struct rcu_work rcu_work;
>   };


IMO, this change can solve the problem. But, __delete_partition will
depend on the implementation of disk_release(). If disk .release modify
as blocked in the future, then __delete_partition will also be blocked,
which is not expected in rcu callback function.

We may cache index of part[] instead of part[i] itself to fix the use-after-free bug.
https://patchwork.kernel.org/patch/11318767/

Thanks,
Yufen

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Ming Lei]

On Mon, Jan 06, 2020 at 03:39:07PM +0800, Yufen Yu wrote:
> Hi, Ming
> 
> On 2020/1/3 23:16, Ming Lei wrote:
> > Hello Yufen,
> > 
> > OK, we still can move clearing .last_lookup into __delete_partition(),
> > at that time all IO path can observe the partition percpu-refcount killed.
> > 
> > Also the rcu work fn is run after one RCU grace period, at that time,
> > the NULL .last_lookup becomes visible in all IO path too.
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 089e890ab208..79599f5fd5b7 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
> >   		part_stat_inc(part, merges[rw]);
> >   	} else {
> >   		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
> > -		if (!hd_struct_try_get(part)) {
> > -			/*
> > -			 * The partition is already being removed,
> > -			 * the request will be accounted on the disk only
> > -			 *
> > -			 * We take a reference on disk->part0 although that
> > -			 * partition will never be deleted, so we can treat
> > -			 * it as any other partition.
> > -			 */
> > -			part = &rq->rq_disk->part0;
> > -			hd_struct_get(part);
> > -		}
> >   		part_inc_in_flight(rq->q, part, rw);
> >   		rq->part = part;
> >   	}
> > diff --git a/block/genhd.c b/block/genhd.c
> > index ff6268970ddc..e3dec90b1f43 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >   	ptbl = rcu_dereference(disk->part_tbl);
> >   	part = rcu_dereference(ptbl->last_lookup);
> > -	if (part && sector_in_part(part, sector))
> > +	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
> >   		return part;
> >   	for (i = 1; i < ptbl->len; i++) {
> >   		part = rcu_dereference(ptbl->part[i]);
> >   		if (part && sector_in_part(part, sector)) {
> > +                       if (!hd_struct_try_get(part))
> > +                               goto exit;
> >   			rcu_assign_pointer(ptbl->last_lookup, part);
> >   			return part;
> >   		}
> >   	}
> > + exit:
> > +	hd_struct_get(&disk->part0);
> >   	return &disk->part0;
> >   }
> >   EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 1d20c9cf213f..1739f750dbf2 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -262,6 +262,12 @@ static void delete_partition_work_fn(struct work_struct *work)
> >   void __delete_partition(struct percpu_ref *ref)
> >   {
> >   	struct hd_struct *part = container_of(ref, struct hd_struct, ref);
> > +	struct disk_part_tbl *ptbl =
> > +		rcu_dereference_protected(part->disk->part_tbl, 1);
> > +
> > +	rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +	put_device(disk_to_dev(part->disk));
> > +
> >   	INIT_RCU_WORK(&part->rcu_work, delete_partition_work_fn);
> >   	queue_rcu_work(system_wq, &part->rcu_work);
> >   }
> > @@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
> >   	if (!part)
> >   		return;
> > +	get_device(disk_to_dev(disk));
> >   	rcu_assign_pointer(ptbl->part[partno], NULL);
> > -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> > +
> >   	kobject_put(part->holder_dir);
> >   	device_del(part_to_dev(part));
> > @@ -349,6 +356,7 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
> >   	p->nr_sects = len;
> >   	p->partno = partno;
> >   	p->policy = get_disk_ro(disk);
> > +	p->disk = disk;
> >   	if (info) {
> >   		struct partition_meta_info *pinfo = alloc_part_info(disk);
> > diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> > index 8bb63027e4d6..66660ec5e8ee 100644
> > --- a/include/linux/genhd.h
> > +++ b/include/linux/genhd.h
> > @@ -129,6 +129,7 @@ struct hd_struct {
> >   #else
> >   	struct disk_stats dkstats;
> >   #endif
> > +	struct gendisk *disk;
> >   	struct percpu_ref ref;
> >   	struct rcu_work rcu_work;
> >   };
> 
> 
> IMO, this change can solve the problem. But, __delete_partition will
> depend on the implementation of disk_release(). If disk .release modify
> as blocked in the future, then __delete_partition will also be blocked,
> which is not expected in rcu callback function.

__delete_partition() won't be blocked because it just calls queue_rcu_work() to
release the partition instance in wq context.

> 
> We may cache index of part[] instead of part[i] itself to fix the use-after-free bug.
> https://patchwork.kernel.org/patch/11318767/

That approach can fix the issue too, but extra overhead is added in the
fast path because partition retrieval is changed to the following way:

	+       last_lookup = READ_ONCE(ptbl->last_lookup);
	+       if (last_lookup > 0 && last_lookup < ptbl->len) {
	+               part = rcu_dereference(ptbl->part[last_lookup]);
	+               if (part && sector_in_part(part, sector))
	+                       return part;
	+       }

from 
	part = rcu_dereference(ptbl->last_lookup);

So ptbl->part[] has to be fetched, it is fine if the ->part[] array
shares same cacheline with ptbl->last_lookup, but one disk may have
too many partitions, then your approach may introduce one extra cache
miss every time.

READ_ONCE() may imply one read barrier too.


Thanks,
Ming

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Hou Tao]

Hi,

On 2020/1/6 16:11, Ming Lei wrote:
> On Mon, Jan 06, 2020 at 03:39:07PM +0800, Yufen Yu wrote:
>> Hi, Ming
>>
>> On 2020/1/3 23:16, Ming Lei wrote:
>>> Hello Yufen,
>>>
>>> OK, we still can move clearing .last_lookup into __delete_partition(),
>>> at that time all IO path can observe the partition percpu-refcount killed.
>>>
>>> Also the rcu work fn is run after one RCU grace period, at that time,
>>> the NULL .last_lookup becomes visible in all IO path too.
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 089e890ab208..79599f5fd5b7 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
>>>   		part_stat_inc(part, merges[rw]);
>>>   	} else {
>>>   		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
>>> -		if (!hd_struct_try_get(part)) {
>>> -			/*
>>> -			 * The partition is already being removed,
>>> -			 * the request will be accounted on the disk only
>>> -			 *
>>> -			 * We take a reference on disk->part0 although that
>>> -			 * partition will never be deleted, so we can treat
>>> -			 * it as any other partition.
>>> -			 */
>>> -			part = &rq->rq_disk->part0;
>>> -			hd_struct_get(part);
>>> -		}
>>>   		part_inc_in_flight(rq->q, part, rw);
>>>   		rq->part = part;
>>>   	}
>>> diff --git a/block/genhd.c b/block/genhd.c
>>> index ff6268970ddc..e3dec90b1f43 100644
>>> --- a/block/genhd.c
>>> +++ b/block/genhd.c
>>> @@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>>>   	ptbl = rcu_dereference(disk->part_tbl);
>>>   	part = rcu_dereference(ptbl->last_lookup);
>>> -	if (part && sector_in_part(part, sector))
>>> +	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
>>>   		return part;
>>>   	for (i = 1; i < ptbl->len; i++) {
>>>   		part = rcu_dereference(ptbl->part[i]);
>>>   		if (part && sector_in_part(part, sector)) {
>>> +                       if (!hd_struct_try_get(part))
>>> +                               goto exit;
>>>   			rcu_assign_pointer(ptbl->last_lookup, part);
>>>   			return part;
>>>   		}
>>>   	}
>>> + exit:
>>> +	hd_struct_get(&disk->part0);
>>>   	return &disk->part0;
>>>   }
>>>   EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
>>> diff --git a/block/partition-generic.c b/block/partition-generic.c
>>> index 1d20c9cf213f..1739f750dbf2 100644
>>> --- a/block/partition-generic.c
>>> +++ b/block/partition-generic.c
>>> @@ -262,6 +262,12 @@ static void delete_partition_work_fn(struct work_struct *work)
>>>   void __delete_partition(struct percpu_ref *ref)
>>>   {
>>>   	struct hd_struct *part = container_of(ref, struct hd_struct, ref);
>>> +	struct disk_part_tbl *ptbl =
>>> +		rcu_dereference_protected(part->disk->part_tbl, 1);
>>> +
>>> +	rcu_assign_pointer(ptbl->last_lookup, NULL);
>>> +	put_device(disk_to_dev(part->disk));
>>> +
>>>   	INIT_RCU_WORK(&part->rcu_work, delete_partition_work_fn);
>>>   	queue_rcu_work(system_wq, &part->rcu_work);
>>>   }
>>> @@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
>>>   	if (!part)
>>>   		return;
>>> +	get_device(disk_to_dev(disk));
>>>   	rcu_assign_pointer(ptbl->part[partno], NULL);
>>> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
>>> +
>>>   	kobject_put(part->holder_dir);
>>>   	device_del(part_to_dev(part));
>>> @@ -349,6 +356,7 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
>>>   	p->nr_sects = len;
>>>   	p->partno = partno;
>>>   	p->policy = get_disk_ro(disk);
>>> +	p->disk = disk;
>>>   	if (info) {
>>>   		struct partition_meta_info *pinfo = alloc_part_info(disk);
>>> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
>>> index 8bb63027e4d6..66660ec5e8ee 100644
>>> --- a/include/linux/genhd.h
>>> +++ b/include/linux/genhd.h
>>> @@ -129,6 +129,7 @@ struct hd_struct {
>>>   #else
>>>   	struct disk_stats dkstats;
>>>   #endif
>>> +	struct gendisk *disk;
>>>   	struct percpu_ref ref;
>>>   	struct rcu_work rcu_work;
>>>   };
>>
>>
>> IMO, this change can solve the problem. But, __delete_partition will
>> depend on the implementation of disk_release(). If disk .release modify
>> as blocked in the future, then __delete_partition will also be blocked,
>> which is not expected in rcu callback function.
> 
> __delete_partition() won't be blocked because it just calls queue_rcu_work() to
> release the partition instance in wq context.
> 
>>
>> We may cache index of part[] instead of part[i] itself to fix the use-after-free bug.
>> https://patchwork.kernel.org/patch/11318767/
> 
> That approach can fix the issue too, but extra overhead is added in the
> fast path because partition retrieval is changed to the following way:
> 
> 	+       last_lookup = READ_ONCE(ptbl->last_lookup);
> 	+       if (last_lookup > 0 && last_lookup < ptbl->len) {
> 	+               part = rcu_dereference(ptbl->part[last_lookup]);
> 	+               if (part && sector_in_part(part, sector))
> 	+                       return part;
> 	+       }
> 
> from 
> 	part = rcu_dereference(ptbl->last_lookup);
> 
> So ptbl->part[] has to be fetched, it is fine if the ->part[] array
> shares same cacheline with ptbl->last_lookup, but one disk may have
> too many partitions, then your approach may introduce one extra cache
> miss every time.
> 
Yes. The solution you proposed also adds an invocation of percpu_ref_tryget_live()
in the fast path. Not sure which one will have a better performance. However the
reason we prefer the index caching is the simplicity instead of performance.

> READ_ONCE() may imply one read barrier too.
> 
According to the manual, it only matters on Alpha machines and READ_ONCE() also
exists in rcu_dereference().

Regards,
Tao

> 
> Thanks,
> Ming
> 
> 
> .
> 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Ming Lei]

On Mon, Jan 06, 2020 at 05:41:45PM +0800, Hou Tao wrote:
> Hi,
> 
> On 2020/1/6 16:11, Ming Lei wrote:
> > On Mon, Jan 06, 2020 at 03:39:07PM +0800, Yufen Yu wrote:
> >> Hi, Ming
> >>
> >> On 2020/1/3 23:16, Ming Lei wrote:
> >>> Hello Yufen,
> >>>
> >>> OK, we still can move clearing .last_lookup into __delete_partition(),
> >>> at that time all IO path can observe the partition percpu-refcount killed.
> >>>
> >>> Also the rcu work fn is run after one RCU grace period, at that time,
> >>> the NULL .last_lookup becomes visible in all IO path too.
> >>>
> >>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>> index 089e890ab208..79599f5fd5b7 100644
> >>> --- a/block/blk-core.c
> >>> +++ b/block/blk-core.c
> >>> @@ -1365,18 +1365,6 @@ void blk_account_io_start(struct request *rq, bool new_io)
> >>>   		part_stat_inc(part, merges[rw]);
> >>>   	} else {
> >>>   		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
> >>> -		if (!hd_struct_try_get(part)) {
> >>> -			/*
> >>> -			 * The partition is already being removed,
> >>> -			 * the request will be accounted on the disk only
> >>> -			 *
> >>> -			 * We take a reference on disk->part0 although that
> >>> -			 * partition will never be deleted, so we can treat
> >>> -			 * it as any other partition.
> >>> -			 */
> >>> -			part = &rq->rq_disk->part0;
> >>> -			hd_struct_get(part);
> >>> -		}
> >>>   		part_inc_in_flight(rq->q, part, rw);
> >>>   		rq->part = part;
> >>>   	}
> >>> diff --git a/block/genhd.c b/block/genhd.c
> >>> index ff6268970ddc..e3dec90b1f43 100644
> >>> --- a/block/genhd.c
> >>> +++ b/block/genhd.c
> >>> @@ -286,17 +286,21 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
> >>>   	ptbl = rcu_dereference(disk->part_tbl);
> >>>   	part = rcu_dereference(ptbl->last_lookup);
> >>> -	if (part && sector_in_part(part, sector))
> >>> +	if (part && sector_in_part(part, sector) && hd_struct_try_get(part))
> >>>   		return part;
> >>>   	for (i = 1; i < ptbl->len; i++) {
> >>>   		part = rcu_dereference(ptbl->part[i]);
> >>>   		if (part && sector_in_part(part, sector)) {
> >>> +                       if (!hd_struct_try_get(part))
> >>> +                               goto exit;
> >>>   			rcu_assign_pointer(ptbl->last_lookup, part);
> >>>   			return part;
> >>>   		}
> >>>   	}
> >>> + exit:
> >>> +	hd_struct_get(&disk->part0);
> >>>   	return &disk->part0;
> >>>   }
> >>>   EXPORT_SYMBOL_GPL(disk_map_sector_rcu);
> >>> diff --git a/block/partition-generic.c b/block/partition-generic.c
> >>> index 1d20c9cf213f..1739f750dbf2 100644
> >>> --- a/block/partition-generic.c
> >>> +++ b/block/partition-generic.c
> >>> @@ -262,6 +262,12 @@ static void delete_partition_work_fn(struct work_struct *work)
> >>>   void __delete_partition(struct percpu_ref *ref)
> >>>   {
> >>>   	struct hd_struct *part = container_of(ref, struct hd_struct, ref);
> >>> +	struct disk_part_tbl *ptbl =
> >>> +		rcu_dereference_protected(part->disk->part_tbl, 1);
> >>> +
> >>> +	rcu_assign_pointer(ptbl->last_lookup, NULL);
> >>> +	put_device(disk_to_dev(part->disk));
> >>> +
> >>>   	INIT_RCU_WORK(&part->rcu_work, delete_partition_work_fn);
> >>>   	queue_rcu_work(system_wq, &part->rcu_work);
> >>>   }
> >>> @@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
> >>>   	if (!part)
> >>>   		return;
> >>> +	get_device(disk_to_dev(disk));
> >>>   	rcu_assign_pointer(ptbl->part[partno], NULL);
> >>> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> >>> +
> >>>   	kobject_put(part->holder_dir);
> >>>   	device_del(part_to_dev(part));
> >>> @@ -349,6 +356,7 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
> >>>   	p->nr_sects = len;
> >>>   	p->partno = partno;
> >>>   	p->policy = get_disk_ro(disk);
> >>> +	p->disk = disk;
> >>>   	if (info) {
> >>>   		struct partition_meta_info *pinfo = alloc_part_info(disk);
> >>> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> >>> index 8bb63027e4d6..66660ec5e8ee 100644
> >>> --- a/include/linux/genhd.h
> >>> +++ b/include/linux/genhd.h
> >>> @@ -129,6 +129,7 @@ struct hd_struct {
> >>>   #else
> >>>   	struct disk_stats dkstats;
> >>>   #endif
> >>> +	struct gendisk *disk;
> >>>   	struct percpu_ref ref;
> >>>   	struct rcu_work rcu_work;
> >>>   };
> >>
> >>
> >> IMO, this change can solve the problem. But, __delete_partition will
> >> depend on the implementation of disk_release(). If disk .release modify
> >> as blocked in the future, then __delete_partition will also be blocked,
> >> which is not expected in rcu callback function.
> > 
> > __delete_partition() won't be blocked because it just calls queue_rcu_work() to
> > release the partition instance in wq context.
> > 
> >>
> >> We may cache index of part[] instead of part[i] itself to fix the use-after-free bug.
> >> https://patchwork.kernel.org/patch/11318767/
> > 
> > That approach can fix the issue too, but extra overhead is added in the
> > fast path because partition retrieval is changed to the following way:
> > 
> > 	+       last_lookup = READ_ONCE(ptbl->last_lookup);
> > 	+       if (last_lookup > 0 && last_lookup < ptbl->len) {
> > 	+               part = rcu_dereference(ptbl->part[last_lookup]);
> > 	+               if (part && sector_in_part(part, sector))
> > 	+                       return part;
> > 	+       }
> > 
> > from 
> > 	part = rcu_dereference(ptbl->last_lookup);
> > 
> > So ptbl->part[] has to be fetched, it is fine if the ->part[] array
> > shares same cacheline with ptbl->last_lookup, but one disk may have
> > too many partitions, then your approach may introduce one extra cache
> > miss every time.
> > 
> Yes. The solution you proposed also adds an invocation of percpu_ref_tryget_live()
> in the fast path. Not sure which one will have a better performance. However the
> reason we prefer the index caching is the simplicity instead of performance.

No, hd_struct_try_get() and hd_struct_get() is always called once for one IO, the
patch I proposed changes nothing about this usage.

Please take a close look at the patch:

https://lore.kernel.org/linux-block/5cc465cc-d68c-088e-0729-2695279c7853@huawei.com/T/#m8f3e6b4e77eadf006ce142a84c966f50f3a9ae26

which just moves hd_struct_try_get() from blk_account_io_start() into
disk_map_sector_rcu(), doesn't it?


Thanks,
Ming

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Hou Tao]

Hi,

On 2020/1/6 18:05, Ming Lei wrote:
> On Mon, Jan 06, 2020 at 05:41:45PM +0800, Hou Tao wrote:
>> Hi,
>>
[snipped]

>> Yes. The solution you proposed also adds an invocation of percpu_ref_tryget_live()
>> in the fast path. Not sure which one will have a better performance. However the
>> reason we prefer the index caching is the simplicity instead of performance.
> 
> No, hd_struct_try_get() and hd_struct_get() is always called once for one IO, the
> patch I proposed changes nothing about this usage.
> 
> Please take a close look at the patch:
> 
> https://lore.kernel.org/linux-block/5cc465cc-d68c-088e-0729-2695279c7853@huawei.com/T/#m8f3e6b4e77eadf006ce142a84c966f50f3a9ae26
> 
> which just moves hd_struct_try_get() from blk_account_io_start() into
> disk_map_sector_rcu(), doesn't it?
> 
Yes, you are right. And a little suggestion for your patch:

@@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
 	if (!part)
 		return;

+	get_device(disk_to_dev(disk));
 	rcu_assign_pointer(ptbl->part[partno], NULL);
-	rcu_assign_pointer(ptbl->last_lookup, NULL);
+
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));

Could we move the call of get_device() into add_partition, and that will make the assignment of
disk to p->disk in add_partition() be natural ?

Maybe there is no need to add a new disk field in hd_struct, because the kobject of gendisk
is already the parent of hd_struct. But make use of part->__dev.parent after the calling
of device_del() is a bad_idea.

Regards,
Tao


> 
> Thanks,
> Ming
> 
> 
> .
> 

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Ming Lei]

On Tue, Jan 07, 2020 at 07:40:10PM +0800, Hou Tao wrote:
> Hi,
> 
> On 2020/1/6 18:05, Ming Lei wrote:
> > On Mon, Jan 06, 2020 at 05:41:45PM +0800, Hou Tao wrote:
> >> Hi,
> >>
> [snipped]
> 
> >> Yes. The solution you proposed also adds an invocation of percpu_ref_tryget_live()
> >> in the fast path. Not sure which one will have a better performance. However the
> >> reason we prefer the index caching is the simplicity instead of performance.
> > 
> > No, hd_struct_try_get() and hd_struct_get() is always called once for one IO, the
> > patch I proposed changes nothing about this usage.
> > 
> > Please take a close look at the patch:
> > 
> > https://lore.kernel.org/linux-block/5cc465cc-d68c-088e-0729-2695279c7853@huawei.com/T/#m8f3e6b4e77eadf006ce142a84c966f50f3a9ae26
> > 
> > which just moves hd_struct_try_get() from blk_account_io_start() into
> > disk_map_sector_rcu(), doesn't it?
> > 
> Yes, you are right. And a little suggestion for your patch:
> 
> @@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
>  	if (!part)
>  		return;
> 
> +	get_device(disk_to_dev(disk));
>  	rcu_assign_pointer(ptbl->part[partno], NULL);
> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +
>  	kobject_put(part->holder_dir);
>  	device_del(part_to_dev(part));
> 
> Could we move the call of get_device() into add_partition, and that will make the assignment of
> disk to p->disk in add_partition() be natural ?

It isn't necessary to do that way, the parent disk's refcount isn't released
until device_del(part_to_dev(part)). So it is enough to hold disk's refcount
before calling device_del(part).

> 
> Maybe there is no need to add a new disk field in hd_struct, because the kobject of gendisk
> is already the parent of hd_struct. But make use of part->__dev.parent after the calling
> of device_del() is a bad_idea.

Yeah, that is why I added 'disk' field, which can be put with other
fields not accessed in fast path.

BTW, 'struct percpu_ref ref' should have been put just after 'nr_sects',
then these fast path fields can share single cache line.


Thanks,
Ming

Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Yufen Yu]

On 2019/12/31 22:55, houtao (A) wrote:

> diff --git a/block/genhd.c b/block/genhd.c
> index 6e8543ca6912..179e0056fae1 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -279,7 +279,14 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector)
>   		part = rcu_dereference(ptbl->part[i]);
> 
>   		if (part && sector_in_part(part, sector)) {
> -			rcu_assign_pointer(ptbl->last_lookup, part);
> +			struct hd_struct *old;
> +
> +			if (!hd_struct_try_get(part))
> +				break;
> +
> +			old = xchg(&ptbl->last_lookup, part);
> +			if (old)
> +				hd_struct_put(old);
>   			return part;
>   		}
>   	}
> @@ -1231,7 +1238,11 @@ static void disk_replace_part_tbl(struct gendisk *disk,
>   	rcu_assign_pointer(disk->part_tbl, new_ptbl);
> 
>   	if (old_ptbl) {
> -		rcu_assign_pointer(old_ptbl->last_lookup, NULL);
> +		struct hd_struct *part;
> +
> +		part = xchg(&old_ptbl->last_lookup, NULL);
> +		if (part)
> +			hd_struct_put(part);
>   		kfree_rcu(old_ptbl, rcu_head);
>   	}
>   }
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 98d60a59b843..441c1c591c04 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -285,7 +285,8 @@ void delete_partition(struct gendisk *disk, int partno)
>   		return;
> 
>   	rcu_assign_pointer(ptbl->part[partno], NULL);
> -	rcu_assign_pointer(ptbl->last_lookup, NULL);
> +	if (cmpxchg(&ptbl->last_lookup, part, NULL) == part)
> +		hd_struct_put(part);
>   	kobject_put(part->holder_dir);
>   	device_del(part_to_dev(part));
> 
This change will add more calling of hd_struct_try_get()/hd_struct_put()
when set last_lookup. Thus, Tao warried abort extra overhead.

We test the patch on a SSD disk by fio with config:
[global]
rw=randread
bs=4k
numjobs=2
ioengine=libaio
iodepth=32
direct=1
runtime=120
size=100G
group_reporting
time_based

[sda1]
filename=/dev/sda1

[sda2]
filename=/dev/sda2

[sda3]
filename=/dev/sda3

[sda4]
filename=/dev/sda4

Compared with the previous version, io bandwidth have no any degeneration.

Thanks,
Yufen