linux-ext4 - Re: [PATCH] ext4: fix memory leak in ext4_fill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 21 May 2021 09:12:09 -0700
From:   Alexey Makhalov <amakhalov@...are.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>
CC:     <linux-ext4@...r.kernel.org>, <stable@...r.kernel.org>,
        Andreas Dilger <adilger.kernel@...ger.ca>
Subject: Re: [PATCH] ext4: fix memory leak in ext4_fill_super

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

> On May 21, 2021, at 7:29 AM, Theodore Y. Ts'o <tytso@....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@...are.com>
> Cc: stable@...r.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@....edu>
> Cc: Andreas Dilger <adilger.kernel@...ger.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


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)