Re: [Patch net v2 1/2] l2tp: convert l2tp_tunnel_list to idr

[Tom Parkin <tparkin@katalix.com>]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

On  Tue, Jan 10, 2023 at 13:00:29 -0800, Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.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@I-love.SAKURA.ne.jp>
> Cc: Guillaume Nault <gnault@redhat.com>
> Cc: Jakub Sitnicki <jakub@cloudflare.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]