diff mbox series

[net-next,v2,4/7] ipv4: remove get_rttos

Message ID 20250212021142.1497449-5-willemdebruijn.kernel@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: deduplicate cookie logic | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 12 this patch: 12
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 2115 this patch: 2115
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1743 this patch: 1743
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 95 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0

Commit Message

Willem de Bruijn Feb. 12, 2025, 2:09 a.m. UTC 
From: Willem de Bruijn <willemb@google.com>

Initialize the ip cookie tos field when initializing the cookie, in
ipcm_init_sk.

The existing code inverts the standard pattern for initializing cookie
fields. Default is to initialize the field from the sk, then possibly
overwrite that when parsing cmsgs (the unlikely case).

This field inverts that, setting the field to an illegal value and
after cmsg parsing checking whether the value is still illegal and
thus should be overridden.

Be careful to always apply mask INET_DSCP_MASK, as before.

v1->v2
  - limit INET_DSCP_MASK to routing

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 include/net/ip.h | 11 +++--------
 net/ipv4/ping.c  |  6 +++---
 net/ipv4/raw.c   |  6 +++---
 net/ipv4/udp.c   |  6 +++---
 4 files changed, 12 insertions(+), 17 deletions(-)

Comments

Paolo Abeni Feb. 13, 2025, 3:10 p.m. UTC | #1 
On 2/12/25 3:09 AM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Initialize the ip cookie tos field when initializing the cookie, in
> ipcm_init_sk.
> 
> The existing code inverts the standard pattern for initializing cookie
> fields. Default is to initialize the field from the sk, then possibly
> overwrite that when parsing cmsgs (the unlikely case).
> 
> This field inverts that, setting the field to an illegal value and
> after cmsg parsing checking whether the value is still illegal and
> thus should be overridden.
> 
> Be careful to always apply mask INET_DSCP_MASK, as before.

I have a similar doubt here. I'm unsure we can change an established
behavior.

> v1->v2
>   - limit INET_DSCP_MASK to routing

Minor nit, this should come after the '---' separator. Yep, it used to
be the other way around, but we have less uAPI constraints here ;)

/P
Willem de Bruijn Feb. 13, 2025, 4:23 p.m. UTC | #2 
Paolo Abeni wrote:
> On 2/12/25 3:09 AM, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> > 
> > Initialize the ip cookie tos field when initializing the cookie, in
> > ipcm_init_sk.
> > 
> > The existing code inverts the standard pattern for initializing cookie
> > fields. Default is to initialize the field from the sk, then possibly
> > overwrite that when parsing cmsgs (the unlikely case).
> > 
> > This field inverts that, setting the field to an illegal value and
> > after cmsg parsing checking whether the value is still illegal and
> > thus should be overridden.
> > 
> > Be careful to always apply mask INET_DSCP_MASK, as before.
> 
> I have a similar doubt here. I'm unsure we can change an established
> behavior.

This patch does not change behavior.

Does not intend to, at least.

> > v1->v2
> >   - limit INET_DSCP_MASK to routing
> 
> Minor nit, this should come after the '---' separator. Yep, it used to
> be the other way around, but we have less uAPI constraints here ;)

Okay. I have no preference. I thought the latest guidance was to have
it recorded. Is this something to clarify in maintainer-netdev.rst?
Willem de Bruijn Feb. 13, 2025, 4:29 p.m. UTC | #3 
Willem de Bruijn wrote:
> Paolo Abeni wrote:
> > On 2/12/25 3:09 AM, Willem de Bruijn wrote:
> > > From: Willem de Bruijn <willemb@google.com>
> > > 
> > > Initialize the ip cookie tos field when initializing the cookie, in
> > > ipcm_init_sk.
> > > 
> > > The existing code inverts the standard pattern for initializing cookie
> > > fields. Default is to initialize the field from the sk, then possibly
> > > overwrite that when parsing cmsgs (the unlikely case).
> > > 
> > > This field inverts that, setting the field to an illegal value and
> > > after cmsg parsing checking whether the value is still illegal and
> > > thus should be overridden.
> > > 
> > > Be careful to always apply mask INET_DSCP_MASK, as before.
> > 
> > I have a similar doubt here. I'm unsure we can change an established
> > behavior.
> 
> This patch does not change behavior.
> 
> Does not intend to, at least.

I should have added that that is what the cmsg_ipv4 test extension is
for. It was indeed not covered by existing tests, unlike much of the
other changes.

That said, this is the least self evident patch of the series. If you
prefer I can send without.

Either way, I'll follow up with a cmsg_ip.sh refactoring of 
cmsg_ipv6.sh that extends coverage to IPv4.
Paolo Abeni Feb. 13, 2025, 5:23 p.m. UTC | #4 
On 2/13/25 5:23 PM, Willem de Bruijn wrote:
> Paolo Abeni wrote:
>> On 2/12/25 3:09 AM, Willem de Bruijn wrote:
>>> From: Willem de Bruijn <willemb@google.com>
>>>
>>> Initialize the ip cookie tos field when initializing the cookie, in
>>> ipcm_init_sk.
>>>
>>> The existing code inverts the standard pattern for initializing cookie
>>> fields. Default is to initialize the field from the sk, then possibly
>>> overwrite that when parsing cmsgs (the unlikely case).
>>>
>>> This field inverts that, setting the field to an illegal value and
>>> after cmsg parsing checking whether the value is still illegal and
>>> thus should be overridden.
>>>
>>> Be careful to always apply mask INET_DSCP_MASK, as before.
>>
>> I have a similar doubt here. I'm unsure we can change an established
>> behavior.
> 
> This patch does not change behavior.
> 
> Does not intend to, at least.

Doh! I misread the comment and the code so that the patch inverted the
cmsg vs sockopt priority.

Reread more carefully, I'm fine with this patch.

>>> v1->v2
>>>   - limit INET_DSCP_MASK to routing
>>
>> Minor nit, this should come after the '---' separator. Yep, it used to
>> be the other way around, but we have less uAPI constraints here ;)
> 
> Okay. I have no preference. I thought the latest guidance was to have
> it recorded. Is this something to clarify in maintainer-netdev.rst?

It's sort of a recurring topic, so I guess it would help.

Thanks,

Paolo
diff mbox series

Patch

diff --git a/include/net/ip.h b/include/net/ip.h
index 6af16545b3e3..4798500f3398 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -92,7 +92,9 @@  static inline void ipcm_init(struct ipcm_cookie *ipcm)
 static inline void ipcm_init_sk(struct ipcm_cookie *ipcm,
 				const struct inet_sock *inet)
 {
-	ipcm_init(ipcm);
+	*ipcm = (struct ipcm_cookie) {
+		.tos = READ_ONCE(inet->tos),
+	};
 
 	sockcm_init(&ipcm->sockc, &inet->sk);
 
@@ -256,13 +258,6 @@  static inline u8 ip_sendmsg_scope(const struct inet_sock *inet,
 	return RT_SCOPE_UNIVERSE;
 }
 
-static inline __u8 get_rttos(struct ipcm_cookie* ipc, struct inet_sock *inet)
-{
-	u8 dsfield = ipc->tos != -1 ? ipc->tos : READ_ONCE(inet->tos);
-
-	return dsfield & INET_DSCP_MASK;
-}
-
 /* datagram.c */
 int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len);
 int ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len);
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 619ddc087957..85d09f2ecadc 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -705,7 +705,7 @@  static int ping_v4_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	struct ip_options_data opt_copy;
 	int free = 0;
 	__be32 saddr, daddr, faddr;
-	u8 tos, scope;
+	u8 scope;
 	int err;
 
 	pr_debug("ping_v4_sendmsg(sk=%p,sk->num=%u)\n", inet, inet->inet_num);
@@ -768,7 +768,6 @@  static int ping_v4_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		}
 		faddr = ipc.opt->opt.faddr;
 	}
-	tos = get_rttos(&ipc, inet);
 	scope = ip_sendmsg_scope(inet, &ipc, msg);
 
 	if (ipv4_is_multicast(daddr)) {
@@ -779,7 +778,8 @@  static int ping_v4_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	} else if (!ipc.oif)
 		ipc.oif = READ_ONCE(inet->uc_index);
 
-	flowi4_init_output(&fl4, ipc.oif, ipc.sockc.mark, tos, scope,
+	flowi4_init_output(&fl4, ipc.oif, ipc.sockc.mark,
+			   ipc.tos & INET_DSCP_MASK, scope,
 			   sk->sk_protocol, inet_sk_flowi_flags(sk), faddr,
 			   saddr, 0, 0, sk->sk_uid);
 
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 4304a68d1db0..6aace4d55733 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -486,7 +486,7 @@  static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	struct ipcm_cookie ipc;
 	struct rtable *rt = NULL;
 	struct flowi4 fl4;
-	u8 tos, scope;
+	u8 scope;
 	int free = 0;
 	__be32 daddr;
 	__be32 saddr;
@@ -581,7 +581,6 @@  static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 			daddr = ipc.opt->opt.faddr;
 		}
 	}
-	tos = get_rttos(&ipc, inet);
 	scope = ip_sendmsg_scope(inet, &ipc, msg);
 
 	uc_index = READ_ONCE(inet->uc_index);
@@ -606,7 +605,8 @@  static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		}
 	}
 
-	flowi4_init_output(&fl4, ipc.oif, ipc.sockc.mark, tos, scope,
+	flowi4_init_output(&fl4, ipc.oif, ipc.sockc.mark,
+			   ipc.tos & INET_DSCP_MASK, scope,
 			   hdrincl ? ipc.protocol : sk->sk_protocol,
 			   inet_sk_flowi_flags(sk) |
 			    (hdrincl ? FLOWI_FLAG_KNOWN_NH : 0),
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index a9bb9ce5438e..65519b1a1e67 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1281,7 +1281,7 @@  int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	int free = 0;
 	int connected = 0;
 	__be32 daddr, faddr, saddr;
-	u8 tos, scope;
+	u8 scope;
 	__be16 dport;
 	int err, is_udplite = IS_UDPLITE(sk);
 	int corkreq = udp_test_bit(CORK, sk) || msg->msg_flags & MSG_MORE;
@@ -1405,7 +1405,6 @@  int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		faddr = ipc.opt->opt.faddr;
 		connected = 0;
 	}
-	tos = get_rttos(&ipc, inet);
 	scope = ip_sendmsg_scope(inet, &ipc, msg);
 	if (scope == RT_SCOPE_LINK)
 		connected = 0;
@@ -1442,7 +1441,8 @@  int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 		fl4 = &fl4_stack;
 
-		flowi4_init_output(fl4, ipc.oif, ipc.sockc.mark, tos, scope,
+		flowi4_init_output(fl4, ipc.oif, ipc.sockc.mark,
+				   ipc.tos & INET_DSCP_MASK, scope,
 				   sk->sk_protocol, flow_flags, faddr, saddr,
 				   dport, inet->inet_sport, sk->sk_uid);