All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] swapoff tmpfs radix_tree: remember to rcu_read_unlock
@ 2014-02-13  2:45 ` Hugh Dickins
  0 siblings, 0 replies; 6+ messages in thread
From: Hugh Dickins @ 2014-02-13  2:45 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Johannes Weiner, linux-kernel, linux-mm

Running fsx on tmpfs with concurrent memhog-swapoff-swapon, lots of

BUG: sleeping function called from invalid context at kernel/fork.c:606
in_atomic(): 0, irqs_disabled(): 0, pid: 1394, name: swapoff
1 lock held by swapoff/1394:
 #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
followed by
================================================
[ BUG: lock held when returning to user space! ]
3.14.0-rc1 #3 Not tainted
------------------------------------------------
swapoff/1394 is leaving the kernel with locks still held!
1 lock held by swapoff/1394:
 #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
after which the system recovered nicely.

Whoops, I long ago forgot the rcu_read_unlock() on one unlikely branch.

Fixes: e504f3fdd63d ("tmpfs radix_tree: locate_item to speed up swapoff")
Signed-off-by: Hugh Dickins <hughd@google.com>
---

Of course, the truth is that I had been hoping to break Johannes's
patchset in mmotm, was thrilled to get this on that, then despondent
to realize that the only bug I had found was mine.  Surprised I've
not seen it before in 2.5 years: tried again on 3.14-rc1, got the
same after 25 minutes.  Probably not serious enough for -stable,
but please can we slip the fix into 3.14 - sorry, Johannes's
mm-keep-page-cache-radix-tree-nodes-in-check.patch will need a refresh.

 lib/radix-tree.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- 3.14-rc2/lib/radix-tree.c	2013-11-03 15:41:51.000000000 -0800
+++ linux/lib/radix-tree.c	2014-02-09 21:47:22.688092825 -0800
@@ -1253,8 +1253,10 @@ unsigned long radix_tree_locate_item(str
 
 		node = indirect_to_ptr(node);
 		max_index = radix_tree_maxindex(node->height);
-		if (cur_index > max_index)
+		if (cur_index > max_index) {
+			rcu_read_unlock();
 			break;
+		}
 
 		cur_index = __locate(node, item, cur_index, &found_index);
 		rcu_read_unlock();

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] swapoff tmpfs radix_tree: remember to rcu_read_unlock
@ 2014-02-13  2:45 ` Hugh Dickins
  0 siblings, 0 replies; 6+ messages in thread
From: Hugh Dickins @ 2014-02-13  2:45 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Johannes Weiner, linux-kernel, linux-mm

Running fsx on tmpfs with concurrent memhog-swapoff-swapon, lots of

BUG: sleeping function called from invalid context at kernel/fork.c:606
in_atomic(): 0, irqs_disabled(): 0, pid: 1394, name: swapoff
1 lock held by swapoff/1394:
 #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
followed by
================================================
[ BUG: lock held when returning to user space! ]
3.14.0-rc1 #3 Not tainted
------------------------------------------------
swapoff/1394 is leaving the kernel with locks still held!
1 lock held by swapoff/1394:
 #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
after which the system recovered nicely.

Whoops, I long ago forgot the rcu_read_unlock() on one unlikely branch.

Fixes: e504f3fdd63d ("tmpfs radix_tree: locate_item to speed up swapoff")
Signed-off-by: Hugh Dickins <hughd@google.com>
---

Of course, the truth is that I had been hoping to break Johannes's
patchset in mmotm, was thrilled to get this on that, then despondent
to realize that the only bug I had found was mine.  Surprised I've
not seen it before in 2.5 years: tried again on 3.14-rc1, got the
same after 25 minutes.  Probably not serious enough for -stable,
but please can we slip the fix into 3.14 - sorry, Johannes's
mm-keep-page-cache-radix-tree-nodes-in-check.patch will need a refresh.

 lib/radix-tree.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- 3.14-rc2/lib/radix-tree.c	2013-11-03 15:41:51.000000000 -0800
