| Message ID | 20200508044407.1371907-5-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: fix partition use-after-free and optimization | expand |
On Fri, May 08, 2020 at 12:44:07PM +0800, Ming Lei wrote: > gendisk can't be gone when there is IO activity, so not hold > part0's refcount in IO path. > > Cc: Yufen Yu <yuyufen@huawei.com> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Hou Tao <houtao1@huawei.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> This looks correct, although I'd still prefer to centralize the partno checks in the helpers. Also hd_struct_get is unused with this patch isn't it?
On Thu, May 07, 2020 at 11:41:33PM -0700, Christoph Hellwig wrote: > On Fri, May 08, 2020 at 12:44:07PM +0800, Ming Lei wrote: > > gendisk can't be gone when there is IO activity, so not hold > > part0's refcount in IO path. > > > > Cc: Yufen Yu <yuyufen@huawei.com> > > Cc: Christoph Hellwig <hch@infradead.org> > > Cc: Hou Tao <houtao1@huawei.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > This looks correct, although I'd still prefer to centralize the > partno checks in the helpers. Also hd_struct_get is unused with > this patch isn't it? OK, are you fine with the following patch? diff --git a/block/blk.h b/block/blk.h index 133fb0b99759..8efd1ca5c975 100644 --- a/block/blk.h +++ b/block/blk.h @@ -376,19 +376,18 @@ int bdev_resize_partition(struct block_device *bdev, int partno, int disk_expand_part_tbl(struct gendisk *disk, int target); int hd_ref_init(struct hd_struct *part); -static inline void hd_struct_get(struct hd_struct *part) -{ - percpu_ref_get(&part->ref); -} - +/* no need to get/put refcount of part0 */ static inline int hd_struct_try_get(struct hd_struct *part) { - return percpu_ref_tryget_live(&part->ref); + if (part->partno) + return percpu_ref_tryget_live(&part->ref); + return 1; } static inline void hd_struct_put(struct hd_struct *part) { - percpu_ref_put(&part->ref); + if (part->partno) + percpu_ref_put(&part->ref); } static inline void hd_free_part(struct hd_struct *part) diff --git a/block/genhd.c b/block/genhd.c index bf8cbb033d64..d97b95d1a2fd 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -345,7 +345,8 @@ static inline int sector_in_part(struct hd_struct *part, sector_t sector) * * CONTEXT: * RCU read locked. The returned partition pointer is always valid - * because its refcount is grabbed. + * because its refcount is grabbed except for part0, which lifetime + * is same with the disk. * * RETURNS: * Found partition on success, part0 is returned if no partition matches @@ -378,7 +379,6 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector) return part; } } - hd_struct_get(&disk->part0); return &disk->part0; } Thanks, Ming
On Fri, May 08, 2020 at 03:41:57PM +0800, Ming Lei wrote: > On Thu, May 07, 2020 at 11:41:33PM -0700, Christoph Hellwig wrote: > > On Fri, May 08, 2020 at 12:44:07PM +0800, Ming Lei wrote: > > > gendisk can't be gone when there is IO activity, so not hold > > > part0's refcount in IO path. > > > > > > Cc: Yufen Yu <yuyufen@huawei.com> > > > Cc: Christoph Hellwig <hch@infradead.org> > > > Cc: Hou Tao <houtao1@huawei.com> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > > > This looks correct, although I'd still prefer to centralize the > > partno checks in the helpers. Also hd_struct_get is unused with > > this patch isn't it? > > OK, are you fine with the following patch? Yes, this looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/block/blk-core.c b/block/blk-core.c index 826a8980997d..56cc61354671 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1343,7 +1343,8 @@ void blk_account_io_done(struct request *req, u64 now) part_stat_add(part, nsecs[sgrp], now - req->start_time_ns); part_dec_in_flight(req->q, part, rq_data_dir(req)); - hd_struct_put(part); + if (part->partno) + hd_struct_put(part); part_stat_unlock(); } } diff --git a/block/blk-merge.c b/block/blk-merge.c index a04e991b5ded..b44caa28ec04 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -672,7 +672,8 @@ static void blk_account_io_merge(struct request *req) part_dec_in_flight(req->q, part, rq_data_dir(req)); - hd_struct_put(part); + if (part->partno) + hd_struct_put(part); part_stat_unlock(); } } diff --git a/block/genhd.c b/block/genhd.c index bf8cbb033d64..d97b95d1a2fd 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -345,7 +345,8 @@ static inline int sector_in_part(struct hd_struct *part, sector_t sector) * * CONTEXT: * RCU read locked. The returned partition pointer is always valid - * because its refcount is grabbed. + * because its refcount is grabbed except for part0, which lifetime + * is same with the disk. * * RETURNS: * Found partition on success, part0 is returned if no partition matches @@ -378,7 +379,6 @@ struct hd_struct *disk_map_sector_rcu(struct gendisk *disk, sector_t sector) return part; } } - hd_struct_get(&disk->part0); return &disk->part0; }
gendisk can't be gone when there is IO activity, so not hold part0's refcount in IO path. Cc: Yufen Yu <yuyufen@huawei.com> Cc: Christoph Hellwig <hch@infradead.org> Cc: Hou Tao <houtao1@huawei.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-core.c | 3 ++- block/blk-merge.c | 3 ++- block/genhd.c | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-)