[PATCH 2/2] SUNRPC: Drop all entries from cache_detail when cache_purge()

[PATCH 2/2] SUNRPC: Drop all entries from cache_detail when cache_purge()

[Kinglong Mee]

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 <kinglongmee@gmail.com>
---
 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

Re: [PATCH 2/2] SUNRPC: Drop all entries from cache_detail when cache_purge()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3204 bytes --]

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 <kinglongmee@gmail.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/2] SUNRPC: Drop all entries from cache_detail when cache_purge()

[Kinglong Mee]

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 <kinglongmee@gmail.com>
>> ---
>>  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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html