+++ linux/lib/radix-tree.c	2014-02-09 21:47:22.688092825 -0800
@@ -1253,8 +1253,10 @@ unsigned long radix_tree_locate_item(str
 
 		node = indirect_to_ptr(node);
 		max_index = radix_tree_maxindex(node->height);
-		if (cur_index > max_index)
+		if (cur_index > max_index) {
+			rcu_read_unlock();
 			break;
+		}
 
 		cur_index = __locate(node, item, cur_index, &found_index);
 		rcu_read_unlock();

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] swapoff tmpfs radix_tree: remember to rcu_read_unlock
  2014-02-13  2:45 ` Hugh Dickins
@ 2014-02-13 22:30   ` Andrew Morton
  -1 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2014-02-13 22:30 UTC (permalink / raw)
  To: Hugh Dickins; +Cc: Johannes Weiner, linux-kernel, linux-mm

On Wed, 12 Feb 2014 18:45:07 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:

> Running fsx on tmpfs with concurrent memhog-swapoff-swapon, lots of
> 
> BUG: sleeping function called from invalid context at kernel/fork.c:606
> in_atomic(): 0, irqs_disabled(): 0, pid: 1394, name: swapoff
> 1 lock held by swapoff/1394:
>  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> followed by
> ================================================
> [ BUG: lock held when returning to user space! ]
> 3.14.0-rc1 #3 Not tainted
> ------------------------------------------------
> swapoff/1394 is leaving the kernel with locks still held!
> 1 lock held by swapoff/1394:
>  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> after which the system recovered nicely.
> 
> Whoops, I long ago forgot the rcu_read_unlock() on one unlikely branch.
> 
> Fixes: e504f3fdd63d ("tmpfs radix_tree: locate_item to speed up swapoff")

huh.  Venerable.  I'm surprised that such an obvious blooper wasn't
spotted at review.  Why didn't anyone else hit this.


> Of course, the truth is that I had been hoping to break Johannes's
> patchset in mmotm, was thrilled to get this on that, then despondent
> to realize that the only bug I had found was mine.  Surprised I've
> not seen it before in 2.5 years: tried again on 3.14-rc1, got the
> same after 25 minutes.  Probably not serious enough for -stable,
> but please can we slip the fix into 3.14 - sorry, Johannes's
> mm-keep-page-cache-radix-tree-nodes-in-check.patch will need a refresh.

I fixed it up.

