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 X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D4CC2C5517A for ; Tue, 10 Nov 2020 06:27:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 77FC0207BC for ; Tue, 10 Nov 2020 06:27:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731210AbgKJG14 (ORCPT ); Tue, 10 Nov 2020 01:27:56 -0500 Received: from szxga05-in.huawei.com ([45.249.212.191]:7511 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726010AbgKJG14 (ORCPT ); Tue, 10 Nov 2020 01:27:56 -0500 Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4CVdFs3nQZzhjlN; Tue, 10 Nov 2020 14:27:45 +0800 (CST) Received: from [10.136.114.67] (10.136.114.67) by smtp.huawei.com (10.3.19.211) with Microsoft SMTP Server (TLS) id 14.3.487.0; Tue, 10 Nov 2020 14:27:50 +0800 Subject: Re: [f2fs-dev] [PATCH] f2fs: compress: support chksum To: Jaegeuk Kim CC: , References: <20201102122333.76667-1-yuchao0@huawei.com> <20201102163123.GD529594@google.com> <756e482c-b638-1c09-3868-ae45d33ed2c2@huawei.com> <6b5bce0e-c967-b9cf-3544-a8e65595059c@huawei.com> <20201106211247.GA1474936@google.com> <908682bb-486c-222f-bea7-43fc961ef1b0@huawei.com> <20201109170625.GB2129970@google.com> <3417aea5-ace8-74be-ec26-f491dddea676@huawei.com> <20201110042353.GB1598246@google.com> From: Chao Yu Message-ID: <513c56d7-cefd-37a8-efdf-fa1ac8c2a1d3@huawei.com> Date: Tue, 10 Nov 2020 14:27:49 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20201110042353.GB1598246@google.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.136.114.67] X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2020/11/10 12:23, Jaegeuk Kim wrote: > On 11/10, Chao Yu wrote: >> On 2020/11/10 1:06, Jaegeuk Kim wrote: >>> On 11/09, Chao Yu wrote: >>>> On 2020/11/7 5:12, Jaegeuk Kim wrote: >>>>> On 11/03, Chao Yu wrote: >>>>>> On 2020/11/3 10:02, Chao Yu wrote: >>>>>>> On 2020/11/3 0:31, Jaegeuk Kim wrote: >>>>>>>> On 11/02, Chao Yu wrote: >>>>>>>>> This patch supports to store chksum value with compressed >>>>>>>>> data, and verify the integrality of compressed data while >>>>>>>>> reading the data. >>>>>>>>> >>>>>>>>> The feature can be enabled through specifying mount option >>>>>>>>> 'compress_chksum'. >>>>>>>>> >>>>>>>>> Signed-off-by: Chao Yu >>>>>>>>> --- >>>>>>>>> Documentation/filesystems/f2fs.rst | 1 + >>>>>>>>> fs/f2fs/compress.c | 20 ++++++++++++++++++++ >>>>>>>>> fs/f2fs/f2fs.h | 13 ++++++++++++- >>>>>>>>> fs/f2fs/inode.c | 3 +++ >>>>>>>>> fs/f2fs/super.c | 9 +++++++++ >>>>>>>>> include/linux/f2fs_fs.h | 2 +- >>>>>>>>> 6 files changed, 46 insertions(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst >>>>>>>>> index b8ee761c9922..985ae7d35066 100644 >>>>>>>>> --- a/Documentation/filesystems/f2fs.rst >>>>>>>>> +++ b/Documentation/filesystems/f2fs.rst >>>>>>>>> @@ -260,6 +260,7 @@ compress_extension=%s Support adding specified extension, so that f2fs can enab >>>>>>>>> For other files, we can still enable compression via ioctl. >>>>>>>>> Note that, there is one reserved special extension '*', it >>>>>>>>> can be set to enable compression for all files. >>>>>>>>> +compress_chksum Support verifying chksum of raw data in compressed cluster. >>>>>>>>> inlinecrypt When possible, encrypt/decrypt the contents of encrypted >>>>>>>>> files using the blk-crypto framework rather than >>>>>>>>> filesystem-layer encryption. This allows the use of >>>>>>>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c >>>>>>>>> index 14262e0f1cd6..a4e0d2c745b6 100644 >>>>>>>>> --- a/fs/f2fs/compress.c >>>>>>>>> +++ b/fs/f2fs/compress.c >>>>>>>>> @@ -602,6 +602,7 @@ static int f2fs_compress_pages(struct compress_ctx *cc) >>>>>>>>> f2fs_cops[fi->i_compress_algorithm]; >>>>>>>>> unsigned int max_len, new_nr_cpages; >>>>>>>>> struct page **new_cpages; >>>>>>>>> + u32 chksum = 0; >>>>>>>>> int i, ret; >>>>>>>>> trace_f2fs_compress_pages_start(cc->inode, cc->cluster_idx, >>>>>>>>> @@ -655,6 +656,11 @@ static int f2fs_compress_pages(struct compress_ctx *cc) >>>>>>>>> cc->cbuf->clen = cpu_to_le32(cc->clen); >>>>>>>>> + if (fi->i_compress_flag & 1 << COMPRESS_CHKSUM) >>>>>>>>> + chksum = f2fs_crc32(F2FS_I_SB(cc->inode), >>>>>>>>> + cc->cbuf->cdata, cc->clen); >>>>>>>>> + cc->cbuf->chksum = cpu_to_le32(chksum); >>>>>>>>> + >>>>>>>>> for (i = 0; i < COMPRESS_DATA_RESERVED_SIZE; i++) >>>>>>>>> cc->cbuf->reserved[i] = cpu_to_le32(0); >>>>>>>>> @@ -721,6 +727,7 @@ void f2fs_decompress_pages(struct bio *bio, struct page *page, bool verity) >>>>>>>>> (struct decompress_io_ctx *)page_private(page); >>>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(dic->inode); >>>>>>>>> struct f2fs_inode_info *fi= F2FS_I(dic->inode); >>>>>>>>> + struct f2fs_sb_info *sbi = F2FS_I_SB(dic->inode); >>>>>>>>> const struct f2fs_compress_ops *cops = >>>>>>>>> f2fs_cops[fi->i_compress_algorithm]; >>>>>>>>> int ret; >>>>>>>>> @@ -790,6 +797,19 @@ void f2fs_decompress_pages(struct bio *bio, struct page *page, bool verity) >>>>>>>>> ret = cops->decompress_pages(dic); >>>>>>>>> + if (!ret && fi->i_compress_flag & 1 << COMPRESS_CHKSUM) { >>>>>>>>> + u32 provided = le32_to_cpu(dic->cbuf->chksum); >>>>>>>>> + u32 calculated = f2fs_crc32(sbi, dic->cbuf->cdata, dic->clen); >>>>>>>>> + >>>>>>>>> + if (provided != calculated) { >>>>>>>>> + printk_ratelimited( >>>>>>>>> + "%sF2FS-fs (%s): checksum invalid, nid = %lu, %x vs %x", >>>>>>>>> + KERN_INFO, sbi->sb->s_id, dic->inode->i_ino, >>>>>>>>> + provided, calculated); >>>>>>>>> + ret = -EFSCORRUPTED; >>>>>>>> >>>>>>>> Do we need to change fsck.f2fs to recover this? >>>>>> >>>>>> However, we don't know which one is correct, compressed data or chksum value? >>>>>> if compressed data was corrupted, repairing chksum value doesn't help. >>>>>> >>>>>> Or how about adding chksum values for both raw data and compressed data. >>>>>> >>>>>> #define COMPRESS_DATA_RESERVED_SIZE 3 >>>>>> struct compress_data { >>>>>> __le32 clen; /* compressed data size */ >>>>>> + __le32 raw_chksum; /* raw data chksum */ >>>>>> + __le32 compress_chksum; /* compressed data chksum */ >>>>>> __le32 reserved[COMPRESS_DATA_RESERVED_SIZE]; /* reserved */ >>>>>> u8 cdata[]; /* compressed data */ >>>>>> }; >>>>>> >>>>>> raw_chksum compress_chksum >>>>>> match match -> data is verified, pass >>>>>> not match match -> repair raw_chksum >>>>>> matcth not match -> repair compress_chksum >>>>> >>>>> I think only compress_chksum would be enough. BTW, can we give WARN_ON and >>>>> marking a FSCK flag without returning EFSCORRUPTED, since we don't really >>>>> know who was corrupted. If data was corrupted, we should be able to see app >>>>> corruption. In that case, we can check the kernel log. If checksum was simply >>>> >>>> I don't think that app will always corrupt once data was corrupted, I doubt its >>>> behavior could be slightly abnormal in some cases, e.g. it can just cause apps to >>>> show wrong number in interaction interface. >>> >>> I didn't say we can always get it. But, it's likely to happen with something >>> like that. >>> >>>> >>>> In this case, if we fix chksum in fsck, the wrong data will never be found due to >>>> data's chksum matches data itself after repair. >>> >>> At least, we should see that log as a hint. >>> >>>> >>>> IMO, the chksum and data was a whole dataset, once they are mismatch, we can >>>> not trust either of them, fsck should do nothing on them unless we store parity >>>> bits or replica. >>> >>> Yeah, the point is those are not written as a transaction. Three concerns to me: >> >> Just to confirm: >> >>> 1) agreed to see compressed data corruption by checksum >> >> return EFSBADCRC always like we did as inode chksum feature. > > inode chksum is a bit different, since 4KB write is atomic. If chksum is > corrupted, inode should be broken. Actually, I think the both results are the same, inode chksum doesn't match inode metadata, like current case that cluster chksum doesn't match cluster data, it doesn't matter how it becomes mismatched. And also, in those inode corrupted cases, there should be some cases that hacker or fuzz tester injects random data in chksum intentionally, or bit-flipping happed on chksum value in inode, inode metadata (except inode chksum) is integrated though, we can not distinguish such cases from case of inode metadata (except inode chksum) corruption. Thanks, > >> >> 2) I don't want to see the error messages all the time >> >> Something like below? >> >> if (provided != calculated) { >> if (!is_inode_flag_set(,FI_DATA_CORRUPTED)) { >> printk_ratelimited(); >> WARN_ON() >> set_inode_flag(, FI_DATA_CORRUPTED); >> } >> return EFSBADCRC; > > No, what if only checksum was wrong? I mean giving WARN_ON() and setting > FSCK_FLAG to stop endless WARN_ON(). > >> } >> >> 3) don't touch the data, even if it was corrupted, since there's no way to fix it. >> >> It seems that we don't need to change fsck.f2fs finally... >> >> Thanks, >> >>> >>>> >>>> Thanks, >>>> >>>>> corrupted, next fsck will fix the checksum. So, in general, I hope to keep >>>>> the data as is and raise a flag by the checksum. >>>>> >>>>>> not match not match -> corrupted, can not repair >>>>>> >>>>>> Thanks, >>>>>> >>>>>>> >>>>>>> Yes, prepared to update inode layout in fsck.f2fs w/ kernel side change. > >>>>>>> Thanks, >>>>>>> >>>>>>>> >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> out_vunmap_cbuf: >>>>>>>>> vm_unmap_ram(dic->cbuf, dic->nr_cpages); >>>>>>>>> out_vunmap_rbuf: >>>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>>>>>>> index 99bcf4b44a9c..2ae254ab7b7d 100644 >>>>>>>>> --- a/fs/f2fs/f2fs.h >>>>>>>>> +++ b/fs/f2fs/f2fs.h >>>>>>>>> @@ -147,7 +147,8 @@ struct f2fs_mount_info { >>>>>>>>> /* For compression */ >>>>>>>>> unsigned char compress_algorithm; /* algorithm type */ >>>>>>>>> - unsigned compress_log_size; /* cluster log size */ >>>>>>>>> + unsigned char compress_log_size; /* cluster log size */ >>>>>>>>> + bool compress_chksum; /* compressed data chksum */ >>>>>>>>> unsigned char compress_ext_cnt; /* extension count */ >>>>>>>>> unsigned char extensions[COMPRESS_EXT_NUM][F2FS_EXTENSION_LEN]; /* extensions */ >>>>>>>>> }; >>>>>>>>> @@ -731,6 +732,7 @@ struct f2fs_inode_info { >>>>>>>>> atomic_t i_compr_blocks; /* # of compressed blocks */ >>>>>>>>> unsigned char i_compress_algorithm; /* algorithm type */ >>>>>>>>> unsigned char i_log_cluster_size; /* log of cluster size */ >>>>>>>>> + unsigned short i_compress_flag; /* compress flag */ >>>>>>>>> unsigned int i_cluster_size; /* cluster size */ >>>>>>>>> }; >>>>>>>>> @@ -1270,9 +1272,15 @@ enum compress_algorithm_type { >>>>>>>>> COMPRESS_MAX, >>>>>>>>> }; >>>>>>>>> +enum compress_flag { >>>>>>>>> + COMPRESS_CHKSUM, >>>>>>>>> + COMPRESS_MAX_FLAG, >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> #define COMPRESS_DATA_RESERVED_SIZE 5 >>>>>>>>> struct compress_data { >>>>>>>>> __le32 clen; /* compressed data size */ >>>>>>>>> + __le32 chksum; /* compressed data chksum */ >>>>>>>>> __le32 reserved[COMPRESS_DATA_RESERVED_SIZE]; /* reserved */ >>>>>>>>> u8 cdata[]; /* compressed data */ >>>>>>>>> }; >>>>>>>>> @@ -3882,6 +3890,9 @@ static inline void set_compress_context(struct inode *inode) >>>>>>>>> F2FS_OPTION(sbi).compress_algorithm; >>>>>>>>> F2FS_I(inode)->i_log_cluster_size = >>>>>>>>> F2FS_OPTION(sbi).compress_log_size; >>>>>>>>> + F2FS_I(inode)->i_compress_flag = >>>>>>>>> + F2FS_OPTION(sbi).compress_chksum ? >>>>>>>>> + 1 << COMPRESS_CHKSUM : 0; >>>>>>>>> F2FS_I(inode)->i_cluster_size = >>>>>>>>> 1 << F2FS_I(inode)->i_log_cluster_size; >>>>>>>>> F2FS_I(inode)->i_flags |= F2FS_COMPR_FL; >>>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>>>>>>>> index 657db2fb6739..de8f7fc89efa 100644 >>>>>>>>> --- a/fs/f2fs/inode.c >>>>>>>>> +++ b/fs/f2fs/inode.c >>>>>>>>> @@ -456,6 +456,7 @@ static int do_read_inode(struct inode *inode) >>>>>>>>> le64_to_cpu(ri->i_compr_blocks)); >>>>>>>>> fi->i_compress_algorithm = ri->i_compress_algorithm; >>>>>>>>> fi->i_log_cluster_size = ri->i_log_cluster_size; >>>>>>>>> + fi->i_compress_flag = ri->i_compress_flag; >>>>>>>>> fi->i_cluster_size = 1 << fi->i_log_cluster_size; >>>>>>>>> set_inode_flag(inode, FI_COMPRESSED_FILE); >>>>>>>>> } >>>>>>>>> @@ -634,6 +635,8 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page) >>>>>>>>> &F2FS_I(inode)->i_compr_blocks)); >>>>>>>>> ri->i_compress_algorithm = >>>>>>>>> F2FS_I(inode)->i_compress_algorithm; >>>>>>>>> + ri->i_compress_flag = >>>>>>>>> + cpu_to_le16(F2FS_I(inode)->i_compress_flag); >>>>>>>>> ri->i_log_cluster_size = >>>>>>>>> F2FS_I(inode)->i_log_cluster_size; >>>>>>>>> } >>>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>>>>>>> index 00eff2f51807..f8de4d83a5be 100644 >>>>>>>>> --- a/fs/f2fs/super.c >>>>>>>>> +++ b/fs/f2fs/super.c >>>>>>>>> @@ -146,6 +146,7 @@ enum { >>>>>>>>> Opt_compress_algorithm, >>>>>>>>> Opt_compress_log_size, >>>>>>>>> Opt_compress_extension, >>>>>>>>> + Opt_compress_chksum, >>>>>>>>> Opt_atgc, >>>>>>>>> Opt_err, >>>>>>>>> }; >>>>>>>>> @@ -214,6 +215,7 @@ static match_table_t f2fs_tokens = { >>>>>>>>> {Opt_compress_algorithm, "compress_algorithm=%s"}, >>>>>>>>> {Opt_compress_log_size, "compress_log_size=%u"}, >>>>>>>>> {Opt_compress_extension, "compress_extension=%s"}, >>>>>>>>> + {Opt_compress_chksum, "compress_chksum"}, >>>>>>>>> {Opt_atgc, "atgc"}, >>>>>>>>> {Opt_err, NULL}, >>>>>>>>> }; >>>>>>>>> @@ -934,10 +936,14 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>>>>>>>> F2FS_OPTION(sbi).compress_ext_cnt++; >>>>>>>>> kfree(name); >>>>>>>>> break; >>>>>>>>> + case Opt_compress_chksum: >>>>>>>>> + F2FS_OPTION(sbi).compress_chksum = true; >>>>>>>>> + break; >>>>>>>>> #else >>>>>>>>> case Opt_compress_algorithm: >>>>>>>>> case Opt_compress_log_size: >>>>>>>>> case Opt_compress_extension: >>>>>>>>> + case Opt_compress_chksum: >>>>>>>>> f2fs_info(sbi, "compression options not supported"); >>>>>>>>> break; >>>>>>>>> #endif >>>>>>>>> @@ -1523,6 +1529,9 @@ static inline void f2fs_show_compress_options(struct seq_file *seq, >>>>>>>>> seq_printf(seq, ",compress_extension=%s", >>>>>>>>> F2FS_OPTION(sbi).extensions[i]); >>>>>>>>> } >>>>>>>>> + >>>>>>>>> + if (F2FS_OPTION(sbi).compress_chksum) >>>>>>>>> + seq_puts(seq, ",compress_chksum"); >>>>>>>>> } >>>>>>>>> static int f2fs_show_options(struct seq_file *seq, struct dentry *root) >>>>>>>>> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h >>>>>>>>> index a5dbb57a687f..7dc2a06cf19a 100644 >>>>>>>>> --- a/include/linux/f2fs_fs.h >>>>>>>>> +++ b/include/linux/f2fs_fs.h >>>>>>>>> @@ -273,7 +273,7 @@ struct f2fs_inode { >>>>>>>>> __le64 i_compr_blocks; /* # of compressed blocks */ >>>>>>>>> __u8 i_compress_algorithm; /* compress algorithm */ >>>>>>>>> __u8 i_log_cluster_size; /* log of cluster size */ >>>>>>>>> - __le16 i_padding; /* padding */ >>>>>>>>> + __le16 i_compress_flag; /* compress flag */ >>>>>>>>> __le32 i_extra_end[0]; /* for attribute size calculation */ >>>>>>>>> } __packed; >>>>>>>>> __le32 i_addr[DEF_ADDRS_PER_INODE]; /* Pointers to data blocks */ >>>>>>>>> -- >>>>>>>>> 2.26.2 >>>>>>>> . >>>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Linux-f2fs-devel mailing list >>>>>>> Linux-f2fs-devel@lists.sourceforge.net >>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>>>>>> . >>>>>>> >>>>> . >>>>> >>> . >>> > . > 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 X-Spam-Level: X-Spam-Status: No, score=-11.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1186CC388F7 for ; Tue, 10 Nov 2020 06:28:28 +0000 (UTC) Received: from lists.sourceforge.net (lists.sourceforge.net [216.105.38.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4662B2065D; Tue, 10 Nov 2020 06:28:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=sourceforge.net header.i=@sourceforge.net header.b="W40veK3O"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=sf.net header.i=@sf.net header.b="TGMDAwEj" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4662B2065D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linux-f2fs-devel-bounces@lists.sourceforge.net Received: from [127.0.0.1] (helo=sfs-ml-2.v29.lw.sourceforge.com) by sfs-ml-2.v29.lw.sourceforge.com with esmtp (Exim 4.90_1) (envelope-from ) id 1kcN8k-00063Q-71; Tue, 10 Nov 2020 06:28:26 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kcN8X-00062u-D1 for linux-f2fs-devel@lists.sourceforge.net; Tue, 10 Nov 2020 06:28:13 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:CC:To:Subject:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=FXc25obaoasVR3tY0yYYdOWpO6ktsh036JuRK7FF0Zw=; b=W40veK3Oj0jXUljk67IO7AdV/Z vgya7nJ+86XkrUTgLGYR0mT2OfWSCfqfg9+E4eQi4DA7JcjRyOeeMWQijp52j1eccuC0GkGkXQ2Yl gJWUpxPXRdpZLEJncDbSVzGPT3DmdVFHkzwQrA1OutsbNeJU5XmWiOVMDB9k8GRQKIPY=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date: Message-ID:From:References:CC:To:Subject:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=FXc25obaoasVR3tY0yYYdOWpO6ktsh036JuRK7FF0Zw=; b=TGMDAwEjNfweEKgMoTi8w668Hn Wlx3XXJRcv/aV27Rw0GkVuHfftmxPbe+4Bja4ENAmmR4+XfQYtem0aqkQhz/gjW9skDgPCJsK3r+P lTOlwF1xfv37ieRW7J6dSx4zEBQR5UVwK7XoHxl8HsGcvgxJ2J2R2NOmeCz6uKW6Zn6s=; Received: from szxga05-in.huawei.com ([45.249.212.191]) by sfi-mx-1.v28.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92.2) id 1kcN8O-00F8GU-5W for linux-f2fs-devel@lists.sourceforge.net; Tue, 10 Nov 2020 06:28:13 +0000 Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4CVdFs3nQZzhjlN; Tue, 10 Nov 2020 14:27:45 +0800 (CST) Received: from [10.136.114.67] (10.136.114.67) by smtp.huawei.com (10.3.19.211) with Microsoft SMTP Server (TLS) id 14.3.487.0; Tue, 10 Nov 2020 14:27:50 +0800 To: Jaegeuk Kim References: <20201102122333.76667-1-yuchao0@huawei.com> <20201102163123.GD529594@google.com> <756e482c-b638-1c09-3868-ae45d33ed2c2@huawei.com> <6b5bce0e-c967-b9cf-3544-a8e65595059c@huawei.com> <20201106211247.GA1474936@google.com> <908682bb-486c-222f-bea7-43fc961ef1b0@huawei.com> <20201109170625.GB2129970@google.com> <3417aea5-ace8-74be-ec26-f491dddea676@huawei.com> <20201110042353.GB1598246@google.com> From: Chao Yu Message-ID: <513c56d7-cefd-37a8-efdf-fa1ac8c2a1d3@huawei.com> Date: Tue, 10 Nov 2020 14:27:49 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20201110042353.GB1598246@google.com> Content-Language: en-US X-Originating-IP: [10.136.114.67] X-CFilter-Loop: Reflected X-Headers-End: 1kcN8O-00F8GU-5W Subject: Re: [f2fs-dev] [PATCH] f2fs: compress: support chksum X-BeenThere: linux-f2fs-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net On 2020/11/10 12:23, Jaegeuk Kim wrote: > On 11/10, Chao Yu wrote: >> On 2020/11/10 1:06, Jaegeuk Kim wrote: >>> On 11/09, Chao Yu wrote: >>>> On 2020/11/7 5:12, Jaegeuk Kim wrote: >>>>> On 11/03, Chao Yu wrote: >>>>>> On 2020/11/3 10:02, Chao Yu wrote: >>>>>>> On 2020/11/3 0:31, Jaegeuk Kim wrote: >>>>>>>> On 11/02, Chao Yu wrote: >>>>>>>>> This patch supports to store chksum value with compressed >>>>>>>>> data, and verify the integrality of compressed data while >>>>>>>>> reading the data. >>>>>>>>> >>>>>>>>> The feature can be enabled through specifying mount option >>>>>>>>> 'compress_chksum'. >>>>>>>>> >>>>>>>>> Signed-off-by: Chao Yu >>>>>>>>> --- >>>>>>>>> Documentation/filesystems/f2fs.rst | 1 + >>>>>>>>> fs/f2fs/compress.c | 20 ++++++++++++++++++++ >>>>>>>>> fs/f2fs/f2fs.h | 13 ++++++++++++- >>>>>>>>> fs/f2fs/inode.c | 3 +++ >>>>>>>>> fs/f2fs/super.c | 9 +++++++++ >>>>>>>>> include/linux/f2fs_fs.h | 2 +- >>>>>>>>> 6 files changed, 46 insertions(+), 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst >>>>>>>>> index b8ee761c9922..985ae7d35066 100644 >>>>>>>>> --- a/Documentation/filesystems/f2fs.rst >>>>>>>>> +++ b/Documentation/filesystems/f2fs.rst >>>>>>>>> @@ -260,6 +260,7 @@ compress_extension=%s Support adding specified extension, so that f2fs can enab >>>>>>>>> For other files, we can still enable compression via ioctl. >>>>>>>>> Note that, there is one reserved special extension '*', it >>>>>>>>> can be set to enable compression for all files. >>>>>>>>> +compress_chksum Support verifying chksum of raw data in compressed cluster. >>>>>>>>> inlinecrypt When possible, encrypt/decrypt the contents of encrypted >>>>>>>>> files using the blk-crypto framework rather than >>>>>>>>> filesystem-layer encryption. This allows the use of >>>>>>>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c >>>>>>>>> index 14262e0f1cd6..a4e0d2c745b6 100644 >>>>>>>>> --- a/fs/f2fs/compress.c >>>>>>>>> +++ b/fs/f2fs/compress.c >>>>>>>>> @@ -602,6 +602,7 @@ static int f2fs_compress_pages(struct compress_ctx *cc) >>>>>>>>> f2fs_cops[fi->i_compress_algorithm]; >>>>>>>>> unsigned int max_len, new_nr_cpages; >>>>>>>>> struct page **new_cpages; >>>>>>>>> + u32 chksum = 0; >>>>>>>>> int i, ret; >>>>>>>>> trace_f2fs_compress_pages_start(cc->inode, cc->cluster_idx, >>>>>>>>> @@ -655,6 +656,11 @@ static int f2fs_compress_pages(struct compress_ctx *cc) >>>>>>>>> cc->cbuf->clen = cpu_to_le32(cc->clen); >>>>>>>>> + if (fi->i_compress_flag & 1 << COMPRESS_CHKSUM) >>>>>>>>> + chksum = f2fs_crc32(F2FS_I_SB(cc->inode), >>>>>>>>> + cc->cbuf->cdata, cc->clen); >>>>>>>>> + cc->cbuf->chksum = cpu_to_le32(chksum); >>>>>>>>> + >>>>>>>>> for (i = 0; i < COMPRESS_DATA_RESERVED_SIZE; i++) >>>>>>>>> cc->cbuf->reserved[i] = cpu_to_le32(0); >>>>>>>>> @@ -721,6 +727,7 @@ void f2fs_decompress_pages(struct bio *bio, struct page *page, bool verity) >>>>>>>>> (struct decompress_io_ctx *)page_private(page); >>>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(dic->inode); >>>>>>>>> struct f2fs_inode_info *fi= F2FS_I(dic->inode); >>>>>>>>> + struct f2fs_sb_info *sbi = F2FS_I_SB(dic->inode); >>>>>>>>> const struct f2fs_compress_ops *cops = >>>>>>>>> f2fs_cops[fi->i_compress_algorithm]; >>>>>>>>> int ret; >>>>>>>>> @@ -790,6 +797,19 @@ void f2fs_decompress_pages(struct bio *bio, struct page *page, bool verity) >>>>>>>>> ret = cops->decompress_pages(dic); >>>>>>>>> + if (!ret && fi->i_compress_flag & 1 << COMPRESS_CHKSUM) { >>>>>>>>> + u32 provided = le32_to_cpu(dic->cbuf->chksum); >>>>>>>>> + u32 calculated = f2fs_crc32(sbi, dic->cbuf->cdata, dic->clen); >>>>>>>>> + >>>>>>>>> + if (provided != calculated) { >>>>>>>>> + printk_ratelimited( >>>>>>>>> + "%sF2FS-fs (%s): checksum invalid, nid = %lu, %x vs %x", >>>>>>>>> + KERN_INFO, sbi->sb->s_id, dic->inode->i_ino, >>>>>>>>> + provided, calculated); >>>>>>>>> + ret = -EFSCORRUPTED; >>>>>>>> >>>>>>>> Do we need to change fsck.f2fs to recover this? >>>>>> >>>>>> However, we don't know which one is correct, compressed data or chksum value? >>>>>> if compressed data was corrupted, repairing chksum value doesn't help. >>>>>> >>>>>> Or how about adding chksum values for both raw data and compressed data. >>>>>> >>>>>> #define COMPRESS_DATA_RESERVED_SIZE 3 >>>>>> struct compress_data { >>>>>> __le32 clen; /* compressed data size */ >>>>>> + __le32 raw_chksum; /* raw data chksum */ >>>>>> + __le32 compress_chksum; /* compressed data chksum */ >>>>>> __le32 reserved[COMPRESS_DATA_RESERVED_SIZE]; /* reserved */ >>>>>> u8 cdata[]; /* compressed data */ >>>>>> }; >>>>>> >>>>>> raw_chksum compress_chksum >>>>>> match match -> data is verified, pass >>>>>> not match match -> repair raw_chksum >>>>>> matcth not match -> repair compress_chksum >>>>> >>>>> I think only compress_chksum would be enough. BTW, can we give WARN_ON and >>>>> marking a FSCK flag without returning EFSCORRUPTED, since we don't really >>>>> know who was corrupted. If data was corrupted, we should be able to see app >>>>> corruption. In that case, we can check the kernel log. If checksum was simply >>>> >>>> I don't think that app will always corrupt once data was corrupted, I doubt its >>>> behavior could be slightly abnormal in some cases, e.g. it can just cause apps to >>>> show wrong number in interaction interface. >>> >>> I didn't say we can always get it. But, it's likely to happen with something >>> like that. >>> >>>> >>>> In this case, if we fix chksum in fsck, the wrong data will never be found due to >>>> data's chksum matches data itself after repair. >>> >>> At least, we should see that log as a hint. >>> >>>> >>>> IMO, the chksum and data was a whole dataset, once they are mismatch, we can >>>> not trust either of them, fsck should do nothing on them unless we store parity >>>> bits or replica. >>> >>> Yeah, the point is those are not written as a transaction. Three concerns to me: >> >> Just to confirm: >> >>> 1) agreed to see compressed data corruption by checksum >> >> return EFSBADCRC always like we did as inode chksum feature. > > inode chksum is a bit different, since 4KB write is atomic. If chksum is > corrupted, inode should be broken. Actually, I think the both results are the same, inode chksum doesn't match inode metadata, like current case that cluster chksum doesn't match cluster data, it doesn't matter how it becomes mismatched. And also, in those inode corrupted cases, there should be some cases that hacker or fuzz tester injects random data in chksum intentionally, or bit-flipping happed on chksum value in inode, inode metadata (except inode chksum) is integrated though, we can not distinguish such cases from case of inode metadata (except inode chksum) corruption. Thanks, > >> >> 2) I don't want to see the error messages all the time >> >> Something like below? >> >> if (provided != calculated) { >> if (!is_inode_flag_set(,FI_DATA_CORRUPTED)) { >> printk_ratelimited(); >> WARN_ON() >> set_inode_flag(, FI_DATA_CORRUPTED); >> } >> return EFSBADCRC; > > No, what if only checksum was wrong? I mean giving WARN_ON() and setting > FSCK_FLAG to stop endless WARN_ON(). > >> } >> >> 3) don't touch the data, even if it was corrupted, since there's no way to fix it. >> >> It seems that we don't need to change fsck.f2fs finally... >> >> Thanks, >> >>> >>>> >>>> Thanks, >>>> >>>>> corrupted, next fsck will fix the checksum. So, in general, I hope to keep >>>>> the data as is and raise a flag by the checksum. >>>>> >>>>>> not match not match -> corrupted, can not repair >>>>>> >>>>>> Thanks, >>>>>> >>>>>>> >>>>>>> Yes, prepared to update inode layout in fsck.f2fs w/ kernel side change. > >>>>>>> Thanks, >>>>>>> >>>>>>>> >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> out_vunmap_cbuf: >>>>>>>>> vm_unmap_ram(dic->cbuf, dic->nr_cpages); >>>>>>>>> out_vunmap_rbuf: >>>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>>>>>>> index 99bcf4b44a9c..2ae254ab7b7d 100644 >>>>>>>>> --- a/fs/f2fs/f2fs.h >>>>>>>>> +++ b/fs/f2fs/f2fs.h >>>>>>>>> @@ -147,7 +147,8 @@ struct f2fs_mount_info { >>>>>>>>> /* For compression */ >>>>>>>>> unsigned char compress_algorithm; /* algorithm type */ >>>>>>>>> - unsigned compress_log_size; /* cluster log size */ >>>>>>>>> + unsigned char compress_log_size; /* cluster log size */ >>>>>>>>> + bool compress_chksum; /* compressed data chksum */ >>>>>>>>> unsigned char compress_ext_cnt; /* extension count */ >>>>>>>>> unsigned char extensions[COMPRESS_EXT_NUM][F2FS_EXTENSION_LEN]; /* extensions */ >>>>>>>>> }; >>>>>>>>> @@ -731,6 +732,7 @@ struct f2fs_inode_info { >>>>>>>>> atomic_t i_compr_blocks; /* # of compressed blocks */ >>>>>>>>> unsigned char i_compress_algorithm; /* algorithm type */ >>>>>>>>> unsigned char i_log_cluster_size; /* log of cluster size */ >>>>>>>>> + unsigned short i_compress_flag; /* compress flag */ >>>>>>>>> unsigned int i_cluster_size; /* cluster size */ >>>>>>>>> }; >>>>>>>>> @@ -1270,9 +1272,15 @@ enum compress_algorithm_type { >>>>>>>>> COMPRESS_MAX, >>>>>>>>> }; >>>>>>>>> +enum compress_flag { >>>>>>>>> + COMPRESS_CHKSUM, >>>>>>>>> + COMPRESS_MAX_FLAG, >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> #define COMPRESS_DATA_RESERVED_SIZE 5 >>>>>>>>> struct compress_data { >>>>>>>>> __le32 clen; /* compressed data size */ >>>>>>>>> + __le32 chksum; /* compressed data chksum */ >>>>>>>>> __le32 reserved[COMPRESS_DATA_RESERVED_SIZE]; /* reserved */ >>>>>>>>> u8 cdata[]; /* compressed data */ >>>>>>>>> }; >>>>>>>>> @@ -3882,6 +3890,9 @@ static inline void set_compress_context(struct inode *inode) >>>>>>>>> F2FS_OPTION(sbi).compress_algorithm; >>>>>>>>> F2FS_I(inode)->i_log_cluster_size = >>>>>>>>> F2FS_OPTION(sbi).compress_log_size; >>>>>>>>> + F2FS_I(inode)->i_compress_flag = >>>>>>>>> + F2FS_OPTION(sbi).compress_chksum ? >>>>>>>>> + 1 << COMPRESS_CHKSUM : 0; >>>>>>>>> F2FS_I(inode)->i_cluster_size = >>>>>>>>> 1 << F2FS_I(inode)->i_log_cluster_size; >>>>>>>>> F2FS_I(inode)->i_flags |= F2FS_COMPR_FL; >>>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>>>>>>>> index 657db2fb6739..de8f7fc89efa 100644 >>>>>>>>> --- a/fs/f2fs/inode.c >>>>>>>>> +++ b/fs/f2fs/inode.c >>>>>>>>> @@ -456,6 +456,7 @@ static int do_read_inode(struct inode *inode) >>>>>>>>> le64_to_cpu(ri->i_compr_blocks)); >>>>>>>>> fi->i_compress_algorithm = ri->i_compress_algorithm; >>>>>>>>> fi->i_log_cluster_size = ri->i_log_cluster_size; >>>>>>>>> + fi->i_compress_flag = ri->i_compress_flag; >>>>>>>>> fi->i_cluster_size = 1 << fi->i_log_cluster_size; >>>>>>>>> set_inode_flag(inode, FI_COMPRESSED_FILE); >>>>>>>>> } >>>>>>>>> @@ -634,6 +635,8 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page) >>>>>>>>> &F2FS_I(inode)->i_compr_blocks)); >>>>>>>>> ri->i_compress_algorithm = >>>>>>>>> F2FS_I(inode)->i_compress_algorithm; >>>>>>>>> + ri->i_compress_flag = >>>>>>>>> + cpu_to_le16(F2FS_I(inode)->i_compress_flag); >>>>>>>>> ri->i_log_cluster_size = >>>>>>>>> F2FS_I(inode)->i_log_cluster_size; >>>>>>>>> } >>>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>>>>>>> index 00eff2f51807..f8de4d83a5be 100644 >>>>>>>>> --- a/fs/f2fs/super.c >>>>>>>>> +++ b/fs/f2fs/super.c >>>>>>>>> @@ -146,6 +146,7 @@ enum { >>>>>>>>> Opt_compress_algorithm, >>>>>>>>> Opt_compress_log_size, >>>>>>>>> Opt_compress_extension, >>>>>>>>> + Opt_compress_chksum, >>>>>>>>> Opt_atgc, >>>>>>>>> Opt_err, >>>>>>>>> }; >>>>>>>>> @@ -214,6 +215,7 @@ static match_table_t f2fs_tokens = { >>>>>>>>> {Opt_compress_algorithm, "compress_algorithm=%s"}, >>>>>>>>> {Opt_compress_log_size, "compress_log_size=%u"}, >>>>>>>>> {Opt_compress_extension, "compress_extension=%s"}, >>>>>>>>> + {Opt_compress_chksum, "compress_chksum"}, >>>>>>>>> {Opt_atgc, "atgc"}, >>>>>>>>> {Opt_err, NULL}, >>>>>>>>> }; >>>>>>>>> @@ -934,10 +936,14 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>>>>>>>> F2FS_OPTION(sbi).compress_ext_cnt++; >>>>>>>>> kfree(name); >>>>>>>>> break; >>>>>>>>> + case Opt_compress_chksum: >>>>>>>>> + F2FS_OPTION(sbi).compress_chksum = true; >>>>>>>>> + break; >>>>>>>>> #else >>>>>>>>> case Opt_compress_algorithm: >>>>>>>>> case Opt_compress_log_size: >>>>>>>>> case Opt_compress_extension: >>>>>>>>> + case Opt_compress_chksum: >>>>>>>>> f2fs_info(sbi, "compression options not supported"); >>>>>>>>> break; >>>>>>>>> #endif >>>>>>>>> @@ -1523,6 +1529,9 @@ static inline void f2fs_show_compress_options(struct seq_file *seq, >>>>>>>>> seq_printf(seq, ",compress_extension=%s", >>>>>>>>> F2FS_OPTION(sbi).extensions[i]); >>>>>>>>> } >>>>>>>>> + >>>>>>>>> + if (F2FS_OPTION(sbi).compress_chksum) >>>>>>>>> + seq_puts(seq, ",compress_chksum"); >>>>>>>>> } >>>>>>>>> static int f2fs_show_options(struct seq_file *seq, struct dentry *root) >>>>>>>>> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h >>>>>>>>> index a5dbb57a687f..7dc2a06cf19a 100644 >>>>>>>>> --- a/include/linux/f2fs_fs.h >>>>>>>>> +++ b/include/linux/f2fs_fs.h >>>>>>>>> @@ -273,7 +273,7 @@ struct f2fs_inode { >>>>>>>>> __le64 i_compr_blocks; /* # of compressed blocks */ >>>>>>>>> __u8 i_compress_algorithm; /* compress algorithm */ >>>>>>>>> __u8 i_log_cluster_size; /* log of cluster size */ >>>>>>>>> - __le16 i_padding; /* padding */ >>>>>>>>> + __le16 i_compress_flag; /* compress flag */ >>>>>>>>> __le32 i_extra_end[0]; /* for attribute size calculation */ >>>>>>>>> } __packed; >>>>>>>>> __le32 i_addr[DEF_ADDRS_PER_INODE]; /* Pointers to data blocks */ >>>>>>>>> -- >>>>>>>>> 2.26.2 >>>>>>>> . >>>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Linux-f2fs-devel mailing list >>>>>>> Linux-f2fs-devel@lists.sourceforge.net >>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>>>>>> . >>>>>>> >>>>> . >>>>> >>> . >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel