All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/4] inet: Separate DSCP from ECN bits using new dscp_t type
@ 2022-02-04 13:58 Guillaume Nault
  2022-02-04 13:58 ` [PATCH net-next 1/4] ipv6: Define dscp_t and stop taking ECN bits into account in fib6-rules Guillaume Nault
                   ` (6 more replies)
  0 siblings, 7 replies; 10+ messages in thread
From: Guillaume Nault @ 2022-02-04 13:58 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski
  Cc: netdev, Hideaki YOSHIFUJI, David Ahern,
	Toke Høiland-Jørgensen, Shuah Khan, linux-kselftest,
	Russell Strong, Dave Taht

The networking stack currently doesn't clearly distinguish between DSCP
and ECN bits. The entire DSCP+ECN bits are stored in u8 variables (or
structure fields), and each part of the stack handles them in their own
way, using different macros. This has created several bugs in the past
and some uncommon code paths are still unfixed.

Such bugs generally manifest by selecting invalid routes because of ECN
bits interfering with FIB routes and rules lookups (more details in the
LPC 2021 talk[1] and in the RFC of this series[2]).

This patch series aims at preventing the introduction of such bugs (and
detecting existing ones), by introducing a dscp_t type, representing
"sanitised" DSCP values (that is, with no ECN information), as opposed
to plain u8 values that contain both DSCP and ECN information. dscp_t
makes it clear for the reader what we're working on, and Sparse can
flag invalid interactions between dscp_t and plain u8.

This series converts only a few variables and structures:

  * 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 any more. This contrasts
    with the previous behaviour where all 8 bits of the Traffic Class
    field were used. It is believed that this change is acceptable as
    matching ECN bits wasn't usable for IPv4, so only IPv6-only
    deployments could be depending on it. Also the previous behaviour
    made DSCP-based ip6-rules fail for packets with both a DSCP and an
    ECN mark, which is another reason why any such deploy is unlikely.

  * 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 would never match, as
    the packets would have their ECN bits cleared before doing the
    rule lookup.

  * Patch 3 converts the fc_tos field of struct fib_config. This is
    equivalent to patch 2, but for IPv4 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 packet, since their ECN bits are cleared before
    the lookup.

  * 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 had user
    facing consequences, this patch shouldn't have any side effect and
    is there to give an overview of what future conversion patches will
    look like. Conversions are quite mechanical, but imply some code
    churn, which is the price for the extra clarity a possibility of
    type checking.

To summarise, all the behaviour changes required for the dscp_t type
approach to work should be contained in patches 1-3. These changes are
edge cases of ip-route and ip-rule that don't currently work properly.
So they should be safe. Also, a kernel selftest is added for each of
them.

Finally, this work also paves the way for allowing the usage of the 3
high order DSCP bits in IPv4 (a few call paths already handle them, but
in general the stack clears them before IPv4 rule and route lookups).

References:
  [1] LPC 2021 talk:
        - https://linuxplumbersconf.org/event/11/contributions/943/
        - Direct link to slide deck:
            https://linuxplumbersconf.org/event/11/contributions/943/attachments/901/1780/inet_tos_lpc2021.pdf
  [2] RFC version of this series:
      - https://lore.kernel.org/netdev/cover.1638814614.git.gnault@redhat.com/

Changes since RFC:
  - Use simple mask instead of a bit shift to converting between u8
    and dscp_t (Toke).
  - Reword patch 4 to make it clear that no behaviour change is
    intended (Toke).
  - Add kernel selftests.
  - Rebase on latest net-next.

Guillaume Nault (4):
  ipv6: Define dscp_t and stop taking ECN bits into account in
    fib6-rules
  ipv4: Stop taking ECN bits into account in fib4-rules
  ipv4: Reject routes specifying ECN bits in rtm_tos
  ipv4: Use dscp_t in struct fib_alias

 include/net/inet_dscp.h                       | 57 ++++++++++++++
 include/net/ip_fib.h                          |  3 +-
 include/net/ipv6.h                            |  6 ++
 net/ipv4/fib_frontend.c                       | 11 ++-
 net/ipv4/fib_lookup.h                         |  3 +-
 net/ipv4/fib_rules.c                          | 18 +++--
 net/ipv4/fib_semantics.c                      | 14 ++--
 net/ipv4/fib_trie.c                           | 58 ++++++++------
 net/ipv4/route.c                              |  3 +-
 net/ipv6/fib6_rules.c                         | 19 +++--
 tools/testing/selftests/net/fib_rule_tests.sh | 60 ++++++++++++++-
 tools/testing/selftests/net/fib_tests.sh      | 76 +++++++++++++++++++
 12 files changed, 278 insertions(+), 50 deletions(-)
 create mode 100644 include/net/inet_dscp.h

