Received-SPF: softfail (google.com: domain of transitioning linux-ext4-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <2f9933b3-a574-23e1-e632-72fc29e582cf@nvidia.com>
Date:   Fri, 25 Feb 2022 13:33:33 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH -v3] ext4: don't BUG if kernel subsystems dirty pages
 without asking ext4 first
Content-Language: en-US
To:     Theodore Ts'o <tytso@mit.edu>, Eric Biggers <ebiggers@kernel.org>
Cc:     Lee Jones <lee.jones@linaro.org>, linux-ext4@vger.kernel.org,
        Christoph Hellwig <hch@lst.de>,
        Dave Chinner <dchinner@redhat.com>,
        Goldwyn Rodrigues <rgoldwyn@suse.com>,
        "Darrick J . Wong" <darrick.wong@oracle.com>,
        Bob Peterson <rpeterso@redhat.com>,
        Damien Le Moal <damien.lemoal@wdc.com>,
        Andreas Gruenbacher <agruenba@redhat.com>,
        Ritesh Harjani <riteshh@linux.ibm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Johannes Thumshirn <jth@kernel.org>, linux-xfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, cluster-devel@redhat.com,
        linux-kernel@vger.kernel.org
References: <Yg0m6IjcNmfaSokM@google.com> <Yhks88tO3Em/G370@mit.edu>
 <YhlBUCi9O30szf6l@sol.localdomain> <YhlFRoJ3OdYMIh44@mit.edu>
 <YhlIvw00Y4MkAgxX@mit.edu>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <YhlIvw00Y4MkAgxX@mit.edu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NEMxc2g3QlVBaldNdC9SMTlkRVlsbk95SFc5K0dsNjVOZGRxcjV5K1BiWU5o?=
 =?utf-8?B?TXVBWmRDMGx5ZXl0dXE2Y0NwRlZEVkFLallVb01KaElvbW5KWkVjTWVpVG9J?=
 =?utf-8?B?RGlXRytZd3dpcG0vdkoxKzFHaEV2R0FDL25WeFMvdjJYWDdLbysxQWtvTFIw?=
 =?utf-8?B?ak5iRVhhcDloN05zN0ZjMmdreHZhL2pLTTZtakFWUElpc09PczlQc2x3UFJo?=
 =?utf-8?B?bUU3ZEJISm5zbGc5OU5yRHdQUnNvazZKbWNQcUc0YWNIdVptRmZRR2pQZC9I?=
 =?utf-8?B?bExVVWhvOGpad1MzVmk5eWVtMjAxaG1BbVFibElQbk4yWm1EMytSWDk1a0pn?=
 =?utf-8?B?OEFuaTFtdkZ2eXEzL0RQRjNuWS85R002RFJMY052cTNvdjhERUZsWjJ0SFZT?=
 =?utf-8?B?bm9SNlVhQTQ0UkVhVmZ5cVR0SGhuSXVEWnNvQlhwTjNVbkZOS1ZVeWJ5QTFq?=
 =?utf-8?B?VDAvZ0M2SFpmN09zQ2J2RVF6aFpKejRMbzF5Zm5tWHIxSzAvYWlDaXpVRjhK?=
 =?utf-8?B?dnptSTVrc2MxVEdham5qRGoyNVpacTFJQlBOQ3RINTNHUDE2clpSNW9uRmFH?=
 =?utf-8?B?d3VONnhKNS9VWDBxVXhxVHM0OGNwVnk5L2Jva1dyUUhCMlNUeXA5aTc2dFR3?=
 =?utf-8?B?bVhLcHBRNDhOc3RmZjJFaEZCT1Y0R1BmUU5QSWZ2MnBRWDNYSnBiWWFjT1do?=
 =?utf-8?B?WjdNalI4eVlzdGlodk1GbnByb1N2NmFaektKd1MwN2RVZk9lVmNKQy9Ya1hw?=
 =?utf-8?B?MnpVY0U4WXpBYVBoZjRpZjFVeVFzU0hzbFBzcUkvM2YyK2k4M0cxWUF5MGt6?=
 =?utf-8?B?STBqdnhWSXRteWFKYUhpSVUva3Fkd2dhUVZJNnlJT3hpRUYyNkZ0WUN4aWlN?=
 =?utf-8?B?R3ZtTUovTGg2V1BwSjdnb0szMUxvOG5pdWxZMkVBR0J4elhSVDREeC9HeTB0?=
 =?utf-8?B?L2FHWVp1eTF1RW0yWHdQcDFVVXJUYVlxUnh6UDVjank1UjE5TFR5ZXhtWmto?=
 =?utf-8?B?STYvYmtGb0ZWeE9kOFpIcnRFUzhoTXRXS2ExUkVWYVREYS9uV2g0ZUpNenFT?=
 =?utf-8?B?YUQ5M1E2bnVFVFQ2Nm5UOWxzMVlhMURwbkRuMEttYzNINnlCRTM4bGFBaEFU?=
 =?utf-8?B?anljblRFdVR1QjZBcHphejFMSHlBZGVoN2t2SEhqVkl1Q3ZzampvRHBkZzhy?=
 =?utf-8?B?VjJDMWRoK0dWOEtRaG04UlBrcEk4VjdOYjgzYzhWTFdCZW9IVk1SVHBrVmkz?=
 =?utf-8?B?S2lieUNnd25Sc2Iyc09MNWQrcE9lYlFUZDdtWm9LSEt0bGtnRWNqYUxnNkNO?=
 =?utf-8?B?WnRoT2czbUJOZitVOXlzdGNDejhzOUlHcGhoUXlYWDRoajlmSGdpLzl1ckxx?=
 =?utf-8?B?U1NNRE5Td3RPVGdGNE5EbXRhVVY3UEs2UkVhajNsbUkrT2FreE53NUNWZHhz?=
 =?utf-8?B?S01tY2t2Z3QxVDcrTUxETUtid243NnhWYmRETnZrUWszWFdybmFRa2lCV0lH?=
 =?utf-8?B?OEY4RUl0REVsUVR3R2prZ0ViVDc0aHVmUUtoS2hxVGtqaW1Hb1JGczFjNDVW?=
 =?utf-8?B?N3MxVGJ6d05xS1J4dVZmUkNJL2RveXN2SjlJV1lDSkxLSUt4dldteitWbVhs?=
 =?utf-8?B?TGJzaENrYTlUZVRPMGhIR3pFT2tvMDk0cTAweDVDZWZ4dWJNM3FrR09kWlE0?=
 =?utf-8?B?NDFjMVlmVjFKc1JRYjJMaXFtSzhNRWVNSGRRbWdIY21HclRXempyZFBua2Nl?=
 =?utf-8?B?ZXhNc0RoWVJzLzNiL0d0Q0djeldpdTFFa0ZzLzIxcWtPMEdkbHFwdSt3UWxH?=
 =?utf-8?B?bnNWTEsyTitPRmNWcHBCZWU0bzhIeGdBekJlZURDR3lBWUg5RzFmNEFvMERI?=
 =?utf-8?B?ZXJmQnVrMTJWckk4Wlp4YmszcDU3L0FHd1cxRkhoWnAxVlVvSmlBcmFhekcx?=
 =?utf-8?B?QlQ3OHJXdnZ2cmtKYllGZ1pIejFRVVdvUkJ1dDgwM3ZWcWx0WUtqanVHMXR5?=
 =?utf-8?B?akxqZVUrSStSRHA5K0hXSS9BMERZNndyVlluZ0lxTXI4TUxkYWJHeDVCSXQ1?=
 =?utf-8?B?ZXMrTG5jMDBuVFloTkZDRTFmNFlVaUJGVDZvSjJKVE1TNWUrVmpicEh2MnVN?=
 =?utf-8?B?cW5KY1NOcnpaeFFvMFNpVnBFN1kxSGJ4cjBTZmo0K2lWK2ZkTG5VSTR0Vk1R?=
 =?utf-8?Q?5P6fwexbWxX5gL9qU1d9PkY=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 506e02e4-d65b-477b-5be8-08d9f8a67b7d
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Feb 2022 21:33:37.0958
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 01V3pwCNaEB80XWkEiegMaAafngjOoFR3nYM0eKejkwnh30yc5+xOa/ojSfmrza5RfhyQaVlo6GWnZcSYJmZcQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR12MB1153
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=no 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-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

