802.11: fix header length when HT Control header is present

[sebastien-meter]

When the ORDER bit is set in the 802.11 flags, the header length needs to be incremented by the size of the HTControl field (4 B).

[infrastation]

Thank you for preparing this bug fix.

The macro FC_ORDER() correlates with IEEE 802.11-2012 Section 8.2.4.1.1, where in the Frame Control field the B15 bit is called "Order"; Section 8.2.4.1.10 Ibid. says that B15 indicates the presence of an HT Control in FC for QoS Data and Management frames only, but for non-QoS Data frames the meaning is different.

IEEE 802.11-2020 Section 9.2.4.1.1 calls B15 "+HTC" (the nomenclature in print-802_11.c is out of date, but that's a separate space for improvement); Section 9.2.4.1.10 Ibid. defines the semantics a bit differently: in QoS Data and Management frames it means the presence of HT Control or CMMG HT Control (both of which are 32-bit, so for the purpose of the header length calculation this is the same) in FC, but there is no mention of non-QoS Data frames.

This way, from the specifications it seems that the proposed header length increment should be guarded by the existing DATA_FRAME_IS_QOS() test within case T_DATA and should be added to case T_MGMT as well. Is there anything I misunderstood?

Once this is clarified, it would be useful to add comments to the new code to refer to the exact clause(s) it implements. Also if you rebase on the current master branch, the second commit will disappear.

[sebastien-meter]

Thank you for preparing this bug fix.

The macro FC_ORDER() correlates with IEEE 802.11-2012 Section 8.2.4.1.1, where in the Frame Control field the B15 bit is called "Order"; Section 8.2.4.1.10 Ibid. says that B15 indicates the presence of an HT Control in FC for QoS Data and Management frames only, but for non-QoS Data frames the meaning is different.

IEEE 802.11-2020 Section 9.2.4.1.1 calls B15 "+HTC" (the nomenclature in print-802_11.c is out of date, but that's a separate space for improvement); Section 9.2.4.1.10 Ibid. defines the semantics a bit differently: in QoS Data and Management frames it means the presence of HT Control or CMMG HT Control (both of which are 32-bit, so for the purpose of the header length calculation this is the same) in FC, but there is no mention of non-QoS Data frames.

This way, from the specifications it seems that the proposed header length increment should be guarded by the existing DATA_FRAME_IS_QOS() test within case T_DATA and should be added to case T_MGMT as well. Is there anything I misunderstood?

Once this is clarified, it would be useful to add comments to the new code to refer to the exact clause(s) it implements. Also if you rebase on the current master branch, the second commit will disappear.

Thank you for your comment, I agree with you, I updated the code accordingly and added a reference to the section of the standard that defines the HT Control field.

[sebastien-meter]

Thank you for preparing this bug fix.
The macro FC_ORDER() correlates with IEEE 802.11-2012 Section 8.2.4.1.1, where in the Frame Control field the B15 bit is called "Order"; Section 8.2.4.1.10 Ibid. says that B15 indicates the presence of an HT Control in FC for QoS Data and Management frames only, but for non-QoS Data frames the meaning is different.
IEEE 802.11-2020 Section 9.2.4.1.1 calls B15 "+HTC" (the nomenclature in print-802_11.c is out of date, but that's a separate space for improvement); Section 9.2.4.1.10 Ibid. defines the semantics a bit differently: in QoS Data and Management frames it means the presence of HT Control or CMMG HT Control (both of which are 32-bit, so for the purpose of the header length calculation this is the same) in FC, but there is no mention of non-QoS Data frames.
This way, from the specifications it seems that the proposed header length increment should be guarded by the existing DATA_FRAME_IS_QOS() test within case T_DATA and should be added to case T_MGMT as well. Is there anything I misunderstood?
Once this is clarified, it would be useful to add comments to the new code to refer to the exact clause(s) it implements. Also if you rebase on the current master branch, the second commit will disappear.

Thank you for your comment, I agree with you, I updated the code accordingly and added a reference to the section of the standard that defines the HT Control field.

Based on your comment I also decided to push a commit to change the name of the bit, please let me know if you don't think we should do that here.

[infrastation]

Thank you. Looking at 802.11-2020 Section 9.2.4.6 again, I notice it says that HT Control is present in Control Wrapper frames regardless of +HTC, 802.11-2012 Section 8.2.4.6 says the same.

[sebastien-meter]

Thank you. Looking at 802.11-2020 Section 9.2.4.6 again, I notice it says that HT Control is present in Control Wrapper frames regardless of +HTC, 802.11-2012 Section 8.2.4.6 says the same.

Yes, I didn't change this because I believe this is already included in CTRL_CONTROL_WRAPPER_HDRLEN

#define CTRL_CONTROL_WRAPPER_HDRLEN     (IEEE802_11_FC_LEN+IEEE802_11_DUR_LEN+\
                                         IEEE802_11_ADDR1_LEN+\
                                         IEEE802_11_CARRIED_FC_LEN+\
                                         IEEE802_11_HT_CONTROL_LEN)

[infrastation]

I see. Since it isn't obvious that different places in the code account for different cases of HT Control length, it would be useful to add a comment to case CTRL_CONTROL_WRAPPER to say that the result already includes the length. Also the macro definition makes it clear there is a proper constant for the length, so it would be better to use len += IEEE802_11_HT_CONTROL_LEN; in the new code.

[sebastien-meter]

I see. Since it isn't obvious that different places in the code account for different cases of HT Control length, it would be useful to add a comment to case CTRL_CONTROL_WRAPPER to say that the result already includes the length. Also the macro definition makes it clear there is a proper constant for the length, so it would be better to use len += IEEE802_11_HT_CONTROL_LEN; in the new code.

Good points, done.

[infrastation]

Thank you. This is going to be merged soon unless anyone objects.

[infrastation]

Thank you.