netdev - Re: [PATCH net-next 4/9] l2tp: add tunnel/session get

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240806143725.GU2636630@kernel.org>
Date: Tue, 6 Aug 2024 15:37:25 +0100
From: Simon Horman <horms@...nel.org>
To: James Chapman <jchapman@...alix.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, dsahern@...nel.org,
	tparkin@...alix.com
Subject: Re: [PATCH net-next 4/9] l2tp: add tunnel/session get_next helpers

On Mon, Aug 05, 2024 at 12:35:28PM +0100, James Chapman wrote:
> l2tp management APIs and procfs/debugfs iterate over l2tp tunnel and
> session lists. Since these lists are now implemented using IDR, we can
> use IDR get_next APIs to iterate them. Add tunnel/session get_next
> functions to do so.
> 
> The session get_next functions get the next session in a given tunnel
> and need to account for l2tpv2 and l2tpv3 differences:
> 
>  * l2tpv2 sessions are keyed by tunnel ID / session ID. Iteration for
>    a given tunnel ID, TID, can therefore start with a key given by
>    TID/0 and finish when the next entry's tunnel ID is not TID. This
>    is possible only because the tunnel ID part of the key is the upper
>    16 bits and the session ID part the lower 16 bits; when idr_next
>    increments the key value, it therefore finds the next sessions of
>    the current tunnel before those of the next tunnel. Entries with
>    session ID 0 are always skipped because they are used internally by
>    pppol2tp.
> 
>  * l2tpv3 sessions are keyed by session ID. Iteration starts at the
>    first IDR entry and skips entries where the tunnel does not
>    match. Iteration must also consider session ID collisions and walk
>    the list of colliding sessions (if any) for one which matches the
>    supplied tunnel.
> 
> Signed-off-by: James Chapman <jchapman@...alix.com>
> Signed-off-by: Tom Parkin <tparkin@...alix.com>
> ---
>  net/l2tp/l2tp_core.c | 122 +++++++++++++++++++++++++++++++++++++++++++
>  net/l2tp/l2tp_core.h |   3 ++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 0c661d499a6f..05e388490cd9 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -257,6 +257,28 @@ struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth)
>  }
>  EXPORT_SYMBOL_GPL(l2tp_tunnel_get_nth);
>  
> +struct l2tp_tunnel *l2tp_tunnel_get_next(const struct net *net, unsigned long *key)

nit: Please consider limiting lines to 80 columns wide,
as is still preferred by Networking code in the general case.

Flagged by checkpatch.pl --max-line-length=80"

...

> @@ -347,6 +369,106 @@ struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth)
>  }
>  EXPORT_SYMBOL_GPL(l2tp_session_get_nth);
>  
> +static struct l2tp_session *l2tp_v2_session_get_next(const struct net *net, u16 tid, unsigned long *key)
> +{
> +	struct l2tp_net *pn = l2tp_pernet(net);
> +	struct l2tp_session *session = NULL;
> +
> +	/* Start searching within the range of the tid */
> +	if (*key == 0)
> +		*key = l2tp_v2_session_key(tid, 0);
> +
> +	rcu_read_lock_bh();
> +again:
> +	session = idr_get_next_ul(&pn->l2tp_v2_session_idr, key);
> +	if (session) {
> +		struct l2tp_tunnel *tunnel = READ_ONCE(session->tunnel);
> +
> +		/* ignore sessions with id 0 as they are internal for pppol2tp */
> +		if (session->session_id == 0) {
> +			(*key)++;
> +			goto again;
> +		}
> +
> +		if (tunnel && tunnel->tunnel_id == tid &&

Here it is assumed that tunnel may be NULL.

> +		    refcount_inc_not_zero(&session->ref_count)) {
> +			rcu_read_unlock_bh();
> +			return session;
> +		}
> +
> +		(*key)++;
> +		if (tunnel->tunnel_id == tid)

But here tunnel is dereference unconditionally.
Is that safe?

Flagged by Smatch.

> +			goto again;
> +	}
> +	rcu_read_unlock_bh();
> +
> +	return NULL;
> +}

...