[06/14] wifi: cfg80211: Update the default DSCP-to-UP mapping

Commit Message

Korenblit, Miriam Rachel Dec. 11, 2023, 7:05 a.m. UTC

From: Ilan Peer <[e-mail]>

The default DSCP-to-UP mapping method defined in RFC8325
applied to packets marked per recommendations in RFC4594 and
destined to 802.11 WLAN clients will yield a number of inconsistent
QoS mappings.

To handle this, modify the mapping of specific DSCP values for
which the default mapping will create inconsistencies, based on
the recommendations in section 4 in RFC8325.

Signed-off-by: Ilan Peer <[e-mail]>
Reviewed-by: Greenman, Gregory <[e-mail]>
Reviewed-by: Berg, Johannes <[e-mail]>
Signed-off-by: Miri Korenblit <[e-mail]>
---
 net/wireless/util.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Johannes Berg Dec. 12, 2023, 9:39 a.m. UTC | #1

> 
> The default DSCP-to-UP mapping method defined in RFC8325
> applied to packets marked per recommendations in RFC4594 and
> destined to 802.11 WLAN clients will yield a number of inconsistent
> QoS mappings.
> 
> To handle this, modify the mapping of specific DSCP values for
> which the default mapping will create inconsistencies, based on
> the recommendations in section 4 in RFC8325.
> 

This breaks ap_qosmap_default_acm ap_qosmap_default test cases.

How should we handle that?

johannes

Peer, Ilan Dec. 13, 2023, 11:52 a.m. UTC | #2

Hi,

> -----Original Message-----
> From: Johannes Berg <[e-mail]>
> Sent: Tuesday, December 12, 2023 11:40
> To: Korenblit, Miriam Rachel <[e-mail]>
> Cc: [e-mail]; Peer, Ilan <[e-mail]>;
> Greenman, Gregory <[e-mail]>; Jouni Malinen <[e-mail]>
> Subject: Re: [PATCH 06/14] wifi: cfg80211: Update the default DSCP-to-UP
> mapping
> 
> >
> > The default DSCP-to-UP mapping method defined in RFC8325 applied to
> > packets marked per recommendations in RFC4594 and destined to 802.11
> > WLAN clients will yield a number of inconsistent QoS mappings.
> >
> > To handle this, modify the mapping of specific DSCP values for which
> > the default mapping will create inconsistencies, based on the
> > recommendations in section 4 in RFC8325.
> >
> 
> This breaks ap_qosmap_default_acm ap_qosmap_default test cases.
> 
> How should we handle that?

Corresponding change for hostap:  https://patchwork.ozlabs.org/project/hostap/patch/[e-mail]/.

Regards,

Ilan.

Jouni Malinen Dec. 17, 2023, 9:42 a.m. UTC | #3

On Mon, Dec 11, 2023 at 09:05:24AM +0200, Miri Korenblit wrote:
> The default DSCP-to-UP mapping method defined in RFC8325
> applied to packets marked per recommendations in RFC4594 and
> destined to 802.11 WLAN clients will yield a number of inconsistent
> QoS mappings.
> 
> To handle this, modify the mapping of specific DSCP values for
> which the default mapping will create inconsistencies, based on
> the recommendations in section 4 in RFC8325.

Could this commit message give some more justification for why these
exact specifications are the best source of this information? For
example, should this also reference the Wi-Fi QoS Management
specification?

> diff --git a/net/wireless/util.c b/net/wireless/util.c
> @@ -980,7 +980,53 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb,
>  		}
>  	}
>  
> +	/* The default mapping as defined in RFC8325 */
>  	ret = dscp >> 5;

Could you point to the exact location in RFC 8325 that has that as the
default mapping? Figure 1 has this note: "Note: All unused codepoints
are RECOMMENDED to be mapped to UP 0".

Section 2.4 does describe what many APs do, but that section is called
"Default UP-to-DSCP Mappings and Conflicts" which does not sound like a
recommended default mapping in RFC 8325..

