| Message ID | 1340515324-2152-5-git-send-email-ncardwell@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, 2012-06-24 at 01:22 -0400, Neal Cardwell wrote: > The code in tcp_v6_conn_request() was implicitly assuming that > tcp_v6_send_synack() would take care of dst_release(), much as > tcp_v4_send_synack() already does. This resulted in > tcp_v6_conn_request() leaking a dst if sysctl_tw_recycle is enabled. > > This commit restructures tcp_v6_send_synack() so that it accepts a dst > pointer and takes care of releasing the dst that is passed in, to plug > the leak and avoid future surprises by bringing the IPv6 behavior in > line with the IPv4 side. > > Signed-off-by: Neal Cardwell <ncardwell@google.com> > --- Neal, your patches are for net-next, right ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2012-06-24 at 01:22 -0400, Neal Cardwell wrote: > The code in tcp_v6_conn_request() was implicitly assuming that > tcp_v6_send_synack() would take care of dst_release(), much as > tcp_v4_send_synack() already does. This resulted in > tcp_v6_conn_request() leaking a dst if sysctl_tw_recycle is enabled. > > This commit restructures tcp_v6_send_synack() so that it accepts a dst > pointer and takes care of releasing the dst that is passed in, to plug > the leak and avoid future surprises by bringing the IPv6 behavior in > line with the IPv4 side. I feel a bit uncomfortable to send a mix of 3 patches to fix one bug. Could you instead send pure fix (fixing dst leak) for net tree ? Then, when fix is incorporated in net-next, send the cleanups ? This also clashes with this pending work, so it would ease things if you can respin the cleanups for net-next http://patchwork.ozlabs.org/patch/166737/ Thanks ! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 24, 2012 at 3:41 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Sun, 2012-06-24 at 01:22 -0400, Neal Cardwell wrote: >> The code in tcp_v6_conn_request() was implicitly assuming that >> tcp_v6_send_synack() would take care of dst_release(), much as >> tcp_v4_send_synack() already does. This resulted in >> tcp_v6_conn_request() leaking a dst if sysctl_tw_recycle is enabled. >> >> This commit restructures tcp_v6_send_synack() so that it accepts a dst >> pointer and takes care of releasing the dst that is passed in, to plug >> the leak and avoid future surprises by bringing the IPv6 behavior in >> line with the IPv4 side. > > I feel a bit uncomfortable to send a mix of 3 patches to fix one bug. > > Could you instead send pure fix (fixing dst leak) for net tree ? > > Then, when fix is incorporated in net-next, send the cleanups ? > > This also clashes with this pending work, so it would ease things if you > can respin the cleanups for net-next > > http://patchwork.ozlabs.org/patch/166737/ Yes, the patches in this series were generated as patches against the "net" tree (sorry for not indicating that). The dst leak on the v6 sysctl_tw_recycle code path (patches 2-5) seems like a pretty low priority, so I think we could simplify your plan even a little further... How about this as a plan: we could apply the first patch in the series (tcp: heed result of security_inet_conn_request() in tcp_v6_conn_request()) to the net tree now, and skip patches 2-5 for now. Once your pending synack work is in net-next, I can respin patches 2-5 for net-next. How does that sound? neal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2012-06-24 at 13:12 -0400, Neal Cardwell wrote: > http://patchwork.ozlabs.org/patch/166737/ > > Yes, the patches in this series were generated as patches against the > "net" tree (sorry for not indicating that). > > The dst leak on the v6 sysctl_tw_recycle code path (patches 2-5) seems > like a pretty low priority, so I think we could simplify your plan > even a little further... How about this as a plan: we could apply the > first patch in the series (tcp: heed result of > security_inet_conn_request() in tcp_v6_conn_request()) to the net tree > now, and skip patches 2-5 for now. Once your pending synack work is in > net-next, I can respin patches 2-5 for net-next. How does that sound? > It sounds good, thanks ! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Neal Cardwell <ncardwell@google.com> Date: Sun, 24 Jun 2012 13:12:33 -0400 > Yes, the patches in this series were generated as patches against the > "net" tree (sorry for not indicating that). > > The dst leak on the v6 sysctl_tw_recycle code path (patches 2-5) seems > like a pretty low priority, so I think we could simplify your plan > even a little further... How about this as a plan: we could apply the > first patch in the series (tcp: heed result of > security_inet_conn_request() in tcp_v6_conn_request()) to the net tree > now, and skip patches 2-5 for now. Once your pending synack work is in > net-next, I can respin patches 2-5 for net-next. How does that sound? I've applied the first patch to 'net' and you can simply respin your patches against net-next right now since I rejected Eric's SYN-ACK patches. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 500a296..846fe80 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -475,7 +475,8 @@ out: } -static int tcp_v6_send_synack(struct sock *sk, +static int tcp_v6_send_synack(struct sock *sk, struct dst_entry *dst, + struct flowi6 *fl6, struct request_sock *req, struct request_values *rvp, u16 queue_mapping) @@ -484,12 +485,10 @@ static int tcp_v6_send_synack(struct sock *sk, struct ipv6_pinfo *np = inet6_sk(sk); struct sk_buff * skb; struct ipv6_txoptions *opt = np->opt; - struct flowi6 fl6; - struct dst_entry *dst; int err = -ENOMEM; - dst = inet6_csk_route_req(sk, &fl6, req); - if (!dst) + /* First, grab a route. */ + if (!dst && (dst = inet6_csk_route_req(sk, fl6, req)) == NULL) goto done; skb = tcp_make_synack(sk, dst, req, rvp); @@ -497,9 +496,9 @@ static int tcp_v6_send_synack(struct sock *sk, if (skb) { __tcp_v6_send_check(skb, &treq->loc_addr, &treq->rmt_addr); - fl6.daddr = treq->rmt_addr; + fl6->daddr = treq->rmt_addr; skb_set_queue_mapping(skb, queue_mapping); - err = ip6_xmit(sk, skb, &fl6, opt, np->tclass); + err = ip6_xmit(sk, skb, fl6, opt, np->tclass); err = net_xmit_eval(err); } @@ -513,8 +512,10 @@ done: static int tcp_v6_rtx_synack(struct sock *sk, struct request_sock *req, struct request_values *rvp) { + struct flowi6 fl6; + TCP_INC_STATS_BH(sock_net(sk), TCP_MIB_RETRANSSEGS); - return tcp_v6_send_synack(sk, req, rvp, 0); + return tcp_v6_send_synack(sk, NULL, &fl6, req, rvp, 0); } static void tcp_v6_reqsk_destructor(struct request_sock *req) @@ -1200,7 +1201,7 @@ have_isn: if (security_inet_conn_request(sk, skb, req)) goto drop_and_release; - if (tcp_v6_send_synack(sk, req, + if (tcp_v6_send_synack(sk, dst, &fl6, req, (struct request_values *)&tmp_ext, skb_get_queue_mapping(skb)) || want_cookie)
The code in tcp_v6_conn_request() was implicitly assuming that tcp_v6_send_synack() would take care of dst_release(), much as tcp_v4_send_synack() already does. This resulted in tcp_v6_conn_request() leaking a dst if sysctl_tw_recycle is enabled. This commit restructures tcp_v6_send_synack() so that it accepts a dst pointer and takes care of releasing the dst that is passed in, to plug the leak and avoid future surprises by bringing the IPv6 behavior in line with the IPv4 side. Signed-off-by: Neal Cardwell <ncardwell@google.com> --- net/ipv6/tcp_ipv6.c | 19 ++++++++++--------- 1 files changed, 10 insertions(+), 9 deletions(-)