| Message ID | 1a645f4a0bc60ad18e7c0916642883ce8a43c013.1735835456.git.gnault@redhat.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] sctp: Prepare sctp_v4_get_dst() to dscp_t conversion. | expand |
On Thu, Jan 2, 2025 at 11:34 AM Guillaume Nault <gnault@redhat.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. In sctp_v4_xmit(), there's the similar tos/dscp thing, although it's not for fl4.flowi4_tos. 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? Thanks. > > Signed-off-by: Guillaume Nault <gnault@redhat.com> > --- > include/net/inet_sock.h | 6 ++++++ > net/sctp/protocol.c | 10 +++++++--- > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h > index 3ccbad881d74..1086256549fa 100644 > --- a/include/net/inet_sock.h > +++ b/include/net/inet_sock.h > @@ -19,6 +19,7 @@ > #include <linux/netdevice.h> > > #include <net/flow.h> > +#include <net/inet_dscp.h> > #include <net/sock.h> > #include <net/request_sock.h> > #include <net/netns/hash.h> > @@ -302,6 +303,11 @@ static inline unsigned long inet_cmsg_flags(const struct inet_sock *inet) > return READ_ONCE(inet->inet_flags) & IP_CMSG_ALL; > } > > +static inline dscp_t inet_sk_dscp(const struct inet_sock *inet) > +{ > + return inet_dsfield_to_dscp(READ_ONCE(inet->tos)); > +} > + > #define inet_test_bit(nr, sk) \ > test_bit(INET_FLAGS_##nr, &inet_sk(sk)->inet_flags) > #define inet_set_bit(nr, sk) \ > diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c > index 8b9a1b96695e..29727ed1008e 100644 > --- a/net/sctp/protocol.c > +++ b/net/sctp/protocol.c > @@ -43,6 +43,7 @@ > #include <net/addrconf.h> > #include <net/inet_common.h> > #include <net/inet_ecn.h> > +#include <net/inet_sock.h> > #include <net/udp_tunnel.h> > #include <net/inet_dscp.h> > > @@ -427,16 +428,19 @@ static void sctp_v4_get_dst(struct sctp_transport *t, union sctp_addr *saddr, > struct dst_entry *dst = NULL; > union sctp_addr *daddr = &t->ipaddr; > union sctp_addr dst_saddr; > - u8 tos = READ_ONCE(inet_sk(sk)->tos); > + dscp_t dscp; > > if (t->dscp & SCTP_DSCP_SET_MASK) > - tos = t->dscp & SCTP_DSCP_VAL_MASK; > + dscp = inet_dsfield_to_dscp(t->dscp); > + else > + dscp = inet_sk_dscp(inet_sk(sk)); > + > memset(&_fl, 0x0, sizeof(_fl)); > fl4->daddr = daddr->v4.sin_addr.s_addr; > fl4->fl4_dport = daddr->v4.sin_port; > fl4->flowi4_proto = IPPROTO_SCTP; > if (asoc) { > - fl4->flowi4_tos = tos & INET_DSCP_MASK; > + fl4->flowi4_tos = inet_dscp_to_dsfield(dscp); > fl4->flowi4_scope = ip_sock_rt_scope(asoc->base.sk); > fl4->flowi4_oif = asoc->base.sk->sk_bound_dev_if; > fl4->fl4_sport = htons(asoc->base.bind_addr.port); > -- > 2.39.2 >
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@redhat.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.
On Fri, Jan 3, 2025 at 11:59 AM Guillaume Nault <gnault@redhat.com> wrote: > > 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@redhat.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. 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. > > > 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>
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h index 3ccbad881d74..1086256549fa 100644 --- a/include/net/inet_sock.h +++ b/include/net/inet_sock.h @@ -19,6 +19,7 @@ #include <linux/netdevice.h> #include <net/flow.h> +#include <net/inet_dscp.h> #include <net/sock.h> #include <net/request_sock.h> #include <net/netns/hash.h> @@ -302,6 +303,11 @@ static inline unsigned long inet_cmsg_flags(const struct inet_sock *inet) return READ_ONCE(inet->inet_flags) & IP_CMSG_ALL; } +static inline dscp_t inet_sk_dscp(const struct inet_sock *inet) +{ + return inet_dsfield_to_dscp(READ_ONCE(inet->tos)); +} + #define inet_test_bit(nr, sk) \ test_bit(INET_FLAGS_##nr, &inet_sk(sk)->inet_flags) #define inet_set_bit(nr, sk) \ diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c index 8b9a1b96695e..29727ed1008e 100644 --- a/net/sctp/protocol.c +++ b/net/sctp/protocol.c @@ -43,6 +43,7 @@ #include <net/addrconf.h> #include <net/inet_common.h> #include <net/inet_ecn.h> +#include <net/inet_sock.h> #include <net/udp_tunnel.h> #include <net/inet_dscp.h> @@ -427,16 +428,19 @@ static void sctp_v4_get_dst(struct sctp_transport *t, union sctp_addr *saddr, struct dst_entry *dst = NULL; union sctp_addr *daddr = &t->ipaddr; union sctp_addr dst_saddr; - u8 tos = READ_ONCE(inet_sk(sk)->tos); + dscp_t dscp; if (t->dscp & SCTP_DSCP_SET_MASK) - tos = t->dscp & SCTP_DSCP_VAL_MASK; + dscp = inet_dsfield_to_dscp(t->dscp); + else + dscp = inet_sk_dscp(inet_sk(sk)); + memset(&_fl, 0x0, sizeof(_fl)); fl4->daddr = daddr->v4.sin_addr.s_addr; fl4->fl4_dport = daddr->v4.sin_port; fl4->flowi4_proto = IPPROTO_SCTP; if (asoc) { - fl4->flowi4_tos = tos & INET_DSCP_MASK; + fl4->flowi4_tos = inet_dscp_to_dsfield(dscp); fl4->flowi4_scope = ip_sock_rt_scope(asoc->base.sk); fl4->flowi4_oif = asoc->base.sk->sk_bound_dev_if; fl4->fl4_sport = htons(asoc->base.bind_addr.port);
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. Signed-off-by: Guillaume Nault <gnault@redhat.com> --- include/net/inet_sock.h | 6 ++++++ net/sctp/protocol.c | 10 +++++++--- 2 files changed, 13 insertions(+), 3 deletions(-)