* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
[not found] <20191231110945.10857-1-yuyufen@huawei.com>
@ 2019-12-31 14:55 ` Hou Tao
2019-12-31 23:11 ` Paul E. McKenney
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Hou Tao @ 2019-12-31 14:55 UTC (permalink / raw)
To: Yufen Yu, axboe
Cc: linux-block, ming.lei, hch, zhengchuan, yi.zhang, paulmck, joel, rcu
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));
>
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2019-12-31 14:55 ` [PATCH] block: make sure last_lookup set as NULL after part deleted Hou Tao
@ 2019-12-31 23:11 ` Paul E. McKenney
2020-01-01 2:33 ` htbegin
2020-01-03 23:45 ` Joel Fernandes
2020-01-02 1:23 ` Ming Lei
2020-01-03 12:43 ` Yufen Yu
2 siblings, 2 replies; 20+ messages in thread
From: Paul E. McKenney @ 2019-12-31 23:11 UTC (permalink / raw)
To: Hou Tao
Cc: Yufen Yu, axboe, linux-block, ming.lei, hch, zhengchuan,
yi.zhang, joel, rcu
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));
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2019-12-31 23:11 ` Paul E. McKenney
@ 2020-01-01 2:33 ` htbegin
2020-01-01 3:39 ` htbegin
2020-01-03 23:45 ` Joel Fernandes
1 sibling, 1 reply; 20+ messages in thread
From: htbegin @ 2020-01-01 2:33 UTC (permalink / raw)
To: paulmck
Cc: Hou Tao, Yufen Yu, axboe, linux-block, ming.lei, hch, zhengchuan,
yi.zhang, joel, rcu
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));
> > >
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-01 2:33 ` htbegin
@ 2020-01-01 3:39 ` htbegin
0 siblings, 0 replies; 20+ messages in thread
From: htbegin @ 2020-01-01 3:39 UTC (permalink / raw)
To: paulmck
Cc: Hou Tao, Yufen Yu, axboe, linux-block, ming.lei, hch, zhengchuan,
yi.zhang, joel, rcu
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));
> > > >
> > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2019-12-31 14:55 ` [PATCH] block: make sure last_lookup set as NULL after part deleted Hou Tao
2019-12-31 23:11 ` Paul E. McKenney
@ 2020-01-02 1:23 ` Ming Lei
2020-01-03 3:06 ` Hou Tao
2020-01-03 12:43 ` Yufen Yu
2 siblings, 1 reply; 20+ messages in thread
From: Ming Lei @ 2020-01-02 1:23 UTC (permalink / raw)
To: Hou Tao
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-02 1:23 ` Ming Lei
@ 2020-01-03 3:06 ` Hou Tao
2020-01-03 4:18 ` Ming Lei
0 siblings, 1 reply; 20+ messages in thread
From: Hou Tao @ 2020-01-03 3:06 UTC (permalink / raw)
To: Ming Lei
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
>
>
> .
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-03 3:06 ` Hou Tao
@ 2020-01-03 4:18 ` Ming Lei
2020-01-03 7:35 ` Hou Tao
0 siblings, 1 reply; 20+ messages in thread
From: Ming Lei @ 2020-01-03 4:18 UTC (permalink / raw)
To: Hou Tao
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-03 4:18 ` Ming Lei
@ 2020-01-03 7:35 ` Hou Tao
2020-01-03 8:17 ` Ming Lei
0 siblings, 1 reply; 20+ messages in thread
From: Hou Tao @ 2020-01-03 7:35 UTC (permalink / raw)
To: Ming Lei
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
>
>
> .
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-03 7:35 ` Hou Tao
@ 2020-01-03 8:17 ` Ming Lei
2020-01-03 12:03 ` Yufen Yu
0 siblings, 1 reply; 20+ messages in thread
From: Ming Lei @ 2020-01-03 8:17 UTC (permalink / raw)
To: Hou Tao
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-03 8:17 ` Ming Lei
@ 2020-01-03 12:03 ` Yufen Yu
2020-01-03 15:16 ` Ming Lei
0 siblings, 1 reply; 20+ messages in thread
From: Yufen Yu @ 2020-01-03 12:03 UTC (permalink / raw)
To: Ming Lei, Hou Tao
Cc: axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck, joel, rcu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2019-12-31 14:55 ` [PATCH] block: make sure last_lookup set as NULL after part deleted Hou Tao
2019-12-31 23:11 ` Paul E. McKenney
2020-01-02 1:23 ` Ming Lei
@ 2020-01-03 12:43 ` Yufen Yu
2 siblings, 0 replies; 20+ messages in thread
From: Yufen Yu @ 2020-01-03 12:43 UTC (permalink / raw)
To: houtao (A), axboe
Cc: linux-block, ming.lei, hch, zhengchuan, zhangyi (F), paulmck, joel, rcu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-03 12:03 ` Yufen Yu
@ 2020-01-03 15:16 ` Ming Lei
2020-01-06 7:39 ` Yufen Yu
0 siblings, 1 reply; 20+ messages in thread
From: Ming Lei @ 2020-01-03 15:16 UTC (permalink / raw)
To: Yufen Yu
Cc: Hou Tao, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2019-12-31 23:11 ` Paul E. McKenney
2020-01-01 2:33 ` htbegin
@ 2020-01-03 23:45 ` Joel Fernandes
2020-01-04 9:16 ` Hou Tao
1 sibling, 1 reply; 20+ messages in thread
From: Joel Fernandes @ 2020-01-03 23:45 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Hou Tao, Yufen Yu, axboe, linux-block, ming.lei, hch, zhengchuan,
yi.zhang, rcu
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));
> > >
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-03 23:45 ` Joel Fernandes
@ 2020-01-04 9:16 ` Hou Tao
0 siblings, 0 replies; 20+ messages in thread
From: Hou Tao @ 2020-01-04 9:16 UTC (permalink / raw)
To: Joel Fernandes, Paul E. McKenney
Cc: Yufen Yu, axboe, linux-block, ming.lei, hch, zhengchuan, yi.zhang, rcu
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));
>>>>
>>>
>
> .
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-03 15:16 ` Ming Lei
@ 2020-01-06 7:39 ` Yufen Yu
2020-01-06 8:11 ` Ming Lei
0 siblings, 1 reply; 20+ messages in thread
From: Yufen Yu @ 2020-01-06 7:39 UTC (permalink / raw)
To: Ming Lei
Cc: Hou Tao, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-06 7:39 ` Yufen Yu
@ 2020-01-06 8:11 ` Ming Lei
2020-01-06 9:41 ` Hou Tao
0 siblings, 1 reply; 20+ messages in thread
From: Ming Lei @ 2020-01-06 8:11 UTC (permalink / raw)
To: Yufen Yu
Cc: Hou Tao, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-06 8:11 ` Ming Lei
@ 2020-01-06 9:41 ` Hou Tao
2020-01-06 10:05 ` Ming Lei
0 siblings, 1 reply; 20+ messages in thread
From: Hou Tao @ 2020-01-06 9:41 UTC (permalink / raw)
To: Ming Lei, Yufen Yu
Cc: axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck, joel, rcu
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
>
>
> .
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-06 9:41 ` Hou Tao
@ 2020-01-06 10:05 ` Ming Lei
2020-01-07 11:40 ` Hou Tao
0 siblings, 1 reply; 20+ messages in thread
From: Ming Lei @ 2020-01-06 10:05 UTC (permalink / raw)
To: Hou Tao
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-06 10:05 ` Ming Lei
@ 2020-01-07 11:40 ` Hou Tao
2020-01-08 3:19 ` Ming Lei
0 siblings, 1 reply; 20+ messages in thread
From: Hou Tao @ 2020-01-07 11:40 UTC (permalink / raw)
To: Ming Lei
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
>
>
> .
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] block: make sure last_lookup set as NULL after part deleted
2020-01-07 11:40 ` Hou Tao
@ 2020-01-08 3:19 ` Ming Lei
0 siblings, 0 replies; 20+ messages in thread
From: Ming Lei @ 2020-01-08 3:19 UTC (permalink / raw)
To: Hou Tao
Cc: Yufen Yu, axboe, linux-block, hch, zhengchuan, yi.zhang, paulmck,
joel, rcu
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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-01-08 3:20 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20191231110945.10857-1-yuyufen@huawei.com>
2019-12-31 14:55 ` [PATCH] block: make sure last_lookup set as NULL after part deleted Hou Tao
2019-12-31 23:11 ` Paul E. McKenney
2020-01-01 2:33 ` htbegin
2020-01-01 3:39 ` htbegin
2020-01-03 23:45 ` Joel Fernandes
2020-01-04 9:16 ` Hou Tao
2020-01-02 1:23 ` Ming Lei
2020-01-03 3:06 ` Hou Tao
2020-01-03 4:18 ` Ming Lei
2020-01-03 7:35 ` Hou Tao
2020-01-03 8:17 ` Ming Lei
2020-01-03 12:03 ` Yufen Yu
2020-01-03 15:16 ` Ming Lei
2020-01-06 7:39 ` Yufen Yu
2020-01-06 8:11 ` Ming Lei
2020-01-06 9:41 ` Hou Tao
2020-01-06 10:05 ` Ming Lei
2020-01-07 11:40 ` Hou Tao
2020-01-08 3:19 ` Ming Lei
2020-01-03 12:43 ` Yufen Yu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).