* [PATCH v2] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge
@ 2020-04-05 17:38 Yihao Wu
2020-04-05 17:57 ` [PATCH v3] " Yihao Wu
0 siblings, 1 reply; 6+ messages in thread
From: Yihao Wu @ 2020-04-05 17:38 UTC (permalink / raw)
To: J . Bruce Fields, Chuck Lever, NeilBrown, Sasha Levin; +Cc: linux-nfs
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 <wuyihao@linux.alibaba.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge
2020-04-05 17:38 [PATCH v2] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge Yihao Wu
@ 2020-04-05 17:57 ` Yihao Wu
2020-04-05 18:07 ` Chuck Lever
2020-04-06 0:03 ` NeilBrown
0 siblings, 2 replies; 6+ messages in thread
From: Yihao Wu @ 2020-04-05 17:57 UTC (permalink / raw)
To: J . Bruce Fields, Chuck Lever, NeilBrown, Sasha Levin; +Cc: linux-nfs
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 <wuyihao@linux.alibaba.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge
2020-04-05 17:57 ` [PATCH v3] " Yihao Wu
@ 2020-04-05 18:07 ` Chuck Lever
2020-04-05 18:31 ` Yihao Wu
2020-04-06 0:03 ` NeilBrown
1 sibling, 1 reply; 6+ messages in thread
From: Chuck Lever @ 2020-04-05 18:07 UTC (permalink / raw)
To: Yihao Wu; +Cc: Bruce Fields, Neil Brown, Sasha Levin, Linux NFS Mailing List
Hi Yihao-
> On Apr 5, 2020, at 1:57 PM, Yihao Wu <wuyihao@linux.alibaba.com> 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 <wuyihao@linux.alibaba.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge
2020-04-05 18:07 ` Chuck Lever
@ 2020-04-05 18:31 ` Yihao Wu
2020-04-05 18:35 ` Chuck Lever
0 siblings, 1 reply; 6+ messages in thread
From: Yihao Wu @ 2020-04-05 18:31 UTC (permalink / raw)
To: Chuck Lever; +Cc: Bruce Fields, Neil Brown, Sasha Levin, Linux NFS Mailing List
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge
2020-04-05 18:31 ` Yihao Wu
@ 2020-04-05 18:35 ` Chuck Lever
0 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2020-04-05 18:35 UTC (permalink / raw)
To: Yihao Wu; +Cc: Bruce Fields, Neil Brown, Sasha Levin, Linux NFS Mailing List
> On Apr 5, 2020, at 2:31 PM, Yihao Wu <wuyihao@linux.alibaba.com> 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 <neilb@suse.de>
to what I have in my tree.
--
Chuck Lever
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge
2020-04-05 17:57 ` [PATCH v3] " Yihao Wu
2020-04-05 18:07 ` Chuck Lever
@ 2020-04-06 0:03 ` NeilBrown
1 sibling, 0 replies; 6+ messages in thread
From: NeilBrown @ 2020-04-06 0:03 UTC (permalink / raw)
To: Yihao Wu, J . Bruce Fields, Chuck Lever, Sasha Levin; +Cc: linux-nfs
[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]
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 <wuyihao@linux.alibaba.com>
> ---
> 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 <neilb@suse.de>
Thanks for finding the bug and testing the solution!
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-06 0:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-05 17:38 [PATCH v2] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge Yihao Wu
2020-04-05 17:57 ` [PATCH v3] " Yihao Wu
2020-04-05 18:07 ` Chuck Lever
2020-04-05 18:31 ` Yihao Wu
2020-04-05 18:35 ` Chuck Lever
2020-04-06 0:03 ` NeilBrown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox