hostapd: add measurement report value for beacon reports

[PolynomialDivision]

Add the measurement report value to the beacon reports send via ubus. It
is possible to derive from the measurement report if a station refused to
do a beacon report and why. It is important to know why a station refuses
to do a beacon-report. In particular, we should not request a beacon
report from a station again that refused a beacon-report before.

The rejection reasons can be found by looking at the bits defined by:

• MEASUREMENT_REPORT_MODE_ACCEPT
• MEASUREMENT_REPORT_MODE_REJECT_LATE
• MEASUREMENT_REPORT_MODE_REJECT_INCAPABLE
• MEASUREMENT_REPORT_MODE_REJECT_REFUSED

[blocktrron]

I'd just add the raw field instead of constructing a blobmsg.

[Ian-Clowes]

From #issuecomment-1007915918... and follow-ups...

At this level of bus communication I don't think expanding the bits is required, so for that part would just add the reported value. That makes it easier to add a simple tests on the DAWN end as well.

The case that led us to this of "REFUSED with data" is "special". I think as a happy accident it keeps the access to rep-> from SEGVing. So for a spec conformant REFUSED the message buffer will be too short and the other parts of building the message should be protected.

But I also saw those ACCEPT responses with no data as well:

Thu Jan 6 20:23:45 2022 daemon.notice hostapd: wlan0: BEACON-RESP-RX bb:bb:bb:bb:bb:bb 199 00
...so think the length test in the calling function has to be moved here as well (and deleted there otherwise it will still stop some REFUSED reporting).

Based on those "empty" ACCEPT messages we would then see a new type of NULL BSSID on the DAWN side, and should trap them before getting too far into processing them. If they are very common we should perhaps make them INFO messages rather than WARNING so they have to be turned on by the user.

It was useful that you copied the spec text. I re-read the section on the PARALLEL bit. I'm not sure it reflects an error condition, more of a "be aware". Maybe the rep_mode == MEASUREMENT_REPORT_MODE_ACCEPT test should be removed while we get some real world experience of inspecting that bit in DAWN? The length status message field gives us extra info to make decisions on.

So I end up with:

        blobmsg_add_u8(&b, "report-mode", rep_mode);
        blobmsg_add_u8(&b, "report-fully-formed", (len >= sizeof(struct rrm_measurement_beacon_report)) ? 1 : 0);

	if (len >= sizeof(struct rrm_measurement_beacon_report)) {
            ...
            blobmsg_add_u16(&b, "antenna-id", rep->antenna_id);
	    blobmsg_add_u16(&b, "parent-tsf", rep->parent_tsf);
	} else {
            // Not sure anything goes here.  Are empty field values needed?
        }

And removal of this in the caller:

            if (len < sizeof(struct rrm_measurement_beacon_report))
                return;

Users might also update DAWN without wanting to do the same with hostapd in the short term. Making DAWN compatible with old and new messages would be a good thing.

[blocktrron]

This is not specific to DAWN, so what DAWN does or does not with the message is not of relevance.

Expanding the Field for all elements in the measurement list is not the nicest thing one can do, but altering this behavior is something we should consider down the line (as soon as other measurement types are added).

[Ian-Clowes]

This is not specific to DAWN, so what DAWN does or does not with the message is not of relevance.

I agree it won't be the only consumer, but it is a tangible example of use so is of some relevance.

I'm not sure which bit looked specific to DAWN. Exposing the report mode and a way to distinguish messages with a useful payload can be useful for all.

[Ian-Clowes]

I'm not sure if it might also be useful / possible to add the optional elements as a raw hex-string. That would allow clients to inspect for things like RRM and xHT capabilities if the client device provided them, but I'm not sure if a beacon is a sensible place to discover them rather than waiting for a probe.

[PolynomialDivision]

I would take @Ian-Clowes code and remove the "else-part" completely. Is that fine @blocktrron ?

[blocktrron]

I would like to stick with just relaying the response-code and not trying to detect & handle some (maybe) invalid messages within hostapd.

Having this information downstream is enough to decide how to act depending on the needs of the application.

[Ian-Clowes]

I would like to stick with just relaying the response-code and not trying to detect & handle some (maybe) invalid messages within hostapd.

Having this information downstream is enough to decide how to act depending on the needs of the application.

I don't have time right now to think of the detail for the code, but just to be clear on the scenarios.

We have "good" devices that:

• Set the response code to 0 and provide data
• Set the response code to non-0 and provide no data

I've also seen both of these "bad" things:

• Response code non-0 and data provided. My Samsung A3 sends just a bunch of 0x00 - looks like itis pushing the struct onto the wire when it shoudn't do.
• Response code of 0 with no data. I didn't look which device this was - IIRC it is two of my local MACs.

At the moment protection from trying the "no data" scenarios is provided by the message len check prior to laying the struct over the buffer with a cast.

So in short some blend of length and response checks is probably needed in hostapd. Just a length check (like now) will suppress sharing of non-zero response codes. Just a response code check will barf if there is not actually any data.

I'd continue to ask for the remaining Information Elements to be passed as a hex string as well, or a subset if there is a length problem. They are potentially useful to the downsteam client.

BTW, I'm thinking of pulling hostapd locally so I can experimnet with it a bit. I'm happy to develop this if needed, but am not currently doing that.

[blocktrron]

So in short some blend of length and response checks is probably needed in hostapd. Just a length check (like now) will suppress sharing of non-zero response codes. Just a response code check will barf if there is not actually any data.

This is just further driving down the current implementation.

• hostapd receives 1 measurement response with a response-code
• This response contains n reports (here specifically beacon-reports) where n can be 0
• if n > 0 - hostapd sends n reports to subscibers

This is not good. Trying to design around this shortcoming of the current event is not desirable. That's what i was referring to with

Expanding the Field for all elements in the measurement list is not the nicest thing one can do, but altering this behavior is something we should consider down the line (as soon as other measurement types are added).

Read: I don't like including the response-code to the beacon-report event (as it inherits information from the container which is not related to the actual report). However, if it is required to evaluate the contained information, it's a compromise we can take.

Better: We should dispatch a single event (e.g. measurement-report) which contains all measurement reports within this action frame. This is more flexible, also in regards to adding more measurement types.

[Ian-Clowes]

Better: We should dispatch a single event (e.g. measurement-report) which contains all measurement reports within this action frame. This is more flexible, also in regards to adding more measurement types.

If that means creating a ubus message which reflects the fixed part of the 802.11 frame as a mandatory blob-ed section then each optional IE in a blob table then that would seem great to me. very happy to try and create it.

[blocktrron]

If there's already parsing in place, i would not return a list of measurements as unparsed elements, the measurements sub-elements might be a good idea to do so.

[Ansuel]

In short... pls don't add a table and just provide the raw value here... we can make the change later and add reason (if needed)

But currently i think it would be better to just provide the raw value and the tool parse it.
(or even do both a provide ALSO the raw value)

[PolynomialDivision]

Changed it.

[Ansuel]

@PolynomialDivision also i think you have to fix the commit description

[Ansuel]

@blocktrron what do you think? I think we can merge this