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 E8CDEC4345F for ; Wed, 1 May 2024 22:49:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0183F6B0092; Wed, 1 May 2024 18:49:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F09C86B0093; Wed, 1 May 2024 18:49:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DAA9E6B0095; Wed, 1 May 2024 18:49:30 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id BD2176B0092 for ; Wed, 1 May 2024 18:49:30 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 60ABF80C47 for ; Wed, 1 May 2024 22:49:30 +0000 (UTC) X-FDA: 82071320100.26.3216291 Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by imf30.hostedemail.com (Postfix) with ESMTP id 740EC80003 for ; Wed, 1 May 2024 22:49:28 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=lYXfI9VE; dmarc=pass (policy=quarantine) header.from=fromorbit.com; spf=pass (imf30.hostedemail.com: domain of david@fromorbit.com designates 209.85.216.54 as permitted sender) smtp.mailfrom=david@fromorbit.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714603768; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=rWcAbZK+N9gGi5DZfbKEGFQw8hTCqAY7aSRPf/sIHHU=; b=VgC/eevDSxAb5rJhPktHuNHIp//bmDq6BvKZao2R6fvTbc+wN65bMMATEbr7KF0psIExNI Te4CSkBSIcDmGeEAPN7FNAmg2CkIZ6deQnb5GVx9EmFHeHCKe6nzZwz7CtSDuXkCnF/EBl zvtrASNu559TCtiHFfLB5qakqpB7YXc= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=lYXfI9VE; dmarc=pass (policy=quarantine) header.from=fromorbit.com; spf=pass (imf30.hostedemail.com: domain of david@fromorbit.com designates 209.85.216.54 as permitted sender) smtp.mailfrom=david@fromorbit.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714603768; a=rsa-sha256; cv=none; b=zYdlsiEFzMbF6Z8ELkJ/IB/Md4JA+6RgXxXyzsgTdb5KLzlBPna/bpX+R1Zb6PTPjqB21M vfTdtlIbVKNWnfLfl5tfSN3VI1pN75aN8MkSy9VzwnN47I7hkt/60dFzgfQvjIdihhSzVf OQYE3MpbNF5jvIF0oZ/YqobGSZZkhow= Received: by mail-pj1-f54.google.com with SMTP id 98e67ed59e1d1-2b276bf798cso1646622a91.1 for ; Wed, 01 May 2024 15:49:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1714603767; x=1715208567; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=rWcAbZK+N9gGi5DZfbKEGFQw8hTCqAY7aSRPf/sIHHU=; b=lYXfI9VEahcvlpGiHBERbHXtowLkpvdNqWnn/R1D3Smy5SJeZln8E2Rp6uxDUnVlz2 OXxg4eo/snC8is87kl2bgRszE/quv4emvOo2L45GPRxF2N2PLiwsfupnBUPuI10RV9TS 6U/Utx7xrO7c6Jk447bw1bI3aQuQhSsxgww5y5J4Rpd77raAcZj0lJo9Z/q8221Q0yAt aIUpCOX2GTH9FmGyYOixkrOKUE1khPIMjdQOv6t3bKYWFn5/JYZAhwpxFbOKpSmlIw+H IOf+w4j/EztQHEsNYBz+0BXcfQ+hHFfLdRBr1e/v0CYacXuL50noTf4jhN8guFEOM5O+ Vyeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714603767; x=1715208567; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=rWcAbZK+N9gGi5DZfbKEGFQw8hTCqAY7aSRPf/sIHHU=; b=wEVvbLaUBJYML0LAqwId4hQTni1aauMGaKKcOetbYMBZkQCXwYtm0pb0ntZMYVAMM9 oPZLO/m9dZLOxomxesoymi6Ct9gSXi+sjr32XHPaPva7tk6hapFCh0SGW9lzXT6OeoHc tD/BaZiKZXfoIRzZzFpuSYEE2jWk5nP5L7ARengDz3Cm0bF6rWKue0X2Hz8VGuwi2k1w QjzSM9BLJun4Y/MHDA9/D0NUuJRcWrHaJSsZdMLW7xrLQg5eEr+LIwnSlGkKBDKlTYLv KWfhpR5owu13zSqHnnex5AjT4k20w64qk7ZowecyywI7+dQViogFGfBliw5yCascNJh3 x+1g== X-Forwarded-Encrypted: i=1; AJvYcCUskt16Fhf0SEeZchGVQwUKQWQ1fBC8i42TDV/TplYU85e5ZxRn6GQyttEBdCF0FO25+c9zzPae1bt3O3sWQfo5JDk= X-Gm-Message-State: AOJu0YybbSXCHrc8dRqTS4Gz6pk1+vzxf7fMb4NuCOPAX2ymjNWSZEpP +XcLE/XSCf+AH7q6wvTE3x3FUCndDv7jrFYpOPHulPEduHGIrL6bhfLHITWwsyw= X-Google-Smtp-Source: AGHT+IFUVpD0ohKjsMi728ug09zevbMivjbV42nyO/wzm5OAgymbTe1EO9Wt0LCxXfVXbYRIPjbf4g== X-Received: by 2002:a17:90a:3f87:b0:2ac:d9f:de9b with SMTP id m7-20020a17090a3f8700b002ac0d9fde9bmr331337pjc.45.1714603767063; Wed, 01 May 2024 15:49:27 -0700 (PDT) Received: from dread.disaster.area (pa49-179-32-121.pa.nsw.optusnet.com.au. [49.179.32.121]) by smtp.gmail.com with ESMTPSA id d20-20020a17090ae29400b0029bf32b524esm1846257pjz.13.2024.05.01.15.49.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 May 2024 15:49:26 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1s2IlQ-000G1W-1B; Thu, 02 May 2024 08:49:24 +1000 Date: Thu, 2 May 2024 08:49:24 +1000 From: Dave Chinner To: Ritesh Harjani Cc: Zhang Yi , 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 Message-ID: References: <87le4t4tcp.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87le4t4tcp.fsf@gmail.com> X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 740EC80003 X-Stat-Signature: esg5u4goyscjg85to8qswia3ugri3gin X-HE-Tag: 1714603768-752283 X-HE-Meta: U2FsdGVkX1928WFwn83OoCg7WbhE0Sm7bb9ofcT9SnMOzyFKO2hIdfVaLB7WmGrxAYfRPiKQ58A7R6jZZ2eyTRgVOiPVOHXgq3GF/RJc2cLlfQ9HV4iuI3guBu9tdsYBZzbcmkbJO2fBCzhHeEn0BomR9+WOa/MIav7MDp3Q3uKsmv4ElOcvKrx9tL1qvlzDBUlDp29HoGE/A0DYtr/A3+uUDn47Pyj8PIBlXQgvhdFggCnNnoHho2z2oMZh98fjiIyFE/ecxiyZXya2rowlLv9zTP8szpcP2hVesgtJXTEjRxc5EDBSJSq6XOcNwVO9b/2fbV2RhaJte5abnklFwUORpmQRcKXXcz28gi65LbMiYgIuAOxRVzZAsatXrV2fodT7By28eNCEgVfBskPIHoHTlH60NcaxyK2rbELA/dnNi3bt9vveVCI7JDZzRsFcV0aXVhNOMOR6ZELn8pmOsRNuWhr2e8kvjWDCAN5GiNRdKrax8hFv27qfpAW1qi2YqM4nsXLy35ODIfEFLqYchsNVEecHF0SJ4B456W1ywT/MolkTO29C6GhwVUaNYj1QCYqOXdToq0dS+K2Cg7ZSfbx/g3qk6OV74ZePH2R+/rFtqZkXr0SQ6ufMhC49UW82iM3vtBQsbEyotXn3m7DHd4s5oB1WWF1wU+6QxelXXPRNiPDdr8DauLz1e6DNAn1sxBAWo0jzyFnmSYagRSWE/FTJ18JX0O24POs8389/Ch1pjjCx8FzEMSmpj4Kf+wpq5tJCDzl+SHtezHOhc9jiR7is9wWfkNefIT5+2ZYtB44LjkyliNvUdxOxtp7VwnIGAEjH+PKeddowZbMt3AuBKdUpm5IvG5mdRQIlfZpDsXhyrYvJx0gk0ZQAB+/sJsxN44BaM7W68nJcY9+lsduGN9M3tgB6FXsv1o6u/pnlwJKZCgtF5BamxVCUmjV13hA/8nlu/2cufZ7jYhO4D23 bX7lipRe g9F8cJewj5vsQfB5zroLFZKymT/4LF5FUwGDTgeomwcDrQYCl6oMv6/IxcA/JFUFGtz8SotraGLbjClXj7B3S2tiyA5zCWz+SOGxaEEj5NOThJVxgVC9xzpQQYEUBIu3gMiHtbWZDDvII/ugvZjKahoQhXqJVkYN08gRabxEumxcvkajXcXYbu/UNOxXDgbMw9OHqkkabpo+zDRUNUmS5fEdnBCCyHJEgvycCPJceUKSsCVjEhGmanMmoAz1yuj2KYzovu3rRJyl/q7vURXiLctslAzKhOkKKUh1X9+TrQrKiToHMP1tURqiIMYTgQVKTYJubGv1jUCiRwbSA8I0tJm8aQyd5FyFnIFMvvlWkHGT6wQHe4jJ64y06xYh9wy3k4j8MvJnai+Wdz8nvBdthFGR+AE4i59tibbqS2nXkHspT8Pk8L4gk8PpnjGcrS6kYniSUTjrCPslVBtwHHkgBLRFomjmrjP0HqDF8fT0r3D1pihhUJtHUoq7Om3AbA6MQlPBbTmM0qaCn8ofmKDBiSqvhWieP3bRu0jy1 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote: > 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. True, but you can make exactly the same argument for write() vs fallocate(). Yet this path in ext4_fallocate() locks out concurrent write()s and waits for DIOs in flight to drain. What makes buffered writes triggered by page faults special? i.e. if you are going to say "we don't need serialisation between writes and fallocate() allocating unwritten extents", then why is it still explicitly serialising against both buffered and direct IO and not just truncate and other fallocate() operations? > But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation. Yes, that's the behaviour preallocation has had in XFS since we introduced the MMAPLOCK almost a decade ago. This was long before the file_invalidation_lock() was even a glimmer in Jan's eye. btrfs does the same thing, for the same reasons. COW support makes extent tree manipulations excitingly complex at times... > 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. Yup, I never said the code in the patch is wrong or unnecessary; I'm commenting on the high level race condition that lead to the bug beting triggered. i.e. that racing data modification operations with low level extent manipulations is often dangerous and a potential source of very subtle, hard to trigger, reproduce and debug issues like the one reported... -Dave. -- Dave Chinner david@fromorbit.com