-- 
2.21.3


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH net-next 0/4] inet: Separate DSCP from ECN bits using new dscp_t type
@ 2021-12-06 18:22 Guillaume Nault
  2021-12-06 18:22 ` [PATCH net-next 4/4] ipv4: Use dscp_t in struct fib_alias Guillaume Nault
  0 siblings, 1 reply; 10+ messages in thread
From: Guillaume Nault @ 2021-12-06 18:22 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski
  Cc: netdev, Hideaki YOSHIFUJI, David Ahern,
	Toke Høiland-Jørgensen, Russell Strong

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 :).

The problem to solve
====================

Currently, the networking stack generally doesn't clearly distinguish
between DSCP and ECN bits. The entire DSCP+ECN bits are stored in a
single u8 variable (or structure field), and each part of the stack
handles them in their own way, using different macros.

This has been, and still is, the source of several bugs:

  * Invalid use of RT_TOS(), leading to regression in VXLAN:
      - a0dced17ad9d ("Revert "vxlan: fix tos value before xmit"")

  * Other invalid uses of RT_TOS() in IPv6 code, where it doesn't make
    sense (RT_TOS() follows the old RFC 1349, which was never meant for
    IPv6):
      - grep for 'ip6_make_flowinfo(RT_TOS(tos)' for several examples
        that need to be fixed.

  * Failure to properly mask the ECN bits before doing IPv4 route
    lookups, leading to returning a wrong route:
      - 2e5a6266fbb1 ("netfilter: rpfilter: mask ecn bits before fib lookup"),
      - 8d2b51b008c2 ("udp: mask TOS bits in udp_v4_early_demux()"),
      - 21fdca22eb7d ("ipv4: Ignore ECN bits for fib lookups in fib_compute_spec_dst()"),
      - 1ebf179037cb ("ipv4: Fix tos mask in inet_rtm_getroute()"),
      - some more, uncommon, code paths still need to be fixed.

Also, this creates inconsistencies between subsystems:

 * IPv4 routes accept tos/dsfield options that have ECN bits set, but
   no packet can actually match them, as their ECN bits are cleared
   before lookup.

 * IPv4 rules accept tos/dsfield options that have the high order ECN
   bit set (but rejects those using the low order ECN bit). Like IPv4
   routes, such rules can't match actual packets as the rule lookup is
   done after clearing the packets ECN bits.

 * IPv6 routes accept the tos/dsfield options without any restriction,
   but silently ignores them.

 * IPv6 rules also accept any value of tos/dsfield, but compares all
   8 bits, including ECN. Therefore, a rule matching packets with a
   particular DSCP value stops working as soon as ECN is used (a work
   around is to define 4 rules for each DSCP value to match, one for
   each possible ECN code point).

 * Modules that want to distinguish between the DSCP and ECN bits
   create their own local macros (Netfilter, SCTP).

What this RFC does
==================

This patch series creates a dscp_t type, meant to represent pure DSCP
values, excluding ECN bits. Doing so allows to clearly separate the
interpretation of DSCP and ECN bits and enables Sparse to detect
invalid combinations of dscp_t with the plain u8 variables generally
used to store the full TOS/Traffic Class/DS field.

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.

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).

  * 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.

  * 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.

  * 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.

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

Future work
===========

This is a minimal series, the objective of this RFC is just to validate
the dscp_t approach. More work will be needed to fully iron out the
problem:

  * Convert more internal structures to dscp_t (in particular the
    flowi4_tos field of struct flowi4).

  * Slowly remove the uses of IPTOS_TOS_MASK, and hence RT_TOS(), in
    the kernel, as it clears only one of the ECN bits and the TOS is
    going to be masked again anyway by IPTOS_RT_MASK, which is
    stricter.

  * Finally, start allowing high DSCP values in IPv4 routes and rules
    (the IPTOS_RT_MASK used in IPv4 clears the 3 high order bits of the
    DS field).

  * Maybe find a way to warn users that use the ignored tos/dsfield
    option with ip -6 route.

Feedbacks welcome!

Thanks,

William

[1] LPC talk: https://linuxplumbersconf.org/event/11/contributions/943/
[2] LPC slides: https://linuxplumbersconf.org/event/11/contributions/943/attachments/901/1780/inet_tos_lpc2021.pdf


Guillaume Nault (4):
  ipv6: Define dscp_t and stop taking ECN bits into account in ip6-rules
  ipv4: Stop taking ECN bits into account in ip4-rules
  ipv4: Reject routes specifying ECN bits in rtm_tos
  ipv4: Use dscp_t in struct fib_alias

 include/net/inet_dscp.h  | 54 +++++++++++++++++++++++++++++++++++++
 include/net/ip_fib.h     |  3 ++-
 include/net/ipv6.h       |  6 +++++
 net/ipv4/fib_frontend.c  | 10 ++++++-
 net/ipv4/fib_lookup.h    |  3 ++-
 net/ipv4/fib_rules.c     | 17 ++++++------
 net/ipv4/fib_semantics.c | 14 +++++-----
 net/ipv4/fib_trie.c      | 58 +++++++++++++++++++++++-----------------
 net/ipv4/route.c         |  3 ++-
 net/ipv6/fib6_rules.c    | 18 ++++++++-----
 10 files changed, 138 insertions(+), 48 deletions(-)
 create mode 100644 include/net/inet_dscp.h

-- 
2.21.3


^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-02-08  5:33 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-04 13:58 [PATCH net-next 0/4] inet: Separate DSCP from ECN bits using new dscp_t type Guillaume Nault
2022-02-04 13:58 ` [PATCH net-next 1/4] ipv6: Define dscp_t and stop taking ECN bits into account in fib6-rules Guillaume Nault
2022-02-04 13:58 ` [PATCH net-next 2/4] ipv4: Stop taking ECN bits into account in fib4-rules Guillaume Nault
2022-02-04 13:58 ` [PATCH net-next 3/4] ipv4: Reject routes specifying ECN bits in rtm_tos Guillaume Nault
2022-02-04 18:19   ` Shuah Khan
2022-02-04 13:58 ` [PATCH net-next 4/4] ipv4: Use dscp_t in struct fib_alias Guillaume Nault
2022-02-07  6:08 ` [PATCH net-next 0/4] inet: Separate DSCP from ECN bits using new dscp_t type David Ahern
2022-02-07 19:03 ` Toke Høiland-Jørgensen
2022-02-08  5:10 ` patchwork-bot+netdevbpf
  -- strict thread matches above, loose matches on Subject: below --
2021-12-06 18:22 Guillaume Nault
2021-12-06 18:22 ` [PATCH net-next 4/4] ipv4: Use dscp_t in struct fib_alias Guillaume Nault

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.