| Message ID | 1386703583-8386-1-git-send-email-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 10, 2013 at 08:26:19PM +0100, Marek Vasut wrote: > When finishing the ahash request, the ahash_op_unaligned_done() will > call complete() on the request. Yet, this will not call the correct > complete callback. The correct complete callback was previously stored > in the requests' private data, as seen in ahash_op_unaligned(). This > patch restores the correct complete callback and .data field of the > request before calling complete() on it. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David S. Miller <davem@davemloft.net> > Cc: Fabio Estevam <fabio.estevam@freescale.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: linux-crypto@vger.kernel.org > --- > crypto/ahash.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/crypto/ahash.c b/crypto/ahash.c > index 793a27f..a92dc38 100644 > --- a/crypto/ahash.c > +++ b/crypto/ahash.c > @@ -213,7 +213,10 @@ static void ahash_op_unaligned_done(struct crypto_async_request *req, int err) > > ahash_op_unaligned_finish(areq, err); > > - complete(data, err); > + areq->base.complete = complete; > + areq->base.data = data; > + > + complete(&areq->base, err); This looks completely bogus. While restoring areq isn't wrong per se, calling complete with &areq->base makes no sense. The original completion data is in the variable "data". Which driver relies on this behaviour? Also, does your subsequent patches rely on this? Cheers,
On Friday, December 20, 2013 at 01:04:08 PM, Herbert Xu wrote: > On Tue, Dec 10, 2013 at 08:26:19PM +0100, Marek Vasut wrote: > > When finishing the ahash request, the ahash_op_unaligned_done() will > > call complete() on the request. Yet, this will not call the correct > > complete callback. The correct complete callback was previously stored > > in the requests' private data, as seen in ahash_op_unaligned(). This > > patch restores the correct complete callback and .data field of the > > request before calling complete() on it. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > Cc: David S. Miller <davem@davemloft.net> > > Cc: Fabio Estevam <fabio.estevam@freescale.com> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: linux-crypto@vger.kernel.org > > --- > > > > crypto/ahash.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/crypto/ahash.c b/crypto/ahash.c > > index 793a27f..a92dc38 100644 > > --- a/crypto/ahash.c > > +++ b/crypto/ahash.c > > @@ -213,7 +213,10 @@ static void ahash_op_unaligned_done(struct > > crypto_async_request *req, int err) > > > > ahash_op_unaligned_finish(areq, err); > > > > - complete(data, err); > > + areq->base.complete = complete; > > + areq->base.data = data; > > + > > + complete(&areq->base, err); > > This looks completely bogus. While restoring areq isn't wrong per > se, calling complete with &areq->base makes no sense. The original > completion data is in the variable "data". Is there some documentation for this so I can understand why this is wrong, please? I really don't quite get it, sorry. Actually, is there some documentation for writing crypto API drivers at all please ? > Which driver relies on this behaviour? This one. > Also, does your subsequent patches rely on this? Yes > Cheers, Best regards, Marek Vasut
On Fri, Dec 27, 2013 at 01:21:36AM +0100, Marek Vasut wrote: > > > > - complete(data, err); > > > + areq->base.complete = complete; > > > + areq->base.data = data; > > > + > > > + complete(&areq->base, err); > > > > This looks completely bogus. While restoring areq isn't wrong per > > se, calling complete with &areq->base makes no sense. The original > > completion data is in the variable "data". > > Is there some documentation for this so I can understand why this is wrong, > please? I really don't quite get it, sorry. Actually, is there some > documentation for writing crypto API drivers at all please ? Well it's wrong because the completion function (req->base.complete) is meant to take data (req->base.data) as its first argument. So giving it a pointer to req->base makes no sense. Cheers,
On Monday, December 30, 2013 05:01:13 PM Herbert Xu wrote: > On Fri, Dec 27, 2013 at 01:21:36AM +0100, Marek Vasut wrote: > > > > > > - complete(data, err); > > > > + areq->base.complete = complete; > > > > + areq->base.data = data; > > > > + > > > > + complete(&areq->base, err); > > > > > > This looks completely bogus. While restoring areq isn't wrong per > > > se, calling complete with &areq->base makes no sense. The original > > > completion data is in the variable "data". > > > > Is there some documentation for this so I can understand why this is wrong, > > please? I really don't quite get it, sorry. Actually, is there some > > documentation for writing crypto API drivers at all please ? > > Well it's wrong because the completion function (req->base.complete) > is meant to take data (req->base.data) as its first argument. So > giving it a pointer to req->base makes no sense. > The crypto_completion_t typdef is defined as: typedef void (*crypto_completion_t)(struct crypto_async_request *req, int err); so I believe &areq->base is the proper first argument (which is actually just the req parameter on the ahash_op_unaligned_done function). If you are going to restore areq, you really should restore all fields that were changed - result, base.complete, base.data - and set priv to NULL. Since the ahash_request_priv structure is freed in ahash_op_unaligned_finish you'll need to save the value of priv->result in order to restore areq->result (u8 *result = priv->result; or similar). Additionally, you should probably also fix up ahash_def_finup_done2 and ahash_def_finup_done1. Thanks, Tom > Cheers, >
On Fri, Jan 03, 2014 at 09:10:30AM -0600, Tom Lendacky wrote: > On Monday, December 30, 2013 05:01:13 PM Herbert Xu wrote: > > On Fri, Dec 27, 2013 at 01:21:36AM +0100, Marek Vasut wrote: > > > > > > > > - complete(data, err); > > > > > + areq->base.complete = complete; > > > > > + areq->base.data = data; > > > > > + > > > > > + complete(&areq->base, err); > > > > > > > > This looks completely bogus. While restoring areq isn't wrong per > > > > se, calling complete with &areq->base makes no sense. The original > > > > completion data is in the variable "data". > > > > > > Is there some documentation for this so I can understand why this is wrong, > > > please? I really don't quite get it, sorry. Actually, is there some > > > documentation for writing crypto API drivers at all please ? > > > > Well it's wrong because the completion function (req->base.complete) > > is meant to take data (req->base.data) as its first argument. So > > giving it a pointer to req->base makes no sense. > > > > The crypto_completion_t typdef is defined as: > > typedef void (*crypto_completion_t)(struct crypto_async_request *req, int err); > > so I believe &areq->base is the proper first argument (which is actually just > the req parameter on the ahash_op_unaligned_done function). You are right. This unaligned code obviously has never worked. I will apply Marek's patch to fix this up. > Additionally, you should probably also fix up ahash_def_finup_done2 and > ahash_def_finup_done1. Yep I'll fix them up too. Thanks!
On Friday, January 03, 2014 at 11:33:44 PM, Herbert Xu wrote: > On Fri, Jan 03, 2014 at 09:10:30AM -0600, Tom Lendacky wrote: > > On Monday, December 30, 2013 05:01:13 PM Herbert Xu wrote: > > > On Fri, Dec 27, 2013 at 01:21:36AM +0100, Marek Vasut wrote: > > > > > > - complete(data, err); > > > > > > + areq->base.complete = complete; > > > > > > + areq->base.data = data; > > > > > > + > > > > > > + complete(&areq->base, err); > > > > > > > > > > This looks completely bogus. While restoring areq isn't wrong per > > > > > se, calling complete with &areq->base makes no sense. The original > > > > > completion data is in the variable "data". > > > > > > > > Is there some documentation for this so I can understand why this is > > > > wrong, please? I really don't quite get it, sorry. Actually, is > > > > there some documentation for writing crypto API drivers at all > > > > please ? > > > > > > Well it's wrong because the completion function (req->base.complete) > > > is meant to take data (req->base.data) as its first argument. So > > > giving it a pointer to req->base makes no sense. > > > > The crypto_completion_t typdef is defined as: > > > > typedef void (*crypto_completion_t)(struct crypto_async_request *req, int > > err); > > > > so I believe &areq->base is the proper first argument (which is actually > > just the req parameter on the ahash_op_unaligned_done function). > > You are right. This unaligned code obviously has never worked. > I will apply Marek's patch to fix this up. > > > Additionally, you should probably also fix up ahash_def_finup_done2 and > > ahash_def_finup_done1. > > Yep I'll fix them up too. I'll try to cook some patches shortly, I will probably also cook some documentation for writing drivers using the crypto API, what do you say ? Best regards, Marek Vasut
On Tue, Dec 10, 2013 at 08:26:19PM +0100, Marek Vasut wrote: > When finishing the ahash request, the ahash_op_unaligned_done() will > call complete() on the request. Yet, this will not call the correct > complete callback. The correct complete callback was previously stored > in the requests' private data, as seen in ahash_op_unaligned(). This > patch restores the correct complete callback and .data field of the > request before calling complete() on it. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David S. Miller <davem@davemloft.net> > Cc: Fabio Estevam <fabio.estevam@freescale.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: linux-crypto@vger.kernel.org All patches applied. Thanks!
On Sunday, January 05, 2014 at 02:05:29 PM, Herbert Xu wrote: > On Tue, Dec 10, 2013 at 08:26:19PM +0100, Marek Vasut wrote: > > When finishing the ahash request, the ahash_op_unaligned_done() will > > call complete() on the request. Yet, this will not call the correct > > complete callback. The correct complete callback was previously stored > > in the requests' private data, as seen in ahash_op_unaligned(). This > > patch restores the correct complete callback and .data field of the > > request before calling complete() on it. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > Cc: David S. Miller <davem@davemloft.net> > > Cc: Fabio Estevam <fabio.estevam@freescale.com> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: linux-crypto@vger.kernel.org > > All patches applied. Thanks! Uh, I think this was not intended to happen! We still discuss this patch will need rework, do we not? Best regards, Marek Vasut
On Sun, Jan 05, 2014 at 04:33:21PM +0100, Marek Vasut wrote: > On Sunday, January 05, 2014 at 02:05:29 PM, Herbert Xu wrote: > > On Tue, Dec 10, 2013 at 08:26:19PM +0100, Marek Vasut wrote: > > > When finishing the ahash request, the ahash_op_unaligned_done() will > > > call complete() on the request. Yet, this will not call the correct > > > complete callback. The correct complete callback was previously stored > > > in the requests' private data, as seen in ahash_op_unaligned(). This > > > patch restores the correct complete callback and .data field of the > > > request before calling complete() on it. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > Cc: David S. Miller <davem@davemloft.net> > > > Cc: Fabio Estevam <fabio.estevam@freescale.com> > > > Cc: Shawn Guo <shawn.guo@linaro.org> > > > Cc: linux-crypto@vger.kernel.org > > > > All patches applied. Thanks! > > Uh, I think this was not intended to happen! We still discuss this patch will > need rework, do we not? Please send your new fixes as incremental patches on top of this. Thanks,
On Monday, January 06, 2014 at 12:29:27 AM, Herbert Xu wrote: > On Sun, Jan 05, 2014 at 04:33:21PM +0100, Marek Vasut wrote: > > On Sunday, January 05, 2014 at 02:05:29 PM, Herbert Xu wrote: > > > On Tue, Dec 10, 2013 at 08:26:19PM +0100, Marek Vasut wrote: > > > > When finishing the ahash request, the ahash_op_unaligned_done() will > > > > call complete() on the request. Yet, this will not call the correct > > > > complete callback. The correct complete callback was previously > > > > stored in the requests' private data, as seen in > > > > ahash_op_unaligned(). This patch restores the correct complete > > > > callback and .data field of the request before calling complete() on > > > > it. > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > > Cc: David S. Miller <davem@davemloft.net> > > > > Cc: Fabio Estevam <fabio.estevam@freescale.com> > > > > Cc: Shawn Guo <shawn.guo@linaro.org> > > > > Cc: linux-crypto@vger.kernel.org > > > > > > All patches applied. Thanks! > > > > Uh, I think this was not intended to happen! We still discuss this patch > > will need rework, do we not? > > Please send your new fixes as incremental patches on top of this. I sent a new set of patches and added a bit of documentation too. I am still working on a better piece of documentation for writing crypto drivers now. Best regards, Marek Vasut
diff --git a/crypto/ahash.c b/crypto/ahash.c index 793a27f..a92dc38 100644 --- a/crypto/ahash.c +++ b/crypto/ahash.c @@ -213,7 +213,10 @@ static void ahash_op_unaligned_done(struct crypto_async_request *req, int err) ahash_op_unaligned_finish(areq, err); - complete(data, err); + areq->base.complete = complete; + areq->base.data = data; + + complete(&areq->base, err); } static int ahash_op_unaligned(struct ahash_request *req,
When finishing the ahash request, the ahash_op_unaligned_done() will call complete() on the request. Yet, this will not call the correct complete callback. The correct complete callback was previously stored in the requests' private data, as seen in ahash_op_unaligned(). This patch restores the correct complete callback and .data field of the request before calling complete() on it. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: David S. Miller <davem@davemloft.net> Cc: Fabio Estevam <fabio.estevam@freescale.com> Cc: Shawn Guo <shawn.guo@linaro.org> Cc: linux-crypto@vger.kernel.org --- crypto/ahash.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)