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=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,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 0F048C433E0 for ; Fri, 8 Jan 2021 09:37:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AC13E233CF for ; Fri, 8 Jan 2021 09:37:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727838AbhAHJhE (ORCPT ); Fri, 8 Jan 2021 04:37:04 -0500 Received: from verein.lst.de ([213.95.11.211]:43255 "EHLO verein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727294AbhAHJhE (ORCPT ); Fri, 8 Jan 2021 04:37:04 -0500 Received: by verein.lst.de (Postfix, from userid 2407) id BE02E67373; Fri, 8 Jan 2021 10:36:21 +0100 (CET) Date: Fri, 8 Jan 2021 10:36:21 +0100 From: Christoph Hellwig To: Satya Tangirala Cc: Bob Peterson , Christoph Hellwig , Alexander Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Jens Axboe Subject: Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb Message-ID: <20210108093621.GA3788@lst.de> References: <20201224044954.1349459-1-satyat@google.com> <20210107162000.GA2693@lst.de> <1137375419.42956970.1610036857271.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.17 (2007-11-01) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 07, 2021 at 11:08:39PM +0000, Satya Tangirala wrote: > > error = sb->s_op->freeze_super(sb); > > else > > @@ -600,6 +602,7 @@ int thaw_bdev(struct block_device *bdev) > > if (!sb) > > goto out; > > > > + bdev->bd_fsfreeze_sb = NULL; > This causes bdev->bd_fsfreeze_sb to be set to NULL even if the call to > thaw_super right after this line fail. So if a caller tries to call > thaw_bdev() again after receiving such an error, that next call won't even > try to call thaw_super(). Is that what we want here? (I don't know much > about this code, but from a cursory glance I think this difference is > visible to emergency_thaw_bdev() in fs/buffer.c) Yes, that definitively is an issue. > > I think the second difference (decrementing bd_fsfreeze_count when > get_active_super() returns NULL) doesn't change anything w.r.t the > use-after-free. It does however, change the behaviour of the function > slightly, and it might be caller visible (because from a cursory glance, it > looks like we're reading the bd_fsfreeze_count from some other places like > fs/super.c). Even before 040f04bd2e82, the code wouldn't decrement > bd_fsfreeze_count when get_active_super() returned NULL - so is this change > in behaviour intentional? And if so, maybe it should go in a separate > patch? Yes, that would be a change in behavior. And I'm not sure why we would want to change it. But if so we should do it in a separate patch that documents the why, on top of the patch that already is in the block tree.