On Fri, Jan 3, 2025 at 11:59 AM Guillaume Nault [off-list ref] wrote:

On Fri, Jan 03, 2025 at 10:35:55AM -0500, Xin Long wrote:

quoted

On Thu, Jan 2, 2025 at 11:34 AM Guillaume Nault [off-list ref] wrote:

quoted

Define inet_sk_dscp() to get a dscp_t value from struct inet_sock, so
that sctp_v4_get_dst() can easily set ->flowi4_tos from a dscp_t
variable. For the SCTP_DSCP_SET_MASK case, we can just use
inet_dsfield_to_dscp() to get a dscp_t value.

Then, when converting ->flowi4_tos from __u8 to dscp_t, we'll just have
to drop the inet_dscp_to_dsfield() conversion function.

With inet_dsfield_to_dscp() && inet_dsfield_to_dscp(), the logic
looks like: tos(dsfield) -> dscp_t -> tos(dsfield)
It's a bit confusing, but it has been doing that all over routing places.

The objective is to have DSCP values stored in dscp_t variables in the
kernel and keep __u8 values in user space APIs and packet headers. In
practice this means using inet_dscp_to_dsfield() and
inet_dsfield_to_dscp() at boundaries with user space or networking.

However, since core kernel functions and structures are getting updated
incrementally, some inet_dscp_to_dsfield() and inet_dsfield_to_dscp()
conversions are temporarily needed between already converted and not yet
converted parts of the stack.

quoted

In sctp_v4_xmit(), there's the similar tos/dscp thing, although it's not
for fl4.flowi4_tos.

The sctp_v4_xmit() case is special because its dscp variable, despite
its name, doesn't only carry a DSCP value, but also ECN bits.
Converting it to a dscp_t variable would lose the ECN information.

To be more precise, this is only the case if the SCTP_DSCP_SET_MASK
flag is not set. That is, when the "dscp" variable is set using
inet->tos. Since inet->tos contains both DSCP and ECN bits, this allows
the socket owner to manage ECN. I don't know if that's intented by the
SCTP code. If that isn't, and the ECN bits aren't supposed to be taken
into account here, then I'm happy to send a patch to convert
sctp_v4_xmit() to dscp_t too.

From the beginning SCTP sends its packet via ip_queue_xmit() where it
allows the socket owners to manage ECN, like TCP. So let's just leave it.

quoted

Also, I'm curious there are still a few places under net/ using:

  fl4.flowi4_tos = tos & INET_DSCP_MASK;

Will you consider changing all of them with
inet_dsfield_to_dscp() && inet_dsfield_to_dscp() as well?

Yes, I have a few more cases to convert. But some of them will have to
stay. For example, in net/ipv4/ip_output.c, __ip_queue_xmit() has
"fl4->flowi4_tos = tos & INET_DSCP_MASK;", but we can't just convert
that "tos" variable to dscp_t because it carries both DSCP and ECN
values. Although ->flowi4_tos isn't concerned with ECN, these ECN bits
are used later to set the IP header.

There are other cases that I'm not planning to convert, for example
because the value is read from a UAPI structure that can't be updated.
For example the "fl4.flowi4_tos = params->tos & INET_DSCP_MASK;" case
in bpf_ipv4_fib_lookup(), where "params" is a struct bpf_fib_lookup,
exported in UAPI.

To summarise, the plan is to incrementally convert most ->flowi4_tos
assignments, so that we have a dscp_t variable at hand. Then I'll send
a patch converting all ->flowi4_tos users at once. Most of it should
consist of trivial inet_dscp_to_dsfield() removals, thanks to the
previous dscp_t conversions. The cases that won't follow that pattern
will be explained in the commit message, but the idea is to have as few
of them as possible.

BTW, the reason for this work is to avoid having ECN bits interfering
with route lookups. We had several such issues and regressions in the
past because of ->flowi4_tos having ECN bits set in specific scenarios.

Got it, thanks for the detailed explanation.

Acked-by: Xin Long <lucien.xin@gmail.com>