ip_ecn: don't touch the DSCP bits and flow_id

[pouria]

Summary

don't touch the DSCP bits
in ip_ecn_ingress, ip6_ecn_ingress and ip6_ecn_egress
except for NO_CARE for backward compatibility.
raised in D53516

Test Plan

ipsec, gif and stf should be tested.

[pouria]

sys/netinet/ip_ecn.c

133–135

@rscheff
I think the NOCARE is redundant. I copied the inner bits to the outer for ECN_NOCARE mode to preserve ipv4 previous behaviour, but I couldn't preserve backward compatibility for ipv6 without extra complexity due to ECN bit masking in ipv6 wrapper.
I handled IPv4 this way because I thought those who use this mode probably don't know how to handle DSCP and ECN fields. However, I removed the copying part from ECN_FORBIDDEN and IMHO consumers must handle their DSCP bits differently.

IMHO it's better to remove ECN_NOCARE mode, because net developers must decide if they want to support ECN for their module or not.

[pouria]

friendly reminder on our conversation about privacy concerns over IPv6 flow_id.
@rscheff

[pouria]

friendly ping @tuexen @rscheff

[rscheff]

sys/netinet/ip_ecn.c

225

You have Macros to get the ECN and DSCP bits inside a flowlabel for IPv6:

ip6.h:#define IPV6_TRAFFIC_CLASS(ip6) ((ntohl((ip6)->ip6_flow) >> IPV6_FLOWLABEL_LEN) & 0xff)
ip6.h:#define IPV6_DSCP(ip6) ((ntohl((ip6)->ip6_flow) >> IPV6_FLOWLABEL_LEN) & 0xfc)
ip6.h:#define IPV6_ECN(ip6) ((ntohl((ip6)->ip6_flow) >> IPV6_FLOWLABEL_LEN) & 0x03)

using them should improve readability (and avoid inline magic numbers). not sure if some pre-compiler magic exists to make the same macro to allow different expansions on read vs. write access though (as there exists for parameter type checking).

Also, RFC6040 is quiet about DSCP - so I wonder, shouldn't there be options to reuse the same DSCP in the outer as in the inner, rather than resetting them too (or overriding, or mapping). Probably some different discussion though.

[rscheff]

(You may want to override the global macros for this module only, though. i guess refactoring everythgin to work with pointers to the network header just for readabiliy enhancement is not worth it - but still accessor macros which describe what you are doing would enhance readability.

[pouria]

sys/netinet/ip_ecn.c

225

Unfortunately, those macros take an IPv6 header as input, and we only have a flow_id in our function.
IMHO, it won't be worth changing the existing global macros.
What do you recommend?

[rscheff]

maybe something like this; I suppose if this is after all the includes, it's scoped only within this source file.

#undef IPV6_ECN
#define IPV6_ECN(ipv6_flow) ((ntohl(ip6_flow) >> IPV6_FLOWLABEL_LEN) & 0x03)

conversely, the setting of the ECN can be done again with a macro override in the subsequent function.
Gleb may also have some input on this kinds of shenanigans; with the macro I feel more people would more readly understand what is going on here.