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

Commit Message

Willem de Bruijn Feb. 12, 2025, 2:09 a.m. UTC

From: Willem de Bruijn <[e-mail]>

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 <[e-mail]>
---
 include/net/sock.h     | 1 +
 net/can/raw.c          | 2 +-
 net/packet/af_packet.c | 9 ++++-----
 3 files changed, 6 insertions(+), 6 deletions(-)

Comments

Paolo Abeni Feb. 13, 2025, 2:48 p.m. UTC | #1

On 2/12/25 3:09 AM, Willem de Bruijn wrote:
> From: Willem de Bruijn <[e-mail]>
> 
> 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

Willem de Bruijn Feb. 13, 2025, 3:35 p.m. UTC | #2

Paolo Abeni wrote:
> On 2/12/25 3:09 AM, Willem de Bruijn wrote:
> > From: Willem de Bruijn <[e-mail]>
> > 
> > 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.

Paolo Abeni Feb. 13, 2025, 6:12 p.m. UTC | #3

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 <[e-mail]>
>>>
>>> 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

Patch

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