netdev - Re: [PATCH net-next] sctp: Prepare sctp_v4_get_dst() to dscp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z3gXieFRj+xqozJE@debian>
Date: Fri, 3 Jan 2025 17:59:53 +0100
From: Guillaume Nault <gnault@...hat.com>
To: Xin Long <lucien.xin@...il.com>
Cc: David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	netdev@...r.kernel.org, Simon Horman <horms@...nel.org>,
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	linux-sctp@...r.kernel.org, Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH net-next] sctp: Prepare sctp_v4_get_dst() to dscp_t
 conversion.

On Fri, Jan 03, 2025 at 10:35:55AM -0500, Xin Long wrote:
> On Thu, Jan 2, 2025 at 11:34 AM Guillaume Nault <gnault@...hat.com> wrote:
> >
> > 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.

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

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

> Thanks.