| Message ID | 1442553564-3476-2-git-send-email-iamjoonsoo.kim@lge.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On (09/18/15 14:19), Joonsoo Kim wrote: [..] > @@ -61,7 +61,8 @@ static struct crypto_alg alg = { > .cra_module = THIS_MODULE, > .cra_u = { .compress = { > .coa_compress = crypto842_compress, > - .coa_decompress = crypto842_decompress } } > + .coa_decompress = crypto842_decompress, > + .coa_decompress_noctx = NULL } } > }; > > static int __init crypto842_mod_init(void) > diff --git a/crypto/compress.c b/crypto/compress.c > index c33f076..abb36a8 100644 > --- a/crypto/compress.c > +++ b/crypto/compress.c > @@ -33,12 +33,21 @@ static int crypto_decompress(struct crypto_tfm *tfm, > dlen); > } > > +static int crypto_decompress_noctx(struct crypto_tfm *tfm, > + const u8 *src, unsigned int slen, > + u8 *dst, unsigned int *dlen) > +{ > + return tfm->__crt_alg->cra_compress.coa_decompress_noctx(src, slen, > + dst, dlen); > +} hm... well... sorry, if irrelevant. if the algorithm can have a _noctx() decompression function, does it automatically guarantee that this algorithm never dereferences a passed `struct crypto_tfm *tfm' pointer in its decompress function? in other words, can we simply pass that `shared tfm pointer' to the existing decompress function instead of defining new symbols, new callbacks, etc.? cot_decompress_noctx() == cot_decompress(shared_ftm) ? just a thought. [..] > +struct crypto_comp *crypto_alloc_comp_noctx(const char *alg_name, > + u32 type, u32 mask) > +{ > + struct crypto_comp *comp; > + struct crypto_tfm *tfm; > + struct compress_tfm *ops; > + > + comp = crypto_alloc_comp(alg_name, type, mask); > + if (IS_ERR(comp)) > + return comp; > + > + tfm = crypto_comp_tfm(comp); > + if (!tfm->__crt_alg->cra_compress.coa_decompress_noctx) { > + crypto_free_comp(comp); > + return ERR_PTR(-EINVAL); -ENOMEM? -ss -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On (09/18/15 14:19), Joonsoo Kim wrote: [..] > static int __init lzo_mod_init(void) > diff --git a/include/linux/crypto.h b/include/linux/crypto.h > index e71cb70..31152b1 100644 > --- a/include/linux/crypto.h > +++ b/include/linux/crypto.h > @@ -355,6 +355,8 @@ struct compress_alg { > unsigned int slen, u8 *dst, unsigned int *dlen); > int (*coa_decompress)(struct crypto_tfm *tfm, const u8 *src, > unsigned int slen, u8 *dst, unsigned int *dlen); > + int (*coa_decompress_noctx)(const u8 *src, unsigned int slen, > + u8 *dst, unsigned int *dlen); > }; > > > @@ -538,6 +540,9 @@ struct compress_tfm { > int (*cot_decompress)(struct crypto_tfm *tfm, > const u8 *src, unsigned int slen, > u8 *dst, unsigned int *dlen); > + int (*cot_decompress_noctx)(struct crypto_tfm *tfm, > + const u8 *src, unsigned int slen, > + u8 *dst, unsigned int *dlen); > }; > > #define crt_ablkcipher crt_u.ablkcipher > @@ -1836,6 +1841,14 @@ static inline void crypto_free_comp(struct crypto_comp *tfm) > crypto_free_tfm(crypto_comp_tfm(tfm)); > } > > +struct crypto_comp *crypto_alloc_comp_noctx(const char *alg_name, > + u32 type, u32 mask); > + this should be EXPORT_SYMBOL_GPL(). -ss -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 18, 2015 at 02:19:16PM +0900, Joonsoo Kim wrote: > Until now, tfm object embeds (de)compression context in it and > (de)compression in crypto API requires tfm object to use > this context. But, there are some algorithms that doesn't need > such context to operate. Therefore, this patch introduce new crypto > decompression API that calls decompression function via sharable tfm > object. Concurrent calls to decompress_noctx function through sharable > tfm object will be okay because caller don't need any context in tfm and > tfm is only used for fetching function pointer to decompress_noctx > function. This can reduce overhead of maintaining multiple tfm > if decompression doesn't require any context to operate. > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> Have you looked into include/crypto/compress.h? The compress type itself is obsolete and should be replaced with the pcomp interface. However, we made some mistakes with the pcomp interface. First of all it doesn't have a non-partial interface like the digest function for crypto_shash. But the biggest problem is that it should be completely stateless but is not. IOW we should not store anything in the tfm that is per-request. That should all go into the request structure. Fortunately it seems that pcomp doesn't actually have any users so we can just rip it out and start from scratch and redo it properly. So to recap, please abandon any efforts on the crypto_compress interface as it is old and obsolete. Instead reshape crypto_pcomp into a proper stateless interface which should then give you what you want. When you do so, just keep in mind that we need to find a way to support IPComp. That means the ability to allocate requests in thread context and then use them to compress/decompress in IRQ context. Cheers,
On Tue, Sep 22, 2015 at 08:43:46PM +0800, Herbert Xu wrote: > On Fri, Sep 18, 2015 at 02:19:16PM +0900, Joonsoo Kim wrote: > > Until now, tfm object embeds (de)compression context in it and > > (de)compression in crypto API requires tfm object to use > > this context. But, there are some algorithms that doesn't need > > such context to operate. Therefore, this patch introduce new crypto > > decompression API that calls decompression function via sharable tfm > > object. Concurrent calls to decompress_noctx function through sharable > > tfm object will be okay because caller don't need any context in tfm and > > tfm is only used for fetching function pointer to decompress_noctx > > function. This can reduce overhead of maintaining multiple tfm > > if decompression doesn't require any context to operate. > > > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > Have you looked into include/crypto/compress.h? Okay. Now, I have looked it. > > The compress type itself is obsolete and should be replaced with > the pcomp interface. > > However, we made some mistakes with the pcomp interface. First > of all it doesn't have a non-partial interface like the digest > function for crypto_shash. > > But the biggest problem is that it should be completely stateless > but is not. IOW we should not store anything in the tfm that > is per-request. That should all go into the request structure. I'm not a expert on this area but I have different opinion. IIUC, what partial compression aims at is to support partial data compression. It isn't for stateless compression and they are different. Current implementation would work well for it's own purpose. And, making it stateless looks not good to me. If we make it stateless, we should include algorithm's state in request object. It enforces much memory to request object and, unlike crypto_shash which can be allocated in stack, user needs to manage life time of request objects and it requires additional complexity to compression user. Moreover, to use sharable tfm concurrently from my original intend, many request objects would be needed and if it cannot be allocated in the stack, it requires much memory. It's not suitable for my purpose. If compression is obsolete, what I think the best is leaving pcomp as is and implementing sharable tfm in pcomp, And then, make lzo and others support pcomp interface and sharable tfm. Maybe, I'm wrong so please correct me. Thanks. > Fortunately it seems that pcomp doesn't actually have any users > so we can just rip it out and start from scratch and redo it > properly. > > So to recap, please abandon any efforts on the crypto_compress > interface as it is old and obsolete. Instead reshape crypto_pcomp > into a proper stateless interface which should then give you what > you want. > > When you do so, just keep in mind that we need to find a way to > support IPComp. That means the ability to allocate requests in > thread context and then use them to compress/decompress in IRQ > context. > > Cheers, > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 21, 2015 at 03:18:17PM +0900, Sergey Senozhatsky wrote: > On (09/18/15 14:19), Joonsoo Kim wrote: > [..] > > static int __init lzo_mod_init(void) > > diff --git a/include/linux/crypto.h b/include/linux/crypto.h > > index e71cb70..31152b1 100644 > > --- a/include/linux/crypto.h > > +++ b/include/linux/crypto.h > > @@ -355,6 +355,8 @@ struct compress_alg { > > unsigned int slen, u8 *dst, unsigned int *dlen); > > int (*coa_decompress)(struct crypto_tfm *tfm, const u8 *src, > > unsigned int slen, u8 *dst, unsigned int *dlen); > > + int (*coa_decompress_noctx)(const u8 *src, unsigned int slen, > > + u8 *dst, unsigned int *dlen); > > }; > > > > > > @@ -538,6 +540,9 @@ struct compress_tfm { > > int (*cot_decompress)(struct crypto_tfm *tfm, > > const u8 *src, unsigned int slen, > > u8 *dst, unsigned int *dlen); > > + int (*cot_decompress_noctx)(struct crypto_tfm *tfm, > > + const u8 *src, unsigned int slen, > > + u8 *dst, unsigned int *dlen); > > }; > > > > #define crt_ablkcipher crt_u.ablkcipher > > @@ -1836,6 +1841,14 @@ static inline void crypto_free_comp(struct crypto_comp *tfm) > > crypto_free_tfm(crypto_comp_tfm(tfm)); > > } > > > > +struct crypto_comp *crypto_alloc_comp_noctx(const char *alg_name, > > + u32 type, u32 mask); > > + > > this should be EXPORT_SYMBOL_GPL(). > Will do in next version. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 21, 2015 at 02:38:59PM +0900, Sergey Senozhatsky wrote: > On (09/18/15 14:19), Joonsoo Kim wrote: > [..] > > @@ -61,7 +61,8 @@ static struct crypto_alg alg = { > > .cra_module = THIS_MODULE, > > .cra_u = { .compress = { > > .coa_compress = crypto842_compress, > > - .coa_decompress = crypto842_decompress } } > > + .coa_decompress = crypto842_decompress, > > + .coa_decompress_noctx = NULL } } > > }; > > > > static int __init crypto842_mod_init(void) > > diff --git a/crypto/compress.c b/crypto/compress.c > > index c33f076..abb36a8 100644 > > --- a/crypto/compress.c > > +++ b/crypto/compress.c > > @@ -33,12 +33,21 @@ static int crypto_decompress(struct crypto_tfm *tfm, > > dlen); > > } > > > > +static int crypto_decompress_noctx(struct crypto_tfm *tfm, > > + const u8 *src, unsigned int slen, > > + u8 *dst, unsigned int *dlen) > > +{ > > + return tfm->__crt_alg->cra_compress.coa_decompress_noctx(src, slen, > > + dst, dlen); > > +} > > > hm... well... sorry, if irrelevant. > if the algorithm can have a _noctx() decompression function, does it > automatically guarantee that this algorithm never dereferences a passed > `struct crypto_tfm *tfm' pointer in its decompress function? in other words, > can we simply pass that `shared tfm pointer' to the existing decompress > function instead of defining new symbols, new callbacks, etc.? > > cot_decompress_noctx() == cot_decompress(shared_ftm) ? > > just a thought. Will think it if I implement next version in this way. Due to Herbert comment, interface will be changed so much in next version. > > [..] > > +struct crypto_comp *crypto_alloc_comp_noctx(const char *alg_name, > > + u32 type, u32 mask) > > +{ > > + struct crypto_comp *comp; > > + struct crypto_tfm *tfm; > > + struct compress_tfm *ops; > > + > > + comp = crypto_alloc_comp(alg_name, type, mask); > > + if (IS_ERR(comp)) > > + return comp; > > + > > + tfm = crypto_comp_tfm(comp); > > + if (!tfm->__crt_alg->cra_compress.coa_decompress_noctx) { > > + crypto_free_comp(comp); > > + return ERR_PTR(-EINVAL); > > -ENOMEM? No, it's failure isn't related to NOMEM. Just not support it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On (09/25/15 14:26), Joonsoo Kim wrote: [..] > > > +struct crypto_comp *crypto_alloc_comp_noctx(const char *alg_name, > > > + u32 type, u32 mask); > > > + > > > > this should be EXPORT_SYMBOL_GPL(). > > > > Will do in next version. > so you want to go with _noctx() callbacks implementation? that CRYPTO_ALG_TFM_MAY_SHARE flag looks quite simple to me. or you guys hate it? -ss -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 25, 2015 at 04:56:10PM +0900, Sergey Senozhatsky wrote: > > so you want to go with _noctx() callbacks implementation? > that CRYPTO_ALG_TFM_MAY_SHARE flag looks quite simple to > me. or you guys hate it? I think we should just replace crypto_pcomp with a new interface that does what you guys want. The current crypto_compress interface is simply broken because it stores per-request state in the tfm. This runs counter to every other crypto type, e.g., hash or aead. The tfm should only hold shared data, e.g., compression algorithm parameters but not per-request state. As the original pcomp author has disappeared I think you could even drop the partial stuff and just do a straight compression interface. Cheers,
diff --git a/crypto/842.c b/crypto/842.c index 98e387e..1b6cdab 100644 --- a/crypto/842.c +++ b/crypto/842.c @@ -61,7 +61,8 @@ static struct crypto_alg alg = { .cra_module = THIS_MODULE, .cra_u = { .compress = { .coa_compress = crypto842_compress, - .coa_decompress = crypto842_decompress } } + .coa_decompress = crypto842_decompress, + .coa_decompress_noctx = NULL } } }; static int __init crypto842_mod_init(void) diff --git a/crypto/compress.c b/crypto/compress.c index c33f076..abb36a8 100644 --- a/crypto/compress.c +++ b/crypto/compress.c @@ -33,12 +33,21 @@ static int crypto_decompress(struct crypto_tfm *tfm, dlen); } +static int crypto_decompress_noctx(struct crypto_tfm *tfm, + const u8 *src, unsigned int slen, + u8 *dst, unsigned int *dlen) +{ + return tfm->__crt_alg->cra_compress.coa_decompress_noctx(src, slen, + dst, dlen); +} + int crypto_init_compress_ops(struct crypto_tfm *tfm) { struct compress_tfm *ops = &tfm->crt_compress; ops->cot_compress = crypto_compress; ops->cot_decompress = crypto_decompress; + ops->cot_decompress_noctx = NULL; return 0; } @@ -46,3 +55,30 @@ int crypto_init_compress_ops(struct crypto_tfm *tfm) void crypto_exit_compress_ops(struct crypto_tfm *tfm) { } + +struct crypto_comp *crypto_alloc_comp_noctx(const char *alg_name, + u32 type, u32 mask) +{ + struct crypto_comp *comp; + struct crypto_tfm *tfm; + struct compress_tfm *ops; + + comp = crypto_alloc_comp(alg_name, type, mask); + if (IS_ERR(comp)) + return comp; + + tfm = crypto_comp_tfm(comp); + if (!tfm->__crt_alg->cra_compress.coa_decompress_noctx) { + crypto_free_comp(comp); + return ERR_PTR(-EINVAL); + } + + ops = &tfm->crt_compress; + + /* Only allow noctx ops to comp_noctx */ + ops->cot_compress = NULL; + ops->cot_decompress = NULL; + ops->cot_decompress_noctx = crypto_decompress_noctx; + + return comp; +} diff --git a/crypto/crypto_null.c b/crypto/crypto_null.c index 941c9a4..3560b75 100644 --- a/crypto/crypto_null.c +++ b/crypto/crypto_null.c @@ -146,7 +146,8 @@ static struct crypto_alg null_algs[3] = { { .cra_module = THIS_MODULE, .cra_u = { .compress = { .coa_compress = null_compress, - .coa_decompress = null_compress } } + .coa_decompress = null_compress, + .coa_decompress_noctx = NULL } } } }; MODULE_ALIAS_CRYPTO("compress_null"); diff --git a/crypto/deflate.c b/crypto/deflate.c index 95d8d37..c0b0a40 100644 --- a/crypto/deflate.c +++ b/crypto/deflate.c @@ -203,7 +203,8 @@ static struct crypto_alg alg = { .cra_exit = deflate_exit, .cra_u = { .compress = { .coa_compress = deflate_compress, - .coa_decompress = deflate_decompress } } + .coa_decompress = deflate_decompress, + .coa_decompress_noctx = NULL } } }; static int __init deflate_mod_init(void) diff --git a/crypto/lz4.c b/crypto/lz4.c index aefbcea..d38ce2a 100644 --- a/crypto/lz4.c +++ b/crypto/lz4.c @@ -86,7 +86,8 @@ static struct crypto_alg alg_lz4 = { .cra_exit = lz4_exit, .cra_u = { .compress = { .coa_compress = lz4_compress_crypto, - .coa_decompress = lz4_decompress_crypto } } + .coa_decompress = lz4_decompress_crypto, + .coa_decompress_noctx = NULL } } }; static int __init lz4_mod_init(void) diff --git a/crypto/lz4hc.c b/crypto/lz4hc.c index a1d3b5b..0cb38a7 100644 --- a/crypto/lz4hc.c +++ b/crypto/lz4hc.c @@ -86,7 +86,8 @@ static struct crypto_alg alg_lz4hc = { .cra_exit = lz4hc_exit, .cra_u = { .compress = { .coa_compress = lz4hc_compress_crypto, - .coa_decompress = lz4hc_decompress_crypto } } + .coa_decompress = lz4hc_decompress_crypto, + .coa_decompress_noctx = NULL } } }; static int __init lz4hc_mod_init(void) diff --git a/crypto/lzo.c b/crypto/lzo.c index 4b3e925..ec0f7b3 100644 --- a/crypto/lzo.c +++ b/crypto/lzo.c @@ -89,7 +89,8 @@ static struct crypto_alg alg = { .cra_exit = lzo_exit, .cra_u = { .compress = { .coa_compress = lzo_compress, - .coa_decompress = lzo_decompress } } + .coa_decompress = lzo_decompress, + .coa_decompress_noctx = NULL } } }; static int __init lzo_mod_init(void) diff --git a/include/linux/crypto.h b/include/linux/crypto.h index e71cb70..31152b1 100644 --- a/include/linux/crypto.h +++ b/include/linux/crypto.h @@ -355,6 +355,8 @@ struct compress_alg { unsigned int slen, u8 *dst, unsigned int *dlen); int (*coa_decompress)(struct crypto_tfm *tfm, const u8 *src, unsigned int slen, u8 *dst, unsigned int *dlen); + int (*coa_decompress_noctx)(const u8 *src, unsigned int slen, + u8 *dst, unsigned int *dlen); }; @@ -538,6 +540,9 @@ struct compress_tfm { int (*cot_decompress)(struct crypto_tfm *tfm, const u8 *src, unsigned int slen, u8 *dst, unsigned int *dlen); + int (*cot_decompress_noctx)(struct crypto_tfm *tfm, + const u8 *src, unsigned int slen, + u8 *dst, unsigned int *dlen); }; #define crt_ablkcipher crt_u.ablkcipher @@ -1836,6 +1841,14 @@ static inline void crypto_free_comp(struct crypto_comp *tfm) crypto_free_tfm(crypto_comp_tfm(tfm)); } +struct crypto_comp *crypto_alloc_comp_noctx(const char *alg_name, + u32 type, u32 mask); + +static inline void crypto_free_comp_noctx(struct crypto_comp *tfm) +{ + crypto_free_comp(tfm); +} + static inline int crypto_has_comp(const char *alg_name, u32 type, u32 mask) { type &= ~CRYPTO_ALG_TYPE_MASK; @@ -1871,5 +1884,12 @@ static inline int crypto_comp_decompress(struct crypto_comp *tfm, src, slen, dst, dlen); } +static inline int crypto_comp_decompress_noctx(struct crypto_comp *tfm, + const u8 *src, unsigned int slen, + u8 *dst, unsigned int *dlen) +{ + return crypto_comp_crt(tfm)->cot_decompress_noctx(crypto_comp_tfm(tfm), + src, slen, dst, dlen); +} #endif /* _LINUX_CRYPTO_H */
Until now, tfm object embeds (de)compression context in it and (de)compression in crypto API requires tfm object to use this context. But, there are some algorithms that doesn't need such context to operate. Therefore, this patch introduce new crypto decompression API that calls decompression function via sharable tfm object. Concurrent calls to decompress_noctx function through sharable tfm object will be okay because caller don't need any context in tfm and tfm is only used for fetching function pointer to decompress_noctx function. This can reduce overhead of maintaining multiple tfm if decompression doesn't require any context to operate. Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> --- crypto/842.c | 3 ++- crypto/compress.c | 36 ++++++++++++++++++++++++++++++++++++ crypto/crypto_null.c | 3 ++- crypto/deflate.c | 3 ++- crypto/lz4.c | 3 ++- crypto/lz4hc.c | 3 ++- crypto/lzo.c | 3 ++- include/linux/crypto.h | 20 ++++++++++++++++++++ 8 files changed, 68 insertions(+), 6 deletions(-)