[PATCH net-next v2 0/7] net: deduplicate cookie logic

[PATCH net-next v2 0/7] net: deduplicate cookie logic

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Reuse standard sk, ip and ipv6 cookie init handlers where possible.

Avoid repeated open coding of the same logic.
Harmonize feature sets across protocols.
Make IPv4 and IPv6 logic more alike.
Simplify adding future new fields with a single init point.

v1->v2:
  - limit INET_DSCP_MASK to routing
  - remove no longer used local variable (fix build warning)

Willem de Bruijn (7):
  tcp: only initialize sockcm tsflags field
  net: initialize mark in sockcm_init
  ipv4: initialize inet socket cookies with sockcm_init
  ipv4: remove get_rttos
  icmp: reflect tos through ip cookie rather than updating inet_sk
  ipv6: replace ipcm6_init calls with ipcm6_init_sk
  ipv6: initialize inet socket cookies with sockcm_init

 include/net/ip.h       | 16 +++++-----------
 include/net/ipv6.h     | 11 ++---------
 include/net/sock.h     |  1 +
 net/can/raw.c          |  2 +-
 net/ipv4/icmp.c        |  6 ++----
 net/ipv4/ping.c        |  6 +++---
 net/ipv4/raw.c         |  6 +++---
 net/ipv4/tcp.c         |  2 +-
 net/ipv4/udp.c         |  6 +++---
 net/ipv6/ping.c        |  3 ---
 net/ipv6/raw.c         | 15 +++------------
 net/ipv6/udp.c         | 10 +---------
 net/l2tp/l2tp_ip6.c    |  8 +-------
 net/packet/af_packet.c |  9 ++++-----
 14 files changed, 30 insertions(+), 71 deletions(-)

-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH net-next v2 1/7] tcp: only initialize sockcm tsflags field

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

TCP only reads the tsflags field. Don't bother initializing others.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/ipv4/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 992d5c9b2487..5798a900b7bf 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1127,7 +1127,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 		/* 'common' sending to sendq */
 	}
 
-	sockcm_init(&sockc, sk);
+	sockc = (struct sockcm_cookie) { .tsflags = READ_ONCE(sk->sk_tsflags)};
 	if (msg->msg_controllen) {
 		err = sock_cmsg_send(sk, msg, &sockc);
 		if (unlikely(err)) {
-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH net-next v2 2/7] net: initialize mark in sockcm_init

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Avoid open coding initialization of sockcm fields.
Avoid reading the sk_priority field twice.

This ensures all callers, existing and future, will correctly try a
cmsg passed mark before sk_mark.

This patch extends support for cmsg mark to:
packet_spkt and packet_tpacket and net/can/raw.c.

This patch extends support for cmsg priority to:
packet_spkt and packet_tpacket.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 include/net/sock.h     | 1 +
 net/can/raw.c          | 2 +-
 net/packet/af_packet.c | 9 ++++-----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 8036b3b79cd8..767a60e80086 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1828,6 +1828,7 @@ static inline void sockcm_init(struct sockcm_cookie *sockc,
 			       const struct sock *sk)
 {
 	*sockc = (struct sockcm_cookie) {
+		.mark = READ_ONCE(sk->sk_mark),
 		.tsflags = READ_ONCE(sk->sk_tsflags),
 		.priority = READ_ONCE(sk->sk_priority),
 	};
diff --git a/net/can/raw.c b/net/can/raw.c
index 46e8ed9d64da..9b1d5f036f57 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -963,7 +963,7 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 	skb->dev = dev;
 	skb->priority = sockc.priority;
-	skb->mark = READ_ONCE(sk->sk_mark);
+	skb->mark = sockc.mark;
 	skb->tstamp = sockc.transmit_time;
 
 	skb_setup_tx_timestamp(skb, &sockc);
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c131e5ceea37..3e9ddf72cd03 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2102,8 +2102,8 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
 
 	skb->protocol = proto;
 	skb->dev = dev;
-	skb->priority = READ_ONCE(sk->sk_priority);
-	skb->mark = READ_ONCE(sk->sk_mark);
+	skb->priority = sockc.priority;
+	skb->mark = sockc.mark;
 	skb_set_delivery_type_by_clockid(skb, sockc.transmit_time, sk->sk_clockid);
 	skb_setup_tx_timestamp(skb, &sockc);
 
@@ -2634,8 +2634,8 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 
 	skb->protocol = proto;
 	skb->dev = dev;
-	skb->priority = READ_ONCE(po->sk.sk_priority);
-	skb->mark = READ_ONCE(po->sk.sk_mark);
+	skb->priority = sockc->priority;
+	skb->mark = sockc->mark;
 	skb_set_delivery_type_by_clockid(skb, sockc->transmit_time, po->sk.sk_clockid);
 	skb_setup_tx_timestamp(skb, sockc);
 	skb_zcopy_set_nouarg(skb, ph.raw);
@@ -3039,7 +3039,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		goto out_unlock;
 
 	sockcm_init(&sockc, sk);
-	sockc.mark = READ_ONCE(sk->sk_mark);
 	if (msg->msg_controllen) {
 		err = sock_cmsg_send(sk, msg, &sockc);
 		if (unlikely(err))
-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH net-next v2 3/7] ipv4: initialize inet socket cookies with sockcm_init

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Avoid open coding the same logic.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 include/net/ip.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 9f5e33e371fc..6af16545b3e3 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -94,9 +94,8 @@ static inline void ipcm_init_sk(struct ipcm_cookie *ipcm,
 {
 	ipcm_init(ipcm);
 
-	ipcm->sockc.mark = READ_ONCE(inet->sk.sk_mark);
-	ipcm->sockc.priority = READ_ONCE(inet->sk.sk_priority);
-	ipcm->sockc.tsflags = READ_ONCE(inet->sk.sk_tsflags);
+	sockcm_init(&ipcm->sockc, &inet->sk);
+
 	ipcm->oif = READ_ONCE(inet->sk.sk_bound_dev_if);
 	ipcm->addr = inet->inet_saddr;
 	ipcm->protocol = inet->inet_num;
-- 
2.48.1.502.g6dc24dfdaf-goog

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

[Willem de Bruijn]

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

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);
 
-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH net-next v2 5/7] icmp: reflect tos through ip cookie rather than updating inet_sk

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Do not modify socket fields if it can be avoided.

The current code predates the introduction of ip cookies in commit
aa6615814533 ("ipv4: processing ancillary IP_TOS or IP_TTL"). Now that
cookies exist and support tos, update that field directly.

v1->v2:
  - remove no longer used local variable inet

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/ipv4/icmp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 094084b61bff..c42030fb4ff6 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -405,7 +405,6 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 	bool apply_ratelimit = false;
 	struct flowi4 fl4;
 	struct sock *sk;
-	struct inet_sock *inet;
 	__be32 daddr, saddr;
 	u32 mark = IP4_REPLY_MARK(net, skb->mark);
 	int type = icmp_param->data.icmph.type;
@@ -424,12 +423,11 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 	sk = icmp_xmit_lock(net);
 	if (!sk)
 		goto out_bh_enable;
-	inet = inet_sk(sk);
 
 	icmp_param->data.icmph.checksum = 0;
 
 	ipcm_init(&ipc);
-	inet->tos = ip_hdr(skb)->tos;
+	ipc.tos = ip_hdr(skb)->tos;
 	ipc.sockc.mark = mark;
 	daddr = ipc.addr = ip_hdr(skb)->saddr;
 	saddr = fib_compute_spec_dst(skb);
@@ -735,8 +733,8 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
 	icmp_param.data.icmph.checksum	 = 0;
 	icmp_param.skb	  = skb_in;
 	icmp_param.offset = skb_network_offset(skb_in);
-	inet_sk(sk)->tos = tos;
 	ipcm_init(&ipc);
+	ipc.tos = tos;
 	ipc.addr = iph->saddr;
 	ipc.opt = &icmp_param.replyopts.opt;
 	ipc.sockc.mark = mark;
-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH net-next v2 6/7] ipv6: replace ipcm6_init calls with ipcm6_init_sk

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

This initializes tclass and dontfrag before cmsg parsing, removing the
need for explicit checks against -1 in each caller.

Leave hlimit set to -1, because its full initialization
(in ip6_sk_dst_hoplimit) requires more state (dst, flowi6, ..).

This also prepares for calling sockcm_init in a follow-on patch.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 include/net/ipv6.h  | 9 ---------
 net/ipv6/raw.c      | 8 +-------
 net/ipv6/udp.c      | 7 +------
 net/l2tp/l2tp_ip6.c | 8 +-------
 4 files changed, 3 insertions(+), 29 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index f5c43ad1565e..46a679d9b334 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -363,15 +363,6 @@ struct ipcm6_cookie {
 	struct ipv6_txoptions *opt;
 };
 
-static inline void ipcm6_init(struct ipcm6_cookie *ipc6)
-{
-	*ipc6 = (struct ipcm6_cookie) {
-		.hlimit = -1,
-		.tclass = -1,
-		.dontfrag = -1,
-	};
-}
-
 static inline void ipcm6_init_sk(struct ipcm6_cookie *ipc6,
 				 const struct sock *sk)
 {
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index a45aba090aa4..ae68d3f7dd32 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -777,7 +777,7 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	fl6.flowi6_mark = READ_ONCE(sk->sk_mark);
 	fl6.flowi6_uid = sk->sk_uid;
 
-	ipcm6_init(&ipc6);
+	ipcm6_init_sk(&ipc6, sk);
 	ipc6.sockc.tsflags = READ_ONCE(sk->sk_tsflags);
 	ipc6.sockc.mark = fl6.flowi6_mark;
 	ipc6.sockc.priority = READ_ONCE(sk->sk_priority);
@@ -891,9 +891,6 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	if (hdrincl)
 		fl6.flowi6_flags |= FLOWI_FLAG_KNOWN_NH;
 
-	if (ipc6.tclass < 0)
-		ipc6.tclass = np->tclass;
-
 	fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
 
 	dst = ip6_dst_lookup_flow(sock_net(sk), sk, &fl6, final_p);
@@ -904,9 +901,6 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	if (ipc6.hlimit < 0)
 		ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst);
 
-	if (ipc6.dontfrag < 0)
-		ipc6.dontfrag = inet6_test_bit(DONTFRAG, sk);
-
 	if (msg->msg_flags&MSG_CONFIRM)
 		goto do_confirm;
 
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index c6ea438b5c75..7096b7e84c10 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1494,7 +1494,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	int is_udplite = IS_UDPLITE(sk);
 	int (*getfrag)(void *, char *, int, int, int, struct sk_buff *);
 
-	ipcm6_init(&ipc6);
+	ipcm6_init_sk(&ipc6, sk);
 	ipc6.gso_size = READ_ONCE(up->gso_size);
 	ipc6.sockc.tsflags = READ_ONCE(sk->sk_tsflags);
 	ipc6.sockc.mark = READ_ONCE(sk->sk_mark);
@@ -1704,9 +1704,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	security_sk_classify_flow(sk, flowi6_to_flowi_common(fl6));
 
-	if (ipc6.tclass < 0)
-		ipc6.tclass = np->tclass;
-
 	fl6->flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6->flowlabel);
 
 	dst = ip6_sk_dst_lookup_flow(sk, fl6, final_p, connected);
