Message ID | 20230718125847.3869700-6-ardb@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c923:0:b0:3e4:2afc:c1 with SMTP id j3csp1741153vqt; Tue, 18 Jul 2023 06:21:10 -0700 (PDT) X-Google-Smtp-Source: APBJJlFewO4fn99jd/sSx7K0BwhadT6dEdenqGJbsZlfzHIOjYfBNh7ek8l+NSfHtWtAu49nFInH X-Received: by 2002:a17:906:6682:b0:992:7462:a22c with SMTP id z2-20020a170906668200b009927462a22cmr12450673ejo.36.1689686470101; Tue, 18 Jul 2023 06:21:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689686470; cv=none; d=google.com; s=arc-20160816; b=RhIoq8uRdK9JDtj7bcqSmgEA0zMYS3H3SFqn26v08L7FEAanSxuwPFjs7CJHtNVGd3 BgDi6MPypYgSxQc9NxUJvvkt2Yfh+MwRRDviw/tYL53CDCDPLzyOmkDgtc0G/Wk0JFGR HOsdxCJirfSSNMeMyKj19GVxPDtgw3UVHl2cUAh03eqmWtctA4cyeMncEU6RMvQmWvLh fjvBvQopb3D3H2VHq5xl3TKwk3xIYSh2a3I5xwaSv/A5/K2p9tIlr/4a3HFMg8OhY6mx Z2lDajelMv0LrT9KOU6eHwKOF5FNTvdA9rcVOYmdkh6JCPnF6GxcMjJPv54P2izFqjXN Z0Mw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=VRkJIutGAQCy9V+Cnln/i2QHE7ZQe0Zo2jvJEyGa0Ng=; fh=/RacsB/FIyaIQZLsCl51O/4lZGjWuNGrCjxQndCzekA=; b=XlpBsVtGbaxR+OVL54wc6aqbv0Mkl3t6M+slogPZLoToyIq/+BoSkjJVpZd9dzufhq INTDnicAgl1NsO6XODn66w6hNMsp8ZsYI7BCBLs1dC2wcBbwmFT7mz2KB4WOQ5zDXK3W Th97pDC34gwx5WfFRSLyPJcunXssQ9GIjESzup11Xu+aW4gLjmhcY/3KYC99dvvGNObP 3EXlzORth7MgxpB00B6hpPHbI1Rf9cjyemrNShRvjRwaa7CuZAk1dl3atq35KcnwoqUd DE7E9A9sSVomKEkNZAlxmmdEgrFhq60nSU6zk25mifWopSHbd2vY2CfQW92epdqBjvBf i7Og== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="SDalzP/Y"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j21-20020a170906255500b00993a37985dfsi1093692ejb.179.2023.07.18.06.20.36; Tue, 18 Jul 2023 06:21:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="SDalzP/Y"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232421AbjGRNAj (ORCPT <rfc822;assdfgzxcv4@gmail.com> + 99 others); Tue, 18 Jul 2023 09:00:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40376 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232500AbjGRNAJ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 18 Jul 2023 09:00:09 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A4C041BEE; Tue, 18 Jul 2023 05:59:52 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 2607D6157D; Tue, 18 Jul 2023 12:59:52 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3D366C433CA; Tue, 18 Jul 2023 12:59:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1689685191; bh=ODoi2e+vcqIb799pflsdMhm1nGdkubtWgbUegajrVfU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=SDalzP/YaXdnw/eO5FhMttHT+uo4cUQ0HEfzV/eVDhjrh2vL2AqqxXcAqh13yqJn4 AVbM5TSLu3q0pr2cIE2wClirkeGHzrnHwdeCo5pdmULzTvNEpku6b70jtE1ArlykWc iaLRyhhRjQiHzGqDFnFYCpXBS8sZnvAGylTxo/SCUEVIt3zDITwUPYFYeNX441iuA5 UPvKWFmqWGmWjFjxj3GAvUxK0AMTKP3bw+ZiMK2blBchprVp/PM9vi8CkYksqH9lwv 0KW/Y7eXnjmHRBPOokrRrf/vKW7znD2how5Rm7j4/YU0ipme/NjIaZ8mn9lIi/NwrT dtMt3ZU+yjBpQ== From: Ard Biesheuvel <ardb@kernel.org> To: linux-crypto@vger.kernel.org Cc: Ard Biesheuvel <ardb@kernel.org>, Herbert Xu <herbert@gondor.apana.org.au>, Eric Biggers <ebiggers@kernel.org>, Kees Cook <keescook@chromium.org>, Haren Myneni <haren@us.ibm.com>, Nick Terrell <terrelln@fb.com>, Minchan Kim <minchan@kernel.org>, Sergey Senozhatsky <senozhatsky@chromium.org>, Jens Axboe <axboe@kernel.dk>, Giovanni Cabiddu <giovanni.cabiddu@intel.com>, Richard Weinberger <richard@nod.at>, David Ahern <dsahern@kernel.org>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Steffen Klassert <steffen.klassert@secunet.com>, linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, qat-linux@intel.com, linuxppc-dev@lists.ozlabs.org, linux-mtd@lists.infradead.org, netdev@vger.kernel.org Subject: [RFC PATCH 05/21] ubifs: Pass worst-case buffer size to compression routines Date: Tue, 18 Jul 2023 14:58:31 +0200 Message-Id: <20230718125847.3869700-6-ardb@kernel.org> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230718125847.3869700-1-ardb@kernel.org> References: <20230718125847.3869700-1-ardb@kernel.org> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=1089; i=ardb@kernel.org; h=from:subject; bh=ODoi2e+vcqIb799pflsdMhm1nGdkubtWgbUegajrVfU=; b=owGbwMvMwCFmkMcZplerG8N4Wi2JIWVbT9FzF2aeyo1FL46LhUf9emTqUM1q5dWwJ2Dzr1cTG sRaswQ7SlkYxDgYZMUUWQRm/3238/REqVrnWbIwc1iZQIYwcHEKwETqRRgZriduT8+o3D0pY5fL upjHTB/UclQ3ycjOPOLy0+e+sqX/b4Z/Wgf3MzUs1I/L09XztZ/lvWNdMcPstI16pS5bDvIdeTO NGwA= X-Developer-Key: i=ardb@kernel.org; a=openpgp; fpr=F43D03328115A198C90016883D200E9CA6329909 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771764679840685630 X-GMAIL-MSGID: 1771764679840685630 |
Series |
crypto: consolidate and clean up compression APIs
|
|
Commit Message
Ard Biesheuvel
July 18, 2023, 12:58 p.m. UTC
Currently, the ubifs code allocates a worst case buffer size to
recompress a data node, but does not pass the size of that buffer to the
compression code. This means that the compression code will never use
the additional space, and might fail spuriously due to lack of space.
So let's multiply out_len by WORST_COMPR_FACTOR after allocating the
buffer. Doing so is guaranteed not to overflow, given that the preceding
kmalloc_array() call would have failed otherwise.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
fs/ubifs/journal.c | 2 ++
1 file changed, 2 insertions(+)
Comments
On Tue, Jul 18, 2023 at 02:58:31PM +0200, Ard Biesheuvel wrote: > Currently, the ubifs code allocates a worst case buffer size to > recompress a data node, but does not pass the size of that buffer to the > compression code. This means that the compression code will never use > the additional space, and might fail spuriously due to lack of space. > > So let's multiply out_len by WORST_COMPR_FACTOR after allocating the > buffer. Doing so is guaranteed not to overflow, given that the preceding > kmalloc_array() call would have failed otherwise. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > fs/ubifs/journal.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c > index dc52ac0f4a345f30..4e5961878f336033 100644 > --- a/fs/ubifs/journal.c > +++ b/fs/ubifs/journal.c > @@ -1493,6 +1493,8 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in > if (!buf) > return -ENOMEM; > > + out_len *= WORST_COMPR_FACTOR; > + > dlen = le32_to_cpu(dn->ch.len) - UBIFS_DATA_NODE_SZ; > data_size = dn_size - UBIFS_DATA_NODE_SZ; > compr_type = le16_to_cpu(dn->compr_type); This looks like another case where data that would be expanded by compression should just be stored uncompressed instead. In fact, it seems that UBIFS does that already. ubifs_compress() has this: /* * If the data compressed only slightly, it is better to leave it * uncompressed to improve read speed. */ if (in_len - *out_len < UBIFS_MIN_COMPRESS_DIFF) goto no_compr; So it's unclear why the WORST_COMPR_FACTOR thing is needed at all. - Eric
On Wed, 19 Jul 2023 at 00:38, Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jul 18, 2023 at 02:58:31PM +0200, Ard Biesheuvel wrote: > > Currently, the ubifs code allocates a worst case buffer size to > > recompress a data node, but does not pass the size of that buffer to the > > compression code. This means that the compression code will never use > > the additional space, and might fail spuriously due to lack of space. > > > > So let's multiply out_len by WORST_COMPR_FACTOR after allocating the > > buffer. Doing so is guaranteed not to overflow, given that the preceding > > kmalloc_array() call would have failed otherwise. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > fs/ubifs/journal.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c > > index dc52ac0f4a345f30..4e5961878f336033 100644 > > --- a/fs/ubifs/journal.c > > +++ b/fs/ubifs/journal.c > > @@ -1493,6 +1493,8 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in > > if (!buf) > > return -ENOMEM; > > > > + out_len *= WORST_COMPR_FACTOR; > > + > > dlen = le32_to_cpu(dn->ch.len) - UBIFS_DATA_NODE_SZ; > > data_size = dn_size - UBIFS_DATA_NODE_SZ; > > compr_type = le16_to_cpu(dn->compr_type); > > This looks like another case where data that would be expanded by compression > should just be stored uncompressed instead. > > In fact, it seems that UBIFS does that already. ubifs_compress() has this: > > /* > * If the data compressed only slightly, it is better to leave it > * uncompressed to improve read speed. > */ > if (in_len - *out_len < UBIFS_MIN_COMPRESS_DIFF) > goto no_compr; > > So it's unclear why the WORST_COMPR_FACTOR thing is needed at all. > It is not. The buffer is used for decompression in the truncation path, so none of this logic even matters. Even if the subsequent recompression of the truncated data node could result in expansion beyond the uncompressed size of the original data (which seems impossible to me), increasing the size of this buffer would not help as it is the input buffer for the compression not the output buffer.
在 2023/7/19 16:33, Ard Biesheuvel 写道: > On Wed, 19 Jul 2023 at 00:38, Eric Biggers <ebiggers@kernel.org> wrote: >> >> On Tue, Jul 18, 2023 at 02:58:31PM +0200, Ard Biesheuvel wrote: >>> Currently, the ubifs code allocates a worst case buffer size to >>> recompress a data node, but does not pass the size of that buffer to the >>> compression code. This means that the compression code will never use I think you mean the 'out_len' which describes the lengh of 'buf' is passed into ubifs_decompress, which effects the result of decompressor(eg. lz4 uses length to calculate the buffer end pos). So, we should pass the real lenghth of 'buf'. Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com> >>> the additional space, and might fail spuriously due to lack of space. >>> >>> So let's multiply out_len by WORST_COMPR_FACTOR after allocating the >>> buffer. Doing so is guaranteed not to overflow, given that the preceding >>> kmalloc_array() call would have failed otherwise. >>> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >>> --- >>> fs/ubifs/journal.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c >>> index dc52ac0f4a345f30..4e5961878f336033 100644 >>> --- a/fs/ubifs/journal.c >>> +++ b/fs/ubifs/journal.c >>> @@ -1493,6 +1493,8 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in >>> if (!buf) >>> return -ENOMEM; >>> >>> + out_len *= WORST_COMPR_FACTOR; >>> + >>> dlen = le32_to_cpu(dn->ch.len) - UBIFS_DATA_NODE_SZ; >>> data_size = dn_size - UBIFS_DATA_NODE_SZ; >>> compr_type = le16_to_cpu(dn->compr_type); >> >> This looks like another case where data that would be expanded by compression >> should just be stored uncompressed instead. >> >> In fact, it seems that UBIFS does that already. ubifs_compress() has this: >> >> /* >> * If the data compressed only slightly, it is better to leave it >> * uncompressed to improve read speed. >> */ >> if (in_len - *out_len < UBIFS_MIN_COMPRESS_DIFF) >> goto no_compr; >> >> So it's unclear why the WORST_COMPR_FACTOR thing is needed at all. >> > > It is not. The buffer is used for decompression in the truncation > path, so none of this logic even matters. Even if the subsequent > recompression of the truncated data node could result in expansion > beyond the uncompressed size of the original data (which seems > impossible to me), increasing the size of this buffer would not help > as it is the input buffer for the compression not the output buffer. > . >
On Wed, 19 Jul 2023 at 16:23, Zhihao Cheng <chengzhihao1@huawei.com> wrote: > > 在 2023/7/19 16:33, Ard Biesheuvel 写道: > > On Wed, 19 Jul 2023 at 00:38, Eric Biggers <ebiggers@kernel.org> wrote: > >> > >> On Tue, Jul 18, 2023 at 02:58:31PM +0200, Ard Biesheuvel wrote: > >>> Currently, the ubifs code allocates a worst case buffer size to > >>> recompress a data node, but does not pass the size of that buffer to the > >>> compression code. This means that the compression code will never use > > I think you mean the 'out_len' which describes the lengh of 'buf' is > passed into ubifs_decompress, which effects the result of > decompressor(eg. lz4 uses length to calculate the buffer end pos). > So, we should pass the real lenghth of 'buf'. > Yes, that is what I meant. But Eric makes a good point, and looking a bit more closely, there is really no need for the multiplication here: we know the size of the decompressed data, so we don't need the additional space. I intend to drop this patch, and replace it with the following: ----------------8<-------------- Currently, when truncating a data node, a decompression buffer is allocated that is twice the size of the data node's uncompressed size. However, the fact that this space is available is not communicated to the compression routines, as out_len itself is not updated. The additional space is not needed even in the theoretical worst case where compression might lead to inadvertent expansion: first of all, increasing the size of the input buffer does not help mitigate that issue. And given the truncation of the data node and the fact that the original data compressed well enough to pass the UBIFS_MIN_COMPRESS_DIFF test, there is no way on this particular code path that compression could result in expansion beyond the original decompressed size, and so no mitigation is necessary to begin with. So let's just drop WORST_COMPR_FACTOR here. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index dc52ac0f4a345f30..0b55cbfe0c30505e 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1489,7 +1489,7 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in int err, dlen, compr_type, out_len, data_size; out_len = le32_to_cpu(dn->size); - buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS); + buf = kmalloc(out_len, GFP_NOFS); if (!buf) return -ENOMEM;
在 2023/7/19 22:38, Ard Biesheuvel 写道: > On Wed, 19 Jul 2023 at 16:23, Zhihao Cheng <chengzhihao1@huawei.com> wrote: >> >> 在 2023/7/19 16:33, Ard Biesheuvel 写道: >>> On Wed, 19 Jul 2023 at 00:38, Eric Biggers <ebiggers@kernel.org> wrote: >>>> >>>> On Tue, Jul 18, 2023 at 02:58:31PM +0200, Ard Biesheuvel wrote: >>>>> Currently, the ubifs code allocates a worst case buffer size to >>>>> recompress a data node, but does not pass the size of that buffer to the >>>>> compression code. This means that the compression code will never use >> >> I think you mean the 'out_len' which describes the lengh of 'buf' is >> passed into ubifs_decompress, which effects the result of >> decompressor(eg. lz4 uses length to calculate the buffer end pos). >> So, we should pass the real lenghth of 'buf'. >> > > Yes, that is what I meant. > > But Eric makes a good point, and looking a bit more closely, there is > really no need for the multiplication here: we know the size of the > decompressed data, so we don't need the additional space. > Right, we get 'out_len' from 'dn->size' which is the length of uncompressed data. ubifs_compress makes sure the compressed length is smaller than original length. > I intend to drop this patch, and replace it with the following: > > ----------------8<-------------- > > Currently, when truncating a data node, a decompression buffer is > allocated that is twice the size of the data node's uncompressed size. > However, the fact that this space is available is not communicated to > the compression routines, as out_len itself is not updated. > > The additional space is not needed even in the theoretical worst case > where compression might lead to inadvertent expansion: first of all, > increasing the size of the input buffer does not help mitigate that > issue. And given the truncation of the data node and the fact that the > original data compressed well enough to pass the UBIFS_MIN_COMPRESS_DIFF > test, there is no way on this particular code path that compression > could result in expansion beyond the original decompressed size, and so > no mitigation is necessary to begin with. > > So let's just drop WORST_COMPR_FACTOR here. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c > index dc52ac0f4a345f30..0b55cbfe0c30505e 100644 > --- a/fs/ubifs/journal.c > +++ b/fs/ubifs/journal.c > @@ -1489,7 +1489,7 @@ static int truncate_data_node(const struct > ubifs_info *c, const struct inode *in > int err, dlen, compr_type, out_len, data_size; > > out_len = le32_to_cpu(dn->size); > - buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS); > + buf = kmalloc(out_len, GFP_NOFS); > if (!buf) > return -ENOMEM; > . > This version looks better. Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index dc52ac0f4a345f30..4e5961878f336033 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1493,6 +1493,8 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in if (!buf) return -ENOMEM; + out_len *= WORST_COMPR_FACTOR; + dlen = le32_to_cpu(dn->ch.len) - UBIFS_DATA_NODE_SZ; data_size = dn_size - UBIFS_DATA_NODE_SZ; compr_type = le16_to_cpu(dn->compr_type);