From: Kui-Feng Lee <sinquersw@xxxxxxxxx>
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@xxxxxxxxxx>
> > ---
> > 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.
