Re: [PATCH net-next 0/4] inet: Separate DSCP from ECN bits using new dscp_t type

Guillaume Nault [off-list ref] writes:

Following my talk at LPC 2021 [1], here's a patch series whose
objective is to start fixing the problems with how DSCP and ECN bits
are handled in the kernel. This approach seemed to make consensus among
the participants, although it implies a few behaviour changes for some
corner cases of ip rule and ip route. Let's see if this consensus can
survive a wider review :).

I like the approach, although I must admit to not being too familiar
with the parts of the code you're touching in this series. But I think
the typedefs make sense, and I (still) think it's a good idea to do the
conversion. I think the main thing to ensure from a backwards
compatibility PoV is that we don't silently change behaviour in a way
that is hard to detect. I.e., rejecting invalid configuration is fine
even if it was "allowed" before, but, say, changing the matching
behaviour so an existing rule set will still run unchanged but behave
differently is best avoided.

Note, this patch series differs slightly from that of the original talk
(slide 14 [2]). For the talk, I just cleared the ECN bits, while in
this series, I do a bit-shift. This way dscp_t really represents DSCP
values, as defined in RFCs. Also I've renamed the helper functions to
replace "u8" by "dsfield", as I felt "u8" was ambiguous. Using
"dsfield" makes it clear that dscp_t to u8 conversion isn't just a
plain cast, but that a bit-shift happens and the result has the two ECN
bits.

I like the names, but why do the bitshift? I get that it's conceptually
"cleaner", but AFAICT the shifted values are not actually used for
anything other than being shifted back again? In which case you're just
adding operations in the fast path for no reason...

The new dscp_t type is then used to convert several field members:

  * Patch 1 converts the tclass field of struct fib6_rule. It
    effectively forbids the use of ECN bits in the tos/dsfield option
    of ip -6 rule. Rules now match packets solely based on their DSCP
    bits, so ECN doesn't influence the result anymore. This contrasts
    with previous behaviour where all 8 bits of the Traffic Class field
    was used. It is believed this change is acceptable as matching ECN
    bits wasn't usable for IPv4, so only IPv6-only deployments could be
    depending on it (that is, it's unlikely enough that a someone uses
    ip6 rules matching ECN bits in production).

I think this is OK, cf the "break explicitly" thing I wrote above.

  * Patch 2 converts the tos field of struct fib4_rule. This one too
    effectively forbids defining ECN bits, this time in ip -4 rule.
    Before that, setting ECN bit 1 was accepted, while ECN bit 0 was
    rejected. But even when accepted, the rule wouldn't match as the
    packets would normally have their ECN bits cleared while doing the
    rule lookup.

As above.

  * Patch 3 converts the fc_tos field of struct fib_config. This is
    like patch 2, but for ip4 routes. Routes using a tos/dsfield option
    with any ECN bit set is now rejected. Before this patch, they were
    accepted but, as with ip4 rules, these routes couldn't match any
    real packet, since callers were supposed to clear their ECN bits
    beforehand.

Didn't work at all, so also fine.

  * Patch 4 converts the fa_tos field of struct fib_alias. This one is
    pure internal u8 to dscp_t conversion. While patches 1-3 dealed
    with user facing consequences, this patch shouldn't have any side
    effect and is just there to give an overview of what such
    conversion patches will look like. These are quite mechanical, but
    imply some code churn.

This is reasonable, and I think the code churn is worth the extra
clarity. You should probably spell out in the commit message that it's
not intended to change behaviour, though.

Note that there's no equivalent of patch 3 for IPv6 (ip route), since
the tos/dsfield option is silently ignored for IPv6 routes.

Shouldn't we just start rejecting them, like for v4?

-Toke