Hi again I have hacked a new fragment_add_seq_check() that uses a timeout to handle the re-transmission in the file attached to bug 15021.
While playing around with this I have started thinking if this is a general issue with fragment_add_seq_check()? When re-assembly is completed fragment_add_seq_check() moves the re-assembled frame to another hash table and stops considering it if more fragments are dissected. I am not sure why it is done that way but if a re-transmission for that frame is dissected after that point, the re-transmission starts a new re-assembly instead of being put into the completed one. Is this the intended/preferred behavior of fragment_add_seq_check() or should it handle re-transmissions past the point where the frame is re-assembled? I guess ZigBee APS isn't the only protocol where this could happen? /Kenneth 2018-08-02 21:36 GMT+02:00 Kenneth Soerensen <[email protected]>: > Hi Jaap and John > > I don't think ZigBee knowledge is required here and I guess this is a > general issue for protocols with short sequence numbers. > > My concern with your suggestion is that we will need to maintain a sliding > window and perform rollover detection for each node pair and direction (A > ZigBee network has multiple nodes communicating in both directions). > > Currently, the APS dissector is using the generic fragment_add_seq_check(). > > I have looked at fragment_add_seq_single_aging(), which seems to handle > something like this. However, this is for protocols with a single sequence > number (as John mentioned). Furthermore, the aging is based on frame > numbers, which will not work for ZigBee because the number of frames > between APS packets with the same sequence number will depend heavily on > the amount of other traffic in the network. > > I have considered to create a function which uses timestamps for aging > instead. A fragmented APS packet is considered to be completely transferred > within a reasonable time compared to the time between sequence number > rollover. But then again the _fragment_item struct used by the re-assembler > does not store packet timestamps. > > The first fragment in an APS packet has a special attribute so I have also > considered to simply re-start the reassembly for that sequence number when > a first fragment is dissected. > > /Kenneth > > > 2018-08-02 21:13 GMT+02:00 John Thacker <[email protected]>: > >> I had to deal with this problem when I added support for protocols like >> PPP MP with a single sequence number that increments each fragment instead >> of separate sequence numbers and fragment numbers. (See Appendix A of RFC >> 4623 (PWE3 Fragmentation and Reassembly) for a list of protocols that use >> this style, including PPP MP (RFC 1990), PWE3 MPLS (RFC 4385), L2TPv2 (RFC >> 2661), L2TPv3 (RFC 3931), ATM, and Frame Relay.) >> >> The problem happened in some long captures I had with some dropped >> packets and where the "short sequence number" variant was used for MLPPP, >> which is 12 bits (so a little bit bigger than the 8 bits with ZigBee, but >> not having separate fragment sequence numbers effectively costs a couple >> bits.) See in particular fragment_add_seq_single_aging(), and all the >> fragment_add_seq_single_* functions in general. My approach, which may not >> have been optimal, used a REASSEMBLE_FLAGS_AGING flag and a max_age >> parameter to discard partially reassembled fragments. When wireshark gets a >> new fragment and looks in the table of partially reassembled fragments to >> add it to, if the partially reassembled fragment comes from a frame number >> that is very old, that partially reassembled fragment is discarded. I ended >> up using the previously unused frame field of the partially reassembled >> fragment head for this (fh->frame), storing the frame number of most >> recently added fragment in this as well. >> >> Take a look at those functions, as it may provide some help for your >> approach. It worked for me on those files that I had. >> >> John Thacker >> >> On Thu, Aug 2, 2018 at 2:04 PM Jaap Keuter <[email protected]> wrote: >> >>> Hi, >>> >>> Not burdened by any ZigBee domain knowledge I would say that a seq# >>> rollover would require a clearing of the non-reassembled fragments. But not >>> all of them because we could still be in the process of reassembling the >>> part of the stream with the not-yet rolled over seq#. A sliding window of >>> non-reassembled fragments, of about half the seq# range, moved forward by >>> the next received seq#, could be sufficient. All in all this would be an >>> extension of the generic reassembly routenes, assuming they are used... >>> >>> Thanks, >>> Jaap >>> >>> On 2 Aug 2018, at 12:17, Kenneth Soerensen <[email protected]> wrote: >>> >>> Hi >>> >>> I have filled bug https://bugs.wireshark.org/bug >>> zilla/show_bug.cgi?id=15021 >>> >>> Any idea how we can fix this? >>> >>> The packet re-assembler is confused by ZigBee APS re-using sequence >>> numbers, which makes it hard to distinguish what fragments belong to >>> specific re-assembled packets. >>> >>> /Kenneth >>> >>> >>> ____________________________________________________________ >>> _______________ >>> Sent via: Wireshark-dev mailing list <[email protected]> >>> Archives: https://www.wireshark.org/lists/wireshark-dev >>> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev >>> mailto:[email protected]?subject=unsubscr >>> ibe >> >> >> ____________________________________________________________ >> _______________ >> Sent via: Wireshark-dev mailing list <[email protected]> >> Archives: https://www.wireshark.org/lists/wireshark-dev >> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev >> mailto:[email protected]?subject=unsubscr >> ibe >> > >
___________________________________________________________________________ Sent via: Wireshark-dev mailing list <[email protected]> Archives: https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
