From patchwork Tue Sep 1 15:42:44 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brad Bosch X-Patchwork-Id: 32756 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@bilbo.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ozlabs.org (ozlabs.org [203.10.76.45]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mx.ozlabs.org", Issuer "CA Cert Signing Authority" (verified OK)) by bilbo.ozlabs.org (Postfix) with ESMTPS id B1D36B7B94 for ; Wed, 2 Sep 2009 01:43:20 +1000 (EST) Received: by ozlabs.org (Postfix) id A1FE0DDD0B; Wed, 2 Sep 2009 01:43:20 +1000 (EST) Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 351E4DDD01 for ; Wed, 2 Sep 2009 01:43:20 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754566AbZIAPnJ (ORCPT ); Tue, 1 Sep 2009 11:43:09 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754583AbZIAPnI (ORCPT ); Tue, 1 Sep 2009 11:43:08 -0400 Received: from qmta05.emeryville.ca.mail.comcast.net ([76.96.30.48]:37503 "EHLO QMTA05.emeryville.ca.mail.comcast.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754532AbZIAPnH (ORCPT ); Tue, 1 Sep 2009 11:43:07 -0400 Received: from OMTA02.emeryville.ca.mail.comcast.net ([76.96.30.19]) by QMTA05.emeryville.ca.mail.comcast.net with comcast id bPvi1c0060QkzPwA5TjAoK; Tue, 01 Sep 2009 15:43:10 +0000 Received: from waldo.imnotcreative.homeip.net ([66.151.13.191]) by OMTA02.emeryville.ca.mail.comcast.net with comcast id bTim1c00B47KXvm8NTip1o; Tue, 01 Sep 2009 15:43:08 +0000 Received: from brad by waldo.imnotcreative.homeip.net with local (Exim 4.69) (envelope-from ) id 1MiVVM-0005pM-Ea; Tue, 01 Sep 2009 10:42:44 -0500 MIME-Version: 1.0 Message-ID: <19101.16628.347039.619378@waldo.imnotcreative.homeip.net> Date: Tue, 1 Sep 2009 10:42:44 -0500 To: Herbert Xu Cc: linux-crypto@vger.kernel.org, netdev@vger.kernel.org, offbase0@gmail.com Subject: Re: Crypto oops in async_chainiv_do_postponed From: "Brad Bosch" In-Reply-To: <20090831220459.GA15713@gondor.apana.org.au> References: <19095.1264.682820.125602@waldo.imnotcreative.homeip.net> <20090829104606.GA13141@gondor.apana.org.au> <19099.63038.425414.514063@waldo.imnotcreative.homeip.net> <20090831220459.GA15713@gondor.apana.org.au> X-Mailer: VM 8.0.9 under Emacs 22.2.1 (i486-pc-linux-gnu) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Herbert Xu writes: > On Mon, Aug 31, 2009 at 11:11:42AM -0500, Brad Bosch wrote: > > > > OK. I was looking for something subtle because the crash takes a long > > time to happen. But do you agree that the race I described above also > > a real bug? > > No I don't think it is. CHAINV_STATE_INUSE guarantees that only > one entity can use ctx->err at any time. I don't see how you are protecting ctx->err with the INUSE flag. For example: If two threads enter async_chainiv_givencrypt at the same time, one thread will call async_chainiv_postpone_request (INUSE will be clear until set by async_chainiv_postpone_request) and the other thread will call async_chainiv_givencrypt_tail (INUSE may or may not be set yet). Now, ctx-err may be used by both async_chainiv_postpone_request to store the return value from skcipher_enqueue_givcrypt and by async_chainiv_givencrypt_tail to store the return value from crypto_ablkcipher_encrypt at the same time. This can cause the calling function to think async_chainiv_givencrypt has completed it's work, when in fact, the work was defered. The patch I proposed earlier (included again below) avoids this and also makes the error handling simpler and more direct without requiring ctx->err at all. I still don't understand why ctx->err was required in the first place. Did I miss something with regard to the use of ctx->err? Now, as to the other bug... > > Where we subtract the offset the pointer can never be NULL. Please > try my patch. OK. I see now that your offset patch should indeed solve that problem. But why did you choose to fix it in a complex way? My suggestion just adds a single test while yours adds new parameters, a new function and an extra function call. Thanks for your help. --Brad --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: chainiv.c =================================================================== RCS file: /share/cvs/sdg/kernels/kernel.wms/kernel_2_6_27/src/crypto/chainiv.c,v retrieving revision 1.1.1.1.4.2 diff -u -r1.1.1.1.4.2 chainiv.c --- chainiv.c 10 Mar 2009 05:16:24 -0000 1.1.1.1.4.2 +++ chainiv.c 27 Aug 2009 19:40:27 -0000 @@ -36,7 +36,6 @@ unsigned long state; spinlock_t lock; - int err; struct crypto_queue queue; struct work_struct postponed; @@ -114,10 +113,9 @@ return chainiv_init_common(tfm); } -static int async_chainiv_schedule_work(struct async_chainiv_ctx *ctx) +static void async_chainiv_schedule_work(struct async_chainiv_ctx *ctx) { int queued; - int err = ctx->err; if (!ctx->queue.qlen) { smp_mb__before_clear_bit(); @@ -125,14 +123,11 @@ if (!ctx->queue.qlen || test_and_set_bit(CHAINIV_STATE_INUSE, &ctx->state)) - goto out; + return; } queued = schedule_work(&ctx->postponed); BUG_ON(!queued); - -out: - return err; } static int async_chainiv_postpone_request(struct skcipher_givcrypt_request *req) @@ -148,8 +143,8 @@ if (test_and_set_bit(CHAINIV_STATE_INUSE, &ctx->state)) return err; - ctx->err = err; - return async_chainiv_schedule_work(ctx); + async_chainiv_schedule_work(ctx); + return err; } static int async_chainiv_givencrypt_tail(struct skcipher_givcrypt_request *req) @@ -158,18 +153,20 @@ struct async_chainiv_ctx *ctx = crypto_ablkcipher_ctx(geniv); struct ablkcipher_request *subreq = skcipher_givcrypt_reqctx(req); unsigned int ivsize = crypto_ablkcipher_ivsize(geniv); + int err; memcpy(req->giv, ctx->iv, ivsize); memcpy(subreq->info, ctx->iv, ivsize); - ctx->err = crypto_ablkcipher_encrypt(subreq); - if (ctx->err) + err = crypto_ablkcipher_encrypt(subreq); + if (err) goto out; memcpy(ctx->iv, subreq->info, ivsize); out: - return async_chainiv_schedule_work(ctx); + async_chainiv_schedule_work(ctx); + return err; } static int async_chainiv_givencrypt(struct skcipher_givcrypt_request *req) @@ -236,7 +233,7 @@ spin_unlock_bh(&ctx->lock); if (!req) { - async_chainiv_schedule_work(ctx); + async_chainiv_schedule_work(ctx); return; }