On Thu, 2 Aug 2018 14:11:12 +0900 Minchan Kim minchan@kernel.org wrote:
If zram supports writeback feature, it's no more syncrhonous device beause zram does synchronous IO opeation for incompressible page.
Do not pretend to be syncrhonous IO device. It makes system very sluggish as waiting IO completion from upper layer.
Furthermore, it makes user-after-free problem because swap think the opearion is done when the IO functions returns so it could free page by will(e.g., lock_page_or_retry and goto out_release in do_swap_page) but in fact, IO is asynchrnous so driver could access just freed page afterward.
This patch fixes the problem.
That changelog is rather hard to follow. Please review my edits:
: If zram supports writeback feature, it's no longer a BD_CAP_SYNCHRONOUS_IO : device beause zram does synchronous IO operations for incompressible pages. : : Do not pretend to be synchronous IO device. It makes the system very : sluggish due to waiting for IO completion from upper layers. : : Furthermore, it causes a user-after-free problem because swap thinks the : opearion is done when the IO functions returns so it can free the page : (e.g., lock_page_or_retry and goto out_release in do_swap_page) but in : fact, IO is asynchrnous so the driver could access a just freed page : afterward. : : This patch fixes the problem.
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 7436b2d27fa3..0b6eda1bd77a 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -298,7 +298,8 @@ static void reset_bdev(struct zram *zram) zram->backing_dev = NULL; zram->old_block_size = 0; zram->bdev = NULL;
- zram->disk->queue->backing_dev_info->capabilities |=
BDI_CAP_SYNCHRONOUS_IO;} @@ -400,6 +401,8 @@ static ssize_t backing_dev_store(struct device *dev, zram->backing_dev = backing_dev; zram->bitmap = bitmap; zram->nr_pages = nr_pages;
- zram->disk->queue->backing_dev_info->capabilities &=
~BDI_CAP_SYNCHRONOUS_IO;pr_info("setup backing device %s\n", file_name);
A reader looking at this would wonder "why the heck are we doing that". Adding a code comment would help them.
Is it legitimate to be altering the bdi capabilities at this level? Or is this hacky?
If "yes" then should reset_bdev() be unconditionally setting BDI_CAP_SYNCHRONOUS_IO? Shouldn't it be restoring that flag to its previous setting?