Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 27 Sep 2019 16:46:08 +0000 (UTC)
From:      Mark Johnston <markj@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r352802 - head/sys/vm
Message-ID:  <201909271646.x8RGk8IC055252@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: markj
Date: Fri Sep 27 16:46:08 2019
New Revision: 352802
URL: https://svnweb.freebsd.org/changeset/base/352802

Log:
  Fix a race in vm_page_swapqueue().
  
  vm_page_swapqueue() atomically transitions a page between queues.  To do
  so, it must hold the page queue lock for the old queue.  However, once
  the queue index has been updated, the queue lock no longer protects the
  page's queue state.  Thus, we must speculatively remove the page from
  the old queue before committing the queue state update, and roll back if
  the update fails.
  
  Reported and tested by:	pho
  Reviewed by:	kib
  Sponsored by:	Intel, Netflix
  Differential Revision:	https://reviews.freebsd.org/D21791

Modified:
  head/sys/vm/vm_page.c

Modified: head/sys/vm/vm_page.c
==============================================================================
--- head/sys/vm/vm_page.c	Fri Sep 27 16:44:29 2019	(r352801)
+++ head/sys/vm/vm_page.c	Fri Sep 27 16:46:08 2019	(r352802)
@@ -3462,27 +3462,58 @@ void
 vm_page_swapqueue(vm_page_t m, uint8_t oldq, uint8_t newq)
 {
 	struct vm_pagequeue *pq;
+	vm_page_t next;
+	bool queued;
 
 	KASSERT(oldq < PQ_COUNT && newq < PQ_COUNT && oldq != newq,
 	    ("vm_page_swapqueue: invalid queues (%d, %d)", oldq, newq));
-	KASSERT((m->oflags & VPO_UNMANAGED) == 0,
-	    ("vm_page_swapqueue: page %p is unmanaged", m));
 	vm_page_assert_locked(m);
 
-	/*
-	 * Atomically update the queue field and set PGA_REQUEUE while
-	 * ensuring that PGA_DEQUEUE has not been set.
-	 */
 	pq = &vm_pagequeue_domain(m)->vmd_pagequeues[oldq];
 	vm_pagequeue_lock(pq);
-	if (!vm_page_pqstate_cmpset(m, oldq, newq, PGA_DEQUEUE, PGA_REQUEUE)) {
+
+	/*
+	 * The physical queue state might change at any point before the page
+	 * queue lock is acquired, so we must verify that we hold the correct
+	 * lock before proceeding.
+	 */
+	if (__predict_false(m->queue != oldq)) {
 		vm_pagequeue_unlock(pq);
 		return;
 	}
-	if ((m->aflags & PGA_ENQUEUED) != 0) {
-		vm_pagequeue_remove(pq, m);
+
+	/*
+	 * Once the queue index of the page changes, there is nothing
+	 * synchronizing with further updates to the physical queue state.
+	 * Therefore we must remove the page from the queue now in anticipation
+	 * of a successful commit, and be prepared to roll back.
+	 */
+	if (__predict_true((m->aflags & PGA_ENQUEUED) != 0)) {
+		next = TAILQ_NEXT(m, plinks.q);
+		TAILQ_REMOVE(&pq->pq_pl, m, plinks.q);
 		vm_page_aflag_clear(m, PGA_ENQUEUED);
+		queued = true;
+	} else {
+		queued = false;
 	}
+
+	/*
+	 * Atomically update the queue field and set PGA_REQUEUE while
+	 * ensuring that PGA_DEQUEUE has not been set.
+	 */
+	if (__predict_false(!vm_page_pqstate_cmpset(m, oldq, newq, PGA_DEQUEUE,
+	    PGA_REQUEUE))) {
+		if (queued) {
+			vm_page_aflag_set(m, PGA_ENQUEUED);
+			if (next != NULL)
+				TAILQ_INSERT_BEFORE(next, m, plinks.q);
+			else
+				TAILQ_INSERT_TAIL(&pq->pq_pl, m, plinks.q);
+		}
+		vm_pagequeue_unlock(pq);
+		return;
+	}
+	vm_pagequeue_cnt_dec(pq);
 	vm_pagequeue_unlock(pq);
 	vm_page_pqbatch_submit(m, newq);
 }

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201909271646.x8RGk8IC055252>