| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 27 Aug 2015 14:47:14 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc: Joonsoo Kim <js1304@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Minchan Kim <minchan@...nel.org>,
Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org,
linux-crypto@...r.kernel.org,
Herbert Xu <herbert@...dor.apana.org.au>,
"David S. Miller" <davem@...emloft.net>,
Stephan Mueller <smueller@...onox.de>,
Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH v2 8/8] zram: use decompress_noctx
On (08/27/15 13:07), Sergey Senozhatsky wrote:
[..]
> > -struct zcomp_strm *zcomp_strm_find(struct zcomp *comp)
> > +struct zcomp_strm *zcomp_strm_find(struct zcomp *comp, bool decomp)
> >
I think we can hide zcomp_strm_find()/_release (make them static),
and instead introduce:
a) zcomp_decompress_begin()/zcomp_decompress_end()
calls zcomp_strm_find()/zcomp_strm_release() for tfms that
require context for decompression; may return NULL, may sleep.
b) zcomp_compress_begin()/zcomp_compress_end()
always calls zcomp_strm_find()/zcomp_strm_release();
never return NULL; may sleep.
-ss
> > {
> > + /* We don't need decompression context so zstrm neither */
> > + if (decomp && test_bit(ZCOMP_DECOMPRESS_NOCTX, &comp->flags))
> > + return NULL;
> > +
> > return comp->strm_find(comp);
> > }
>
> No. zcomp_strm_find() should never return NULL. And no, I don't like
> "if (decomp)" change.
>
>
> >
> > @@ -307,6 +311,11 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > {
> > *dst_len = PAGE_SIZE << 1;
> >
> > + if (!zstrm) {
> > + return crypto_comp_compress_noctx(comp->alg, src, PAGE_SIZE,
> > + dst, dst_len);
> > + }
> > +
> > return crypto_comp_compress(zstrm->tfm, src, PAGE_SIZE, dst, dst_len);
> > }
> >
> > @@ -316,12 +325,18 @@ int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > {
> > unsigned int size = PAGE_SIZE;
> >
> > + if (!zstrm) {
> > + return crypto_comp_decompress_noctx(comp->alg, src, src_len,
> > + dst, &size);
> > + }
> > +
> > return crypto_comp_decompress(zstrm->tfm, src, src_len, dst, &size);
> > }
> >
> > void zcomp_destroy(struct zcomp *comp)
> > {
> > comp->destroy(comp);
> > + crypto_put_comp(comp->alg);
> > kfree(comp);
> > }
> >
> > @@ -344,12 +359,23 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> > return ERR_PTR(-ENOMEM);
> >
> > comp->name = compress;
> > + comp->alg = crypto_get_comp(compress, 0, 0);
> > + if (!comp->alg) {
> > + kfree(comp);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (crypto_has_compress_noctx(comp->alg))
> > + set_bit(ZCOMP_COMPRESS_NOCTX, &comp->flags);
>
> do you use ZCOMP_COMPRESS_NOCTX algs in this patch set?
>
>
> > + if (crypto_has_decompress_noctx(comp->alg))
> > + set_bit(ZCOMP_DECOMPRESS_NOCTX, &comp->flags);
> >
> > if (max_strm > 1)
> > zcomp_strm_multi_create(comp, max_strm);
> > else
> > zcomp_strm_single_create(comp);
> > if (!comp->stream) {
> > + crypto_put_comp(comp->alg);
> > kfree(comp);
> > return ERR_PTR(-ENOMEM);
> > }
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > index c47db4d..6c137c8 100644
> > --- a/drivers/block/zram/zcomp.h
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -13,6 +13,11 @@
> > #include <linux/crypto.h>
> > #include <linux/mutex.h>
> >
> > +enum {
> > + ZCOMP_COMPRESS_NOCTX,
> > + ZCOMP_DECOMPRESS_NOCTX,
> > +};
>
> Can it be handled via crypto api? check if ->foo_noctx() is !NULL ?
>
> > struct zcomp_strm {
> > struct crypto_comp *tfm;
> >
> > @@ -27,6 +32,8 @@ struct zcomp_strm {
> > struct zcomp {
> > void *stream;
> > const char *name;
> > + struct crypto_alg *alg;
> > + unsigned long flags;
> >
> > struct zcomp_strm *(*strm_find)(struct zcomp *comp);
> > void (*strm_release)(struct zcomp *comp, struct zcomp_strm *zstrm);
> > @@ -39,7 +46,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf);
> > struct zcomp *zcomp_create(const char *comp, int max_strm);
> > void zcomp_destroy(struct zcomp *comp);
> >
> > -struct zcomp_strm *zcomp_strm_find(struct zcomp *comp);
> > +struct zcomp_strm *zcomp_strm_find(struct zcomp *comp, bool decomp);
> > void zcomp_strm_release(struct zcomp *comp, struct zcomp_strm *zstrm);
> >
> > int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index b3044d3..8283bd3 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -623,7 +623,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> > /* Use a temporary buffer to decompress the page */
> > uncmem = kmalloc(PAGE_SIZE, GFP_NOIO);
> >
> > - zstrm = zcomp_strm_find(zram->comp);
> > + zstrm = zcomp_strm_find(zram->comp, true);
>
> No, I don't like this.
>
> > user_mem = kmap_atomic(page);
> > if (!is_partial_io(bvec))
> > uncmem = user_mem;
> > @@ -647,7 +647,8 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> > ret = 0;
> > out_cleanup:
> > kunmap_atomic(user_mem);
> > - zcomp_strm_release(zram->comp, zstrm);
> > + if (zstrm)
> > + zcomp_strm_release(zram->comp, zstrm);
> > if (is_partial_io(bvec))
> > kfree(uncmem);
> > return ret;
> > @@ -676,14 +677,14 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > ret = -ENOMEM;
> > goto out;
> > }
> > - zstrm = zcomp_strm_find(zram->comp);
> > + zstrm = zcomp_strm_find(zram->comp, true);
> > ret = zram_decompress_page(zram, zstrm, uncmem, index);
> > if (ret)
> > goto out;
> > }
> >
> > if (!zstrm)
> > - zstrm = zcomp_strm_find(zram->comp);
> > + zstrm = zcomp_strm_find(zram->comp, false);
>
> No. I don't like this.
>
> -ss
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists