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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 82D32C10F1A for ; Wed, 1 May 2024 12:20:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 11DAE6B008A; Wed, 1 May 2024 08:20:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0CCAF6B0095; Wed, 1 May 2024 08:20:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ED63B6B0096; Wed, 1 May 2024 08:20:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id CF2986B008A for ; Wed, 1 May 2024 08:20:02 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 737091A085D for ; Wed, 1 May 2024 12:20:02 +0000 (UTC) X-FDA: 82069733844.30.989D694 Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) by imf21.hostedemail.com (Postfix) with ESMTP id C77181C0023 for ; Wed, 1 May 2024 12:20:00 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=EBce+s6e; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf21.hostedemail.com: domain of ritesh.list@gmail.com designates 209.85.214.176 as permitted sender) smtp.mailfrom=ritesh.list@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714566000; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:in-reply-to:references:dkim-signature; bh=Q+bXJh1WFpGfj02zZsMPGyMBrA1lTWkDff/10uQbdd4=; b=oCgjnL7MpgcDqdEVwAY1dnvpDtTzDL9J505pxHpYJSWHKaYB9WxcwzyXXmXgkCwuWQOMaO 7gdoRBQherTWrtmRJU6A54L5WNGu+NCGz06XKzM4nfSheYUrlhvtvp8mSIb59FH4Pj0DK1 zCUJR6qRMlx2ol7IC3gD8/fAZ1qLa/A= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714566000; a=rsa-sha256; cv=none; b=W0eEmN0B8M1/w7mcuN5k7u+S+8CQcIrPhe6jgwSLl26dl7nZEG5pj/QWY4CHoqrEF4SkO2 XGks/KA5I6TGTMPrE72nPuXhy7Wcc4mEFDm/GX2369UUiqA6J+tKcBsUsGEdVIo/fVyLND yRqV2cpgYkons8TxhBkW4rs7AeaYw1U= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=EBce+s6e; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf21.hostedemail.com: domain of ritesh.list@gmail.com designates 209.85.214.176 as permitted sender) smtp.mailfrom=ritesh.list@gmail.com Received: by mail-pl1-f176.google.com with SMTP id d9443c01a7336-1e50a04c317so36841015ad.1 for ; Wed, 01 May 2024 05:20:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714565999; x=1715170799; darn=kvack.org; h=in-reply-to:subject:cc:to:from:message-id:date:from:to:cc:subject :date:message-id:reply-to; bh=Q+bXJh1WFpGfj02zZsMPGyMBrA1lTWkDff/10uQbdd4=; b=EBce+s6eaa25nhscEDI60IIRFmV+66+GPzMD/x1GzNk24IQ3lu70knUcrHRvWubLPy ZONYxKOoKI2OLEKOGNNhm5AkjYt+eE0nzQBf8o5XD98/GOjrLUkCWpsMgcYVO4I1t4Fp vl8VwPCrQjiURrxdtS9o1tGyt3K7nPStw5XddQ+TVyHy1+qSnIGPBLhSGjFdTf8Ylbyu solfH+jSMzUxoGBEkmupWxlU9a/5bw3IqRPbGBWbgrvUbhkJbmjl4irX8pO0nud9p+5k IJEZNJMkiBy5y5MUOQ+bT8mdDaKZP+bxBovUhOxJXkG18QAQDvRYxZw7nhdw6iZpBcgi /Whw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714565999; x=1715170799; h=in-reply-to:subject:cc:to:from:message-id:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=Q+bXJh1WFpGfj02zZsMPGyMBrA1lTWkDff/10uQbdd4=; b=jlcZNqSbri8L4BQnlxgLsuhnD0arctmUBrrUhuxgXH7yxMlk/ASyqvB1DW3uh4Xtxy VQqQtVisfCv3ZfLXSFxTZy+bIj0SJ26p4RWjrwiyfdvtmS7AkhmxwUookzEWj3wxNp/K AZQDMf75gCE6XLCjc0Rh49B90hdPC8fVHSTbAxOQLhI2fgb2ywIyR8bQI2YV1lRVyUW7 bTAifgJq72/2sgRUBw93Op/D3DE9Btu2FpsryVgzdStXsPiglSm5gKEVCKa9Dbhrmltp 3aOhChraEABjkUe8Otju+XwSqc4La7WwRBRcLgTzAjwyqFVnM4V0wsTeIjr21UUlnLxJ 6Tdw== X-Forwarded-Encrypted: i=1; AJvYcCW6h2bTnySQULI+XJRsZaQy5rIPT9cJX0Bugj7xi5r5LxW2uLuxUVTKqvQWC1oBBNKHZaOt33QI2AIGe1Fj6G3b5J8= X-Gm-Message-State: AOJu0Yx7Il79BV/th6Ffnwm/BzgqT/+F0dd+InlGDyycwRykI/Sty2oH XThAkhchhtPEktl2PCt0OApRi3tF5M5X8rQwL7b8BU7LJNLa0kMC X-Google-Smtp-Source: AGHT+IHH66m34ehe2oGyA8sEbgvqCf0QfvwZyF4PH1MwqMJKPg649zwjVE/zpqgicmyWWWJMHAmO0A== X-Received: by 2002:a17:902:c213:b0:1e4:24bc:426e with SMTP id 19-20020a170902c21300b001e424bc426emr2154990pll.28.1714565998423; Wed, 01 May 2024 05:19:58 -0700 (PDT) Received: from dw-tp ([171.76.84.250]) by smtp.gmail.com with ESMTPSA id i3-20020a170902c94300b001ebd72d55c0sm5600171pla.18.2024.05.01.05.19.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 May 2024 05:19:57 -0700 (PDT) Date: Wed, 01 May 2024 17:49:50 +0530 Message-Id: <87le4t4tcp.fsf@gmail.com> From: Ritesh Harjani (IBM) To: Dave Chinner , Zhang Yi Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz, hch@infradead.org, djwong@kernel.org, willy@infradead.org, zokeefe@google.com, yi.zhang@huawei.com, chengzhihao1@huawei.com, yukuai3@huawei.com, wangkefeng.wang@huawei.com Subject: Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block In-Reply-To: X-Rspamd-Queue-Id: C77181C0023 X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: fpzzbea3bkkkxaj1hzir8cfh3afifc8o X-HE-Tag: 1714566000-105140 X-HE-Meta: U2FsdGVkX19+yoAkghvs5K/5yKTihEDnrzkadTGgDypBOLRWBTrE6GqVwdIxLE/OU1qjgGjAqVbopPsVqAtUB3dhQ2JI3fGGbrOB9GylaLxZk+fuR+xu8LcoFr6w83WmWOp+tlJ9SBtMAAROvr8jzFRWX4tgVoSNACPhFzRykb7BTUsDpj5sxIy2Ww382wu5cRcwc5Exk+GHsZlQDz9wRnAK6K2rISbzsI0Z/aH8WkLg2zswxNEPhJPBJ35PuRocb39FT6DpQnejqLtRKdW7czQAQ4CfvKpibrAPnuiLoLXgkSGhqS5nR4Xnh0C36wYMgfl0gMpmX2cF+MZUZZfQNDsAvvCt+/jBEJDlFzem7Ad6gE5moP7du+PWOsjZYozWrEVKxSImJT4xfCC5qD7e5Nt3dHV/snQbh499gFjonwJeA8C3vLGsslfBGe9fRJJZoYpYHaFC9q4Y45czWwwbw1+ImDlAGUkPavdW1L8m89qfafsHHvZfXnxUXfC0HkxK5AesXjZurqGnRM40juSNcWfWK+o7t25ht+4GWukY3pfwuFdCVnO9rHGJODG1nbeENx/vFNZe/EzyTzR+0MRubUL+tgLjuOFjpMportyf/X88gZErUM2TwYYN5Jha2omCH9QzY7BWe1qI5hG1qx8qoFch40OFDZkn/60AP+0GupfLqAtui7YEPC7fRDuN9IErkqmFATkQm2BOxOQqKj3XzMtSjW03saGaCucDP32P9wZVEBROLdL6zcazSL3J1EXrW7oSUB8kHKTZ6xT+/9rQpSGm+fXu47QSdZWauud54jNkPrr0F3SbQECQGn87LSmwySaO4rI43+eD1Ya/7X34wKcdf7h4SMSXRrt4xkjuy1BLX+2F1ukRB9rJ1hvDwVzFyxJslsDQxTj9eC+qP2tDpZElM/spWu/Q4DR5ENFKzt/HXTktG25kJjb3aH00+jXVUHonswY8dL1om9OHAKE vm2cL49y /fGgwxjEJNWuFuZVUJX/Q5hTWbxXy+ibFU8UqhTa++ubvUlDXJV1d/XB9l23ROfWPrwHw5xseCL5lkxP81gVCNgZ+/Jh0H2DGNfvknqL5K62Ckhyg8fqJFa0Zo8LIWCSft9/qzDk7lI+N5fed1S/XRBHMpcEV70ZhjCVvdRgAEzW4uZcN1UyD8AiaD5hFTlAWIDsdufEiX84LAk95OorCSPiQqvSTLyZ4tfSYFziS6hRtgSE4jmlOUFUIDAY3ZazlQiYcNcSv9ZS9mBmeBBSwcSRJiSZjPM1t7qsOdqL0uVvcWOPNhJu34cB7eogSKRYHa4nCz61GOfUOf7xnoPvpYVpJQUw+Jj0Qhzptq48ihRl5ReX8I7FBwocQE7Gzes2NlnIrMiDBz1YQEu/Cxag1r9W59f5YCylOrMwPNrBB+m+SZWcczU/SKbEALaRNG2ApATl7OpKY52yGqp7xTveUFk4iaNi2ih/smKuy9OMWgU9ZVc08aTbj86iUzQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Dave Chinner writes: > On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote: >> From: Zhang Yi >> >> Now we lookup extent status entry without holding the i_data_sem before >> inserting delalloc block, it works fine in buffered write path and >> because it holds i_rwsem and folio lock, and the mmap path holds folio >> lock, so the found extent locklessly couldn't be modified concurrently. >> But it could be raced by fallocate since it allocate block whitout >> holding i_rwsem and folio lock. >> >> ext4_page_mkwrite() ext4_fallocate() >> block_page_mkwrite() >> ext4_da_map_blocks() >> //find hole in extent status tree >> ext4_alloc_file_blocks() >> ext4_map_blocks() >> //allocate block and unwritten extent >> ext4_insert_delayed_block() >> ext4_da_reserve_space() >> //reserve one more block >> ext4_es_insert_delayed_block() >> //drop unwritten extent and add delayed extent by mistake > > Shouldn't this be serialised by the file invalidation lock? Hole > punching via fallocate must do this to avoid data use-after-free > bugs w.r.t racing page faults and all the other fallocate ops need > to serialise page faults to avoid page cache level data corruption. > Yet here we see a problem resulting from a fallocate operation > racing with a page fault.... IIUC, fallocate operations which invalidates the page cache contents needs to take th invalidate_lock in exclusive mode to prevent page fault operations from loading pages for stale mappings (blocks which were marked free might get reused). This can cause stale data exposure. Here the fallocate operation require allocation of unwritten extents and does not require truncate of pagecache range. So I guess, it is not strictly necessary to hold the invalidate lock here. But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation. I guess we could use the invalidate lock for fallocate operation in ext4 too. However, I think we still require the current patch. The reason is ext4_da_map_blocks() call here first tries to lookup the extent status cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it takes the i_data_sem in write mode and just inserts an entry into extent status cache w/o re-checking for the same under the exclusive lock. ...So I believe we still should have this patch which re-verify under the write lock if whether any other operation has inserted any entry already or not. > > Ah, I see that the invalidation lock is only picked up deep inside > ext4_punch_hole(), ext4_collapse_range(), ext4_insert_range() and > ext4_zero_range(). They all do the same flush, lock, and dio wait > preamble but each do it just a little bit differently. The allocation path does > it just a little bit differently again and does not take the > invalidate lock... Yes, I think it is not stricly required to take invalidate lock in the allocation path of fallocate. Hence it could expose such a problem which existed in ext4_da_map_blocks(), right? > > Perhaps the ext4 fallocate code should be factored so that all the > fallocate operations run the same flush, lock and wait code rather > than having 5 slightly different copies of the same code? Yes. I agree. These paths can be refactored and if we are doing so, we may as well just use the invalidate lock as you suggested. -ritesh