From: Chao Yu <yuchao0@huawei.com> To: Gao Xiang <gaoxiang25@huawei.com> Cc: Chao Yu <chao@kernel.org>, <linux-erofs@lists.ozlabs.org>, Gao Xiang <xiang@kernel.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v4] erofs: support superblock checksum Date: Mon, 28 Oct 2019 20:36:00 +0800 [thread overview] Message-ID: <df7d7427-e7ca-5135-5db2-640eda30d253@huawei.com> (raw) In-Reply-To: <20191023084536.GA16289@architecture4> On 2019/10/23 16:45, Gao Xiang wrote: > Hi Chao, > > On Wed, Oct 23, 2019 at 04:15:29PM +0800, Chao Yu wrote: >> Hi, Xiang, Pratik, >> >> On 2019/10/23 12:05, Gao Xiang wrote: > > <snip> > >>> } >>> >>> +static int erofs_superblock_csum_verify(struct super_block *sb, void *sbdata) >>> +{ >>> + struct erofs_super_block *dsb; >>> + u32 expected_crc, nblocks, crc; >>> + void *kaddr; >>> + struct page *page; >>> + int i; >>> + >>> + dsb = kmemdup(sbdata + EROFS_SUPER_OFFSET, >>> + EROFS_BLKSIZ - EROFS_SUPER_OFFSET, GFP_KERNEL); >>> + if (!dsb) >>> + return -ENOMEM; >>> + >>> + expected_crc = le32_to_cpu(dsb->checksum); >>> + nblocks = le32_to_cpu(dsb->chksum_blocks); >> >> Now, we try to use nblocks's value before checking its validation, I guess fuzz >> test can easily make the value extreme larger, result in checking latter blocks >> unnecessarily. >> >> IMO, we'd better >> 1. check validation of superblock to make sure all fields in sb are valid >> 2. use .nblocks to count and check payload blocks following sb > > That is quite a good point. :-) > > My first thought is to check the following payloads of sb (e.g, some per-fs > metadata should be checked at mount time together. or for small images, check > the whole image at the mount time) as well since if we introduce a new feature > to some kernel version, forward compatibility needs to be considered. So it's > better to make proper scalability, for this case, we have some choices: > 1) limit `chksum_blocks' upbound at runtime (e.g. refuse >= 65536 blocks, > totally 256M.) > 2) just get rid of the whole `chksum_blocks' mess and checksum the first 4k > at all, don't consider any latter scalability. Xiang, sorry for later reply... I prefer method 2), let's enable chksum feature only on superblock first, chksum_blocks feature can be added later. Thanks, > > Some perferred idea about this? I plan to release erofs-utils v1.0 tomorrow > and hold up this feature for the next erofs-utils release, but I think we can > get it ready for v5.5 since it is not quite complex feature... > > Thanks, > Gao Xiang > > . >
WARNING: multiple messages have this Message-ID (diff)
From: Chao Yu <yuchao0@huawei.com> To: Gao Xiang <gaoxiang25@huawei.com> Cc: Gao Xiang <xiang@kernel.org>, linux-erofs@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4] erofs: support superblock checksum Date: Mon, 28 Oct 2019 20:36:00 +0800 [thread overview] Message-ID: <df7d7427-e7ca-5135-5db2-640eda30d253@huawei.com> (raw) In-Reply-To: <20191023084536.GA16289@architecture4> On 2019/10/23 16:45, Gao Xiang wrote: > Hi Chao, > > On Wed, Oct 23, 2019 at 04:15:29PM +0800, Chao Yu wrote: >> Hi, Xiang, Pratik, >> >> On 2019/10/23 12:05, Gao Xiang wrote: > > <snip> > >>> } >>> >>> +static int erofs_superblock_csum_verify(struct super_block *sb, void *sbdata) >>> +{ >>> + struct erofs_super_block *dsb; >>> + u32 expected_crc, nblocks, crc; >>> + void *kaddr; >>> + struct page *page; >>> + int i; >>> + >>> + dsb = kmemdup(sbdata + EROFS_SUPER_OFFSET, >>> + EROFS_BLKSIZ - EROFS_SUPER_OFFSET, GFP_KERNEL); >>> + if (!dsb) >>> + return -ENOMEM; >>> + >>> + expected_crc = le32_to_cpu(dsb->checksum); >>> + nblocks = le32_to_cpu(dsb->chksum_blocks); >> >> Now, we try to use nblocks's value before checking its validation, I guess fuzz >> test can easily make the value extreme larger, result in checking latter blocks >> unnecessarily. >> >> IMO, we'd better >> 1. check validation of superblock to make sure all fields in sb are valid >> 2. use .nblocks to count and check payload blocks following sb > > That is quite a good point. :-) > > My first thought is to check the following payloads of sb (e.g, some per-fs > metadata should be checked at mount time together. or for small images, check > the whole image at the mount time) as well since if we introduce a new feature > to some kernel version, forward compatibility needs to be considered. So it's > better to make proper scalability, for this case, we have some choices: > 1) limit `chksum_blocks' upbound at runtime (e.g. refuse >= 65536 blocks, > totally 256M.) > 2) just get rid of the whole `chksum_blocks' mess and checksum the first 4k > at all, don't consider any latter scalability. Xiang, sorry for later reply... I prefer method 2), let's enable chksum feature only on superblock first, chksum_blocks feature can be added later. Thanks, > > Some perferred idea about this? I plan to release erofs-utils v1.0 tomorrow > and hold up this feature for the next erofs-utils release, but I think we can > get it ready for v5.5 since it is not quite complex feature... > > Thanks, > Gao Xiang > > . >
next prev parent reply other threads:[~2019-10-28 12:36 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-22 18:06 [PATCH-v3] erofs: code for verifying superblock checksum of an erofs image Pratik Shinde 2019-10-22 18:06 ` Pratik Shinde 2019-10-23 4:05 ` [PATCH v4] erofs: support superblock checksum Gao Xiang 2019-10-23 4:05 ` Gao Xiang 2019-10-23 8:15 ` Chao Yu 2019-10-23 8:15 ` Chao Yu 2019-10-23 8:45 ` Gao Xiang 2019-10-23 8:45 ` Gao Xiang 2019-10-28 12:36 ` Chao Yu [this message] 2019-10-28 12:36 ` Chao Yu 2019-10-28 13:44 ` Gao Xiang 2019-10-28 13:44 ` Gao Xiang 2019-10-28 14:32 ` [PATCH v5] " Gao Xiang 2019-10-28 14:32 ` Gao Xiang 2019-10-30 2:33 ` Chao Yu 2019-10-30 2:33 ` Chao Yu 2019-10-30 2:56 ` Gao Xiang 2019-10-30 2:56 ` Gao Xiang 2019-10-30 5:08 ` [PATCH v6] " Gao Xiang 2019-10-30 5:08 ` Gao Xiang 2019-11-04 2:49 ` [PATCH v7] " Gao Xiang 2019-11-04 2:49 ` Gao Xiang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=df7d7427-e7ca-5135-5db2-640eda30d253@huawei.com \ --to=yuchao0@huawei.com \ --cc=chao@kernel.org \ --cc=gaoxiang25@huawei.com \ --cc=linux-erofs@lists.ozlabs.org \ --cc=linux-kernel@vger.kernel.org \ --cc=xiang@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.