linux-kernel - Problems with bdev_write

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

Message-ID: <20150517170200.0d966434@notabene.brown>
Date:	Sun, 17 May 2015 17:02:00 +1000
From:	NeilBrown <neilb@...e.de>
To:	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	Charles Bertsch <cbertsch@....net>
Cc:	linux-raid@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>
Subject: Problems with bdev_write_page().

Hi Matthew,
 I've just been looking at bdev_write_page().
You can read about why here:

  http://marc.info/?t=142984068300001&r=1&w=2

it ends with a "git bisect" which points the finger at you.

If I look at bdev_write_page() it says:

 * On entry, the page should be locked and not currently under writeback.
 * On exit, if the write started successfully, the page will be unlocked and
 * under writeback.  If the write failed already (eg the driver failed to
 * queue the page to the device), the page will still be locked.  If the
 * caller is a ->writepage implementation, it will need to unlock the page.

So the page is unlocked on success.

In __mpage_writepage() I find

			if (!bdev_write_page(bdev, blocks[0] << (blkbits - 9),
								page, wbc)) {
				clean_buffers(page, first_unmapped);

so if bdev_write_page() succeeds, i.e. if it returns '0', then
clean_buffers() is called.  At this point the page is unlocked remember.

clean_buffers may call

		try_to_free_buffers(page);

(without first locking the page, so still unlocked)..
try_to_free_buffers starts:

	BUG_ON(!PageLocked(page));

Opps.

Can you propose a fix for Charles, who can trigger this bug and nicely
bisected it for us - thanks Charles!!!

Also while looking at the code, I notice that brd_rw_page() unconditionally
calls page_endio() and, in the WRITE case, page_endio unconditionally calls
end_page_writeback(), which has

	if (!test_clear_page_writeback(page))
		BUG();

and so cannot tolerate being called twice in a row.
So if brd_rw_page() ever returned an error (which seems possible though not
likely), end_page_writeback() would be called once by page_endio() and once
in the error path of bdev_write_page(), and the BUG above would be triggered.

I'll leave that for you to sort out too :-)

Thanks,
NeilBrown

Content of type "application/pgp-signature" skipped