> +	/* Handle specific DSCP values for which the default mapping doesn't
> +	 * adhere to the intended usage of the DSCP value. See section 4 in
> +	 * RFC8325.
> +	 */
> +	switch (dscp >> 2) {

What about "Standard: DF" (0 --> 0) and "Low-Priority Data: CS1" (8 ->
1)? Are those omitted here because the "dscp >> 5" happens to have same
value? It would seem to be good to have at least a comment here pointing
that out in particular taken into account that comment above about the
status of that "default mapping" and how it relates to RFC 8325.

> +	case 10:
> +	case 12:
> +	case 14:
> +		/* High throughput data: AF11, AF12, AF13 */
> +		ret = 0;
> +		break;
> +	case 16:
> +		/* Operations, Administration, and Maintenance and Provisioning:
> +		 * CS2
> +		 */
> +		ret = 0;
> +		break;
> +	case 18:
> +	case 20:
> +	case 22:
> +		/* Low latency data: AF21, AF22, AF23 */
> +		ret = 3;
> +		break;
> +	case 24:
> +		/* Broadcasting video: CS23 */
> +		ret = 4;
> +		break;

Typo.. Should be "CS3" instead of "CS23".

What about "Multimedia Streaming: AF31, AF32, AF33"? Shouldn't those
values 26, 28, 30 be mapped to 4? If not, it would be good to add a
comment explaining why that is not here while it is included in RFC 8325
Figure 1.

What about "Real-Time Interactive: CS4"? Shouldn't that value 32 be
mapped to 4? If not, it would be good to add a comment explaining why
that is not here while it is included in RFC 8325 Figure 1.

What about "Multimedia Conferencing: AF41, AF42, AF43"? Shouldn't those
values 34, 36, 38 be mapped to 4? If not, it would be good to add a
comment explaining why that is not here while it is included in RFC 8325
Figure 1.

> +	case 40:
> +		/* Signaling: CS5 */
> +		ret = 5;
> +		break;
> +	case 44:
> +		/* Voice Admit */
> +		ret = 6;
> +		break;

To be consistent, that comment should be "VOICE-ADMIT: VA".

> +	case 46:
> +		/* Telephony traffic: EF */
> +		ret = 6;
> +		break;
> +	case 48:
> +		/* Network Control Traffic: CS6 */
> +		ret = 7;
> +		break;
> +	}

This does not include "Network Control: CS7". Is there a reason for not
covering that case 56 now? I know it is "reserved for future use", but
RFC 8325 does provide mapping for it as well.

Peer, Ilan Dec. 17, 2023, 3:11 p.m. UTC | #4

Hi,

> -----Original Message-----
> From: Jouni Malinen <[e-mail]>
> Sent: Sunday, December 17, 2023 11:42
> To: Korenblit, Miriam Rachel <[e-mail]>
> Cc: [e-mail]; [e-mail]; Peer, Ilan
> <[e-mail]>; Greenman, Gregory <[e-mail]>;
> Berg, Johannes <[e-mail]>
> Subject: Re: [PATCH 06/14] wifi: cfg80211: Update the default DSCP-to-UP
> mapping
> 
> On Mon, Dec 11, 2023 at 09:05:24AM +0200, Miri Korenblit wrote:
> > The default DSCP-to-UP mapping method defined in RFC8325 applied to
> > packets marked per recommendations in RFC4594 and destined to 802.11
> > WLAN clients will yield a number of inconsistent QoS mappings.
> >
> > To handle this, modify the mapping of specific DSCP values for which
> > the default mapping will create inconsistencies, based on the
> > recommendations in section 4 in RFC8325.
> 
> Could this commit message give some more justification for why these exact
> specifications are the best source of this information? For example, should this
> also reference the Wi-Fi QoS Management specification?
> 

I was using these specifications since they were mentioned in the Wi-Fi QoS Management specification
and because RFC8325 was specifically mentioned in the WFA test plan specifications.
I'll update the commit message. 

> > diff --git a/net/wireless/util.c b/net/wireless/util.c @@ -980,7
> > +980,53 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb,
> >  		}
> >  	}
> >
> > +	/* The default mapping as defined in RFC8325 */
> >  	ret = dscp >> 5;
> 
> Could you point to the exact location in RFC 8325 that has that as the default
> mapping? Figure 1 has this note: "Note: All unused codepoints are
> RECOMMENDED to be mapped to UP 0".
> 
> Section 2.4 does describe what many APs do, but that section is called
> "Default UP-to-DSCP Mappings and Conflicts" which does not sound like a
> recommended default mapping in RFC 8325..
> 

The term "Default DSCP-to-UP" is defined in section 2.3 in RFC8325. As noted there, this is
not a specification definition, but more a term to describe what is a "common practice in the
networking industry".

