| Message ID | e0dd0339-a15e-814d-ac5a-5f51bc15d73c@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] SUNRPC/cache: Fix unsafe traverse caused double-free in cache_purge | expand |
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
>> 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 <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
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
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);
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(-)