unsigned long radix_tree_locate_item(struct radix_tree_root *root, void *item)
{
	struct radix_tree_node *node;
	unsigned long max_index;
	unsigned long cur_index = 0;
	unsigned long found_index = -1;

	do {
		rcu_read_lock();
		node = rcu_dereference_raw(root->rnode);
		if (!radix_tree_is_indirect_ptr(node)) {
			rcu_read_unlock();
			if (node == item)
				found_index = 0;
			break;
		}

		node = indirect_to_ptr(node);
		max_index = radix_tree_maxindex(node->path &
						RADIX_TREE_HEIGHT_MASK);
		if (cur_index > max_index) {
			rcu_read_unlock();
			break;
		}

		cur_index = __locate(node, item, cur_index, &found_index);
		rcu_read_unlock();
		cond_resched();
	} while (cur_index != 0 && cur_index <= max_index);

	return found_index;
}


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] swapoff tmpfs radix_tree: remember to rcu_read_unlock
@ 2014-02-13 22:30   ` Andrew Morton
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2014-02-13 22:30 UTC (permalink / raw)
  To: Hugh Dickins; +Cc: Johannes Weiner, linux-kernel, linux-mm

On Wed, 12 Feb 2014 18:45:07 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:

> Running fsx on tmpfs with concurrent memhog-swapoff-swapon, lots of
> 
> BUG: sleeping function called from invalid context at kernel/fork.c:606
> in_atomic(): 0, irqs_disabled(): 0, pid: 1394, name: swapoff
> 1 lock held by swapoff/1394:
>  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> followed by
> ================================================
> [ BUG: lock held when returning to user space! ]
> 3.14.0-rc1 #3 Not tainted
> ------------------------------------------------
> swapoff/1394 is leaving the kernel with locks still held!
> 1 lock held by swapoff/1394:
>  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> after which the system recovered nicely.
> 
> Whoops, I long ago forgot the rcu_read_unlock() on one unlikely branch.
> 
> Fixes: e504f3fdd63d ("tmpfs radix_tree: locate_item to speed up swapoff")

huh.  Venerable.  I'm surprised that such an obvious blooper wasn't
spotted at review.  Why didn't anyone else hit this.


> Of course, the truth is that I had been hoping to break Johannes's
> patchset in mmotm, was thrilled to get this on that, then despondent
> to realize that the only bug I had found was mine.  Surprised I've
> not seen it before in 2.5 years: tried again on 3.14-rc1, got the
> same after 25 minutes.  Probably not serious enough for -stable,
> but please can we slip the fix into 3.14 - sorry, Johannes's
> mm-keep-page-cache-radix-tree-nodes-in-check.patch will need a refresh.

I fixed it up.

unsigned long radix_tree_locate_item(struct radix_tree_root *root, void *item)
{
	struct radix_tree_node *node;
	unsigned long max_index;
	unsigned long cur_index = 0;
	unsigned long found_index = -1;

	do {
		rcu_read_lock();
		node = rcu_dereference_raw(root->rnode);
		if (!radix_tree_is_indirect_ptr(node)) {
			rcu_read_unlock();
			if (node == item)
				found_index = 0;
			break;
		}

		node = indirect_to_ptr(node);
		max_index = radix_tree_maxindex(node->path &
						RADIX_TREE_HEIGHT_MASK);
		if (cur_index > max_index) {
			rcu_read_unlock();
			break;
		}

		cur_index = __locate(node, item, cur_index, &found_index);
		rcu_read_unlock();
		cond_resched();
	} while (cur_index != 0 && cur_index <= max_index);

	return found_index;
}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] swapoff tmpfs radix_tree: remember to rcu_read_unlock
  2014-02-13 22:30   ` Andrew Morton
@ 2014-02-15 23:53     ` Hugh Dickins
  -1 siblings, 0 replies; 6+ messages in thread
From: Hugh Dickins @ 2014-02-15 23:53 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Hugh Dickins, Johannes Weiner, linux-kernel, linux-mm

On Thu, 13 Feb 2014, Andrew Morton wrote:
> On Wed, 12 Feb 2014 18:45:07 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
> 
> > Running fsx on tmpfs with concurrent memhog-swapoff-swapon, lots of
> > 
> > BUG: sleeping function called from invalid context at kernel/fork.c:606
> > in_atomic(): 0, irqs_disabled(): 0, pid: 1394, name: swapoff
> > 1 lock held by swapoff/1394:
> >  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> > followed by
> > ================================================
> > [ BUG: lock held when returning to user space! ]
> > 3.14.0-rc1 #3 Not tainted
> > ------------------------------------------------
> > swapoff/1394 is leaving the kernel with locks still held!
> > 1 lock held by swapoff/1394:
> >  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> > after which the system recovered nicely.
> > 
> > Whoops, I long ago forgot the rcu_read_unlock() on one unlikely branch.
> > 
> > Fixes: e504f3fdd63d ("tmpfs radix_tree: locate_item to speed up swapoff")
> 
> huh.  Venerable.  I'm surprised that such an obvious blooper wasn't
> spotted at review.  Why didn't anyone else hit this.

No surprise that it missed review, obvious though it is in the fix.

And not much surprise that noone else hit this: for most people, even
those using tmpfs and pushing out to swap, swapoff is just something
that happens shortly before the screen goes blank when you shutdown
(and, I haven't noticed how distros order it these days, but swapoff
is anyway better done after unmounting tmpfss, to avoid its slowness).

And it does need the swapped tmpfs file to be truncated or unlinked
while swapoff is searching through it racily with RCU lookups.

What puzzled me more was, why hadn't I seen it before?  I don't run
that fsx test particularly often, but have certainly run it dozens
of times between then and now.  I think the answer must be where I
said "after which the system recovered nicely": I probably did hit
it before, but wasn't attending to the screen at the time, the
warnings got scrolled off by timestamps I was printing, and I
failed to check dmesg or /var/log/messages afterwards.

> 
> 
> > Of course, the truth is that I had been hoping to break Johannes's
> > patchset in mmotm, was thrilled to get this on that, then despondent
> > to realize that the only bug I had found was mine.  Surprised I've
> > not seen it before in 2.5 years: tried again on 3.14-rc1, got the
> > same after 25 minutes.  Probably not serious enough for -stable,
> > but please can we slip the fix into 3.14 - sorry, Johannes's
> > mm-keep-page-cache-radix-tree-nodes-in-check.patch will need a refresh.
> 
> I fixed it up.

Thanks!

Hugh

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] swapoff tmpfs radix_tree: remember to rcu_read_unlock
@ 2014-02-15 23:53     ` Hugh Dickins
  0 siblings, 0 replies; 6+ messages in thread
