netdev - Re: [Patch net v2 1/2] l2tp: convert l2tp_tunnel

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: <20230111112910.GA4173@katalix.com>
Date:   Wed, 11 Jan 2023 11:29:10 +0000
From:   Tom Parkin <tparkin@...alix.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, saeed@...nel.org, gnault@...hat.com,
        Cong Wang <cong.wang@...edance.com>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [Patch net v2 1/2] l2tp: convert l2tp_tunnel_list to idr

On  Tue, Jan 10, 2023 at 13:00:29 -0800, Cong Wang wrote:
> From: Cong Wang <cong.wang@...edance.com>
> 
> l2tp uses l2tp_tunnel_list to track all registered tunnels and
> to allocate tunnel ID's. IDR can do the same job.
> 
> More importantly, with IDR we can hold the ID before a successful
> registration so that we don't need to worry about late error
> handling, it is not easy to rollback socket changes.
> 
> This is a preparation for the following fix.
> 
> Cc: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Cc: Guillaume Nault <gnault@...hat.com>
> Cc: Jakub Sitnicki <jakub@...udflare.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Cong Wang <cong.wang@...edance.com>
> ---
>  net/l2tp/l2tp_core.c | 85 ++++++++++++++++++++++----------------------
>  1 file changed, 42 insertions(+), 43 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 9a1415fe3fa7..894bc9ff0e71 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
<snip>
> @@ -1455,12 +1456,19 @@ static int l2tp_validate_socket(const struct sock *sk, const struct net *net,
>  int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  			 struct l2tp_tunnel_cfg *cfg)
>  {
> -	struct l2tp_tunnel *tunnel_walk;
> -	struct l2tp_net *pn;
> +	struct l2tp_net *pn = l2tp_pernet(net);
> +	u32 tunnel_id = tunnel->tunnel_id;
>  	struct socket *sock;
>  	struct sock *sk;
>  	int ret;
>  
> +	spin_lock_bh(&pn->l2tp_tunnel_idr_lock);
> +	ret = idr_alloc_u32(&pn->l2tp_tunnel_idr, NULL, &tunnel_id, tunnel_id,
> +			    GFP_ATOMIC);
> +	spin_unlock_bh(&pn->l2tp_tunnel_idr_lock);
> +	if (ret)
> +		return ret;
> +

I believe idr_alloc_u32 will return one of ENOSPC or ENOMEM on
failure, whereas previously this ID check explicitly returned EEXIST
when there was an existing tunnel in the list with the specified ID.

The return code is directly reflected back to userspace in the
pppol2tp case at least (via. the connect handler).

I don't know whether the failure return code could be considered part
of the userspace API or not, but should we be trying to return the
same error code for the "that ID is already in use" case?

-- 
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)