| Message ID | d10cdf1d-4a67-48df-b389-3a51f60e9431@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | zram: unsafe zram_get_element call in zram_read_page() | expand |
On (23/11/06 22:54), Vasily Averin wrote: > @@ -1362,14 +1362,14 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index, > ret = zram_read_from_zspool(zram, page, index); > zram_slot_unlock(zram, index); > } else { > + unsigned long entry = zram_get_element(zram, index); > /* > * The slot should be unlocked before reading from the backing > * device. > */ > zram_slot_unlock(zram, index); > > - ret = read_from_bdev(zram, page, zram_get_element(zram, index), > - parent); > + ret = read_from_bdev(zram, page, entry, parent); Hmmm, We may want to do more here. Basically, we probably need to re-confirm after read_from_bdev() that the entry at index still has ZRAM_WB set and, if so, that it points to the same blk_idx. IOW, check that it has not been free-ed and re-used under us. Minchan, what do you think? Is that scenario possible?
On (23/11/07 16:39), Sergey Senozhatsky wrote: > On (23/11/06 22:54), Vasily Averin wrote: > > @@ -1362,14 +1362,14 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index, > > ret = zram_read_from_zspool(zram, page, index); > > zram_slot_unlock(zram, index); > > } else { > > + unsigned long entry = zram_get_element(zram, index); > > /* > > * The slot should be unlocked before reading from the backing > > * device. > > */ > > zram_slot_unlock(zram, index); > > > > - ret = read_from_bdev(zram, page, zram_get_element(zram, index), > > - parent); > > + ret = read_from_bdev(zram, page, entry, parent); > > Hmmm, > We may want to do more here. Basically, we probably need to re-confirm > after read_from_bdev() that the entry at index still has ZRAM_WB set > and, if so, that it points to the same blk_idx. IOW, check that it has > not been free-ed and re-used under us. > > Minchan, what do you think? Is that scenario possible? Basically --- diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index f5e3756fc21a..2d69f6efcee3 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1364,14 +1364,21 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index, ret = zram_read_from_zspool(zram, page, index); zram_slot_unlock(zram, index); } else { + unsigned long idx = zram_get_element(zram, index); /* * The slot should be unlocked before reading from the backing * device. */ zram_slot_unlock(zram, index); - ret = read_from_bdev(zram, page, zram_get_element(zram, index), - parent); + ret = read_from_bdev(zram, page, idx, parent); + if (ret == 0) { + zram_slot_lock(zram, index); + if (!zram_test_flag(zram, index, ZRAM_WB) || + idx != zram_get_element(zram, index)) + ret = -EINVAL; + zram_slot_unlock(zram, index); + } } /* Should NEVER happen. Return bio error if it does. */
On 11/7/23 13:40, Sergey Senozhatsky wrote: > On (23/11/07 16:39), Sergey Senozhatsky wrote: >> Hmmm, >> We may want to do more here. Basically, we probably need to re-confirm >> after read_from_bdev() that the entry at index still has ZRAM_WB set >> and, if so, that it points to the same blk_idx. IOW, check that it has >> not been free-ed and re-used under us. > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -1364,14 +1364,21 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index, > ret = zram_read_from_zspool(zram, page, index); > zram_slot_unlock(zram, index); > } else { > + unsigned long idx = zram_get_element(zram, index); > /* > * The slot should be unlocked before reading from the backing > * device. > */ > zram_slot_unlock(zram, index); > > - ret = read_from_bdev(zram, page, zram_get_element(zram, index), > - parent); > + ret = read_from_bdev(zram, page, idx, parent); > + if (ret == 0) { > + zram_slot_lock(zram, index); > + if (!zram_test_flag(zram, index, ZRAM_WB) || > + idx != zram_get_element(zram, index)) > + ret = -EINVAL; > + zram_slot_unlock(zram, index); > + } Why overwritten page can not be pushed to WB to the same blk_idx? However I'm agree that this is VERY unlikely case, and this check is better than nothing.
On (23/11/07 21:19), Vasily Averin wrote: > On 11/7/23 13:40, Sergey Senozhatsky wrote: > > On (23/11/07 16:39), Sergey Senozhatsky wrote: > >> Hmmm, > >> We may want to do more here. Basically, we probably need to re-confirm > >> after read_from_bdev() that the entry at index still has ZRAM_WB set > >> and, if so, that it points to the same blk_idx. IOW, check that it has > >> not been free-ed and re-used under us. > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -1364,14 +1364,21 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index, > > ret = zram_read_from_zspool(zram, page, index); > > zram_slot_unlock(zram, index); > > } else { > > + unsigned long idx = zram_get_element(zram, index); > > /* > > * The slot should be unlocked before reading from the backing > > * device. > > */ > > zram_slot_unlock(zram, index); > > > > - ret = read_from_bdev(zram, page, zram_get_element(zram, index), > > - parent); > > + ret = read_from_bdev(zram, page, idx, parent); > > + if (ret == 0) { > > + zram_slot_lock(zram, index); > > + if (!zram_test_flag(zram, index, ZRAM_WB) || > > + idx != zram_get_element(zram, index)) > > + ret = -EINVAL; > > + zram_slot_unlock(zram, index); > > + } > > Why overwritten page can not be pushed to WB to the same blk_idx? Yeah, so I thought about it too but didn't want to go too deep into it. We probably can only address it if we synchronize free_page (?), read_page() and writeback(), so that we never have concurrent bitmap modifications when one of the operations is in progress.
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index d77d3664ca08..9ac3d4e51d26 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1362,14 +1362,14 @@ static int zram_read_page(struct zram *zram, struct page *page, u32 index, ret = zram_read_from_zspool(zram, page, index); zram_slot_unlock(zram, index); } else { + unsigned long entry = zram_get_element(zram, index); /* * The slot should be unlocked before reading from the backing * device. */ zram_slot_unlock(zram, index); - ret = read_from_bdev(zram, page, zram_get_element(zram, index), - parent); + ret = read_from_bdev(zram, page, entry, parent); } /* Should NEVER happen. Return bio error if it does. */
Taken lock is required to access the content of zram entry. Fixes: 8e19d540d107 ("zram: extend zero pages to same element pages") Signed-off-by: Vasily Averin <vasily.averin@linux.dev> --- drivers/block/zram/zram_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)