We were having a bit of discussion on netdev@vger regarding the new definition of before48 which was motivated at the end of the email http://www.mail-archive.com/dccp@xxxxxxxxxxxxxxx/msg01153.html The result of the netdev discussion was that one can not argue that before48/after48 is safer or less ambiguous to use unless one performs an audit against all occurrences in the code. Therefore such a due analysis is provided below. 1) General cases ================ CASE 1: calls of the form * if (before48(a,b)) or * if (after48(b,a)) are not ambiguous due to the definition of before48 (see above link for details). CASE 2: calls of the form * if (!before48(a, b)) or * if (!after48(b, a)) can be ambiguous - it depends on the context if the ambiguity requires further resolution. 2) Detailed breakdown of before48/after48 usage =============================================== Here is the complete list of cases where before48/after48 are currently used in the code, plus analysis whether the use of the new definition of before48 introduces problems or not. net/dccp/dccp.h-136-static inline u64 max48(const u64 seq1, const u64 seq2) net/dccp/dccp.h-137-{ net/dccp/dccp.h:138: return after48(seq1, seq2) ? seq1 : seq2; net/dccp/dccp.h-139-} net/dccp/dccp.h-140- ==> CASE 1 - safe. net/dccp/ccids/ccid2.c-586- /* it's a later packet */ net/dccp/ccids/ccid2.c:587: else if (after48(seqno, hctx->ccid2hctx_rpseq)) { net/dccp/ccids/ccid2.c-588- hctx->ccid2hctx_rpdupack++; ==> CASE 1 - safe. net/dccp/ccids/ccid2.c-614- ackno = DCCP_SKB_CB(skb)->dccpd_ack_seq; net/dccp/ccids/ccid2.c:615: if (after48(ackno, hctx->ccid2hctx_high_ack)) net/dccp/ccids/ccid2.c-616- hctx->ccid2hctx_high_ack = ackno; ==> CASE 1 - safe. net/dccp/ccids/ccid2.c:650: while (after48(seqp->ccid2s_seq, ackno)) { net/dccp/ccids/ccid2.c-651- if (seqp == hctx->ccid2hctx_seqt) { net/dccp/ccids/ccid2.c-652- done = 1; net/dccp/ccids/ccid2.c-653- break; ==> CASE 1 - safe. net/dccp/ccids/lib/packet_history.c-225- if (!list_empty(li_list)) { net/dccp/ccids/lib/packet_history.c-226- list_for_each_entry_safe(entry, next, rx_list, dccphrx_node) { net/dccp/ccids/lib/packet_history.c-227- if (num_later == 0) { net/dccp/ccids/lib/packet_history.c:228: if (after48(nonloss_seqno, net/dccp/ccids/lib/packet_history.c-229- entry->dccphrx_seqno)) { net/dccp/ccids/lib/packet_history.c-230- list_del_init(&entry->dccphrx_node); net/dccp/ccids/lib/packet_history.c-231- dccp_rx_hist_entry_delete(hist, entry); ==> CASE 1 - safe. net/dccp/ackvec.c-268- if (av->dccpav_vec_len == 0) { net/dccp/ackvec.c-269- av->dccpav_buf[av->dccpav_buf_head] = state; net/dccp/ackvec.c-270- av->dccpav_vec_len = 1; net/dccp/ackvec.c:271: } else if (after48(ackno, av->dccpav_buf_ackno)) { net/dccp/ackvec.c-272- const u64 delta = dccp_delta_seqno(av->dccpav_buf_ackno, ==> CASE 1 - safe. net/dccp/ackvec.c-431- /* net/dccp/ackvec.c-432- * If our AVR sequence number is greater than the ack, go net/dccp/ackvec.c-433- * forward in the AVR list until it is not so. net/dccp/ackvec.c-434- */ net/dccp/ackvec.c-435- list_for_each_entry_from(avr, &av->dccpav_records, net/dccp/ackvec.c-436- dccpavr_node) { net/dccp/ackvec.c:437: if (!after48(avr->dccpavr_ack_seqno, *ackno)) net/dccp/ackvec.c-438- goto found; net/dccp/ackvec.c-439- } net/dccp/ackvec.c-440- /* End of the dccpav_records list, not found, exit */ net/dccp/ackvec.c-441- break; ==> CASE 2: The code jumps to found whenever !before48(*ackno, avr->dccpavr_ack_seqno) or if the two arguments are 2^47 apart. * with the old definition the case "2^47 apart" was considered as 'not found'; * with the new definition the case "2^47 apart" is considered as 'found'. However: Please correct me if I am wrong, but I think that the case "2^47 apart" is entirely irrelevant here. My argument to support this is that Ack Vectors refer to successive sequence numbers (modulo wraparound, but we are considering this already). The highest possible difference of sequence numbers that fit into a single Ack Vector is 16192 (cf. RFC 4340, 11.4) - which is far less than 2^47. Hence this use is safe. net/dccp/ackvec.c:60: BUG_ON(before48(avr->dccpavr_ack_seqno, net/dccp/ackvec.c-61- head->dccpavr_ack_seqno)); ==> CASE 1 - safe. net/dccp/minisocks.c:199: if (after48(DCCP_SKB_CB(skb)->dccpd_seq, dreq->dreq_isr)) { net/dccp/minisocks.c-200- dccp_pr_debug("Retransmitted REQUEST\n"); ==> CASE 1 - safe. net/dccp/ccids/ccid2.c:619: while (before48(seqp->ccid2s_seq, ackno)) { net/dccp/ccids/ccid2.c-620- seqp = seqp->ccid2s_next; ==> CASE 1 - safe. net/dccp/ccids/ccid2.c-701- * Check for NUMDUPACK net/dccp/ccids/ccid2.c-702- */ net/dccp/ccids/ccid2.c-703- seqp = hctx->ccid2hctx_seqt; net/dccp/ccids/ccid2.c:704: while (before48(seqp->ccid2s_seq, hctx->ccid2hctx_high_ack)) { ==> CASE 1 - safe. net/dccp/input.c-74- * Step 5: Prepare sequence numbers for Sync net/dccp/input.c-75- * If P.type == Sync or P.type == SyncAck, net/dccp/input.c-76- * If S.AWL <= P.ackno <= S.AWH and P.seqno >= S.SWL, net/dccp/input.c-77- * / * P is valid, so update sequence number variables net/dccp/input.c-78- * accordingly. After this update, P will pass the tests net/dccp/input.c-79- * in Step 6. A SyncAck is generated if necessary in net/dccp/input.c-80- * Step 15 * / net/dccp/input.c-81- * Update S.GSR, S.SWL, S.SWH net/dccp/input.c-82- * Otherwise, net/dccp/input.c-83- * Drop packet and return net/dccp/input.c-84- */ net/dccp/input.c-85- if (dh->dccph_type == DCCP_PKT_SYNC || net/dccp/input.c-86- dh->dccph_type == DCCP_PKT_SYNCACK) { net/dccp/input.c-87- if (between48(DCCP_SKB_CB(skb)->dccpd_ack_seq, net/dccp/input.c-88- dp->dccps_awl, dp->dccps_awh) && net/dccp/input.c:89: !before48(DCCP_SKB_CB(skb)->dccpd_seq, dp->dccps_swl)) net/dccp/input.c-90- dccp_update_gsr(sk, DCCP_SKB_CB(skb)->dccpd_seq); ==> CASE 2: Here swl corresponds to SWL as defined in RFC 4340, 7.5.1. * with the old definition, gsr was not updated when P.seqno and S.SWL are 2^47 apart; * with the new definition, gsr is updated when P.seqno and S.SWL are 2^47 apart. ==> TODO: To handle this case correctly, we need to reconsider the intention: "P.seqno >= S.SWL". This can without problem be captured by the dccp_delta_seqno(a, b) function, which returns a value >= 0 when a==b or a is `before' b in the new definition. Hence, I'd suggest to replace !before48(DCCP_SKB_CB(skb)->dccpd_seq, dp->dccps_swl) with dccp_delta_seqno(dp->dccps_swl, DCCP_SKB_CB(skb)->dccpd_seq) >= 0 net/dccp/input.c-187- case DCCP_PKT_REQUEST: net/dccp/input.c-188- /* Step 7 net/dccp/input.c-189- * or (S.is_server and P.type == Response) net/dccp/input.c-190- * or (S.is_client and P.type == Request) net/dccp/input.c-191- * or (S.state >= OPEN and P.type == Request net/dccp/input.c-192- * and P.seqno >= S.OSR) net/dccp/input.c-193- * or (S.state >= OPEN and P.type == Response net/dccp/input.c-194- * and P.seqno >= S.OSR) net/dccp/input.c-195- * or (S.state == RESPOND and P.type == Data), net/dccp/input.c-196- * Send Sync packet acknowledging P.seqno net/dccp/input.c-197- * Drop packet and return net/dccp/input.c-198- */ net/dccp/input.c-199- if (dp->dccps_role != DCCP_ROLE_LISTEN) net/dccp/input.c-200- goto send_sync; net/dccp/input.c-201- goto check_seq; net/dccp/input.c-202- case DCCP_PKT_RESPONSE: net/dccp/input.c-203- if (dp->dccps_role != DCCP_ROLE_CLIENT) net/dccp/input.c-204- goto send_sync; net/dccp/input.c-205-check_seq: net/dccp/input.c:206: if (!before48(DCCP_SKB_CB(skb)->dccpd_seq, dp->dccps_osr)) { net/dccp/input.c-207-send_sync: net/dccp/input.c-208- dccp_send_sync(sk, DCCP_SKB_CB(skb)->dccpd_seq, net/dccp/input.c-209- DCCP_PKT_SYNC); ==> CASE 2: Here we have the same case in principle as above. A sync shall be sent if the condition "P.seqno >= S.OSR" is true. * with the old definition, a sync is not sent if P.seqno and S.OSR are 2^47 apart; * with the new definition, a sync is sent if P.seqno and S.OSR are 2^47 apart. Nothing harmful happens, the packet is dropped eventually anyway. It seems that whether using the old or new definition here is debatable; to make it consistent with the above case; one could consider replacing !before48(DCCP_SKB_CB(skb)->dccpd_seq, dp->dccps_osr) with dccp_delta_seqno(dp->dccps_osr, DCCP_SKB_CB(skb)->dccpd_seq) >= 0 if desired. 3) Conclusion ============= There are only two cases which need special consideration, all other uses of before48/after48 in the DCCP code are unproblematic. I shall provide a patch to update the two cases. - To unsubscribe from this list: send the line "unsubscribe dccp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
