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: <20231017003058.6254-1-kuniyu@amazon.com>
Date: Mon, 16 Oct 2023 17:30:58 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <sinquersw@...il.com>
CC: <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
	<daniel@...earbox.net>, <davem@...emloft.net>, <dsahern@...nel.org>,
	<edumazet@...gle.com>, <haoluo@...gle.com>, <john.fastabend@...il.com>,
	<jolsa@...nel.org>, <kpsingh@...nel.org>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <martin.lau@...ux.dev>,
	<mykolal@...com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<sdf@...gle.com>, <song@...nel.org>, <yonghong.song@...ux.dev>
Subject: Re: [PATCH v1 bpf-next 03/11] tcp: Clean up goto labels in cookie_v[46]_check().

From: Kui-Feng Lee <sinquersw@...il.com>
Date: Mon, 16 Oct 2023 17:00:39 -0700
> 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.

I think it's hard to follow a function where goto and return
are mixed, so I cleaned up the labels while at it.