Re: [PATCH 15/16] zram: fix synchronous reads

[Minchan Kim <minchan@kernel.org>]

On Thu, Apr 06, 2023 at 03:58:10PM -0700, Andrew Morton wrote:
> On Thu, 6 Apr 2023 15:05:22 -0700 Minchan Kim <minchan@kernel.org> wrote:
> 
> > On Thu, Apr 06, 2023 at 04:41:01PM +0200, Christoph Hellwig wrote:
> > > Currently nothing waits for the synchronous reads before accessing
> > > the data.  Switch them to an on-stack bio and submit_bio_wait to
> > > make sure the I/O has actually completed when the work item has been
> > > flushed.  This also removes the call to page_endio that would unlock
> > > a page that has never been locked.
> > > 
> > > Drop the partial_io/sync flag, as chaining only makes sense for the
> > > asynchronous reads of the entire page.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Acked-by: Minchan Kim <minchan@kernel.org>
> > 
> > So this fixes zram_rw_page + CONFIG_ZRAM_WRITEBACK feature on
> > ppc some arch where PAGE_SIZE is not 4K.
> > 
> > IIRC, we didn't have any report since the writeback feature was
> > introduced. Then, we may skip having the fix into stable?
> 
> Someone may develop such a use case in the future.  And backporting
> this fix will be difficult, unless people backport all the other
> patches, which is also difficult.

I think the simple fix is just bail out for partial IO case from
rw_page path so that bio comes next to serve the rw_page failure.
In the case, zram will always do chained bio so we are fine with
asynchronous IO.

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index b8549c61ff2c..23fa0e03cdc1 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1264,6 +1264,8 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
                struct bio_vec bvec;

                zram_slot_unlock(zram, index);
+               if (partial_io)
+                       return -EAGAIN;

                bvec.bv_page = page;
                bvec.bv_len = PAGE_SIZE;

> 
> What are the user-visible effects of this bug?  It sounds like it will
> give userspace access to unintialized kernel memory, which isn't good.

It's true.

Without better suggestion or objections, I could cook the stable patch.