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:	Mon, 22 Sep 2014 15:22:14 -0400
From:	Rafael Aquini <aquini@...hat.com>
To:	Konstantin Khlebnikov <koct9i@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Konstantin Khlebnikov <k.khlebnikov@...sung.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: [PATCH v2 4/6] mm: introduce common page state for ballooned
 memory

On Mon, Sep 22, 2014 at 10:40:34PM +0400, Konstantin Khlebnikov wrote:
> On Sat, Sep 20, 2014 at 10:23 AM, Andrew Morton
> <akpm@...ux-foundation.org> wrote:
> > On Sat, 20 Sep 2014 09:25:01 +0400 Konstantin Khlebnikov <koct9i@...il.com> wrote:
> >
> >> >
> >> > So I'm going to send "fix for
> >> > mm-balloon_compaction-use-common-page-ballooning-v2" to Linus
> >> > separately, but it has no changelog at all.
> >>
> >> Probably it would be better if you drop everything except actually
> >> fixes and stresstest. This is gone too far, now balloon won't compile
> >> in the middle of patchset. Just tell me and I'll redo the rest.
> >
> > I think it's best if I drop everything:
> >
> > mm-balloon_compaction-ignore-anonymous-pages.patch
> > mm-balloon_compaction-keep-ballooned-pages-away-from-normal-migration-path.patch
> > mm-balloon_compaction-isolate-balloon-pages-without-lru_lock.patch
> > selftests-vm-transhuge-stress-stress-test-for-memory-compaction.patch
> > mm-introduce-common-page-state-for-ballooned-memory.patch
> > mm-balloon_compaction-use-common-page-ballooning.patch
> > mm-balloon_compaction-general-cleanup.patch
> > mm-balloon_compaction-use-common-page-ballooning-v2-fix-1.patch
> >
> > Please go through it and send out a new version?
> >
> >
> 
> I've found yet another bug in this code. It seems here is a nest.
> balloon_page_dequeue can race with  balloon_page_isolate:
> balloon_page_isolate can remove page from list between
> llist_for_each_entry_safe and trylock_page in balloon_page_dequeue.
> balloon_page_dequeue runs under mutex_lock(&vb->balloon_lock);
> both of them lock page using trylock_page so race is tight but it is
> not impossible.
Plausible to happen if stress testing compaction simultaneously with
freezing/unloading the balloon driver. As you noted, it's quite tight
despite not impossible. Nice catch.


> Probably it's really easier to rewrite it than to fix bugs one by one =/
I'm not against a rewrite, but I don't think that rewriting the code to get rid 
of such bugs changes the fact we still have to address them in the actual placed
code as we go on finding them. That's why I thought your inital changeset fine,
with patches for stable going first and code overhaul for next following them up.

For this race you spotted, I think a simple change like the following
might be enough (not-tested)

diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
index 6e45a50..fd3a497 100644
--- a/mm/balloon_compaction.c
+++ b/mm/balloon_compaction.c
@@ -93,6 +93,16 @@ struct page *balloon_page_dequeue(struct
balloon_dev_info *b_dev_info)
                 * to be released by the balloon driver.
                 */
                if (trylock_page(page)) {
+                       /*
+                        * Skip dequeue attempt for this page to a later round
+                        * if balloon_page_isolate() has sucessfully isolated
+                        * it just before we got the page lock here.
+                        */
+                       if (page_count(page) != 1) {
+                               unlock_page(page);
+                               continue
+                       }
+
                        spin_lock_irqsave(&b_dev_info->pages_lock, flags);
                        /*
                         * Raise the page refcount here to prevent any
                         * wrong

--
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/

Powered by blists - more mailing lists