Re: inconsistent lock state in xa_destroy

[Matthew Wilcox]

On Thu, Oct 08, 2020 at 09:06:56AM -0600, Jens Axboe wrote:
> On 10/8/20 9:05 AM, Matthew Wilcox wrote:
> > On Thu, Oct 08, 2020 at 09:01:57AM -0600, Jens Axboe wrote:
> >> On 10/8/20 9:00 AM, syzbot wrote:
> >>> Hello,
> >>>
> >>> syzbot found the following issue on:
> >>>
> >>> HEAD commit:    e4fb79c7 Add linux-next specific files for 20201008
> >>> git tree:       linux-next
> >>> console output: https://syzkaller.appspot.com/x/log.txt?x=12555227900000
> >>> kernel config:  https://syzkaller.appspot.com/x/.config?x=568d41fe4341ed0f
> >>> dashboard link: https://syzkaller.appspot.com/bug?extid=cdcbdc0bd42e559b52b9
> >>> compiler:       gcc (GCC) 10.1.0-syz 20200507
> >>>
> >>> Unfortunately, I don't have any reproducer for this issue yet.
> >>>
> >>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> >>> Reported-by: syzbot+cdcbdc0bd42e559b52b9@xxxxxxxxxxxxxxxxxxxxxxxxx
> >>
> >> Already pushed out a fix for this, it's really an xarray issue where it just
> >> assumes that destroy can irq grab the lock.
> > 
> > ... nice of you to report the issue to the XArray maintainer.
> 
> This is from not even 12h ago, 10h of which I was offline. It wasn't on
> the top of my list of priority items to tackle this morning, but it
> is/was on the list.

How's this?

diff --git a/lib/xarray.c b/lib/xarray.c
index 1e4ed5bce5dc..d84cb98d5485 100644
--- a/lib/xarray.c
+++ b/lib/xarray.c
@@ -1999,21 +1999,32 @@ EXPORT_SYMBOL_GPL(xa_delete_node);	/* For the benefit of the test suite */
  * xa_destroy() - Free all internal data structures.
  * @xa: XArray.
  *
- * After calling this function, the XArray is empty and has freed all memory
- * allocated for its internal data structures.  You are responsible for
- * freeing the objects referenced by the XArray.
- *
- * Context: Any context.  Takes and releases the xa_lock, interrupt-safe.
+ * After calling this function, the XArray is empty and has freed all
+ * memory allocated for its internal data structures.  You are responsible
+ * for freeing the objects referenced by the XArray.
+ *
+ * You do not need to call xa_destroy() if you know the XArray is
+ * already empty.  The IDR used to require this, so you may see some
+ * old code calling idr_destroy() or xa_destroy() on arrays which we
+ * know to be empty, but new code should not do this.
+ *
+ * Context: If the XArray is protected by an IRQ-safe lock, this function
+ * must not be called from interrupt context or with interrupts disabled.
+ * Otherwise it may be called from any context.  It will take and release
+ * the xa_lock with the appropriate disabling & enabling of softirqs
+ * or interrupts.
  */
 void xa_destroy(struct xarray *xa)
 {
 	XA_STATE(xas, xa, 0);
-	unsigned long flags;
+	unsigned int lock_type = xa_lock_type(xa);
 	void *entry;
 
 	xas.xa_node = NULL;
-	xas_lock_irqsave(&xas, flags);
+	xas_lock_type(&xas, lock_type);
 	entry = xa_head_locked(xa);
+	if (!entry)
+		goto out;
 	RCU_INIT_POINTER(xa->xa_head, NULL);
 	xas_init_marks(&xas);
 	if (xa_zero_busy(xa))
@@ -2021,7 +2032,8 @@ void xa_destroy(struct xarray *xa)
 	/* lockdep checks we're still holding the lock in xas_free_nodes() */
 	if (xa_is_node(entry))
 		xas_free_nodes(&xas, xa_to_node(entry));
-	xas_unlock_irqrestore(&xas, flags);
+out:
+	xas_unlock_type(&xas, lock_type);
 }
 EXPORT_SYMBOL(xa_destroy);