Comment # 6
on bug 9160
from Peter Paluch
(In reply to comment #5)
Hello Evan,
Thank you for your insightful comments! Let me go over each of them.
> - If there is no reliable way to determine from the packet whether or not
> this extension is present, I think it make sense to add it as a preference
> on the existing protocol rather than registering it as a separate but nearly
> identical protocol?
It is true that a BPDU itself has no explicit indication whether it contains
this TLV extension. However, regarding the TLV's presence, Cisco's PVST+ and
Rapid PVST+ BPDUs always have the TLV present; standard IEEE STP/RSTP/MSTP
never have this extension. In fact, if a Cisco PVST+/RPVST+ BPDU is missing
this TLV, Cisco switches will refuse to process it (see, for example,
http://www.cisco.com/en/US/docs/switches/lan/catalyst3750/software/release/15.0_2_se/system/message/msg_desc.html
and look for SPANTREE-2-RECV_BAD_TLV error message explanation; the SSTP BPDU
this document mentions is Cisco's name for (R)PVST+ BPDUs). In other words, if
the BPDU is a Cisco (R)PVST+ BPDU, the TLV must be present, otherwise it is a
malformed BPDU; standard STP/RSTP/MSTP BPDUs never have this TLV.
So the detection of the TLV's presence really boils down to detecting whether
the BPDU is a Cisco (R)PVST+ BPDU. Now, in my understanding, the only
appropriate way is to use the LLC/SNAP values from the Ethernet header that
uniquely identify the protocol. However, these values are not accessible in the
packet-bpdu.c routines anymore (the pinfo structure does not seem to have any
relevant elements to carry this information), so the only way to distinguish
upon specific LLC/SNAP values is to register a separate dissector for specific
LLC or SNAP combinations. This approach was also suggested to me by Guy Harris
in a private conversation back in 2010 when I first started looking at
implementing this extension - this is a quotation from his e-mail:
"> I would like to ask if there is a recommended way how to ask for the
LLC/SNAP Ethernet header values in the packet-bpdu.c dissector.
No, but there's a recommended way to solve this problem:
Have separate dissectors for STP/RSTP and PVST+/RPVST+, but have those
dissectors do all their work by calling a common routine, which does all the
dissection, and which takes an additional gboolean argument to indicate whether
this is regular STP or PVST+. Register the PVST+/RPVST+ dissector in the
"llc.cisco_pid" dissector table with the PID 0x010b."
This is exactly what I have implemented. Note that your suggestion with making
the detection of the TLV just a "protocol preference" (I assume you are talking
about configurable protocol preferences in Wireshark settings) would not solve
the issue: I would still need to know if I am processing a standard or a Cisco
BPDU.
I agree myself that if there was a way to look at the LLC/SNAP values from the
existing BPDU dissection routine, it would not be necessary to register a
separate dissector at all. Furthermore, if there was a way of looking at the
802.1Q tag of these (R)PVST+ BPDUs from the BPDU dissector, I would gladly
implement a Native VLAN Mismatch detection as an expert info, as this mismatch
is at the core of some rather unpleasant problems with Cisco's (R)PVST+ in
switched networks, and having Wireshark identify it would be awesome. This all
goes back to the issue of accessing the data from lower layer headers in upper
layer dissectors. Perhaps the packet_info structure shall be extended to
contain this data at some point in the future?
I will be happy to rework this part of the patch if any other reliable way of
detecting whether the dissected BPDU is a standard BPDU or a Cisco (R)PVST+
BPDU is available; for now, I do not know of any other reliable way of
detecting its type aside from registering a separate dissector for the
particular SNAP OUI/PID combination that uniquely identifies Cisco's (R)PVST+.
> - There is no need to use proto_tree_add_uint for tlvtype and tlvlength
> since they are extracted unmodified from the packet - just use
> proto_tree_add_item.
Yes, you are correct. I have replaced all such occurences.
> - Please don't use proto_tree_add_text except for building subtrees. The
> "Data" entry should probably be of type FT_BYTES, and the "Malformed TLV"
> entry should probably be an expert info field instead.
Again, you are correct. Both these suggestions are implemented in the new
uploaded patch.
In addition to the suggested corrections, I have implemented a more robust use
of the expert info API to detect various malformations in the (R)PVST+ TLVs,
and I have done a slight correction to my previous change - for Cisco's
VLAN-bridge STP with SNAP PID 0x010c, the generic BPDU dissector shall be
called instead of the PVST dissector, as this Cisco's STP variant does not use
the TLV extension. Only the (R)PVST+ with the SNAP PID of 0x010b contains the
TLV.
All comments and suggestions are welcome! I will be uploading the new patch
shortly. I hope this one is better than the previous one.
Best regards,
Peter
You are receiving this mail because:
- You are watching all bug changes.
