From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EB644C27C52 for ; Tue, 4 Jun 2024 18:42:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Fz1QjBfrsigqkyyGWOoaTFo56vrc9Nkv/OaEZZmwvaI=; b=Uopt0kOGuCZ0YH NnODV7Zmrgzi55Avvy/A92Vv8u6cjbO5CLA14KmsLbcXvlj3QawO/OeXmbh+EG0QXIgVa1eUvhiDM 10J+wdQ2FELZL8HQxmoxWBcWoVd3YQAhkaRYtCBfI+FsDk3vbFYMk/r6jCQT/8vGcdw+28h32++at WdrmRrDSlBSJFv9lhsB4tPIL1oKLdmtqf7djwmpfk4WyALj2BbYP2s6yzB9Gjk4Bp9dxmrZb2RNGx ICLW6DbfO4IB+ARuzdzZvy8OQBZijYqdzSdizGNVqYxxAtkRRTzVFvTRoSgbAsANHoSJXjbjBRbzC J7esI4FDd+jx4dgRPo1g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sEZ75-00000003Tds-0HuN; Tue, 04 Jun 2024 18:42:27 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sEZ71-00000003Td2-2Ovz for linux-arm-kernel@lists.infradead.org; Tue, 04 Jun 2024 18:42:25 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 4550B61407; Tue, 4 Jun 2024 18:42:22 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A7205C2BBFC; Tue, 4 Jun 2024 18:42:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1717526542; bh=TAmCuYHgvksi5yyEVMkM6NTME/kcFTf5GBRttrygVtI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bfdxaZrUUfeWaewS3+OMZfVZyZ/6y0h0RkZ6D9paQP7HPumPolPQQskFYL6esLdNW UQM3oxuC10eW1VW0GtWsISafG3E+Ng9lq6mc5h7qF2JfGH0E5XSwQWx/oXmBZ5exke 4J12DaZoG4OsnvptXz3U8KjpFgdwJi8hOEElfEam0iOXOI6LSBQICVcSu2gY40vOe2 +6efNfsIHtta3Hi/Er2JkRjiseTsidl6N8ft1Qof67aZbnzknNddD5pK/vsZIxBShe zxXHHdMmluqGD/KEFay+/DxZXT83prvA6O2n78rU2zcsRfVlem8iDQ5+UyII9cckOB 8IkTKkQVdiFwA== Date: Tue, 4 Jun 2024 11:42:20 -0700 From: Eric Biggers To: Herbert Xu Cc: linux-crypto@vger.kernel.org, fsverity@lists.linux.dev, dm-devel@lists.linux.dev, x86@kernel.org, linux-arm-kernel@lists.infradead.org, Ard Biesheuvel , Sami Tolvanen , Bart Van Assche Subject: Re: [PATCH v4 6/8] fsverity: improve performance by using multibuffer hashing Message-ID: <20240604184220.GC1566@sol.localdomain> References: <20240603183731.108986-1-ebiggers@kernel.org> <20240603183731.108986-7-ebiggers@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240604_114223_997204_12741AA8 X-CRM114-Status: GOOD ( 21.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jun 04, 2024 at 05:37:36PM +0800, Herbert Xu wrote: > On Mon, Jun 03, 2024 at 11:37:29AM -0700, Eric Biggers wrote: > > > > + for (i = 0; i < ctx->num_pending; i++) { > > + data[i] = ctx->pending_blocks[i].data; > > + outs[i] = ctx->pending_blocks[i].hash; > > + } > > + > > + desc->tfm = params->hash_alg->tfm; > > + if (params->hashstate) > > + err = crypto_shash_import(desc, params->hashstate); > > + else > > + err = crypto_shash_init(desc); > > + if (err) { > > + fsverity_err(inode, "Error %d importing hash state", err); > > + return false; > > + } > > + err = crypto_shash_finup_mb(desc, data, params->block_size, outs, > > + ctx->num_pending); > > + if (err) { > > + fsverity_err(inode, "Error %d computing block hashes", err); > > + return false; > > + } > > So with ahash operating in synchronous mode (callback == NULL), this > would look like: > > struct ahash_request *reqs[FS_VERITY_MAX_PENDING_DATA_BLOCKS]; > > for (i = 0; i < ctx->num_pending; i++) { > reqs[i] = fsverity_alloc_hash_request(); > if (!req) { > free all reqs; > return false; > } > > if (params->hashstate) > err = crypto_ahash_import(&reqs[i], params->hashstate); > else > err = crypto_ahash_init(&reqs[i]); > > if (err) { > fsverity_err(inode, "Error %d importing hash state", err); > free all reqs; > return false; > } > } > > for (i = 0; i < ctx->num_pending; i++) { > unsigned more; > > if (params->hashstate) > err = crypto_ahash_import(req, params->hashstate); > else > err = crypto_ahash_init(req); > > if (err) { > fsverity_err(inode, "Error %d importing hash state", err); > free all requests; > return false; > } > > more = 0; > if (i + 1 < ctx->num_pending) > more = CRYPTO_TFM_REQ_MORE; > ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP | more, > NULL, NULL); > ahash_request_set_crypt(req, ctx->pending_blocks[i].sg, > ctx->pending_blocks[i].hash, > params->block_size); > > err = crypto_ahash_finup(req); > if (err) { > fsverity_err(inode, "Error %d computing block hashes", err); > free all requests; > return false; > } > } > > You're hiding some of the complexity by not allocating memory > explicitly for each hash state. This might fit on the stack > for two requests, but eventually you will have to allocate memory. > > With the ahash API, the allocation is explicit. > This doesn't make any sense, though. First, the requests need to be enqueued for the task, but crypto_ahash_finup() would only have the ability to enqueue it in a queue associated with the tfm, which is shared by many tasks. So it can't actually work unless the tfm maintained a separate queue for each task, which would be really complex. Second, it adds a memory allocation per block which is very undesirable. You claim that it's needed anyway, but actually it's not; with my API there is only one initial hash state regardless of how high the interleaving factor is. In fact, if multiple initial states were allowed, multibuffer hashing would become much more complex because the underlying algorithm would need to validate that these different states are synced up. My proposal is much simpler and avoids all this unnecessary overhead. Really the only reason to even consider ahash at all would be try to support software hashing and off-CPU hardware accelerators using the "same" code. However, your proposal would not achieve that either, as it would not use the async callback. Note, as far as I know no one actually cares about off-CPU hardware accelerator support in fsverity anyway... - Eric _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel