Received-SPF: Pass (protection.outlook.com: domain of mit.edu designates
 18.9.28.11 as permitted sender) receiver=protection.outlook.com;
 client-ip=18.9.28.11; helo=outgoing.mit.edu;
Date:   Sat, 2 Mar 2019 21:07:23 -0500
From:   "Theodore Y. Ts'o" <tytso@mit.edu>
To:     Lukas Czerner <lczerner@redhat.com>
CC:     <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH] ext4: Add missing brelse() in add_new_gdb_meta_bg()
Message-ID: <20190303020723.GC7930@mit.edu>
References: <20190301171504.8583-1-lczerner@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20190301171504.8583-1-lczerner@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2019 02:07:25.5309
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 0df4b400-1b1c-4c87-b457-08d69f7cfb2d
X-MS-Exchange-CrossTenant-Id: 64afd9ba-0ecf-4acf-bc36-935f6235ba8b
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=64afd9ba-0ecf-4acf-bc36-935f6235ba8b;Ip=[18.9.28.11];Helo=[outgoing.mit.edu]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR01MB5602
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

On Fri, Mar 01, 2019 at 06:15:04PM +0100, Lukas Czerner wrote:
> Currently in add_new_gdb_meta_bg() there is a missing brelse of gdb_bh
> in case ext4_journal_get_write_access() fails. Fix it.
> 
> Fixes: 61a9c11e5e7a ("ext4: add missing brelse() add_new_gdb_meta_bg()'s error path")
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ext4/resize.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> index 48421de803b7..e945f412cf58 100644
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -937,6 +937,8 @@ static int add_new_gdb_meta_bg(struct super_block *sb,
>  	kvfree(o_group_desc);
>  	BUFFER_TRACE(gdb_bh, "get_write_access");
>  	err = ext4_journal_get_write_access(handle, gdb_bh);
> +	if (err)
> +		brelse(gdb_bh);

I believe this isn't the right fix --- or at least, it's not
sufficient.  We're releasing gdb_bh, but there is still a pointer left
in n_group_desc[gdb_num] (which is now invalid), and we've already
replaced o_group_desc with n_group_desc, and incremented s_gdb_count.

So we should move the call to ext4_journal_get_write_access() earlier
in the function.

Ric's comments about checking similar function is also right;
add_new_gdb() doesn't really get the error handling right, but that's
an extremely deprecated interface.  We actually had a bug in the old
resizing ioctl's that was accidentally introduce in 4.4, and no once
until until December of last year.  (I think it was some crazy user
with an enterprise distro still using e2fsprogs 1.42, and they tried
going to a modern kernel, and online resizing didn't work for them.)

Anyway, while fixing add_new_gdb() might be nice, we can save that for
another patch and I don't think it's super high priority since it's an
error handling path for a code path that almost no one uses and was
broken for two years without no one noticing (although maybe Red Hat
would prioritize it differently :-).  But could you resend this with
the call to ext4_journal_get_write_access() moved up earlier in the
function?

Thanks!

					- Ted