| Message ID | 1256115421-12714-2-git-send-email-gilad@codefidence.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
Gilad Ben-Yossef wrote: > A time wait socket is established - we already know if time stamp > option is called for or not. > Not so sure about this. A timewait sock isn't actually established, and new/changed options could appear. There's all sorts of edge cases. There's also some current work to note: http://tools.ietf.org/html/draft-ietf-tcpm-1323bis http://tools.ietf.org/html/draft-gont-tcpm-tcp-timestamps -- 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
William Allen Simpson wrote: > Gilad Ben-Yossef wrote: >> A time wait socket is established - we already know if time stamp >> option is called for or not. >> > Not so sure about this. A timewait sock isn't actually established, > and new/changed options could appear. There's all sorts of edge cases. If you examine the specific context where tcp_parse_options is being called here, the only TCP option which is of interest is the time stamp option, and this code path is only being taken when we already know that the original socket had used the time stamp option. So while I agree that in general you are right, I do believe that in the specific context of this patch we should call tcp_parse_options with the established flag on and let it know we are expecting to see a time stamp option, which is what I was referring to. > > There's also some current work to note: > > http://tools.ietf.org/html/draft-ietf-tcpm-1323bis > > http://tools.ietf.org/html/draft-gont-tcpm-tcp-timestamps Very interesting, thank you. As I noted above, my comment about TIME WAIT sockets being "established" should really only be considered in the context of the specific call to tcp_parse_options() and the "established" parameter of that function. Thanks, Gilad
Gilad Ben-Yossef wrote: > William Allen Simpson wrote: > >> Gilad Ben-Yossef wrote: >>> A time wait socket is established - we already know if time stamp >>> option is called for or not. >>> >> Not so sure about this. A timewait sock isn't actually established, >> and new/changed options could appear. There's all sorts of edge cases. > If you examine the specific context where tcp_parse_options is being > called here, > the only TCP option which is of interest is the time stamp option, and > this code path > is only being taken when we already know that the original socket had > used the time stamp option. > > So while I agree that in general you are right, I do believe that in the > specific context > of this patch we should call tcp_parse_options with the established flag > on and let it > know we are expecting to see a time stamp option, which is what I was > referring to. > No, a major reason for time-wait is rebooted systems. We don't "know" anything about them, and they certainly don't know anything about us. As I mentioned, this is about edge cases. >> >> There's also some current work to note: >> >> http://tools.ietf.org/html/draft-ietf-tcpm-1323bis >> >> http://tools.ietf.org/html/draft-gont-tcpm-tcp-timestamps > > Very interesting, thank you. > > As I noted above, my comment about > TIME WAIT sockets being "established" should really only be considered > in the context of the specific call to tcp_parse_options() and the > "established" > parameter of that function. > My suggestion, as this patch is not essential to the other patches in the series, is to separate it. As I'm relatively new to this list, I don't know the best practice. But I'd like to support the others and delay this for further consideration. -- 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
Hello William, William Allen Simpson wrote: >> If you examine the specific context where tcp_parse_options is being >> called here, >> the only TCP option which is of interest is the time stamp option, >> and this code path >> is only being taken when we already know that the original socket had >> used the time stamp option. >> >> So while I agree that in general you are right, I do believe that in >> the specific context >> of this patch we should call tcp_parse_options with the established >> flag on and let it >> know we are expecting to see a time stamp option, which is what I was >> referring to. >> > No, a major reason for time-wait is rebooted systems. We don't "know" > anything about them, and they certainly don't know anything about us. > > As I mentioned, this is about edge cases. I just read thoroughly the code in question again - We use tcp_parse_option to check if there is a time stamp option in the packet and if so, get the time stamp from it. We do this only when the time wait minisocket has information of time stamp from the original connection. We don't use any other TCP option or other inoformation from the options read via that call. The above statements are true both for the original code and my patch. If there is any corner case with my code it is true for the original code as well. > > My suggestion, as this patch is not essential to the other patches in the > series, is to separate it. As I'm relatively new to this list, I don't > know the best practice. But I'd like to support the others and delay > this for further consideration. I have no objection to separate or drop it altogether if there is a specific technical reason why you think the code is wrong. It certainly is possible I've done some done mistake. In that case, I would love nothing more to hearing what it is and hopefully fixing it. But "Maybe there are edge cases we didn't think about" is not specific enough to work upon :-) Thanks for all the feedback, Gilad
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c index 624c3c9..c49a550 100644 --- a/net/ipv4/tcp_minisocks.c +++ b/net/ipv4/tcp_minisocks.c @@ -100,9 +100,8 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb, struct tcp_options_received tmp_opt; int paws_reject = 0; - tmp_opt.saw_tstamp = 0; if (th->doff > (sizeof(*th) >> 2) && tcptw->tw_ts_recent_stamp) { - tcp_parse_options(skb, &tmp_opt, 0); + tcp_parse_options(skb, &tmp_opt, 1); if (tmp_opt.saw_tstamp) { tmp_opt.ts_recent = tcptw->tw_ts_recent;