* [PATCH 0/2] two fixes for races in SUNRPC. - Take 2.
@ 2013-03-04 6:09 NeilBrown
2013-03-04 6:09 ` [PATCH 1/2] sunrpc/cache: remove races with queuing an upcall NeilBrown
2013-03-04 6:09 ` [PATCH 2/2] sunrpc/cache: use cache_fresh_unlocked consistently and correctly NeilBrown
0 siblings, 2 replies; 4+ messages in thread
From: NeilBrown @ 2013-03-04 6:09 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Bodo Stroesser, linux-nfs
hi,
these two patches fix (I believe) the races recently reported
by Bodo Stroesser.
This version removes the 'return' from cache_deqeue() as discussed.
Thanks,
NeilBrown
---
NeilBrown (2):
sunrpc/cache: remove races with queuing an upcall.
sunrpc/cache: use cache_fresh_unlocked consistently and correctly.
net/sunrpc/cache.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
--
Signature
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 1/2] sunrpc/cache: remove races with queuing an upcall. 2013-03-04 6:09 [PATCH 0/2] two fixes for races in SUNRPC. - Take 2 NeilBrown @ 2013-03-04 6:09 ` NeilBrown 2013-03-04 6:09 ` [PATCH 2/2] sunrpc/cache: use cache_fresh_unlocked consistently and correctly NeilBrown 1 sibling, 0 replies; 4+ messages in thread From: NeilBrown @ 2013-03-04 6:09 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Bodo Stroesser, linux-nfs We currently queue an upcall after setting CACHE_PENDING, and dequeue after clearing CACHE_PENDING. So a request should only be present when CACHE_PENDING is set. However we don't combine the test and the enqueue/dequeue in a protected region, so it is possible (if unlikely) for a race to result in a request being queued without CACHE_PENDING set, or a request to be absent despite CACHE_PENDING. So: include a test for CACHE_PENDING inside the regions of enqueue and dequeue where queue_lock is held, and abort the operation if the value is not as expected. Also remove the 'return' from cache_dequeue() to ensure it always removes all entries: As there is no locking between setting CACHE_PENDING and calling sunrpc_cache_pipe_upcall it is not inconceivable for some other thread to clear CACHE_PENDING and then someone else to set it can call sunrpc_cache_pipe_upcall, both before the original thread completed the call. With this, it perfectly safe and correct to: - call cache_dequeue() if and only if we have just cleared CACHE_PENDING - call sunrpc_cache_pipe_upcall() (via cache_make_upcall) if and only if we have just set CACHE_PENDING. Reported-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> Signed-off-by: NeilBrown <neilb@suse.de> --- net/sunrpc/cache.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 9afa439..0400a92 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1022,6 +1022,9 @@ static void cache_dequeue(struct cache_detail *detail, struct cache_head *ch) struct cache_request *cr = container_of(cq, struct cache_request, q); if (cr->item != ch) continue; + if (test_bit(CACHE_PENDING, &ch->flags)) + /* Lost a race and it is pending again */ + break; if (cr->readers != 0) continue; list_del(&cr->q.list); @@ -1029,7 +1032,6 @@ static void cache_dequeue(struct cache_detail *detail, struct cache_head *ch) cache_put(cr->item, detail); kfree(cr->buf); kfree(cr); - return; } spin_unlock(&queue_lock); } @@ -1151,6 +1153,7 @@ int sunrpc_cache_pipe_upcall(struct cache_detail *detail, struct cache_head *h, struct cache_request *crq; char *bp; int len; + int ret = 0; if (!cache_listeners_exist(detail)) { warn_no_listener(detail); @@ -1182,10 +1185,18 @@ int sunrpc_cache_pipe_upcall(struct cache_detail *detail, struct cache_head *h, crq->len = PAGE_SIZE - len; crq->readers = 0; spin_lock(&queue_lock); - list_add_tail(&crq->q.list, &detail->queue); + if (test_bit(CACHE_PENDING, &h->flags)) + list_add_tail(&crq->q.list, &detail->queue); + else + /* Lost a race, no longer PENDING, so don't enqueue */ + ret = -EAGAIN; spin_unlock(&queue_lock); wake_up(&queue_wait); - return 0; + if (ret == -EAGAIN) { + kfree(buf); + kfree(crq); + } + return ret; } EXPORT_SYMBOL_GPL(sunrpc_cache_pipe_upcall); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] sunrpc/cache: use cache_fresh_unlocked consistently and correctly. 2013-03-04 6:09 [PATCH 0/2] two fixes for races in SUNRPC. - Take 2 NeilBrown 2013-03-04 6:09 ` [PATCH 1/2] sunrpc/cache: remove races with queuing an upcall NeilBrown @ 2013-03-04 6:09 ` NeilBrown 1 sibling, 0 replies; 4+ messages in thread From: NeilBrown @ 2013-03-04 6:09 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Bodo Stroesser, linux-nfs cache_fresh_unlocked() is called when a cache entry has been updated and ensures that if there were any pending upcalls, they are cleared. So every time we update a cache entry, we should call this, and this should be the only way that we try to clear pending calls (that sort of uniformity makes code sooo much easier to read). try_to_negate_entry() will (possibly) mark an entry as negative. If it doesn't, it is because the entry already is VALID. So the entry will be valid on exit, so it is appropriate to call cache_fresh_unlocked(). So tidy up try_to_negate_entry() to do that, and remove partial open-coded cache_fresh_unlocked() from the one call-site of try_to_negate_entry(). In the other branch of the 'switch(cache_make_upcall())', we again have a partial open-coded version of cache_fresh_unlocked(). Replace that with a real call. And again in cache_clean(), use a real call to cache_fresh_unlocked(). These call sites might previously have called cache_revisit_request() if CACHE_PENDING wasn't set. This is never necessary because cache_revisit_request() can only do anything if the item is in the cache_defer_hash, However any time that an item is added to the cache_defer_hash (setup_deferral), the code immediately tests CACHE_PENDING, and removes the entry again if it is clear. So all other places we only need to 'cache_revisit_request' if we've just cleared CACHE_PENDING. Reported-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> Signed-off-by: NeilBrown <neilb@suse.de> --- net/sunrpc/cache.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 0400a92..2e07ba1 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -228,15 +228,14 @@ static int try_to_negate_entry(struct cache_detail *detail, struct cache_head *h write_lock(&detail->hash_lock); rv = cache_is_valid(detail, h); - if (rv != -EAGAIN) { - write_unlock(&detail->hash_lock); - return rv; + if (rv == -EAGAIN) { + set_bit(CACHE_NEGATIVE, &h->flags); + cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY); + rv = -ENOENT; } - set_bit(CACHE_NEGATIVE, &h->flags); - cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY); write_unlock(&detail->hash_lock); cache_fresh_unlocked(h, detail); - return -ENOENT; + return rv; } /* @@ -275,13 +274,10 @@ int cache_check(struct cache_detail *detail, if (!test_and_set_bit(CACHE_PENDING, &h->flags)) { switch (cache_make_upcall(detail, h)) { case -EINVAL: - clear_bit(CACHE_PENDING, &h->flags); - cache_revisit_request(h); rv = try_to_negate_entry(detail, h); break; case -EAGAIN: - clear_bit(CACHE_PENDING, &h->flags); - cache_revisit_request(h); + cache_fresh_unlocked(h, detail); break; } } @@ -457,9 +453,7 @@ static int cache_clean(void) current_index ++; spin_unlock(&cache_list_lock); if (ch) { - if (test_and_clear_bit(CACHE_PENDING, &ch->flags)) - cache_dequeue(current_detail, ch); - cache_revisit_request(ch); + cache_fresh_unlocked(ch, d); cache_put(ch, d); } } else ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 0/2] two fixes for races in SUNRPC.
@ 2013-02-26 6:36 NeilBrown
2013-02-26 6:36 ` [PATCH 2/2] sunrpc/cache: use cache_fresh_unlocked consistently and correctly NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: NeilBrown @ 2013-02-26 6:36 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Bodo Stroesser, linux-nfs
hi,
these two patches fix (I believe) the races recently reported
by Bodo Stroesser.
Thanks,
NeilBrown
---
NeilBrown (2):
sunrpc/cache: remove races with queuing an upcall.
sunrpc/cache: use cache_fresh_unlocked consistently and correctly.
net/sunrpc/cache.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)
--
Signature
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 2/2] sunrpc/cache: use cache_fresh_unlocked consistently and correctly. 2013-02-26 6:36 [PATCH 0/2] two fixes for races in SUNRPC NeilBrown @ 2013-02-26 6:36 ` NeilBrown 0 siblings, 0 replies; 4+ messages in thread From: NeilBrown @ 2013-02-26 6:36 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Bodo Stroesser, linux-nfs cache_fresh_unlocked() is called when a cache entry has been updated and ensures that if there were any pending upcalls, they are cleared. So every time we update a cache entry, we should call this, and this should be the only way that we try to clear pending calls (that sort of uniformity makes code sooo much easier to read). try_to_negate_entry() will (possibly) mark an entry as negative. If it doesn't, it is because the entry already is VALID. So the entry will be valid on exit, so it is appropriate to call cache_fresh_unlocked(). So tidy up try_to_negate_entry() to do that, and remove partial open-coded cache_fresh_unlocked() from the one call-site of try_to_negate_entry(). In the other branch of the 'switch(cache_make_upcall())', we again have a partial open-coded version of cache_fresh_unlocked(). Replace that with a real call. And again in cache_clean(), use a real call to cache_fresh_unlocked(). These call sites might previously have called cache_revisit_request() if CACHE_PENDING wasn't set. This is never necessary because cache_revisit_request() can only do anything if the item is in the cache_defer_hash, However any time that an item is added to the cache_defer_hash (setup_deferral), the code immediately tests CACHE_PENDING, and removes the entry again if it is clear. So all other places we only need to 'cache_revisit_request' if we've just cleared CACHE_PENDING. Reported-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> Signed-off-by: NeilBrown <neilb@suse.de> --- net/sunrpc/cache.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index b48c8ef..7ebee64 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -228,15 +228,14 @@ static int try_to_negate_entry(struct cache_detail *detail, struct cache_head *h write_lock(&detail->hash_lock); rv = cache_is_valid(detail, h); - if (rv != -EAGAIN) { - write_unlock(&detail->hash_lock); - return rv; + if (rv == -EAGAIN) { + set_bit(CACHE_NEGATIVE, &h->flags); + cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY); + rv = -ENOENT; } - set_bit(CACHE_NEGATIVE, &h->flags); - cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY); write_unlock(&detail->hash_lock); cache_fresh_unlocked(h, detail); - return -ENOENT; + return rv; } /* @@ -275,13 +274,10 @@ int cache_check(struct cache_detail *detail, if (!test_and_set_bit(CACHE_PENDING, &h->flags)) { switch (cache_make_upcall(detail, h)) { case -EINVAL: - clear_bit(CACHE_PENDING, &h->flags); - cache_revisit_request(h); rv = try_to_negate_entry(detail, h); break; case -EAGAIN: - clear_bit(CACHE_PENDING, &h->flags); - cache_revisit_request(h); + cache_fresh_unlocked(h, detail); break; } } @@ -457,9 +453,7 @@ static int cache_clean(void) current_index ++; spin_unlock(&cache_list_lock); if (ch) { - if (test_and_clear_bit(CACHE_PENDING, &ch->flags)) - cache_dequeue(current_detail, ch); - cache_revisit_request(ch); + cache_fresh_unlocked(ch, d); cache_put(ch, d); } } else ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-03-04 6:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-04 6:09 [PATCH 0/2] two fixes for races in SUNRPC. - Take 2 NeilBrown 2013-03-04 6:09 ` [PATCH 1/2] sunrpc/cache: remove races with queuing an upcall NeilBrown 2013-03-04 6:09 ` [PATCH 2/2] sunrpc/cache: use cache_fresh_unlocked consistently and correctly NeilBrown -- strict thread matches above, loose matches on Subject: below -- 2013-02-26 6:36 [PATCH 0/2] two fixes for races in SUNRPC NeilBrown 2013-02-26 6:36 ` [PATCH 2/2] sunrpc/cache: use cache_fresh_unlocked consistently and correctly NeilBrown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).