From: Hugh Dickins @ 2014-02-15 23:53 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Hugh Dickins, Johannes Weiner, linux-kernel, linux-mm

On Thu, 13 Feb 2014, Andrew Morton wrote:
> On Wed, 12 Feb 2014 18:45:07 -0800 (PST) Hugh Dickins <hughd@google.com> wrote:
> 
> > Running fsx on tmpfs with concurrent memhog-swapoff-swapon, lots of
> > 
> > BUG: sleeping function called from invalid context at kernel/fork.c:606
> > in_atomic(): 0, irqs_disabled(): 0, pid: 1394, name: swapoff
> > 1 lock held by swapoff/1394:
> >  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> > followed by
> > ================================================
> > [ BUG: lock held when returning to user space! ]
> > 3.14.0-rc1 #3 Not tainted
> > ------------------------------------------------
> > swapoff/1394 is leaving the kernel with locks still held!
> > 1 lock held by swapoff/1394:
> >  #0:  (rcu_read_lock){.+.+.+}, at: [<ffffffff812520a1>] radix_tree_locate_item+0x1f/0x2b6
> > after which the system recovered nicely.
> > 
> > Whoops, I long ago forgot the rcu_read_unlock() on one unlikely branch.
> > 
> > Fixes: e504f3fdd63d ("tmpfs radix_tree: locate_item to speed up swapoff")
> 
> huh.  Venerable.  I'm surprised that such an obvious blooper wasn't
> spotted at review.  Why didn't anyone else hit this.

No surprise that it missed review, obvious though it is in the fix.

And not much surprise that noone else hit this: for most people, even
those using tmpfs and pushing out to swap, swapoff is just something
that happens shortly before the screen goes blank when you shutdown
(and, I haven't noticed how distros order it these days, but swapoff
is anyway better done after unmounting tmpfss, to avoid its slowness).

And it does need the swapped tmpfs file to be truncated or unlinked
while swapoff is searching through it racily with RCU lookups.

What puzzled me more was, why hadn't I seen it before?  I don't run
that fsx test particularly often, but have certainly run it dozens
of times between then and now.  I think the answer must be where I
said "after which the system recovered nicely": I probably did hit
it before, but wasn't attending to the screen at the time, the
warnings got scrolled off by timestamps I was printing, and I
failed to check dmesg or /var/log/messages afterwards.

> 
> 
> > Of course, the truth is that I had been hoping to break Johannes's
> > patchset in mmotm, was thrilled to get this on that, then despondent
> > to realize that the only bug I had found was mine.  Surprised I've
> > not seen it before in 2.5 years: tried again on 3.14-rc1, got the
> > same after 25 minutes.  Probably not serious enough for -stable,
> > but please can we slip the fix into 3.14 - sorry, Johannes's
> > mm-keep-page-cache-radix-tree-nodes-in-check.patch will need a refresh.
> 
> I fixed it up.

Thanks!

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-15 23:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-13  2:45 [PATCH] swapoff tmpfs radix_tree: remember to rcu_read_unlock Hugh Dickins
2014-02-13  2:45 ` Hugh Dickins
2014-02-13 22:30 ` Andrew Morton
2014-02-13 22:30   ` Andrew Morton
2014-02-15 23:53   ` Hugh Dickins
2014-02-15 23:53     ` Hugh Dickins

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.