Re: [PATCH] ext4: fix memory leak in ext4_fill_super

From: Alexey Makhalov <amakhalov@vmware.com>
To: "Theodore Y. Ts'o" <tytso@mit.edu>
Cc: <linux-ext4@vger.kernel.org>, <stable@vger.kernel.org>,
	Andreas Dilger <adilger.kernel@dilger.ca>
Subject: Re: [PATCH] ext4: fix memory leak in ext4_fill_super
Date: Fri, 21 May 2021 09:12:09 -0700	[thread overview]
Message-ID: <D3ECB26C-F77D-43CB-AE6F-F6919ED6CCB9@vmware.com> (raw)
In-Reply-To: <YKfDrcEfu7Gp0dGi@mit.edu>

[-- Attachment #1: Type: text/plain, Size: 3243 bytes --]

Sounds good! Thanks for the guidance and v3) —Alexey

> On May 21, 2021, at 7:29 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> 
> On Fri, May 21, 2021 at 12:43:46AM -0700, Alexey Makhalov wrote:
>> Hi Ted,
>> 
>> Good point! This paragraph can be just dropped as the next one
>> describes the issue with superblock re-read. Will send v2 shortly.
> 
> Thanks; for what it's worth, I'm going to be editing the commit
> description anyway; it's really helpful during the patch review to
> understand how you found the problem, and how to reproduce it.
> However, the commit description when it lands upstream will include a
> link to this mail thread on lore.kernel.org, and so including a long
> stack trace isn't really necessary.
> 
> I'm going to cut it down to something like this:
> 
> ------------------------------
> ext4: fix memory leak in ext4_fill_super
> 
> Buffer head references must be released before calling kill_bdev();
> otherwise the buffer head (and its page referenced by b_data) will not
> be freed by kill_bdev, and subsequently that bh will be leaked.
> 
> If blocksizes differ, sb_set_blocksize() will kill current buffers and
> page cache by using kill_bdev(). And then super block will be reread
> again but using correct blocksize this time. sb_set_blocksize() didn't
> fully free superblock page and buffer head, and being busy, they were
> not freed and instead leaked.
> 
> This can easily be reproduced by calling an infinite loop of:
> 
>  systemctl start <ext4_on_lvm>.mount, and
>  systemctl stop <ext4_on_lvm>.mount
> 
> ... since systemd creates a cgroup for each slice which it mounts, and
> the bh leak get amplified by a dying memory cgroup that also never
> gets freed, and memory consumption is much more easily noticed.
> 
> Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
> Cc: stable@vger.kernel.org
> Link: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F459B4724-842E-4B47-B2E7-D29805431E69%40vmware.com&amp;data=04%7C01%7Camakhalov%40vmware.com%7Ce5ae2270f1134d9a3cce08d91c64cb26%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637572041508854286%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=%2Fa%2FqTcBfL1tYkIq0byM46DXmpxTFOraAly2Ib9sbghE%3D&amp;reserved=0
> Fixes: ce40733ce93d ("ext4: Check for return value from sb_set_blocksize")
> Fixes: ac27a0ec112a ("ext4: initial copy of files from ext3")
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> ------------------------------
> 
> The commit description above is 18 lines (exclusive of trailers and
> headers) versus 71 lines, and is much more to the point for someone
> who might be doing code archeology via "git log".
> 
> When submitting this as a patch, you can either use a separate cover
> letter (git format-patch --cover-letter) or including the explanation
> after the --- in the diff, so that it disappears before the commit is
> added via "git am".  But it's not that hard for me to rework a commit
> description, so I'll take care of it for this patch; no need to send a
> V3.
> 
> Cheers,
> 
> 					- Ted

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]