| Message ID | 20240215012027.11467-4-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | introduce drop reasons for tcp receive path | expand |
From: Jason Xing <kerneljasonxing@gmail.com> Date: Thu, 15 Feb 2024 09:20:19 +0800 > 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> > -- > 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..aeb61c880fbd 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, NOMEM); NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails. > 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); This also seems wrong to me. e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk), then the listener is actually found. > goto out_drop; > + } > out: > return ret; > out_free: > -- > 2.37.3 >
On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > From: Jason Xing <kerneljasonxing@gmail.com> > Date: Thu, 15 Feb 2024 09:20:19 +0800 > > 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> > > -- > > 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..aeb61c880fbd 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, NOMEM); > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails. Thanks for your careful check. It's true. I didn't check the MPTCP path about how to handle it. It also means that what I did to the cookie_v6_check() is also wrong. [...] > > /* 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); > > This also seems wrong to me. > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk), > then the listener is actually found. Initially I thought using a not-that-clear name could be helpfull, though. NO_SOCKET here means no child socket can be used if I add a new description to SKB_DROP_REASON_NO_SOCKET. If the idea is proper, how about using NO_SOCKET for the first point you said to explain that there is no request socket that can be used? If not, for both of the points you mentioned, it seems I have to add back those two new reasons (perhaps with a better name updated)? 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request socket allocation in cookie_v4/6_check()) 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket fetching in cookie_v4/6_check()) Now I'm struggling with the name and whether I should introduce some new reasons like what I did in the old version of the series :S If someone comes up with a good name or a good way to explain them, please tell me, thanks! also cc Eric, David Thanks, Jason > > > > goto out_drop; > > + } > > out: > > return ret; > > out_free: > > -- > > 2.37.3 > >
From: Jason Xing <kerneljasonxing@gmail.com> Date: Fri, 16 Feb 2024 09:28:26 +0800 > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > From: Jason Xing <kerneljasonxing@gmail.com> > > Date: Thu, 15 Feb 2024 09:20:19 +0800 > > > 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> > > > -- > > > 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..aeb61c880fbd 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; I noticed in this case (ret == sk) we can set drop reason in tcp_v4_do_rcv() as INVALID_COOKIE or something else. > > > } > > > - if (!req) > > > + if (!req) { > > > + SKB_DR_SET(reason, NOMEM); > > > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails. > > Thanks for your careful check. It's true. I didn't check the MPTCP > path about how to handle it. > > It also means that what I did to the cookie_v6_check() is also wrong. Yes, same for the v6 change. > > [...] > > > /* 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); > > > > This also seems wrong to me. > > > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk), > > then the listener is actually found. > > Initially I thought using a not-that-clear name could be helpfull, > though. NO_SOCKET here means no child socket can be used if I add a > new description to SKB_DROP_REASON_NO_SOCKET. Currently, NO_SOCKET is used only when sk lookup fails. Mixing different reasons sounds like pushing it back to NOT_SPECIFIED. We could distinguish them by the caller IP though. > > If the idea is proper, how about using NO_SOCKET for the first point > you said to explain that there is no request socket that can be used? > > If not, for both of the points you mentioned, it seems I have to add > back those two new reasons (perhaps with a better name updated)? > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request > socket allocation in cookie_v4/6_check()) > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket > fetching in cookie_v4/6_check()) > > Now I'm struggling with the name and whether I should introduce some > new reasons like what I did in the old version of the series :S Why naming is hard would be because there are multiple reasons of failure. One way to be more specific is moving kfree_skb_reason() into callee as you did in patch 2. > If someone comes up with a good name or a good way to explain them, > please tell me, thanks! For 1. no idea :p For 2. Maybe VALID_COOKIE ? we drop the valid cookie in the same function, but due to LSM or L3 layer, so the reason could be said as L4-specific ? > > also cc Eric, David > > Thanks, > Jason >
On Fri, Feb 16, 2024 at 11:03 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > From: Jason Xing <kerneljasonxing@gmail.com> > Date: Fri, 16 Feb 2024 09:28:26 +0800 > > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > > > From: Jason Xing <kerneljasonxing@gmail.com> > > > Date: Thu, 15 Feb 2024 09:20:19 +0800 > > > > 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> > > > > -- > > > > 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..aeb61c880fbd 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; > > I noticed in this case (ret == sk) we can set drop reason in > tcp_v4_do_rcv() as INVALID_COOKIE or something else. If cookie_v4_check() returns the sk which is the same as the first parameter of its caller (tcp_v4_do_rcv()), then we cannot directly drop it because it is against old behaviours and causes errors. It should go into tcp_rcv_state_process() later. The similar mistake I made was reported by Paolo in the patch [0/11] (see link[1] as below). link[1]: https://lore.kernel.org/netdev/c987d2c79e4a4655166eb8eafef473384edb37fb.camel@redhat.com/ > > > > > > } > > > > - if (!req) > > > > + if (!req) { > > > > + SKB_DR_SET(reason, NOMEM); > > > > > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails. > > > > Thanks for your careful check. It's true. I didn't check the MPTCP > > path about how to handle it. > > > > It also means that what I did to the cookie_v6_check() is also wrong. > > Yes, same for the v6 change. > > > > > > [...] > > > > /* 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); > > > > > > This also seems wrong to me. > > > > > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk), > > > then the listener is actually found. > > > > Initially I thought using a not-that-clear name could be helpfull, > > though. NO_SOCKET here means no child socket can be used if I add a > > new description to SKB_DROP_REASON_NO_SOCKET. > > Currently, NO_SOCKET is used only when sk lookup fails. Mixing > different reasons sounds like pushing it back to NOT_SPECIFIED. > We could distinguish them by the caller IP though. It makes some sense, but I still think NO_SOCKET is just a mixture of three kinds of cases (no sk during lookup process, no child sk, no reqsk). Let me think about it. > > > > > > If the idea is proper, how about using NO_SOCKET for the first point > > you said to explain that there is no request socket that can be used? > > > > If not, for both of the points you mentioned, it seems I have to add > > back those two new reasons (perhaps with a better name updated)? > > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request > > socket allocation in cookie_v4/6_check()) > > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket > > fetching in cookie_v4/6_check()) > > > > Now I'm struggling with the name and whether I should introduce some > > new reasons like what I did in the old version of the series :S > > Why naming is hard would be because there are multiple reasons of > failure. One way to be more specific is moving kfree_skb_reason() > into callee as you did in patch 2. > > > > If someone comes up with a good name or a good way to explain them, > > please tell me, thanks! > > For 1. no idea :p > > For 2. Maybe VALID_COOKIE ? we drop the valid cookie in the same > function, but due to LSM or L3 layer, so the reason could be said > as L4-specific ? Thanks for your idea :) For 2, if we're on the same page and talk about how to handle tcp_get_cookie_sock(), the name is not that appropriate because as you said in your previous email it could fail due to full of accept queue instead of cookie problem. If we're talking about cookie_tcp_check(), the name is also not that good because the drop reason could be no memory which is unrelated to cookie, right? COOKIE, it seems, cannot be the keyword/generic reason to conclude all the reasons for either of them... Thanks, Jason > > > > > > also cc Eric, David > > > > Thanks, > > Jason > >
From: Jason Xing <kerneljasonxing@gmail.com> Date: Fri, 16 Feb 2024 11:50:45 +0800 > On Fri, Feb 16, 2024 at 11:03 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > From: Jason Xing <kerneljasonxing@gmail.com> > > Date: Fri, 16 Feb 2024 09:28:26 +0800 > > > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > > > > > From: Jason Xing <kerneljasonxing@gmail.com> > > > > Date: Thu, 15 Feb 2024 09:20:19 +0800 > > > > > 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> > > > > > -- > > > > > 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..aeb61c880fbd 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; > > > > I noticed in this case (ret == sk) we can set drop reason in > > tcp_v4_do_rcv() as INVALID_COOKIE or something else. > > If cookie_v4_check() returns the sk which is the same as the first > parameter of its caller (tcp_v4_do_rcv()), then we cannot directly > drop it No, I meant we can just set drop reason, not calling kfree_skb_reason() just after cookie_v4_check(). Then, in tcp_v4_do_rcv(), the default reason is NOT_SPECIFIED, but INVALID_COOKIE only when cookie_v4_check() returns the listener. ---8<--- diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 0c50c5a32b84..05cd697a7c07 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -1923,6 +1923,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb) } return 0; } + + reason = SKB_DROP_REASON_INVALID_COOKIE; } else sock_rps_save_rxhash(sk, skb); ---8<--- > because it is against old behaviours and causes errors. It > should go into tcp_rcv_state_process() later. The similar mistake I > made was reported by Paolo in the patch [0/11] (see link[1] as below). > > link[1]: https://lore.kernel.org/netdev/c987d2c79e4a4655166eb8eafef473384edb37fb.camel@redhat.com/ > > > > > > > > > > } > > > > > - if (!req) > > > > > + if (!req) { > > > > > + SKB_DR_SET(reason, NOMEM); > > > > > > > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails. > > > > > > Thanks for your careful check. It's true. I didn't check the MPTCP > > > path about how to handle it. > > > > > > It also means that what I did to the cookie_v6_check() is also wrong. > > > > Yes, same for the v6 change. > > > > > > > > > > [...] > > > > > /* 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); > > > > > > > > This also seems wrong to me. > > > > > > > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk), > > > > then the listener is actually found. > > > > > > Initially I thought using a not-that-clear name could be helpfull, > > > though. NO_SOCKET here means no child socket can be used if I add a > > > new description to SKB_DROP_REASON_NO_SOCKET. > > > > Currently, NO_SOCKET is used only when sk lookup fails. Mixing > > different reasons sounds like pushing it back to NOT_SPECIFIED. > > We could distinguish them by the caller IP though. > > It makes some sense, but I still think NO_SOCKET is just a mixture of > three kinds of cases (no sk during lookup process, no child sk, no > reqsk). > Let me think about it. > > > > > > > > > > > If the idea is proper, how about using NO_SOCKET for the first point > > > you said to explain that there is no request socket that can be used? > > > > > > If not, for both of the points you mentioned, it seems I have to add > > > back those two new reasons (perhaps with a better name updated)? > > > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request > > > socket allocation in cookie_v4/6_check()) > > > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket > > > fetching in cookie_v4/6_check()) > > > > > > Now I'm struggling with the name and whether I should introduce some > > > new reasons like what I did in the old version of the series :S > > > > Why naming is hard would be because there are multiple reasons of > > failure. One way to be more specific is moving kfree_skb_reason() > > into callee as you did in patch 2. > > > > > > > If someone comes up with a good name or a good way to explain them, > > > please tell me, thanks! > > > > For 1. no idea :p > > > > For 2. Maybe VALID_COOKIE ? we drop the valid cookie in the same > > function, but due to LSM or L3 layer, so the reason could be said > > as L4-specific ? > > Thanks for your idea :) > > For 2, if we're on the same page and talk about how to handle > tcp_get_cookie_sock(), the name is not that appropriate because as you > said in your previous email it could fail due to full of accept queue > instead of cookie problem. That's why I wrote _VALID_ COOKIE. Here we know the cookie was valid but somehow 3WHS failed. If we want to be more specific, what is not appropriate would be the place where we set the reason or call kfree_skb(). > > If we're talking about cookie_tcp_check(), the name is also not that > good because the drop reason could be no memory which is unrelated to > cookie, right? > > COOKIE, it seems, cannot be the keyword/generic reason to conclude all > the reasons for either of them... > > Thanks, > Jason > > > > > > > > > > > also cc Eric, David > > > > > > Thanks, > > > Jason > > >
On Fri, Feb 16, 2024 at 12:11 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > From: Jason Xing <kerneljasonxing@gmail.com> > Date: Fri, 16 Feb 2024 11:50:45 +0800 > > On Fri, Feb 16, 2024 at 11:03 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > > > From: Jason Xing <kerneljasonxing@gmail.com> > > > Date: Fri, 16 Feb 2024 09:28:26 +0800 > > > > On Fri, Feb 16, 2024 at 5:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > > > > > > > > > From: Jason Xing <kerneljasonxing@gmail.com> > > > > > Date: Thu, 15 Feb 2024 09:20:19 +0800 > > > > > > 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> > > > > > > -- > > > > > > 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..aeb61c880fbd 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; > > > > > > I noticed in this case (ret == sk) we can set drop reason in > > > tcp_v4_do_rcv() as INVALID_COOKIE or something else. > > > > If cookie_v4_check() returns the sk which is the same as the first > > parameter of its caller (tcp_v4_do_rcv()), then we cannot directly > > drop it > > No, I meant we can just set drop reason, not calling kfree_skb_reason() > just after cookie_v4_check(). > > Then, in tcp_v4_do_rcv(), the default reason is NOT_SPECIFIED, but > INVALID_COOKIE only when cookie_v4_check() returns the listener. > > ---8<--- > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 0c50c5a32b84..05cd697a7c07 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -1923,6 +1923,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb) > } > return 0; > } > + > + reason = SKB_DROP_REASON_INVALID_COOKIE; > } else > sock_rps_save_rxhash(sk, skb); After this, it will go into 'reason = tcp_rcv_state_process()' which will replace INVALID_COOKIE reason if the kernel is shipped with this series. > > ---8<--- > > > > because it is against old behaviours and causes errors. It > > should go into tcp_rcv_state_process() later. The similar mistake I > > made was reported by Paolo in the patch [0/11] (see link[1] as below). > > > > link[1]: https://lore.kernel.org/netdev/c987d2c79e4a4655166eb8eafef473384edb37fb.camel@redhat.com/ > > > > > > > > > > > > > > } > > > > > > - if (!req) > > > > > > + if (!req) { > > > > > > + SKB_DR_SET(reason, NOMEM); > > > > > > > > > > NOMEM is not appropriate when mptcp_subflow_init_cookie_req() fails. > > > > > > > > Thanks for your careful check. It's true. I didn't check the MPTCP > > > > path about how to handle it. > > > > > > > > It also means that what I did to the cookie_v6_check() is also wrong. > > > > > > Yes, same for the v6 change. > > > > > > > > > > > > > > [...] > > > > > > /* 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); > > > > > > > > > > This also seems wrong to me. > > > > > > > > > > e.g. syn_recv_sock() could fail with sk_acceptq_is_full(sk), > > > > > then the listener is actually found. > > > > > > > > Initially I thought using a not-that-clear name could be helpfull, > > > > though. NO_SOCKET here means no child socket can be used if I add a > > > > new description to SKB_DROP_REASON_NO_SOCKET. > > > > > > Currently, NO_SOCKET is used only when sk lookup fails. Mixing > > > different reasons sounds like pushing it back to NOT_SPECIFIED. > > > We could distinguish them by the caller IP though. > > > > It makes some sense, but I still think NO_SOCKET is just a mixture of > > three kinds of cases (no sk during lookup process, no child sk, no > > reqsk). > > Let me think about it. > > > > > > > > > > > > > > > > If the idea is proper, how about using NO_SOCKET for the first point > > > > you said to explain that there is no request socket that can be used? > > > > > > > > If not, for both of the points you mentioned, it seems I have to add > > > > back those two new reasons (perhaps with a better name updated)? > > > > 1. Using SKB_DROP_REASON_REQSK_ALLOC for the first point (request > > > > socket allocation in cookie_v4/6_check()) > > > > 2. Using SKB_DROP_REASON_GET_SOCK for the second point (child socket > > > > fetching in cookie_v4/6_check()) > > > > > > > > Now I'm struggling with the name and whether I should introduce some > > > > new reasons like what I did in the old version of the series :S > > > > > > Why naming is hard would be because there are multiple reasons of > > > failure. One way to be more specific is moving kfree_skb_reason() > > > into callee as you did in patch 2. > > > > > > > > > > If someone comes up with a good name or a good way to explain them, > > > > please tell me, thanks! > > > > > > For 1. no idea :p > > > > > > For 2. Maybe VALID_COOKIE ? we drop the valid cookie in the same > > > function, but due to LSM or L3 layer, so the reason could be said > > > as L4-specific ? > > > > Thanks for your idea :) > > > > For 2, if we're on the same page and talk about how to handle > > tcp_get_cookie_sock(), the name is not that appropriate because as you > > said in your previous email it could fail due to full of accept queue > > instead of cookie problem. > > That's why I wrote _VALID_ COOKIE. Here we know the cookie was valid > but somehow 3WHS failed. If we want to be more specific, what is not > appropriate would be the place where we set the reason or call kfree_skb(). Ah, I see. It does make sense if we use __VALID__. Let us hear more voices, then I can accurately update the changes in the next version :) Really hope it can be done soon. It almost killed me :( Thanks, Jason > > > > > > If we're talking about cookie_tcp_check(), the name is also not that > > good because the drop reason could be no memory which is unrelated to > > cookie, right? > > > > COOKIE, it seems, cannot be the keyword/generic reason to conclude all > > the reasons for either of them... > > > > Thanks, > > Jason > > > > > > > > > > > > > > > > also cc Eric, David > > > > > > > > Thanks, > > > > Jason > > > >
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c index 38f331da6677..aeb61c880fbd 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, NOMEM); 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: