Re: [PATCH net] l2tp: use list_del_rcu in l2tp_session_unhash

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
l2tp: use list_del_rcu in l2tp_session_unhash

An unprivileged local user can pin a host CPU indefinitely in
l2tp_session_get_by_ifname() when l2tp_session_unhash() uses list_del_init()
while the list is being walked under RCU. This commit changes
l2tp_session_unhash() to use list_del_rcu() to match the existing
list_add_rcu(), preventing the infinite loop.

> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 157fc23ce4e14..1455f67e01ddb 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1360,7 +1360,7 @@ static void l2tp_session_unhash(struct l2tp_session *session)
>  		spin_lock_bh(&pn->l2tp_session_idr_lock);
>  
>  		/* Remove from the per-tunnel list */
> -		list_del_init(&session->list);
> +		list_del_rcu(&session->list);

Can this change lead to a use-after-free in l2tp_session_get_by_ifname()?

By switching to list_del_rcu(), the deleted session remains visible to
in-flight RCU readers. If l2tp_session_del_work() drops the session's
reference count to 0 concurrently, the session will be queued for
destruction via kfree_rcu().

If an in-flight RCU reader in l2tp_session_get_by_ifname() encounters
this session before the grace period ends, it will execute:

	list_for_each_entry_rcu(session, &tunnel->session_list, list) {
		if (!strcmp(session->ifname, ifname)) {
			refcount_inc(&session->ref_count);
			rcu_read_unlock_bh();
			return session;
		}
	}

Since refcount_inc() does not protect against zero-reference transitions,
the reader will obtain a pointer to a session that is already pending
destruction.

Once the RCU grace period expires, kfree_rcu() will free the session
memory, causing a use-after-free for the caller.

Should l2tp_session_get_by_ifname() use refcount_inc_not_zero() and skip
the session if it returns false, similar to other getters like
l2tp_v3_session_get()?

[ ... ]