Re: [syzbot] [crypto?] general protection fault in scatterwalk_copychunks (5)

[Nhat Pham]

On Wed, Dec 27, 2023 at 5:47 PM Barry Song <21cnbao@xxxxxxxxx> wrote:
>
> On Thu, Dec 28, 2023 at 9:43 AM Barry Song <21cnbao@xxxxxxxxx> wrote:
> >
> > On Thu, Dec 28, 2023 at 7:26 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> > >
> > > On Wed, Dec 27, 2023 at 3:10 AM Barry Song <21cnbao@xxxxxxxxx> wrote:
> > > >
> > > > On Wed, Dec 27, 2023 at 5:16 PM Chengming Zhou
> > > > <zhouchengming@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > Thanks for your explanation! Maybe it's best for us to return to 2 pages
> > > > > if no other people's comments. And this really need more documentation :-)
> > >
> > > Fine by me. Hmm we're basically wasting one extra page per CPU (since
> > > these buffers are per-CPU), correct? That's not ideal, but not *too*
> > > bad for now I suppose...
> > >
> > > >
> > > > I agree. we need some doc.
> > > >
> > > > besides, i actually think we can skip zswap frontend if
> > > > over-compression is really
> > > > happening.
> > >
> > > IIUC, zsmalloc already checked that - and most people are (or should
> > > be) using zsmalloc for zswap anyway. I wouldn't be opposed to adding
> > > an added layer of protection on the zswap side, but not super high
> > > priority I'd say.
> >
> > Thanks for this info. I guess you mean the below ?
> > unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp)
> > {
> >         ...
> >
> >         if (unlikely(!size || size > ZS_MAX_ALLOC_SIZE))
> >                 return (unsigned long)ERR_PTR(-EINVAL);
>
> BTW, do you think zsmalloc is worth a patch to change the ret from
> EINVAL to ENOSPC?
> This seems more sensible and matches the behaviour of zswap, and zbud, z3fold.
>
>         ret = zpool_malloc(zpool, dlen, gfp, &handle);
>         if (ret == -ENOSPC) {
>                 zswap_reject_compress_poor++;
>                 goto put_dstmem;
>         }
>         if (ret) {
>                 zswap_reject_alloc_fail++;
>                 goto put_dstmem;
>         }
>         buf = z
>

I haven't looked at the code surrounding it too closely, but IMHO this
would be nice. Poor compressibility of the workload's memory is
something we should monitor more carefully. This has come up a couple
times in discussion:

https://lore.kernel.org/linux-mm/CAJD7tkZXS-UJVAFfvxJ0nNgTzWBiqepPYA4hEozi01_qktkitg@xxxxxxxxxxxxxx/
https://lore.kernel.org/all/20231024234509.2680539-1-nphamcs@xxxxxxxxx/T/#u

> >
> > }
> >
> > i find zbud also has similar code:
> > static int zbud_alloc(struct zbud_pool *pool, size_t size, gfp_t gfp,
> >                         unsigned long *handle)
> > {
> >         int chunks, i, freechunks;
> >         struct zbud_header *zhdr = NULL;
> >         enum buddy bud;
> >         struct page *page;
> >
> >         if (!size || (gfp & __GFP_HIGHMEM))
> >                 return -EINVAL;
> >         if (size > PAGE_SIZE - ZHDR_SIZE_ALIGNED - CHUNK_SIZE)
> >                 return -ENOSPC;
> >
> > and z3fold,
> >
> > static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> >                         unsigned long *handle)
> > {
> >         int chunks = size_to_chunks(size);
> >         struct z3fold_header *zhdr = NULL;
> >         struct page *page = NULL;
> >         enum buddy bud;
> >         bool can_sleep = gfpflags_allow_blocking(gfp);
> >
> >         if (!size || (gfp & __GFP_HIGHMEM))
> >                 return -EINVAL;
> >
> >         if (size > PAGE_SIZE)
> >                 return -ENOSPC;
> >
> >
> > Thus, I agree that another layer to check size in zswap isn't necessary now.
>
> Thanks
> Barry