2013-07-10 17:21:18

by David Jeffery

Subject: [PATCH] lockd: protect nlm_blocked access in nlmsvc_retry_blocked

In nlmsvc_retry_blocked, the check that the list is non-empty and acquiring
the pointer of the first entry is unprotected by any lock. This allows a rare
race condition when there is only one entry on the list. A function such as
nlmsvc_grant_callback() can be called, which will temporarily remove the entry
from the list. Between the list_empty() and list_entry(),the list may become
empty, causing an invalid pointer to be used as an nlm_block, leading to a
possible crash.

This patch adds the nlm_block_lock around these calls to prevent concurrent
use of the nlm_blocked list.

Signed-off-by: David Jeffery <[email protected]>


--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -951,6 +951,7 @@ nlmsvc_retry_blocked(void)
unsigned long timeout = MAX_SCHEDULE_TIMEOUT;
struct nlm_block *block;

+ spin_lock(&nlm_blocked_lock);
while (!list_empty(&nlm_blocked) && !kthread_should_stop()) {
block = list_entry(nlm_blocked.next, struct nlm_block, b_list);

@@ -960,6 +961,7 @@ nlmsvc_retry_blocked(void)
timeout = block->b_when - jiffies;
break;
}
+ spin_unlock(&nlm_blocked_lock);

dprintk("nlmsvc_retry_blocked(%p, when=%ld)\n",
block, block->b_when);
@@ -969,7 +971,9 @@ nlmsvc_retry_blocked(void)
retry_deferred_block(block);
} else
nlmsvc_grant_blocked(block);
+ spin_lock(&nlm_blocked_lock);
}
+ spin_unlock(&nlm_blocked_lock);

return timeout;
}

2013-07-11 21:01:13

by J. Bruce Fields

Subject: Re: [PATCH] lockd: protect nlm_blocked access in nlmsvc_retry_blocked

On Wed, Jul 10, 2013 at 01:19:50PM -0400, David Jeffery wrote:
> In nlmsvc_retry_blocked, the check that the list is non-empty and acquiring
> the pointer of the first entry is unprotected by any lock. This allows a rare
> race condition when there is only one entry on the list. A function such as
> nlmsvc_grant_callback() can be called, which will temporarily remove the entry
> from the list. Between the list_empty() and list_entry(),the list may become
> empty, causing an invalid pointer to be used as an nlm_block, leading to a
> possible crash.
>
> This patch adds the nlm_block_lock around these calls to prevent concurrent
> use of the nlm_blocked list.

Thanks! Looks like this bug probably originated from
f904be9cc77f361d37d71468b13ff3d1a1823dea "lockd: Mostly remove BKL from
the server" ?

Applying for 3.11.

--b.

>
> Signed-off-by: David Jeffery <[email protected]>
>
>
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -951,6 +951,7 @@ nlmsvc_retry_blocked(void)
> unsigned long timeout = MAX_SCHEDULE_TIMEOUT;
> struct nlm_block *block;
>
> + spin_lock(&nlm_blocked_lock);
> while (!list_empty(&nlm_blocked) && !kthread_should_stop()) {
> block = list_entry(nlm_blocked.next, struct nlm_block, b_list);
>
> @@ -960,6 +961,7 @@ nlmsvc_retry_blocked(void)
> timeout = block->b_when - jiffies;
> break;
> }
> + spin_unlock(&nlm_blocked_lock);
>
> dprintk("nlmsvc_retry_blocked(%p, when=%ld)\n",
> block, block->b_when);
> @@ -969,7 +971,9 @@ nlmsvc_retry_blocked(void)
> retry_deferred_block(block);
> } else
> nlmsvc_grant_blocked(block);
> + spin_lock(&nlm_blocked_lock);
> }
> + spin_unlock(&nlm_blocked_lock);
>
> return timeout;
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html