| Message ID | 20240318230013.87543-1-21cnbao@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: zswap: fix kernel BUG in sg_init_one | expand |
On Mon, Mar 18, 2024 at 4:12 PM Barry Song <21cnbao@gmail.com> wrote: > > On Tue, Mar 19, 2024 at 12:06 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > On Mon, Mar 18, 2024 at 4:00 PM Barry Song <21cnbao@gmail.com> wrote: > > > > > > From: Barry Song <v-songbaohua@oppo.com> > > > > > > sg_init_one() relies on linearly mapped low memory for the safe > > > utilization of virt_to_page(). Consequently, we have two choices: > > > either employ kmap_to_page() alongside sg_set_page(), or resort to > > > copying high memory contents to a temporary buffer residing in low > > > memory. However, considering the introduction of the WARN_ON_ONCE > > > in commit ef6e06b2ef870 ("highmem: fix kmap_to_page() for > > > kmap_local_page() addresses"), which specifically addresses high > > > memory concerns, it appears that memcpy remains the sole viable > > > option. > > > > > > Reported-and-tested-by: syzbot+adbc983a1588b7805de3@syzkaller.appspotmail.com > > > Closes: https://lore.kernel.org/all/000000000000bbb3d80613f243a6@google.com/ > > > Fixes: 270700dd06ca ("mm/zswap: remove the memcpy if acomp is not sleepable") > > > Signed-off-by: Barry Song <v-songbaohua@oppo.com> > > > --- > > > mm/zswap.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > index 9dec853647c8..17bf6d87b274 100644 > > > --- a/mm/zswap.c > > > +++ b/mm/zswap.c > > > @@ -1080,7 +1080,8 @@ static void zswap_decompress(struct zswap_entry *entry, struct page *page) > > > mutex_lock(&acomp_ctx->mutex); > > > > > > src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); > > > - if (acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) { > > > + if ((acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) || > > > + !virt_addr_valid(src)) { > > > > > > Would it be better to explicitly check is_kmap_addr() here? I am > > particularly worried about hiding a bug where the returned address > > from zpool_map_handle() is not a kmap address, but also not a valid > > linear mapping address. > > > > If we use is_kmap_addr() here, then the virt_addr_valid() check in > > sg_init_one() will catch any non-kmap non-linear mapping addresses. > > WDYT? Am I being paranoid? :) > > we have a possibility that a userspace buffer or vmalloc address is given to > sg_init_one, then it is non-kmap non-linear. but is it possible someday some > people return a vmalloc/vmap address from zpool_map_handle() in the future? > then we still need !virt_addr_valid(). Hmm I guess if zpool_map_handle() ever returns a vmalloc/vmap address we would need to copy the data to the buffer in zswap code, at least until we enlighten the code to deal with them directly. So I guess we can keep virt_addr_valid(). Ideally the comment will clarify this. Thanks!
diff --git a/mm/zswap.c b/mm/zswap.c index 9dec853647c8..17bf6d87b274 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1080,7 +1080,8 @@ static void zswap_decompress(struct zswap_entry *entry, struct page *page) mutex_lock(&acomp_ctx->mutex); src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); - if (acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) { + if ((acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) || + !virt_addr_valid(src)) { memcpy(acomp_ctx->buffer, src, entry->length); src = acomp_ctx->buffer; zpool_unmap_handle(zpool, entry->handle); @@ -1094,7 +1095,7 @@ static void zswap_decompress(struct zswap_entry *entry, struct page *page) BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE); mutex_unlock(&acomp_ctx->mutex); - if (!acomp_ctx->is_sleepable || zpool_can_sleep_mapped(zpool)) + if (src != acomp_ctx->buffer) zpool_unmap_handle(zpool, entry->handle); }