linux-kernel - Re: [PATCH 1/1] hugepages: Fix race between hugetlbfs umount and quota update.

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]

Message-Id: <20110818152846.e76ff944.akpm@linux-foundation.org>
Date:	Thu, 18 Aug 2011 15:28:46 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Andrew Barry <abarry@...y.com>
Cc:	linux-mm <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Mel Gorman <mgorman@...e.de>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Minchan Kim <minchan.kim@...il.com>,
	Rik van Riel <riel@...hat.com>,
	Hugh Dickins <hughd@...gle.com>,
	David Gibson <david@...son.dropbear.id.au>
Subject: Re: [PATCH 1/1] hugepages: Fix race between hugetlbfs umount and
 quota update.

On Wed, 17 Aug 2011 17:01:15 -0500
Andrew Barry <abarry@...y.com> wrote:

> This patch fixes a race between the umount of a hugetlbfs filesystem, and quota
> updates in that filesystem, which can result in the update of the filesystem
> quota record, after the record structure has been freed.
> 
> Rather than an address-space struct pointer, it puts a hugetlbfs_sb_info struct
> pointer into page_private of the page struct. A reference count and an active
> bit are added to the hugetlbfs_sb_info struct; the reference count is increased
> by hugetlb_get_quota and decreased by hugetlb_put_quota. When hugetlbfs is
> unmounted, it frees the hugetlbfs_sb_info struct, but only if the reference
> count is zero, otherwise it clears the active bit. The last hugetlb_put_quota
> then frees the hugetlbfs_sb_info struct.
> 
> Discussion was titled:  Fix refcounting in hugetlbfs quota handling.
> See:  https://lkml.org/lkml/2011/8/11/28

The changelog doesn't actually describe the race - it just asserts that
there is one.  This makes it unnecessarily difficult to review the
fix!  So I didn't really look at the code - I just scanned the trivial
stuff.

The patch was somewhat wordwrapped - please fix the email client then
resend.

> +		if (hugetlb_get_quota(HUGETLBFS_SB(inode->i_mapping->host->i_sb), chg))
> +			hugetlb_put_quota(HUGETLBFS_SB(inode->i_mapping->host->i_sb), chg);
> +	set_page_private(page, (unsigned long)HUGETLBFS_SB(inode->i_mapping->host->i_sb));
> +			hugetlb_put_quota(HUGETLBFS_SB(vma->vm_file->f_mapping->host->i_sb), reserve);
> +	if (hugetlb_get_quota(HUGETLBFS_SB(inode->i_mapping->host->i_sb), chg))
> +		hugetlb_put_quota(HUGETLBFS_SB(inode->i_mapping->host->i_sb), chg);
> +	hugetlb_put_quota(HUGETLBFS_SB(inode->i_mapping->host->i_sb), (chg - freed));

Are all the inode->i_mapping->host pointer hops actually necessary?  I
didn't see anything about them in the changelog and I'd expect that
inode->i_mapping->host is always equal to `inode' for hugetlbfs?

If they _are_ necessary then I'd suggest that the code could be cleaned
up by adding

static struct hugetlbfs_sb_info *inode_to_sb(struct inode *inode)
{
	return HUGETLBFS_SB(inode->i_mapping->host->i_sb);
}

to hugetlbfs.c.  This will reduce the relatively large number of
checkpatch warnings which were added.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/