| Message ID | 20240221025732.68157-4-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | introduce drop reasons for tcp receive path | expand |
On Wed, Feb 21, 2024 at 3:57 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > From: Jason Xing <kernelxing@tencent.com> > > Now it's time to use the prepared definitions to refine this part. > Four reasons used might enough for now, I think. > > Signed-off-by: Jason Xing <kernelxing@tencent.com> > -- > v6: > Link: https://lore.kernel.org/netdev/20240215210922.19969-1-kuniyu@amazon.com/ > 1. Not use NOMEM because of MPTCP (Kuniyuki). I chose to use NO_SOCKET as > an indicator which can be used as three kinds of cases to tell people that we're > unable to get a valid one. It's a relatively general reason like what we did > to TCP_FLAGS. > Any better ideas/suggestions are welcome :) > > v5: > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/ > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/ > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David) > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric) > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD > --- > net/ipv4/syncookies.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c > index 38f331da6677..1028429c78a5 100644 > --- a/net/ipv4/syncookies.c > +++ b/net/ipv4/syncookies.c > @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb) > if (IS_ERR(req)) > goto out; > } > - if (!req) > + if (!req) { > + SKB_DR_SET(reason, NO_SOCKET); > goto out_drop; > + } > > ireq = inet_rsk(req); > > @@ -434,8 +436,10 @@ 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)) > + if (security_inet_conn_request(sk, skb, req)) { > + SKB_DR_SET(reason, SECURITY_HOOK); > goto out_free; > + } > > tcp_ao_syncookie(sk, skb, req, AF_INET); > > @@ -452,8 +456,10 @@ 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)) > + if (IS_ERR(rt)) { > + SKB_DR_SET(reason, IP_OUTNOROUTES); > 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); > @@ -476,10 +482,12 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb) > /* ip_queue_xmit() depends on our flow being setup > * Normal sockets get it right from inet_csk_route_child_sock() > */ > - if (ret) > + if (ret) { > inet_sk(ret)->cork.fl.u.ip4 = fl4; > - else > + } else { > + SKB_DR_SET(reason, NO_SOCKET); > goto out_drop; > + } You can avoid the else here diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c index be88bf586ff9ffba2190a1fd60a1ed3ce5f73d06..d56b0e309cfc0a58dcd277881fe2b364ab3cc668 100644 --- a/net/ipv4/syncookies.c +++ b/net/ipv4/syncookies.c @@ -475,8 +475,11 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb) /* ip_queue_xmit() depends on our flow being setup * Normal sockets get it right from inet_csk_route_child_sock() */ - if (ret) - inet_sk(ret)->cork.fl.u.ip4 = fl4; + if (!ret) { + SKB_DR_SET(reason, NO_SOCKET); + goto out_drop; + } + inet_sk(ret)->cork.fl.u.ip4 = fl4; out: return ret; out_free:
On Wed, Feb 21, 2024 at 5:34 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Feb 21, 2024 at 3:57 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > > > From: Jason Xing <kernelxing@tencent.com> > > > > Now it's time to use the prepared definitions to refine this part. > > Four reasons used might enough for now, I think. > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com> > > -- > > v6: > > Link: https://lore.kernel.org/netdev/20240215210922.19969-1-kuniyu@amazon.com/ > > 1. Not use NOMEM because of MPTCP (Kuniyuki). I chose to use NO_SOCKET as > > an indicator which can be used as three kinds of cases to tell people that we're > > unable to get a valid one. It's a relatively general reason like what we did > > to TCP_FLAGS. > > Any better ideas/suggestions are welcome :) > > > > v5: > > Link: https://lore.kernel.org/netdev/CANn89i+iELpsoea6+C-08m6+=JkneEEM=nAj-28eNtcOCkwQjw@mail.gmail.com/ > > Link: https://lore.kernel.org/netdev/632c6fd4-e060-4b8e-a80e-5d545a6c6b6c@kernel.org/ > > 1. Use SKB_DROP_REASON_IP_OUTNOROUTES instead of introducing a new one (Eric, David) > > 2. Reuse SKB_DROP_REASON_NOMEM to handle failure of request socket allocation (Eric) > > 3. Reuse NO_SOCKET instead of introducing COOKIE_NOCHILD > > --- > > net/ipv4/syncookies.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c > > index 38f331da6677..1028429c78a5 100644 > > --- a/net/ipv4/syncookies.c > > +++ b/net/ipv4/syncookies.c > > @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb) > > if (IS_ERR(req)) > > goto out; > > } > > - if (!req) > > + if (!req) { > > + SKB_DR_SET(reason, NO_SOCKET); > > goto out_drop; > > + } > > > > ireq = inet_rsk(req); > > > > @@ -434,8 +436,10 @@ 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)) > > + if (security_inet_conn_request(sk, skb, req)) { > > + SKB_DR_SET(reason, SECURITY_HOOK); > > goto out_free; > > + } > > > > tcp_ao_syncookie(sk, skb, req, AF_INET); > > > > @@ -452,8 +456,10 @@ 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)) > > + if (IS_ERR(rt)) { > > + SKB_DR_SET(reason, IP_OUTNOROUTES); > > 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); > > @@ -476,10 +482,12 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb) > > /* ip_queue_xmit() depends on our flow being setup > > * Normal sockets get it right from inet_csk_route_child_sock() > > */ > > - if (ret) > > + if (ret) { > > inet_sk(ret)->cork.fl.u.ip4 = fl4; > > - else > > + } else { > > + SKB_DR_SET(reason, NO_SOCKET); > > goto out_drop; > > + } > > You can avoid the else here > > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c > index be88bf586ff9ffba2190a1fd60a1ed3ce5f73d06..d56b0e309cfc0a58dcd277881fe2b364ab3cc668 > 100644 > --- a/net/ipv4/syncookies.c > +++ b/net/ipv4/syncookies.c > @@ -475,8 +475,11 @@ struct sock *cookie_v4_check(struct sock *sk, > struct sk_buff *skb) > /* ip_queue_xmit() depends on our flow being setup > * Normal sockets get it right from inet_csk_route_child_sock() > */ > - if (ret) > - inet_sk(ret)->cork.fl.u.ip4 = fl4; > + if (!ret) { > + SKB_DR_SET(reason, NO_SOCKET); > + goto out_drop; > + } > + inet_sk(ret)->cork.fl.u.ip4 = fl4; > out: > return ret; > out_free: Thanks for your suggestions. I will update it in the v8 patch. Thanks, Jason
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c index 38f331da6677..1028429c78a5 100644 --- a/net/ipv4/syncookies.c +++ b/net/ipv4/syncookies.c @@ -421,8 +421,10 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb) if (IS_ERR(req)) goto out; } - if (!req) + if (!req) { + SKB_DR_SET(reason, NO_SOCKET); goto out_drop; + } ireq = inet_rsk(req); @@ -434,8 +436,10 @@ 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)) + if (security_inet_conn_request(sk, skb, req)) { + SKB_DR_SET(reason, SECURITY_HOOK); goto out_free; + } tcp_ao_syncookie(sk, skb, req, AF_INET); @@ -452,8 +456,10 @@ 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)) + if (IS_ERR(rt)) { + SKB_DR_SET(reason, IP_OUTNOROUTES); 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); @@ -476,10 +482,12 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb) /* ip_queue_xmit() depends on our flow being setup * Normal sockets get it right from inet_csk_route_child_sock() */ - if (ret) + if (ret) { inet_sk(ret)->cork.fl.u.ip4 = fl4; - else + } else { + SKB_DR_SET(reason, NO_SOCKET); goto out_drop; + } out: return ret; out_free: