[PATCH net] rtnetlink: fix leak of SRCU struct in rtnl_link_register

[PATCH net] rtnetlink: fix leak of SRCU struct in rtnl_link_register

[Sabrina Dubroca]

Commit 6b57ff21a310 ("rtnetlink: Protect link_ops by mutex.") swapped
the EEXIST check with the init_srcu_struct, but didn't add cleanup of
the SRCU struct we just allocated in case of error.

Fixes: 6b57ff21a310 ("rtnetlink: Protect link_ops by mutex.")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/core/rtnetlink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index dad4b1054955..8072a43196ef 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -627,6 +627,9 @@ int rtnl_link_register(struct rtnl_link_ops *ops)
 
 	list_add_tail_rcu(&ops->list, &link_ops);
 unlock:
+	if (err)
+		cleanup_srcu_struct(&ops->srcu);
+
 	mutex_unlock(&link_ops_mutex);
 
 	return err;
-- 
2.51.2

Re: [PATCH net] rtnetlink: fix leak of SRCU struct in rtnl_link_register

[Kuniyuki Iwashima]

On Wed, Mar 18, 2026 at 9:15 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> Commit 6b57ff21a310 ("rtnetlink: Protect link_ops by mutex.") swapped
> the EEXIST check with the init_srcu_struct, but didn't add cleanup of
> the SRCU struct we just allocated in case of error.
>
> Fixes: 6b57ff21a310 ("rtnetlink: Protect link_ops by mutex.")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Thanks !

Re: [PATCH net] rtnetlink: fix leak of SRCU struct in rtnl_link_register

[Jakub Kicinski]

On Wed, 18 Mar 2026 17:15:15 +0100 Sabrina Dubroca wrote:
> Commit 6b57ff21a310 ("rtnetlink: Protect link_ops by mutex.") swapped
> the EEXIST check with the init_srcu_struct, but didn't add cleanup of
> the SRCU struct we just allocated in case of error.

FWIW AI has spotted a different bug while reviewing this:

As a related question about the srcu lifecycle for these ops structures,
is there a missing synchronize_rcu() in rtnl_link_unregister() that could
cause a use-after-free?
If rtnl_link_ops_get() traverses the link_ops list under rcu_read_lock(),
and rtnl_link_unregister() removes the ops via list_del_rcu() followed
immediately by synchronize_srcu(&ops->srcu) and cleanup_srcu_struct(),
what happens if an rcu reader finds the ops pointer but is preempted
before calling srcu_read_lock()?
Since no srcu readers are technically active yet, synchronize_srcu()
would return immediately, and cleanup_srcu_struct() would free the
per-CPU data. When the preempted reader resumes, wouldn't it execute
srcu_read_lock() on freed data?
Does rtnl_link_unregister() need a synchronize_rcu() after list_del_rcu()
and before destroying the srcu struct to ensure all rcu-side list
traversals have completed?

Re: [PATCH net] rtnetlink: fix leak of SRCU struct in rtnl_link_register

[Jakub Kicinski]

On Wed, 18 Mar 2026 17:15:15 +0100 Sabrina Dubroca wrote:
>  	list_add_tail_rcu(&ops->list, &link_ops);
>  unlock:
> +	if (err)
> +		cleanup_srcu_struct(&ops->srcu);
> +
>  	mutex_unlock(&link_ops_mutex);

Does the cleanup have to be under the lock?

Re: [PATCH net] rtnetlink: fix leak of SRCU struct in rtnl_link_register

[Sabrina Dubroca]

2026-03-19, 17:05:41 -0700, Jakub Kicinski wrote:
> On Wed, 18 Mar 2026 17:15:15 +0100 Sabrina Dubroca wrote:
> >  	list_add_tail_rcu(&ops->list, &link_ops);
> >  unlock:
> > +	if (err)
> > +		cleanup_srcu_struct(&ops->srcu);
> > +
> >  	mutex_unlock(&link_ops_mutex);
> 
> Does the cleanup have to be under the lock?

Right. I'll send a v2.
(OTOH I don't think we'll ever have to worry about contention on this
lock, but we've probably said that about many locks in the past)

-- 
Sabrina

Re: [PATCH net] rtnetlink: fix leak of SRCU struct in rtnl_link_register

[Jakub Kicinski]

On Fri, 20 Mar 2026 08:23:42 +0100 Sabrina Dubroca wrote:
> 2026-03-19, 17:05:41 -0700, Jakub Kicinski wrote:
> > On Wed, 18 Mar 2026 17:15:15 +0100 Sabrina Dubroca wrote:  
> > >  	list_add_tail_rcu(&ops->list, &link_ops);
> > >  unlock:
> > > +	if (err)
> > > +		cleanup_srcu_struct(&ops->srcu);
> > > +
> > >  	mutex_unlock(&link_ops_mutex);  
> > 
> > Does the cleanup have to be under the lock?  
> 
> Right. I'll send a v2.

SG, I'll mark this as Changes Requested then.

> (OTOH I don't think we'll ever have to worry about contention on this
> lock, but we've probably said that about many locks in the past)

Right. I'm not sure why I feel this way but the way the condition is
placed makes it read to me as if being under the lock was important :S