netdev - Re: RFC: chasing all idr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZzwMsJAVbGJZD5Az@pop-os.localdomain>
Date: Mon, 18 Nov 2024 19:57:36 -0800
From: Cong Wang <xiyou.wangcong@...il.com>
To: Alexandre Ferrieux <alexandre.ferrieux@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, edumazet@...gle.com, jhs@...atatu.com,
	jiri@...nulli.us, horms@...nel.org, netdev@...r.kernel.org
Subject: Re: RFC: chasing all idr_remove() misses

On Mon, Nov 18, 2024 at 07:51:47PM -0800, Cong Wang wrote:
> On Thu, Nov 14, 2024 at 07:24:27PM +0100, Alexandre Ferrieux wrote:
> > Hi,
> > 
> > In the recent fix of u32's IDR leaks, one side remark is that the problem went
> > unnoticed for 7 years due to the NULL result from idr_remove() being ignored at
> > this call site.
> 
> I'd blame the lack of self test coverage. :)
> 
> > 
> > Now, a cursory grep over the whole Linux tree shows 306 out of 386 call sites
> > (excluding those hidden in macros, if any) don't bother to extract the value
> > returned by idr_remove().
> > 
> > Indeed, a failed IDR removal is "mostly harmless" since IDs are not pointers so
> > the mismatch is detectable (and is detected, returning NULL). However, in racy
> > situations you may end up killing an innocent fresh entry, which may really
> > break things a bit later. And in all cases, a true bug is the root cause.
> > 
> > So, unless we have reasons to think cls_u32 was the only place where two ID
> > encodings might lend themselves to confusion, I'm wondering if it wouldn't make
> > sense to chase the issue more systematically:
> > 
> >  - either with WARN_ON[_ONCE](idr_remove()==NULL) on each call site individually
> > (a year-long endeavor implying tens of maintainers)
> > 
> >  - or with WARN_ON[_ONCE] just before returning NULL within idr_remove() itself,
> > or even radix_tree_delete_item().
> > 
> > Opinions ?
> 
> Yeah, or simply WARN_ON uncleaned IDR in idr_destroy(), which is a more
> common pattern.

Something like this (or, of course, move it to caller to reduce the noise):

diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 976b9bd02a1b..20cc690ffb32 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -1559,6 +1559,8 @@ void __rcu **idr_get_free(struct radix_tree_root *root,
 void idr_destroy(struct idr *idr)
 {
        struct radix_tree_node *node = rcu_dereference_raw(idr->idr_rt.xa_head);
+
+       WARN_ON(!idr_is_empty(idr));
        if (radix_tree_is_internal_node(node))
                radix_tree_free_nodes(node);
        idr->idr_rt.xa_head = NULL;