> > +	/* Handle specific DSCP values for which the default mapping doesn't
> > +	 * adhere to the intended usage of the DSCP value. See section 4 in
> > +	 * RFC8325.
> > +	 */
> > +	switch (dscp >> 2) {
> 
> What about "Standard: DF" (0 --> 0) and "Low-Priority Data: CS1" (8 -> 1)?
> Are those omitted here because the "dscp >> 5" happens to have same value?
> It would seem to be good to have at least a comment here pointing that out in
> particular taken into account that comment above about the status of that
> "default mapping" and how it relates to RFC 8325.
> 

Yes. I thought my comment above was clear about it, but I'll rephrase it to make
It clearer.

> > +	case 10:
> > +	case 12:
> > +	case 14:
> > +		/* High throughput data: AF11, AF12, AF13 */
> > +		ret = 0;
> > +		break;
> > +	case 16:
> > +		/* Operations, Administration, and Maintenance and
> Provisioning:
> > +		 * CS2
> > +		 */
> > +		ret = 0;
> > +		break;
> > +	case 18:
> > +	case 20:
> > +	case 22:
> > +		/* Low latency data: AF21, AF22, AF23 */
> > +		ret = 3;
> > +		break;
> > +	case 24:
> > +		/* Broadcasting video: CS23 */
> > +		ret = 4;
> > +		break;
> 
> Typo.. Should be "CS3" instead of "CS23".
> 

Fixed.

> What about "Multimedia Streaming: AF31, AF32, AF33"? Shouldn't those
> values 26, 28, 30 be mapped to 4? If not, it would be good to add a comment
> explaining why that is not here while it is included in RFC 8325 Figure 1.
> 

The default mapping maps AF31, AF32 and AF33 to 4 which is the recommended mapping.
I'll add a comment.

> What about "Real-Time Interactive: CS4"? Shouldn't that value 32 be mapped
> to 4? If not, it would be good to add a comment explaining why that is not
> here while it is included in RFC 8325 Figure 1.
> 

Same here. The default mapping is used. I'll add a comment.

> What about "Multimedia Conferencing: AF41, AF42, AF43"? Shouldn't those
> values 34, 36, 38 be mapped to 4? If not, it would be good to add a comment
> explaining why that is not here while it is included in RFC 8325 Figure 1.
> 

Same.

> > +	case 40:
> > +		/* Signaling: CS5 */
> > +		ret = 5;
> > +		break;
> > +	case 44:
> > +		/* Voice Admit */
> > +		ret = 6;
> > +		break;
> 
> To be consistent, that comment should be "VOICE-ADMIT: VA".
> 

Fixed.

> > +	case 46:
> > +		/* Telephony traffic: EF */
> > +		ret = 6;
> > +		break;
> > +	case 48:
> > +		/* Network Control Traffic: CS6 */
> > +		ret = 7;
> > +		break;
> > +	}
> 
> This does not include "Network Control: CS7". Is there a reason for not
> covering that case 56 now? I know it is "reserved for future use", but RFC
> 8325 does provide mapping for it as well.
> 
 
Same.

Regards,

Ilan.

Patch

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 626b858b4b35..dd93e1950921 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -980,7 +980,53 @@  unsigned int cfg80211_classify8021d(struct sk_buff *skb,
 		}
 	}
 
+	/* The default mapping as defined in RFC8325 */
 	ret = dscp >> 5;
+
+	/* Handle specific DSCP values for which the default mapping doesn't
+	 * adhere to the intended usage of the DSCP value. See section 4 in
+	 * RFC8325.
+	 */
+	switch (dscp >> 2) {
+	case 10:
+	case 12:
+	case 14:
+		/* High throughput data: AF11, AF12, AF13 */
+		ret = 0;
+		break;
+	case 16:
+		/* Operations, Administration, and Maintenance and Provisioning:
+		 * CS2
+		 */
+		ret = 0;
+		break;
+	case 18:
+	case 20:
+	case 22:
+		/* Low latency data: AF21, AF22, AF23 */
+		ret = 3;
+		break;
+	case 24:
+		/* Broadcasting video: CS23 */
+		ret = 4;
+		break;
+	case 40:
+		/* Signaling: CS5 */
+		ret = 5;
+		break;
+	case 44:
+		/* Voice Admit */
+		ret = 6;
+		break;
+	case 46:
+		/* Telephony traffic: EF */
+		ret = 6;
+		break;
+	case 48:
+		/* Network Control Traffic: CS6 */
+		ret = 7;
+		break;
+	}
 out:
 	return array_index_nospec(ret, IEEE80211_NUM_TIDS);
 }