User always free the cache_detail after sunrpc_destroy_cache_detail(),
so, it must cleanup up entries that left in the cache_detail,
otherwise, NULL reference may be caused when using the left entries.
Also, NeriBrown suggests "write a stand-alone cache_purge()."
v2, a stand-alone cache_purge(), not only for sunrpc_destroy_cache_detail
Signed-off-by: Kinglong Mee <[email protected]>
---
net/sunrpc/cache.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 8147e8d..bd6ee79 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -362,11 +362,6 @@ void sunrpc_destroy_cache_detail(struct cache_detail *cd)
cache_purge(cd);
spin_lock(&cache_list_lock);
write_lock(&cd->hash_lock);
- if (cd->entries) {
- write_unlock(&cd->hash_lock);
- spin_unlock(&cache_list_lock);
- goto out;
- }
if (current_detail == cd)
current_detail = NULL;
list_del_init(&cd->others);
@@ -376,9 +371,6 @@ void sunrpc_destroy_cache_detail(struct cache_detail *cd)
/* module must be being unloaded so its safe to kill the worker */
cancel_delayed_work_sync(&cache_cleaner);
}
- return;
-out:
- printk(KERN_ERR "RPC: failed to unregister %s cache\n", cd->name);
}
EXPORT_SYMBOL_GPL(sunrpc_destroy_cache_detail);
@@ -497,13 +489,30 @@ EXPORT_SYMBOL_GPL(cache_flush);
void cache_purge(struct cache_detail *detail)
{
- time_t now = seconds_since_boot();
- if (detail->flush_time >= now)
- now = detail->flush_time + 1;
- /* 'now' is the maximum value any 'last_refresh' can have */
- detail->flush_time = now;
- detail->nextcheck = seconds_since_boot();
- cache_flush();
+ struct cache_head *ch = NULL;
+ struct hlist_head *head = NULL;
+ struct hlist_node *tmp = NULL;
+ int i = 0;
+
+ write_lock(&detail->hash_lock);
+ if (!detail->entries) {
+ write_unlock(&detail->hash_lock);
+ return;
+ }
+
+ dprintk("RPC: %d entries in %s cache\n", detail->entries, detail->name);
+ for (i = 0; i < detail->hash_size; i++) {
+ head = &detail->hash_table[i];
+ hlist_for_each_entry_safe(ch, tmp, head, cache_list) {
+ hlist_del_init(&ch->cache_list);
+ detail->entries--;
+
+ set_bit(CACHE_CLEANED, &ch->flags);
+ cache_fresh_unlocked(ch, detail);
+ cache_put(ch, detail);
+ }
+ }
+ write_unlock(&detail->hash_lock);
}
EXPORT_SYMBOL_GPL(cache_purge);
--
2.9.3
On Mon, Feb 06 2017, Kinglong Mee wrote:
> User always free the cache_detail after sunrpc_destroy_cache_detail(),
> so, it must cleanup up entries that left in the cache_detail,
> otherwise, NULL reference may be caused when using the left entries.
>
> Also, NeriBrown suggests "write a stand-alone cache_purge()."
>
> v2, a stand-alone cache_purge(), not only for sunrpc_destroy_cache_detail
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> net/sunrpc/cache.c | 39 ++++++++++++++++++++++++---------------
> 1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 8147e8d..bd6ee79 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -362,11 +362,6 @@ void sunrpc_destroy_cache_detail(struct cache_detail *cd)
> cache_purge(cd);
> spin_lock(&cache_list_lock);
> write_lock(&cd->hash_lock);
> - if (cd->entries) {
> - write_unlock(&cd->hash_lock);
> - spin_unlock(&cache_list_lock);
> - goto out;
> - }
> if (current_detail == cd)
> current_detail = NULL;
> list_del_init(&cd->others);
> @@ -376,9 +371,6 @@ void sunrpc_destroy_cache_detail(struct cache_detail *cd)
> /* module must be being unloaded so its safe to kill the worker */
> cancel_delayed_work_sync(&cache_cleaner);
> }
> - return;
> -out:
> - printk(KERN_ERR "RPC: failed to unregister %s cache\n", cd->name);
> }
> EXPORT_SYMBOL_GPL(sunrpc_destroy_cache_detail);
>
> @@ -497,13 +489,30 @@ EXPORT_SYMBOL_GPL(cache_flush);
>
> void cache_purge(struct cache_detail *detail)
> {
> - time_t now = seconds_since_boot();
> - if (detail->flush_time >= now)
> - now = detail->flush_time + 1;
> - /* 'now' is the maximum value any 'last_refresh' can have */
> - detail->flush_time = now;
> - detail->nextcheck = seconds_since_boot();
> - cache_flush();
> + struct cache_head *ch = NULL;
> + struct hlist_head *head = NULL;
> + struct hlist_node *tmp = NULL;
> + int i = 0;
> +
> + write_lock(&detail->hash_lock);
> + if (!detail->entries) {
> + write_unlock(&detail->hash_lock);
> + return;
> + }
> +
> + dprintk("RPC: %d entries in %s cache\n", detail->entries, detail->name);
> + for (i = 0; i < detail->hash_size; i++) {
> + head = &detail->hash_table[i];
> + hlist_for_each_entry_safe(ch, tmp, head, cache_list) {
> + hlist_del_init(&ch->cache_list);
> + detail->entries--;
> +
> + set_bit(CACHE_CLEANED, &ch->flags);
> + cache_fresh_unlocked(ch, detail);
> + cache_put(ch, detail);
I'm a little bothered by calling cache_fresh_unlocked() while holding
->hash_lock. No other code does that.
You could probably argue that we don't need ->hash_lock at all here
because by the time we call cache_purge(), there cannot safely be any
other users. Should we just drop the write_lock() call?
NeilBrown
> + }
> + }
> + write_unlock(&detail->hash_lock);
> }
> EXPORT_SYMBOL_GPL(cache_purge);
>
> --
> 2.9.3
>
> --
> 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
On 2/8/2017 08:04, NeilBrown wrote:
> On Mon, Feb 06 2017, Kinglong Mee wrote:
>
>> User always free the cache_detail after sunrpc_destroy_cache_detail(),
>> so, it must cleanup up entries that left in the cache_detail,
>> otherwise, NULL reference may be caused when using the left entries.
>>
>> Also, NeriBrown suggests "write a stand-alone cache_purge()."
>>
>> v2, a stand-alone cache_purge(), not only for sunrpc_destroy_cache_detail
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> net/sunrpc/cache.c | 39 ++++++++++++++++++++++++---------------
>> 1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>> index 8147e8d..bd6ee79 100644
>> --- a/net/sunrpc/cache.c
>> +++ b/net/sunrpc/cache.c
>> @@ -362,11 +362,6 @@ void sunrpc_destroy_cache_detail(struct cache_detail *cd)
>> cache_purge(cd);
>> spin_lock(&cache_list_lock);
>> write_lock(&cd->hash_lock);
>> - if (cd->entries) {
>> - write_unlock(&cd->hash_lock);
>> - spin_unlock(&cache_list_lock);
>> - goto out;
>> - }
>> if (current_detail == cd)
>> current_detail = NULL;
>> list_del_init(&cd->others);
>> @@ -376,9 +371,6 @@ void sunrpc_destroy_cache_detail(struct cache_detail *cd)
>> /* module must be being unloaded so its safe to kill the worker */
>> cancel_delayed_work_sync(&cache_cleaner);
>> }
>> - return;
>> -out:
>> - printk(KERN_ERR "RPC: failed to unregister %s cache\n", cd->name);
>> }
>> EXPORT_SYMBOL_GPL(sunrpc_destroy_cache_detail);
>>
>> @@ -497,13 +489,30 @@ EXPORT_SYMBOL_GPL(cache_flush);
>>
>> void cache_purge(struct cache_detail *detail)
>> {
>> - time_t now = seconds_since_boot();
>> - if (detail->flush_time >= now)
>> - now = detail->flush_time + 1;
>> - /* 'now' is the maximum value any 'last_refresh' can have */
>> - detail->flush_time = now;
>> - detail->nextcheck = seconds_since_boot();
>> - cache_flush();
>> + struct cache_head *ch = NULL;
>> + struct hlist_head *head = NULL;
>> + struct hlist_node *tmp = NULL;
>> + int i = 0;
>> +
>> + write_lock(&detail->hash_lock);
>> + if (!detail->entries) {
>> + write_unlock(&detail->hash_lock);
>> + return;
>> + }
>> +
>> + dprintk("RPC: %d entries in %s cache\n", detail->entries, detail->name);
>> + for (i = 0; i < detail->hash_size; i++) {
>> + head = &detail->hash_table[i];
>> + hlist_for_each_entry_safe(ch, tmp, head, cache_list) {
>> + hlist_del_init(&ch->cache_list);
>> + detail->entries--;
>> +
>> + set_bit(CACHE_CLEANED, &ch->flags);
>> + cache_fresh_unlocked(ch, detail);
>> + cache_put(ch, detail);
>
> I'm a little bothered by calling cache_fresh_unlocked() while holding
> ->hash_lock. No other code does that.
> You could probably argue that we don't need ->hash_lock at all here
> because by the time we call cache_purge(), there cannot safely be any
> other users. Should we just drop the write_lock() call?
No, we can't.
We call cache_purge() without remove the cache_detail from cache_list,
so that, if we drop the write_lock(), cache_clean may access the
cache_detail at the same time, a double free may happen.
Just move the cache_fresh_unlocked() out of write_lock().
thanks,
Kinglong Mee
>
> NeilBrown
>
>
>> + }
>> + }
>> + write_unlock(&detail->hash_lock);
>> }
>> EXPORT_SYMBOL_GPL(cache_purge);
>>
>> --
>> 2.9.3
>>
>> --
>> 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