netdev - Re: [PATCH v1 bpf-next 03/11] tcp: Clean up goto labels in cookie_v[46]

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: <effba957-26ce-4b04-841a-40c863e6931f@gmail.com>
Date: Mon, 16 Oct 2023 17:00:39 -0700
From: Kui-Feng Lee <sinquersw@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 David Ahern <dsahern@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>
Cc: Kuniyuki Iwashima <kuni1840@...il.com>, bpf@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH v1 bpf-next 03/11] tcp: Clean up goto labels in
 cookie_v[46]_check().



On 10/13/23 15:04, Kuniyuki Iwashima wrote:
> We will add a SOCK_OPS hook to validate SYN Cookie.
> 
> We invoke the hook after allocating reqsk.  In case it fails,
> we will respond with RST instead of just dropping the ACK.
> 
> Then, there would be more duplicated error handling patterns.
> To avoid that, let's clean up goto labels.
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
>   net/ipv4/syncookies.c | 22 +++++++++++-----------
>   net/ipv6/syncookies.c |  4 ++--
>   2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 64280cf42667..b0cf6f4d66d8 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -369,11 +369,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
>   	if (!cookie_timestamp_decode(net, &tcp_opt))
>   		goto out;
>   
> -	ret = NULL;
>   	req = cookie_tcp_reqsk_alloc(&tcp_request_sock_ops,
>   				     &tcp_request_sock_ipv4_ops, sk, skb);
>   	if (!req)
> -		goto out;
> +		goto out_drop;
>   
>   	ireq = inet_rsk(req);
>   	treq = tcp_rsk(req);
> @@ -405,10 +404,8 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
>   	 */
>   	RCU_INIT_POINTER(ireq->ireq_opt, tcp_v4_save_options(net, skb));
>   
> -	if (security_inet_conn_request(sk, skb, req)) {
> -		reqsk_free(req);
> -		goto out;
> -	}
> +	if (security_inet_conn_request(sk, skb, req))
> +		goto out_free;
>   
>   	req->num_retrans = 0;
>   
> @@ -425,10 +422,8 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
>   			   ireq->ir_loc_addr, th->source, th->dest, sk->sk_uid);
>   	security_req_classify_flow(req, flowi4_to_flowi_common(&fl4));
>   	rt = ip_route_output_key(net, &fl4);
> -	if (IS_ERR(rt)) {
> -		reqsk_free(req);
> -		goto out;
> -	}
> +	if (IS_ERR(rt))
> +		goto out_free;
>   
>   	/* Try to redo what tcp_v4_send_synack did. */
>   	req->rsk_window_clamp = tp->window_clamp ? :dst_metric(&rt->dst, RTAX_WINDOW);
> @@ -452,5 +447,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
>   	 */
>   	if (ret)
>   		inet_sk(ret)->cork.fl.u.ip4 = fl4;
> -out:	return ret;
> +out:
> +	return ret;
> +out_free:
> +	reqsk_free(req);
> +out_drop:
> +	return NULL;
>   }

Looks like you don't use out_free and out_drop at all
in the patch 5 & 6. Are these changes still necessary?
Especially, the line 'goto out_drop' can be 'return NULL' concisely.


> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index cbee2df8a006..b8ef6efbb60e 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -171,11 +171,10 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>   	if (!cookie_timestamp_decode(net, &tcp_opt))
>   		goto out;
>   
> -	ret = NULL;
>   	req = cookie_tcp_reqsk_alloc(&tcp6_request_sock_ops,
>   				     &tcp_request_sock_ipv6_ops, sk, skb);
>   	if (!req)
> -		goto out;
> +		goto out_drop;
>   
>   	ireq = inet_rsk(req);
>   	treq = tcp_rsk(req);
> @@ -263,5 +262,6 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>   	return ret;
>   out_free:
>   	reqsk_free(req);
> +out_drop:
>   	return NULL;
>   }
Same here!