@@ -1752,8 +1749,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	WRITE_ONCE(up->pending, AF_INET6);
 
 do_append_data:
-	if (ipc6.dontfrag < 0)
-		ipc6.dontfrag = inet6_test_bit(DONTFRAG, sk);
 	up->len += ulen;
 	err = ip6_append_data(sk, getfrag, msg, ulen, sizeof(struct udphdr),
 			      &ipc6, fl6, dst_rt6_info(dst),
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index f4c1da070826..b98d13584c81 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -547,7 +547,7 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	fl6.flowi6_mark = READ_ONCE(sk->sk_mark);
 	fl6.flowi6_uid = sk->sk_uid;
 
-	ipcm6_init(&ipc6);
+	ipcm6_init_sk(&ipc6, sk);
 
 	if (lsa) {
 		if (addr_len < SIN6_LEN_RFC2133)
@@ -634,9 +634,6 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	security_sk_classify_flow(sk, flowi6_to_flowi_common(&fl6));
 
-	if (ipc6.tclass < 0)
-		ipc6.tclass = np->tclass;
-
 	fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
 
 	dst = ip6_dst_lookup_flow(sock_net(sk), sk, &fl6, final_p);
@@ -648,9 +645,6 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	if (ipc6.hlimit < 0)
 		ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, dst);
 
-	if (ipc6.dontfrag < 0)
-		ipc6.dontfrag = inet6_test_bit(DONTFRAG, sk);
-
 	if (msg->msg_flags & MSG_CONFIRM)
 		goto do_confirm;
 
-- 
2.48.1.502.g6dc24dfdaf-goog

[PATCH net-next v2 7/7] ipv6: initialize inet socket cookies with sockcm_init

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Avoid open coding the same logic.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 include/net/ipv6.h | 2 ++
 net/ipv6/ping.c    | 3 ---
 net/ipv6/raw.c     | 9 +++------
 net/ipv6/udp.c     | 3 ---
 4 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 46a679d9b334..9614006f483c 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -371,6 +371,8 @@ static inline void ipcm6_init_sk(struct ipcm6_cookie *ipc6,
 		.tclass = inet6_sk(sk)->tclass,
 		.dontfrag = inet6_test_bit(DONTFRAG, sk),
 	};
+
+	sockcm_init(&ipc6->sockc, sk);
 }
 
 static inline struct ipv6_txoptions *txopt_get(const struct ipv6_pinfo *np)
diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
index 46b8adf6e7f8..84d90dd8b3f0 100644
--- a/net/ipv6/ping.c
+++ b/net/ipv6/ping.c
@@ -119,9 +119,6 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		return -EINVAL;
 
 	ipcm6_init_sk(&ipc6, sk);
-	ipc6.sockc.priority = READ_ONCE(sk->sk_priority);
-	ipc6.sockc.tsflags = READ_ONCE(sk->sk_tsflags);
-	ipc6.sockc.mark = READ_ONCE(sk->sk_mark);
 
 	fl6.flowi6_oif = oif;
 
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index ae68d3f7dd32..fda640ebd53f 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -769,19 +769,16 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	hdrincl = inet_test_bit(HDRINCL, sk);
 
+	ipcm6_init_sk(&ipc6, sk);
+
 	/*
 	 *	Get and verify the address.
 	 */
 	memset(&fl6, 0, sizeof(fl6));
 
-	fl6.flowi6_mark = READ_ONCE(sk->sk_mark);
+	fl6.flowi6_mark = ipc6.sockc.mark;
 	fl6.flowi6_uid = sk->sk_uid;
 
-	ipcm6_init_sk(&ipc6, sk);
-	ipc6.sockc.tsflags = READ_ONCE(sk->sk_tsflags);
-	ipc6.sockc.mark = fl6.flowi6_mark;
-	ipc6.sockc.priority = READ_ONCE(sk->sk_priority);
-
 	if (sin6) {
 		if (addr_len < SIN6_LEN_RFC2133)
 			return -EINVAL;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 7096b7e84c10..3a0d6c5a8286 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1496,9 +1496,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	ipcm6_init_sk(&ipc6, sk);
 	ipc6.gso_size = READ_ONCE(up->gso_size);
-	ipc6.sockc.tsflags = READ_ONCE(sk->sk_tsflags);
-	ipc6.sockc.mark = READ_ONCE(sk->sk_mark);
-	ipc6.sockc.priority = READ_ONCE(sk->sk_priority);
 
 	/* destination address check */
 	if (sin6) {
-- 
2.48.1.502.g6dc24dfdaf-goog

Re: [PATCH net-next v2 0/7] net: deduplicate cookie logic

[David Ahern]

On 2/11/25 7:09 PM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Reuse standard sk, ip and ipv6 cookie init handlers where possible.
> 
> Avoid repeated open coding of the same logic.
> Harmonize feature sets across protocols.
> Make IPv4 and IPv6 logic more alike.
> Simplify adding future new fields with a single init point.
> 
> v1->v2:
>   - limit INET_DSCP_MASK to routing
>   - remove no longer used local variable (fix build warning)
> 
> Willem de Bruijn (7):
>   tcp: only initialize sockcm tsflags field
>   net: initialize mark in sockcm_init
>   ipv4: initialize inet socket cookies with sockcm_init
>   ipv4: remove get_rttos
>   icmp: reflect tos through ip cookie rather than updating inet_sk
>   ipv6: replace ipcm6_init calls with ipcm6_init_sk
>   ipv6: initialize inet socket cookies with sockcm_init
> 
>  include/net/ip.h       | 16 +++++-----------
>  include/net/ipv6.h     | 11 ++---------
>  include/net/sock.h     |  1 +
>  net/can/raw.c          |  2 +-
>  net/ipv4/icmp.c        |  6 ++----
>  net/ipv4/ping.c        |  6 +++---
>  net/ipv4/raw.c         |  6 +++---
>  net/ipv4/tcp.c         |  2 +-
>  net/ipv4/udp.c         |  6 +++---
>  net/ipv6/ping.c        |  3 ---
>  net/ipv6/raw.c         | 15 +++------------
>  net/ipv6/udp.c         | 10 +---------
>  net/l2tp/l2tp_ip6.c    |  8 +-------
>  net/packet/af_packet.c |  9 ++++-----
>  14 files changed, 30 insertions(+), 71 deletions(-)
> 

LGTM. For the set

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next v2 2/7] net: initialize mark in sockcm_init

[Paolo Abeni]

On 2/12/25 3:09 AM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Avoid open coding initialization of sockcm fields.
> Avoid reading the sk_priority field twice.
> 
> This ensures all callers, existing and future, will correctly try a
> cmsg passed mark before sk_mark.
> 
> This patch extends support for cmsg mark to:
> packet_spkt and packet_tpacket and net/can/raw.c.
> 
> This patch extends support for cmsg priority to:
> packet_spkt and packet_tpacket.

I admit I'm a little bit concerned vs possibly impacting existing
applications doing weird thing like passing the relevant cmsg and
expecting it to be ignored.

Too paranoid on my side?

/P

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

[Paolo Abeni]

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

Re: [PATCH net-next v2 2/7] net: initialize mark in sockcm_init

[Willem de Bruijn]

Paolo Abeni wrote:
> On 2/12/25 3:09 AM, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> > 
> > Avoid open coding initialization of sockcm fields.
> > Avoid reading the sk_priority field twice.
> > 
> > This ensures all callers, existing and future, will correctly try a
> > cmsg passed mark before sk_mark.
> > 
> > This patch extends support for cmsg mark to:
> > packet_spkt and packet_tpacket and net/can/raw.c.
> > 
> > This patch extends support for cmsg priority to:
> > packet_spkt and packet_tpacket.
> 
> I admit I'm a little bit concerned vs possibly impacting existing
> applications doing weird thing like passing the relevant cmsg and
> expecting it to be ignored.

We have a history of expanding support for passing variables by cmsg.

These APIs are intended to be uniform across protocols, at least
across all datagram cases. Existing behavior is arbitrary and
unintentional, where a new feature was added only to the protocol
most on the developer's mind.

The goal of deduplicating is exactly to avoid more such arbitrary
limitations as new fields are added.

> Too paranoid on my side?

Not at all!

For correctness, besides code inspection for this series I also
relied on existing kselftests including cmsg_ipv6.sh and
cmsg_so_priority.sh. I added a cmsg_ipv4.sh to verify the subtle
routing point in patch 4. But that is not ready to submit yet.

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

[Willem de Bruijn]

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?

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

[Willem de Bruijn]

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.

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

[Paolo Abeni]

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

Re: [PATCH net-next v2 2/7] net: initialize mark in sockcm_init

[Paolo Abeni]

On 2/13/25 4:35 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>
>>>
>>> Avoid open coding initialization of sockcm fields.
>>> Avoid reading the sk_priority field twice.
>>>
>>> This ensures all callers, existing and future, will correctly try a
>>> cmsg passed mark before sk_mark.
>>>
>>> This patch extends support for cmsg mark to:
>>> packet_spkt and packet_tpacket and net/can/raw.c.
>>>
>>> This patch extends support for cmsg priority to:
>>> packet_spkt and packet_tpacket.
>>
>> I admit I'm a little bit concerned vs possibly impacting existing
>> applications doing weird thing like passing the relevant cmsg and
>> expecting it to be ignored.
> 
> We have a history of expanding support for passing variables by cmsg.

Ok, I should have paid more attention to other cmsg related changes.

git log says this is actually allowed, so I'm fine with the patch.

Thanks,

Paolo

Re: [PATCH net-next v2 0/7] net: deduplicate cookie logic

[Jakub Kicinski]

On Tue, 11 Feb 2025 21:09:46 -0500 Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Reuse standard sk, ip and ipv6 cookie init handlers where possible.
> 
> Avoid repeated open coding of the same logic.
> Harmonize feature sets across protocols.
> Make IPv4 and IPv6 logic more alike.
> Simplify adding future new fields with a single init point.

Sorry for noticing late, looks like this doesn't apply cleanly:

Applying: tcp: only initialize sockcm tsflags field
Applying: net: initialize mark in sockcm_init
Applying: ipv4: initialize inet socket cookies with sockcm_init
Applying: ipv4: remove get_rttos
Applying: icmp: reflect tos through ip cookie rather than updating inet_sk
Using index info to reconstruct a base tree...
M	net/ipv4/icmp.c
Falling back to patching base and 3-way merge...
Auto-merging net/ipv4/icmp.c
Applying: ipv6: replace ipcm6_init calls with ipcm6_init_sk
Applying: ipv6: initialize inet socket cookies with sockcm_init

So CI didn't consume it..

Could you rebase & repost?
-- 
pw-bot: cr

Re: [PATCH net-next v2 0/7] net: deduplicate cookie logic

[Willem de Bruijn]

Jakub Kicinski wrote:
> On Tue, 11 Feb 2025 21:09:46 -0500 Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> > 
> > Reuse standard sk, ip and ipv6 cookie init handlers where possible.
> > 
> > Avoid repeated open coding of the same logic.
> > Harmonize feature sets across protocols.
> > Make IPv4 and IPv6 logic more alike.
> > Simplify adding future new fields with a single init point.
> 
> Sorry for noticing late, looks like this doesn't apply cleanly:
> 
> Applying: tcp: only initialize sockcm tsflags field
> Applying: net: initialize mark in sockcm_init
> Applying: ipv4: initialize inet socket cookies with sockcm_init
> Applying: ipv4: remove get_rttos
> Applying: icmp: reflect tos through ip cookie rather than updating inet_sk
> Using index info to reconstruct a base tree...
> M	net/ipv4/icmp.c
> Falling back to patching base and 3-way merge...
> Auto-merging net/ipv4/icmp.c
> Applying: ipv6: replace ipcm6_init calls with ipcm6_init_sk
> Applying: ipv6: initialize inet socket cookies with sockcm_init
> 
> So CI didn't consume it..
> 
> Could you rebase & repost?

Done. Sorry about that. Not sure what went wrong. I had v2 on top of
netdev-nn/main as of Feb 11, commit ae9b3c0e79bc ("Merge branch
'tcp-allow-to-reduce-max-rto'").

Now rebased on top of current head at commit 412723d54a8b ("Merge
branch 'net-phylink-xpcs-stmmac-support-pcs-eee-configuration'").

Good point that I could have noticed that something was up by looking
at the CI. Will keep that in mind.