pfctl: Add missing 'va' code point name

[kp]

Summary

Add the 'va' (voice-admit, RFC5865) symbolic name.

MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")

[gbe]

LGTM from manpages

[rgrimes]

I have a concern here, some of which goes beyond just this change, about aliasing DSCP to TOS. We (a small congestion control research group) have found that due to ambigious manual pages, headers and other factors, some present in PF it appears are causing mismarked traffic on the internet. DSCP is shifted left by the 2 ECN bits, the manual pages and any other documentation MUST be made explicity clear about this fact as to if your supplying the values as specified in the RFC"s on DSCP (unshifted) or are you applying TOS bits that may or may not have the ECN bits masked out.

From my reading of the code above the value supplied to TOS and to DSCP is the unshifted full byte value, and that may lead to the same mistakes we are seeing in other places.

I have further concerns that above I see verbs for what are officially obsolete by RFC TOS byte values that should probably at a minimum invoke a warning, and preferably invoke an error and no longer function such as "critical, lowdelay, throughput,reliability. We should also probably rip those out of sys/netinet/ip.h and do an exprun and purge there use.

[kp]

In D29069#651186, @rgrimes wrote:

I have a concern here, some of which goes beyond just this change, about aliasing DSCP to TOS. We (a small congestion control research group) have found that due to ambigious manual pages, headers and other factors, some present in PF it appears are causing mismarked traffic on the internet. DSCP is shifted left by the 2 ECN bits, the manual pages and any other documentation MUST be made explicity clear about this fact as to if your supplying the values as specified in the RFC"s on DSCP (unshifted) or are you applying TOS bits that may or may not have the ECN bits masked out.

I'd be happy to extend the man page to make that clearer. Do you have any suggested phrasing?

From my reading of the code above the value supplied to TOS and to DSCP is the unshifted full byte value, and that may lead to the same mistakes we are seeing in other places.

The support for the symbolic names ('af11', 'ef', ...) makes that mistake a little less likely at least.

I have further concerns that above I see verbs for what are officially obsolete by RFC TOS byte values that should probably at a minimum invoke a warning, and preferably invoke an error and no longer function such as "critical, lowdelay, throughput,reliability. We should also probably rip those out of sys/netinet/ip.h and do an exprun and purge there use.

I'm not removing support for these names. That would break existing configuration files.

[rscheff]

I agree with Rod's assessment here.

While TOS is obsolete and a rework towards DSCP is well overdue, not fixing the 2-bit bitshift is IMHO an invitation for further confusion.

DSCP codepoints are values 0..63. Thus the expectation when setting DSCP is, that the relevant shifting etc is all handled by whoever handles a "dscp" value.

DSCP certainly does not map 1:1 to TOS. While sometimes missed, when dealing with TOS, the left-shift (and masking of ECN bits) should be harder to miss when DSCP codepoints are to be set.

I suggest to rework the parser in such a way, that the named DSCP codepoints can only be used with the DSCP keyword, and the legacy TOS values only with the keyword TOS. And the parser can deal with the necessary left-shift (the DSCP input value should be 0..63).

[rgrimes]

I'd be happy to extend the man page to make that clearer. Do you have any suggested phrasing?

I do not, bringing in some more eyes from the Transport group to get better feedback.

From my reading of the code above the value supplied to TOS and to DSCP is the unshifted full byte value, and that may lead to the same mistakes we are seeing in other places.

The support for the symbolic names ('af11', 'ef', ...) makes that mistake a little less likely at least.

Yes, but what we see is people trying to use the Hex values from an RFC, and those end up miss applied because the definition here of DSCP is actually TOS byte.

I have further concerns that above I see verbs for what are officially obsolete by RFC TOS byte values that should probably at a minimum invoke a warning, and preferably invoke an error and no longer function such as "critical, lowdelay, throughput,reliability. We should also probably rip those out of sys/netinet/ip.h and do an exprun and purge there use.

I'm not removing support for these names. That would break existing configuration files.

I gave as an option to emit a warning, and I am ok with that as a first phase, these bit values need to DIE, even the IETF has moved the original defining RFC to HISTORIC status. The continued erronous use of these definitions is causing issues with current and future work on ECN and DSCP.

[kp]

In D29069#651199, @rscheff wrote:

I suggest to rework the parser in such a way, that the named DSCP codepoints can only be used with the DSCP keyword, and the legacy TOS values only with the keyword TOS. And the parser can deal with the necessary left-shift (the DSCP input value should be 0..63).

That will break existing configurations. So no.

For context here: the intent of this patch is diff reduction between pfsense and FreeBSD. Specifically this one: https://github.com/pfsense/FreeBSD-src/commit/9b7b2bd920aa8f142ea1c293983416a24aa1e6a7#diff-a000aa9992a4804e609eaadc9203a8f6681fdadabe279cae7b94cf836ed031c3
That adds keyword support, but only on DSCP, and masks off the ECN bits prior to matching. Both of those are already present in FreeBSD for the tos keyword, so this aims to not break pfsense configuration while removing that patch, by also accepting 'dscp' where we now only accept 'tos'.

So, I'd like to avoid having to change the pfsense control code to cope with new rules for dscp (we're not changing how the tos keyword works regardless), and having 'dscp' and 'tos' work differently is an invitation for configuration errors.

We could minimise the odds of confusion by warning if a tos or dscp value is set with either of the two lower bits (i.e. the ECN bits) set.

[rgrimes]

Given the issues I would rather NOT add dscp support at all, until these issues can be addressed from down/upstream? The current situation, both in pfsense and freebsd is poor at best and may actually be one of the sources of issues we are seeing in our ECN data collection work. We know for a fact people are being told to use these ancient and obsolete TOS values by looking for them in blog posts and how-to's. Using the pry bar that is "reduce diffs between" as a reason to add a bad situation is an ever worse situation!

[kp]

In D29069#651242, @rgrimes wrote:

Given the issues I would rather NOT add dscp support at all, until these issues can be addressed from down/upstream? The current situation, both in pfsense and freebsd is poor at best and may actually be one of the sources of issues we are seeing in our ECN data collection work. We know for a fact people are being told to use these ancient and obsolete TOS values by looking for them in blog posts and how-to's. Using the pry bar that is "reduce diffs between" as a reason to add a bad situation is an ever worse situation!

I'm not sure I follow how this would make things worse. One way or the other I'm not changing how the tos keyword works in pf. I'm also not removing any of the currently supported names for codepoints.

Still, I'll reduce this diff to only add the 'va' codepoint, and see how hard it'll be to teach pfsense to just use 'tos' where it currently uses 'dscp'.

[kp]

Reduce patch to only add the missing voice-admit code point.

[rgrimes]

I am ok with doing this, though it still leaves all the other issues open.