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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 May 2021 10:29:01 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     Alexey Makhalov <amakhalov@...are.com>
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

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://lore.kernel.org/r/459B4724-842E-4B47-B2E7-D29805431E69@vmware.com
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