|
Bugzilla – Full Text Bug Listing |
| Summary: | packet tags are not copied via TCP transport | ||
|---|---|---|---|
| Product: | ns-3 | Reporter: | Vladimir Yashin <vladyash> |
| Component: | network | Assignee: | natale.patriciello |
| Status: | REOPENED --- | ||
| Severity: | normal | CC: | natale.patriciello, pdbarnes, tomh, tommaso.pecorella |
| Priority: | P4 | ||
| Version: | pre-release | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Attachments: |
scratch, that shows that packet tags are not copied via TCP transport
proposed patch |
||
Created attachment 1473 [details]
proposed patch
I found that packet tags are not copied in Packet::AddAtEnd at src/network/model/packet.cc
Proposed patch is in attachment.
All tests run correctly in my build (ver 3.14)
This is more a design issue rather than a bug. The point is: a tag is something that is attached to a packet but it's not sent "on the wire" for real. In a real system you should forget about tagging a packet without including a proper header or something, 'cuz it won't work. I know that it's a common practice to add information to packets in simulations, and to use that information to do something, however it's (in my opinion) a bad practice, as it raises the obvious questions "did you consider where that info will be stored in a real system?, what would be the overhead? Did you consider if this can be done for real?". Not sending the packet tags is obviously a limitation, but it does enforce the concept that what's sent on wire, then the receiver can use it, otherwise the receiver can not use it. From a simulation perspective it could sound limiting and frustrating, but that's how real systems work. Now, if the general consensus is that this feature is useful I'll not object, however my belief is that this feature opens the door to many bad practices, as it's a shortcut to defining proper headers and modifying the protocols so to do something in a reproducible way. Aside from this, packet tags would never be transferred, for example, in nsc. So if we allow this we'll have a different behaviour with and without nsc simulations. Not really a thing I'd be happy with. Based on this, and despite the number of cases where I'd had liked to be able to add a tag to a transmitted packet, I'd rather reject the idea. Tags should be really be seen as a "local" feature limited to the packet processing in a single node, exactly as they are in real systems. T. PS: in IPv6 the process of sending a "Tag" on wire is perfectly legit. However it means you have to build a custom IPv6 Option Header where the tag will be stored. Not easy but not impossible, and it will work as intended even in real-world. There are Options also in the TCP header, but then the tag would have to be scope-limited to L4. It's a design thing, again.
> Now, if the general consensus is that this feature is useful I'll not
> object, however my belief is that this feature opens the door to many bad
> practices, as it's a shortcut to defining proper headers and modifying the
> protocols so to do something in a reproducible way.
I disagree with this; we introduced packet tags to allow people to attach simulation-specific metadata to conduct their simulations more easily. Tags have a limited scope (e.g. they can't be used in emulation) but for the scope that they are valid, I do not think they should be deprecated.
I think that this is a valid bug report that we should look into. I think it is reasonable for a user to expect, based on the documentation, that a packet tag appended at the application layer, for a small datagram, would survive the TCP layer. As background, note that we introduced the distinction of 'byte' tags and 'packet' tags to get around the messy semantics of what happens to packets when they get munged in the stack. For example, TCP may take packets, dump their contents into a (logical) byte buffer, and send them out in different-sized chunks. It becomes vague, then, what to do when some portion of a buffer chunk was originally tagged with a 'packet' tag, while the rest was not or was differently tagged. Here, the problem may be an implementation subtlety. Packet::AddAtEnd(packet) is a packet concatenation operation. The reason that I believe that the proposed patch was not originally implemented was that this is a case of where the semantics of the packet tag are not so clear. If you take two packets A and B, and only B has the packet tag, and then you make a new packet A+B, should A+B also carry the tag? This is not completely clear in the documentation, but I believe that the intent was for the code to do what it does now, i.e. do not copy packet tags when the operation is to merge two packets. Therefore, I am not sure that modifying AddAtEnd() is the desired patch. One solution might be to try to adapt TCP (and there are a few others that may do this) so that it doesn't use the AddAtEnd() for this use case (i.e. so that this test case passes). It should then be defined, however, what kind of operations will result in packet tags being preserved. For instance, passing a large packet with a tag to the TCP layer, such as a 2000 byte application packet. This will result in two TCP segments. What should happen in this case to the packet tag? Another solution would be to document that TCP does not pass packet tags due to the application byte stream semantics of the protocol. Here, perhaps we should try to make sure that only the non-copying Packet methods are used in the implementation. I think we either need to make a clear statement that these protocols that use AddAtEnd() to build packets will not pass packet tags, or else rework them to not use AddAtEnd() in the way that they do so presently, and document how packet tags work through them. (In reply to comment #3) +1 The model library is fairly clear on use cases, I think: > So-called ‘byte’ tags are used to tag a subset of the bytes in the packet byte > buffer while ‘packet’ tags are used to tag the packet itself. The main > difference between these two kinds of tags is what happens when packets are > copied, fragmented, and reassembled: ‘byte’ tags follow bytes while ‘packet’ > tags follow packets. https://www.nsnam.org/docs/models/html/packets.html And again in more detail here: https://www.nsnam.org/docs/models/html/packets.html#adding-and-removing-tags While it doesn't say it explicitly, I think there's a pretty good implication that tags will be transported between nodes within ns3. Again, it's not said explicitly, but it's clearly stated that buffers contain what can appear on the wire, and are exchanged with real systems. Tags and metadata are simulation-only conveniences. Patch has issues, I think.
AddAtEnd claims to "Concatenate the input packet at the end of the current packet."
The proposed patch:
--- a/src/network/model/packet.cc Mon Nov 19 18:18:00 2012 +0400
+++ b/src/network/model/packet.cc Tue Nov 20 14:23:48 2012 +0400
@@ -325,6 +325,8 @@
copy.AddAtStart (m_buffer.GetCurrentEndOffset () - bEnd,
appendPrependOffset);
m_byteTagList.Add (copy);
+ m_packetTagList = packet->m_packetTagList;
+
m_metadata.AddAtEnd (packet->m_metadata);
}
This *replaces* all the packet tags from the "this" packet, with those from the argument packet. (Notice that the ByteTags are added, not replaced.)
The patch should really *add* the argument packet tags to the "this" packet. Note that PacketTagLists are only allowed to have one Tag of a given type? (If they somehow get more than one, only the first one is accessible.)
What should happen here if both packets have the same Tag? or worse, the same tag with different data? Which packet's tags should win?
(In reply to comment #3) > > Now, if the general consensus is that this feature is useful I'll not > > object, however my belief is that this feature opens the door to many bad > > practices, as it's a shortcut to defining proper headers and modifying the > > protocols so to do something in a reproducible way. > > I disagree with this; we introduced packet tags to allow people to attach > simulation-specific metadata to conduct their simulations more easily. Tags > have a limited scope (e.g. they can't be used in emulation) but for the > scope that they are valid, I do not think they should be deprecated. As I said, I'm not against Tags per-se, I'm just concerned about bad habits. On the other hand, as Peter pointed out, the documentation say the Tags should be carried. Hence, this is a valid bug. I'd like to point out also another case where packets (or datagrams) are split. IPv4 (and IPv6 in some very limited cases) can split an L4 packet, TCP *or* UDP. Hence we should look at this case as well: a packet is received by an intermediate node and split in two or more packets. A possible solution (also fixing other issues) is to have PacketTags refactored so to have precise indications on the layer that generated them (and that must consume them). E.g. Application, TCP, UDP, IP and so on. Attaching a "layer" to the tag would ensure that no tag *should* be different across different segments of the same packet. The "should" is due to TCP, as it's a stream-oriented protocol, so nothing really guarantees that a packet is constant across retransmissions. This might as well affect "application level" tags. On the other hand, if we want to be really precise, a *packet* tag only make sense where a packet is used, so UDP, IP and such. TCP and applications based on TCP should avoid packet tags, as they don't make really sense. Last but not least, I'd like to amend the documentation to point out that using packet tags (or byte tags) to carry informations that are, as a matter of fact, out-of-band data is definitely something that should be carefully considered. T. (In reply to comment #4) > I think that this is a valid bug report that we should look into. I think it > is reasonable for a user to expect, based on the documentation, that a > packet tag appended at the application layer, for a small datagram, would > survive the TCP layer. I suspect that yes, this is a reasonable assumption about the TCP implementation. However, in general, I do not think that it is reasonable to expect packet tags to survive the TCP layer since, as you point out below, it is byte stream transport, not a packet transport like UDP. > > As background, note that we introduced the distinction of 'byte' tags and > 'packet' tags to get around the messy semantics of what happens to packets > when they get munged in the stack. For example, TCP may take packets, dump > their contents into a (logical) byte buffer, and send them out in > different-sized chunks. It becomes vague, then, what to do when some > portion of a buffer chunk was originally tagged with a 'packet' tag, while > the rest was not or was differently tagged. right > > Here, the problem may be an implementation subtlety. > Packet::AddAtEnd(packet) is a packet concatenation operation. The reason > that I believe that the proposed patch was not originally implemented was > that this is a case of where the semantics of the packet tag are not so > clear. If you take two packets A and B, and only B has the packet tag, and > then you make a new packet A+B, should A+B also carry the tag? This is not > completely clear in the documentation, but I believe that the intent was for > the code to do what it does now, i.e. do not copy packet tags when the > operation is to merge two packets. right > > Therefore, I am not sure that modifying AddAtEnd() is the desired patch. agreed > > One solution might be to try to adapt TCP (and there are a few others that > may do this) so that it doesn't use the AddAtEnd() for this use case (i.e. > so that this test case passes). It should then be defined, however, what We could do this but I do not think that it is a good idea in general to sort-of implement a feature and let users try to figure out when the feature might work and when it might not. i.e., when is the application datagram big-enough that it deserves to be split and its tags thrown away ? > kind of operations will result in packet tags being preserved. For > instance, passing a large packet with a tag to the TCP layer, such as a 2000 > byte application packet. This will result in two TCP segments. What should > happen in this case to the packet tag? Fragmentation is easy to do for packet tags in general. The problem are the semantics of reassembly. (i.e., AddAtEnd) > > Another solution would be to document that TCP does not pass packet tags due > to the application byte stream semantics of the protocol. Here, perhaps we > should try to make sure that only the non-copying Packet methods are used in > the implementation. > > I think we either need to make a clear statement that these protocols that > use AddAtEnd() to build packets will not pass packet tags, or else rework > them to not use AddAtEnd() in the way that they do so presently, and > document how packet tags work through them. That sounds fine to me. Hello, It seems there is a general consensus here: PacketTags should not be kept by the TCP layer, which provides a stream of bytes, not packets, to the applications. Therefore, use ByteTags, or don't use Tag at all (use your own header which the application will serialize/deserialize). For any other issue, please reopen the bug. Hmm, I read this thread as perhaps consensus on fixing, for the reasons most clearly articulated by Tom: simulator-specific support, for gathering metrics, for example. Closing seems premature. It's certainly a valid bug, and needs work no matter what outcome we decide on. Clearly the documentation needs improvement, that tags (both Packet and Byte) shouldn't be used to carry protocol traffic, duplication of PacketTags in the case of fragmentation, and any other limitations and oddities. (There is a related bug on transporting tags over MPI.) If we are really not going to fix this, then we have work to do: clarify in the documentation what will be supported (on node), and removing existing support for tags carried along between nodes. (In reply to Peter Barnes from comment #10) > Hmm, I read this thread as perhaps consensus on fixing, for the reasons most > clearly articulated by Tom: simulator-specific support, for gathering > metrics, for example. > > Closing seems premature. It's certainly a valid bug, and needs work no > matter what outcome we decide on. > > Clearly the documentation needs improvement, that tags (both Packet and > Byte) shouldn't be used to carry protocol traffic, duplication of PacketTags > in the case of fragmentation, and any other limitations and oddities. > (There is a related bug on transporting tags over MPI.) > > If we are really not going to fix this, then we have work to do: clarify in > the documentation what will be supported (on node), and removing existing > support for tags carried along between nodes. We have many open issues elsewhere to fix, and this has been open for several years without a clear agreed-upon proposal for changing. Upon revisiting today, my suggestion is that we could take the following steps and close this. 1) Remove all packet tags as packets enter the TCP layer, so as to avoid situations where it seems to be working and then stops working. 2) Document in the TCP documentation that packet tags do not survive the TCP layer due to the byte stream semantics, and point to this issue's discussion for background. 3) Document in the Packet documentation more clearly that Packet tags will not survive fragmentation-related operations and that users should not count on them across a fragmentation/reassembling layer, and call out TCP as one such layer. Perhaps we clarify in Packet::AddAtEnd() that protocols using this method should strip packet tags. 4) Review whether other protocols (e.g. LTE RLC, Wi-Fi A-MPDU) should also strip packet tags, and follow similar documentation steps. > We have many open issues elsewhere to fix, and this has been open for > several years without a clear agreed-upon proposal for changing. Yes we have many open issues, but closing issues *just* because they're old doesn't make ns-3 better. It seems you agree that there isn't a clear proposal or consensus yet. > Upon revisiting today, my suggestion is that we could take the following > steps and close this. > ... We also agree that there is work to be done. Good. I think your suggestion amounts to deprecating cross-node use of tags, despite the use cases outlined in the discussion. This runs counter to the opinions from Tom and me. So no consensus on which direction to go. (In reply to Peter Barnes from comment #10) > Hmm, I read this thread as perhaps consensus on fixing, for the reasons most > clearly articulated by Tom: simulator-specific support, for gathering > metrics, for example. Inside TCP there is no notion of an application packet. As well as in LTE [1], in which the scheduler can select to transmit only fractions of the entire "application packet", or can merge a lot of data, and probably in some other L2 devices (Wifi?). If what you want is to gather information about a single packet sent down by an application and received by another node, and you're trying to do this with a Tag that passes through all the layers, you're doing it wrong, at least in my opinion. The only reliable way to trace things is per-byte, using ByteTags. You have to gather metric per-byte. And this is missing from the documentation, as everyone is reporting. We already had a similar discussion when I pointed out the strange use of Tags in LTE. And I agree with Tommaso when he says that using tags is a dangerous practice. Emulation in ns-3 is a strong point in favor of it, to connect ns-3 to real systems, but each time you use a Tag, a small emulator dies in the imaginary emulator's world. Everyone should keep the Tag usage to the minimum. I believe that only FlowMonitor should be allowed to use that feature. A tale of Tags can be that, to avoid metadata's read/write/concatenation, I have kept inside TcpTxBuffer a list of application packets, instead of putting all the bytes in one, single buffer. Mainly for performance reason, because touching metadata is costly... but can you imagine the work to ensure that the buffer is consistent with all the possible requests that the TCP socket, that works "per-byte", can do? [1] https://www.nsnam.org/bugzilla/show_bug.cgi?id=2308 > Closing seems premature. It's certainly a valid bug, and needs work no > matter what outcome we decide on. I believe the only thing to do here is to update the documentation, instructing the user not to expect PacketTags surviving the layers. If it happens, it is just luck, and it is not guaranteed to succeed in future ns-3 releases. For sure I will not take any action to support PacketTags inside TCP actively, and neither I will approve a change in that direction inside the TCP code because it would mean to lose the concept that TCP is a byte stream protocol. Anyone that will try to fix this at simulator level has to answer to the main challenge, which is "What we have to do in the case of merging two tags of the same type?". I believe that the only correct answer is "let the developer decide", therefore providing for each PacketTag subclass a static friend function that, given two instances of a Tag, decides which Tag should survive. Or, better, force Tag subclasses to provide an operator<(), and imposing that the minor of the two will survive. Have a nice day! (In reply to Peter Barnes from comment #12) > > We have many open issues elsewhere to fix, and this has been open for > > several years without a clear agreed-upon proposal for changing. > > Yes we have many open issues, but closing issues *just* because they're old > doesn't make ns-3 better. I wasn't suggesting to close just because it was old; I was pointing out instead that we have lived for many years in this state and without any consensus on how or whether to proceed, it will continue to linger in the tracker only-- better to just take some steps to at least document behavior (limitations) and make it consistent. > > It seems you agree that there isn't a clear proposal or consensus yet. Yes. > > > Upon revisiting today, my suggestion is that we could take the following > > steps and close this. > > ... > > We also agree that there is work to be done. Good. > > I think your suggestion amounts to deprecating cross-node use of tags, > despite the use cases outlined in the discussion. This runs counter to the > opinions from Tom and me. So no consensus on which direction to go. I think that my suggestion would effectively deprecate cross-node use of packet but not byte tags, since Wi-Fi, LTE, and TCP all appear to perform operations on packet objects that make the packet tag semantics vague. (In reply to Tom Henderson from comment #14) > I think that my suggestion would effectively deprecate cross-node use of > packet but not byte tags, since Wi-Fi, LTE, and TCP all appear to perform > operations on packet objects that make the packet tag semantics vague. To be honest it's years I warn about this in the ns-3 users group when the problem arises. And it's also years I don't assume in my code that PacketTags are safe to be used in cross-nodes communications and even in intra-node comms between layers... PacketTags are useful but they can be easily broken. One solution is to: 1) Allow more than one PacketTag of the same type in a packet (packet reassembly). Eventually purging multiple copies of the very same PacketTag (i.e., same type, same values). 2) Copy PacketTags during packet fragmentation. Eventually one could consider adding a "fragmented" flag. However, this would add a lot of burden to the packet processing functions (maybe, dunno, metering would be needed). Just my 2 cents. |
Created attachment 1472 [details] scratch, that shows that packet tags are not copied via TCP transport Hello! I made a simple scenario to show this bug (see attachment). Scenario goes like this: 1. I create a packet, add a custom tag to it and print the value of custom tag 2. I send it via TCP 3. I print the value of custom tag of received packet. The printed value is always default value => custom tag is not present in packet.