Deleting list entry within hlist_for_each_entry_safe is not safe unless
next pointer (tmp) is protected too. It's not, because once hash_lock
is released, cache_clean may delete the entry that tmp points to. Then
cache_purge can walk to a deleted entry and tries to double free it.
Fix this bug by holding only the deleted entry's reference.
Signed-off-by: Yihao Wu <[email protected]>
---
net/sunrpc/cache.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index af0ddd28b081..9649c7fcccd2 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -541,7 +541,8 @@ void cache_purge(struct cache_detail *detail)
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) {
+ while (!hlist_empty(head)) {
+ ch = hlist_entry(head->first, struct cache_head, cache_list);
sunrpc_begin_cache_remove_entry(ch, detail);
spin_unlock(&detail->hash_lock);
sunrpc_end_cache_remove_entry(ch, detail);
--
2.20.1.2432.ga663e714
Deleting list entry within hlist_for_each_entry_safe is not safe unless
next pointer (tmp) is protected too. It's not, because once hash_lock
is released, cache_clean may delete the entry that tmp points to. Then
cache_purge can walk to a deleted entry and tries to double free it.
Fix this bug by holding only the deleted entry's reference.
Signed-off-by: Yihao Wu <[email protected]>
---
v1->v2: Use Neil's better solution
v2->v3: Fix a checkscript warning
net/sunrpc/cache.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index af0ddd28b081..b445874e8e2f 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -541,7 +541,9 @@ void cache_purge(struct cache_detail *detail)
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) {
+ while (!hlist_empty(head)) {
+ ch = hlist_entry(head->first, struct cache_head,
+ cache_list);
sunrpc_begin_cache_remove_entry(ch, detail);
spin_unlock(&detail->hash_lock);
sunrpc_end_cache_remove_entry(ch, detail);
--
2.20.1.2432.ga663e714
Hi Yihao-
> On Apr 5, 2020, at 1:57 PM, Yihao Wu <[email protected]> wrote:
>
> Deleting list entry within hlist_for_each_entry_safe is not safe unless
> next pointer (tmp) is protected too. It's not, because once hash_lock
> is released, cache_clean may delete the entry that tmp points to. Then
> cache_purge can walk to a deleted entry and tries to double free it.
>
> Fix this bug by holding only the deleted entry's reference.
>
> Signed-off-by: Yihao Wu <[email protected]>
> ---
> v1->v2: Use Neil's better solution
> v2->v3: Fix a checkscript warning
>
> net/sunrpc/cache.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index af0ddd28b081..b445874e8e2f 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -541,7 +541,9 @@ void cache_purge(struct cache_detail *detail)
> 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) {
If review/testing shows you need to respin this patch, I note that "tmp" is
now unused and should be removed. I've pulled v3 into my testing branch and
made that minor change. Thanks!
> + while (!hlist_empty(head)) {
> + ch = hlist_entry(head->first, struct cache_head,
> + cache_list);
> sunrpc_begin_cache_remove_entry(ch, detail);
> spin_unlock(&detail->hash_lock);
> sunrpc_end_cache_remove_entry(ch, detail);
> --
> 2.20.1.2432.ga663e714
--
Chuck Lever
>> net/sunrpc/cache.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>> index af0ddd28b081..b445874e8e2f 100644
>> --- a/net/sunrpc/cache.c
>> +++ b/net/sunrpc/cache.c
>> @@ -541,7 +541,9 @@ void cache_purge(struct cache_detail *detail)
>> 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) {
>
> If review/testing shows you need to respin this patch, I note that "tmp" is
> now unused and should be removed. I've pulled v3 into my testing branch and
> made that minor change. Thanks!
>
>
>> + while (!hlist_empty(head)) {
>> + ch = hlist_entry(head->first, struct cache_head,
>> + cache_list);
>> sunrpc_begin_cache_remove_entry(ch, detail);
>> spin_unlock(&detail->hash_lock);
>> sunrpc_end_cache_remove_entry(ch, detail);
>> --
>> 2.20.1.2432.ga663e714
>
> --
> Chuck Lever
>
>
Thanks a lot, Chuck!
If it needs further changes by me, I'll fix the unused 'tmp' along with them.
BTW, if you and Neil think it's proper to add Signed-off-by Neil too later,
please do, since the bug fix owes to Neil's idea :-)
Yihao Wu
> On Apr 5, 2020, at 2:31 PM, Yihao Wu <[email protected]> wrote:
>
>>> net/sunrpc/cache.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>>> index af0ddd28b081..b445874e8e2f 100644
>>> --- a/net/sunrpc/cache.c
>>> +++ b/net/sunrpc/cache.c
>>> @@ -541,7 +541,9 @@ void cache_purge(struct cache_detail *detail)
>>> 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) {
>>
>> If review/testing shows you need to respin this patch, I note that "tmp" is
>> now unused and should be removed. I've pulled v3 into my testing branch and
>> made that minor change. Thanks!
>>
>>
>>> + while (!hlist_empty(head)) {
>>> + ch = hlist_entry(head->first, struct cache_head,
>>> + cache_list);
>>> sunrpc_begin_cache_remove_entry(ch, detail);
>>> spin_unlock(&detail->hash_lock);
>>> sunrpc_end_cache_remove_entry(ch, detail);
>>> --
>>> 2.20.1.2432.ga663e714
>>
>> --
>> Chuck Lever
>>
>>
>
> Thanks a lot, Chuck!
>
> If it needs further changes by me, I'll fix the unused 'tmp' along with them.
>
> BTW, if you and Neil think it's proper to add Signed-off-by Neil too later,
> please do, since the bug fix owes to Neil's idea :-)
Actually a "Suggested-by:" tag is appropriate for attribution. I'll add:
Suggested-by: NeilBrown <[email protected]>
to what I have in my tree.
--
Chuck Lever
On Mon, Apr 06 2020, Yihao Wu wrote:
> Deleting list entry within hlist_for_each_entry_safe is not safe unless
> next pointer (tmp) is protected too. It's not, because once hash_lock
> is released, cache_clean may delete the entry that tmp points to. Then
> cache_purge can walk to a deleted entry and tries to double free it.
>
> Fix this bug by holding only the deleted entry's reference.
>
> Signed-off-by: Yihao Wu <[email protected]>
> ---
> v1->v2: Use Neil's better solution
> v2->v3: Fix a checkscript warning
>
> net/sunrpc/cache.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index af0ddd28b081..b445874e8e2f 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -541,7 +541,9 @@ void cache_purge(struct cache_detail *detail)
> 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) {
> + while (!hlist_empty(head)) {
> + ch = hlist_entry(head->first, struct cache_head,
> + cache_list);
> sunrpc_begin_cache_remove_entry(ch, detail);
> spin_unlock(&detail->hash_lock);
> sunrpc_end_cache_remove_entry(ch, detail);
> --
> 2.20.1.2432.ga663e714
Reviewed-by: NeilBrown <[email protected]>
Thanks for finding the bug and testing the solution!
NeilBrown