On 2/25/22 13:23, Theodore Ts'o wrote:
> [un]pin_user_pages_remote is dirtying pages without properly warning
> the file system in advance.  This was noted by Jan Kara in 2018[1] and

In 2018, [un]pin_user_pages_remote did not exist. And so what Jan reported
was actually that dio_bio_complete() was calling set_page_dirty_lock()
on pages that were not (any longer) set up for that.

> more recently has resulted in bug reports by Syzbot in various Android
> kernels[2].
> 
> This is technically a bug in mm/gup.c, but arguably ext4 is fragile in

Is it, really? unpin_user_pages_dirty_lock() moved the set_page_dirty_lock()
call into mm/gup.c, but that merely refactored things. The callers are
all over the kernel, and those callers are what need changing in order
to fix this.


thanks,
-- 
John Hubbard
NVIDIA

> that a buggy kernel subsystem which dirty pages without properly
> notifying the file system causes ext4 to BUG, while other file systems
> are not (although user data likely will be lost).  I suspect in real
> life it is rare that people are using RDMA into file-backed memory,
> which is why no one has complained to ext4 developers except fuzzing
> programs.
> 
> So instead of crashing with a BUG, issue a warning (since there may be
> potential data loss) and just mark the page as clean to avoid
> unprivileged denial of service attacks until the problem can be
> properly fixed.  More discussion and background can be found in the
> thread starting at [2].
> 
> [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz
> [2] https://lore.kernel.org/r/Yg0m6IjcNmfaSokM@google.com
> 
> Reported-by: syzbot+d59332e2db681cf18f0318a06e994ebbb529a8db@syzkaller.appspotmail.com
> Reported-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>   fs/ext4/inode.c | 27 ++++++++++++++++++++++++++-
>   1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 01c9e4f743ba..008fe8750109 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1993,6 +1993,15 @@ static int ext4_writepage(struct page *page,
>   	else
>   		len = PAGE_SIZE;
>   
> +	/* Should never happen but for bugs in other kernel subsystems */
> +	if (!page_has_buffers(page)) {
> +		ext4_warning_inode(inode,
> +		   "page %lu does not have buffers attached", page->index);
> +		ClearPageDirty(page);
> +		unlock_page(page);
> +		return 0;
> +	}
> +
>   	page_bufs = page_buffers(page);
>   	/*
>   	 * We cannot do block allocation or other extent handling in this
> @@ -2588,12 +2597,28 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
>   			     (mpd->wbc->sync_mode == WB_SYNC_NONE)) ||
>   			    unlikely(page->mapping != mapping)) {
>   				unlock_page(page);
> -				continue;
> +				goto out;
>   			}
>   
>   			wait_on_page_writeback(page);
>   			BUG_ON(PageWriteback(page));
>   
> +			/*
> +			 * Should never happen but for buggy code in
> +			 * other subsystems that call
> +			 * set_page_dirty() without properly warning
> +			 * the file system first.  See [1] for more
> +			 * information.
> +			 *
> +			 * [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz
> +			 */
> +			if (!page_has_buffers(page)) {
> +				ext4_warning_inode(mpd->inode, "page %lu does not have buffers attached", page->index);
> +				ClearPageDirty(page);
> +				unlock_page(page);
> +				continue;
> +			}
> +
>   			if (mpd->map.m_len == 0)
>   				mpd->first_page = page->index;
>   			mpd->next_page = page->index + 1;