can: gw: fix RCU/BH usage in cgw_create_job()

RFC PATCH In review Oliver Hartkopp 2 participants 5 messages Dec 21, 2023 – Apr 29, 2024

Subsystems (2)

CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2023-12-21 12:40 UTC #0 lore raw

RFC PATCH 1/1 can: gw: fix RCU/BH usage in cgw_create_job()

As reported by Sebastian Andrzej Siewior the use of local_bh_disable() is only feasible in uni processor systems to update the modification rules. The usual use-case to update the modification rules is to update the data of the modifications but not the modification types (AND/OR/XOR/SET) or the checksum functions itself.

To omit additional memory allocations to maintain fast modification switching times, the modification description space is doubled at gw-job creation time so that only the reference to the active modification desciption is changed under rcu protection.

Link https://lore.kernel.org/linux-can/[email protected]/ open here

Fixes dd895d7f21b2 ("can: cangw: introduce optional uid to reference created routing jobs")

Reported-by Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/gw.c | 99 ++++++++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 46 deletions(-)

net/can/gw.c +53 ~~-46~~

index 37528826935e..6411f1c60f48 100644
@@ -128,11 +128,13 @@ struct cgw_job {
 	struct hlist_node list;
 	struct rcu_head rcu;
 	u32 handled_frames;
 	u32 dropped_frames;
 	u32 deleted_frames;
-	struct cf_mod mod;
+	struct cf_mod *mod;
+	struct cf_mod mod1;
+	struct cf_mod mod2;
 	union {
 		/* CAN frame data source */
 		struct net_device *dev;
 	} src;
 	union {

@@ -504,11 +506,11 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 	/* clone the given skb, which has not been done in can_rcv()
 	 *
 	 * When there is at least one modification function activated,
 	 * we need to copy the skb as we want to modify skb->data.
 	 */
-	if (gwj->mod.modfunc[0])
+	if (gwj->mod->modfunc[0])
 		nskb = skb_copy(skb, GFP_ATOMIC);
 	else
 		nskb = skb_clone(skb, GFP_ATOMIC);
 
 	if (!nskb) {

@@ -527,12 +529,12 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 
 	/* pointer to modifiable CAN frame */
 	cf = (struct canfd_frame *)nskb->data;
 
 	/* perform preprocessed modification functions if there are any */
-	while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
-		(*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);
+	while (modidx < MAX_MODFUNCTIONS && gwj->mod->modfunc[modidx])
+		(*gwj->mod->modfunc[modidx++])(cf, gwj->mod);
 
 	/* Has the CAN frame been modified? */
 	if (modidx) {
 		/* get available space for the processed CAN frame type */
 		int max_len = nskb->len - offsetof(struct canfd_frame, data);

@@ -544,15 +546,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 			kfree_skb(nskb);
 			return;
 		}
 
 		/* check for checksum updates */
-		if (gwj->mod.csumfunc.crc8)
-			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
+		if (gwj->mod->csumfunc.crc8)
+			(*gwj->mod->csumfunc.crc8)(cf, &gwj->mod->csum.crc8);
 
-		if (gwj->mod.csumfunc.xor)
-			(*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor);
+		if (gwj->mod->csumfunc.xor)
+			(*gwj->mod->csumfunc.xor)(cf, &gwj->mod->csum.xor);
 	}
 
 	/* clear the skb timestamp if not configured the other way */
 	if (!(gwj->flags & CGW_FLAGS_CAN_SRC_TSTAMP))
 		nskb->tstamp = 0;

@@ -651,83 +653,83 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
 	}
 
 	if (gwj->flags & CGW_FLAGS_CAN_FD) {
 		struct cgw_fdframe_mod mb;
 
-		if (gwj->mod.modtype.and) {
-			memcpy(&mb.cf, &gwj->mod.modframe.and, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.and;
+		if (gwj->mod->modtype.and) {
+			memcpy(&mb.cf, &gwj->mod->modframe.and, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.and;
 			if (nla_put(skb, CGW_FDMOD_AND, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 
-		if (gwj->mod.modtype.or) {
-			memcpy(&mb.cf, &gwj->mod.modframe.or, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.or;
+		if (gwj->mod->modtype.or) {
+			memcpy(&mb.cf, &gwj->mod->modframe.or, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.or;
 			if (nla_put(skb, CGW_FDMOD_OR, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 
-		if (gwj->mod.modtype.xor) {
-			memcpy(&mb.cf, &gwj->mod.modframe.xor, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.xor;
+		if (gwj->mod->modtype.xor) {
+			memcpy(&mb.cf, &gwj->mod->modframe.xor, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.xor;
 			if (nla_put(skb, CGW_FDMOD_XOR, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 
-		if (gwj->mod.modtype.set) {
-			memcpy(&mb.cf, &gwj->mod.modframe.set, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.set;
+		if (gwj->mod->modtype.set) {
+			memcpy(&mb.cf, &gwj->mod->modframe.set, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.set;
 			if (nla_put(skb, CGW_FDMOD_SET, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 	} else {
 		struct cgw_frame_mod mb;
 
-		if (gwj->mod.modtype.and) {
-			memcpy(&mb.cf, &gwj->mod.modframe.and, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.and;
+		if (gwj->mod->modtype.and) {
+			memcpy(&mb.cf, &gwj->mod->modframe.and, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.and;
 			if (nla_put(skb, CGW_MOD_AND, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 
-		if (gwj->mod.modtype.or) {
-			memcpy(&mb.cf, &gwj->mod.modframe.or, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.or;
+		if (gwj->mod->modtype.or) {
+			memcpy(&mb.cf, &gwj->mod->modframe.or, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.or;
 			if (nla_put(skb, CGW_MOD_OR, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 
-		if (gwj->mod.modtype.xor) {
-			memcpy(&mb.cf, &gwj->mod.modframe.xor, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.xor;
+		if (gwj->mod->modtype.xor) {
+			memcpy(&mb.cf, &gwj->mod->modframe.xor, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.xor;
 			if (nla_put(skb, CGW_MOD_XOR, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 
-		if (gwj->mod.modtype.set) {
-			memcpy(&mb.cf, &gwj->mod.modframe.set, sizeof(mb.cf));
-			mb.modtype = gwj->mod.modtype.set;
+		if (gwj->mod->modtype.set) {
+			memcpy(&mb.cf, &gwj->mod->modframe.set, sizeof(mb.cf));
+			mb.modtype = gwj->mod->modtype.set;
 			if (nla_put(skb, CGW_MOD_SET, sizeof(mb), &mb) < 0)
 				goto cancel;
 		}
 	}
 
-	if (gwj->mod.uid) {
-		if (nla_put_u32(skb, CGW_MOD_UID, gwj->mod.uid) < 0)
+	if (gwj->mod->uid) {
+		if (nla_put_u32(skb, CGW_MOD_UID, gwj->mod->uid) < 0)
 			goto cancel;
 	}
 
-	if (gwj->mod.csumfunc.crc8) {
+	if (gwj->mod->csumfunc.crc8) {
 		if (nla_put(skb, CGW_CS_CRC8, CGW_CS_CRC8_LEN,
-			    &gwj->mod.csum.crc8) < 0)
+			    &gwj->mod->csum.crc8) < 0)
 			goto cancel;
 	}
 
-	if (gwj->mod.csumfunc.xor) {
+	if (gwj->mod->csumfunc.xor) {
 		if (nla_put(skb, CGW_CS_XOR, CGW_CS_XOR_LEN,
-			    &gwj->mod.csum.xor) < 0)
+			    &gwj->mod->csum.xor) < 0)
 			goto cancel;
 	}
 
 	if (gwj->gwtype == CGW_TYPE_CAN_CAN) {
 		if (gwj->ccgw.filter.can_id || gwj->ccgw.filter.can_mask) {

@@ -1085,21 +1087,25 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	if (mod.uid) {
 		ASSERT_RTNL();
 
 		/* check for updating an existing job with identical uid */
 		hlist_for_each_entry(gwj, &net->can.cgw_list, list) {
-			if (gwj->mod.uid != mod.uid)
+			if (gwj->mod->uid != mod.uid)
 				continue;
 
 			/* interfaces & filters must be identical */
 			if (memcmp(&gwj->ccgw, &ccgw, sizeof(ccgw)))
 				return -EINVAL;
 
 			/* update modifications with disabled softirq & quit */
-			local_bh_disable();
-			memcpy(&gwj->mod, &mod, sizeof(mod));
-			local_bh_enable();
+			if (gwj->mod == &gwj->mod1) {
+				memcpy(&gwj->mod2, &mod, sizeof(mod));
+				rcu_replace_pointer(gwj->mod, &gwj->mod2, true);
+			} else {
+				memcpy(&gwj->mod1, &mod, sizeof(mod));
+				rcu_replace_pointer(gwj->mod, &gwj->mod1, true);
+			}
 			return 0;
 		}
 	}
 
 	/* ifindex == 0 is not allowed for job creation */

@@ -1114,13 +1120,14 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	gwj->dropped_frames = 0;
 	gwj->deleted_frames = 0;
 	gwj->flags = r->flags;
 	gwj->gwtype = r->gwtype;
 	gwj->limit_hops = limhops;
+	gwj->mod = &gwj->mod1;
 
 	/* insert already parsed information */
-	memcpy(&gwj->mod, &mod, sizeof(mod));
+	memcpy(gwj->mod, &mod, sizeof(mod));
 	memcpy(&gwj->ccgw, &ccgw, sizeof(ccgw));
 
 	err = -ENODEV;
 
 	gwj->src.dev = __dev_get_by_index(net, gwj->ccgw.src_idx);

@@ -1219,16 +1226,16 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 		if (gwj->limit_hops != limhops)
 			continue;
 
 		/* we have a match when uid is enabled and identical */
-		if (gwj->mod.uid || mod.uid) {
-			if (gwj->mod.uid != mod.uid)
+		if (gwj->mod->uid || mod.uid) {
+			if (gwj->mod->uid != mod.uid)
 				continue;
 		} else {
 			/* no uid => check for identical modifications */
-			if (memcmp(&gwj->mod, &mod, sizeof(mod)))
+			if (memcmp(gwj->mod, &mod, sizeof(mod)))
 				continue;
 		}
 
 		/* if (r->gwtype == CGW_TYPE_CAN_CAN) - is made sure here */
 		if (memcmp(&gwj->ccgw, &ccgw, sizeof(ccgw)))

Signature · 1 line

2.39.2

Sebastian Andrzej Siewior

2024-01-11 12:14 UTC #1 lore raw

net/can/gw.c struct cgw_job { · line 135

Quoted 12 lines · Oliver Hartkopp

net/can/gw.c +3 -1

@@ -128,11 +128,13 @@ struct cgw_job {
 	struct hlist_node list;
 	struct rcu_head rcu;
 	u32 handled_frames;
 	u32 dropped_frames;
 	u32 deleted_frames;
-	struct cf_mod mod;
+	struct cf_mod *mod;
+	struct cf_mod mod1;
+	struct cf_mod mod2;

I wouldn't put here mod1/2. That cf_mod struct has 736 bytes in my allmod build so it gains some weight. It also introduces a run-time problem, more on that later.
Also *mod requires __rcu annotation.

net/can/gw.c static void can_can_gw_rcv(struct sk_buff *skb, void *data) · line 511

Quoted 8 lines

net/can/gw.c +1 -1

@@ -504,11 +506,11 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 	/* clone the given skb, which has not been done in can_rcv()
 	 *
 	 * When there is at least one modification function activated,
 	 * we need to copy the skb as we want to modify skb->data.
 	 */
-	if (gwj->mod.modfunc[0])
+	if (gwj->mod->modfunc[0])

Almost. You need

	struct cf_mod *mod;
	…

	rcu_read_lock();
	mod = rcu_dereference(gwj->mod);
	if (mod->modfunc[0])
	…

	rcu_read_unlock();
	/* no longer touching mod but I am leaving can_can_gw_rcv()
	 * anyway */

Quoted 20 lines

net/can/gw.c +1 -1

 		nskb = skb_copy(skb, GFP_ATOMIC);
 	else
 		nskb = skb_clone(skb, GFP_ATOMIC);
 
 	if (!nskb) {

@@ -1085,21 +1087,25 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	if (mod.uid) {
 		ASSERT_RTNL();
 
 		/* check for updating an existing job with identical uid */
 		hlist_for_each_entry(gwj, &net->can.cgw_list, list) {
-			if (gwj->mod.uid != mod.uid)
+			if (gwj->mod->uid != mod.uid)
 				continue;
 
 			/* interfaces & filters must be identical */
 			if (memcmp(&gwj->ccgw, &ccgw, sizeof(ccgw)))
 				return -EINVAL;
 
 			/* update modifications with disabled softirq & quit */

net/can/gw.c static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh, · line 1106

Quoted 10 lines

net/can/gw.c +7 -3

-			local_bh_disable();
-			memcpy(&gwj->mod, &mod, sizeof(mod));
-			local_bh_enable();
+			if (gwj->mod == &gwj->mod1) {
+				memcpy(&gwj->mod2, &mod, sizeof(mod));
+				rcu_replace_pointer(gwj->mod, &gwj->mod2, true);
+			} else {
+				memcpy(&gwj->mod1, &mod, sizeof(mod));
+				rcu_replace_pointer(gwj->mod, &gwj->mod1, true);
+			}

Why are you afraid of doing

	mod = kmalloc(sizeof(*mod), GFP_KERNEL);

before invoking cgw_parse_attr()?

Let me try to sell this to you:
- Your stack usage shrinks by 736 bytes (as per previous estimation)
- If this is a new one, you attach the pointer to the struct cgw_job so it is not lost.
- If this is an update, you avoid memcpy() and use rcu_replace_pointer() as it is intended.

The construct of yours has the problem that it does not wait until the RCU grace period completes. Meaning on first update mod1 is used, you switch over to mod2. On the second update mod2 is used and you switch back to mod1. There is no guarantee that all current users of mod -> mod1 are gone by the time you switch from mod2 back to mod1. This of course requires two updates (one after the other).

If you allocate the struct on entry (assuming the salesmen me was successful) you could use

	old_mod = rcu_replace_pointer(gwj->mod, new_mod, lockdep_rtnl_is_held());

to grab the about to be replaced cf_mod and once the replacement is over and at the end, kfree_rcu() on it or

	kfree_rcu_mightsleep(old_mod);

to safe a RCU head.

Quoted 6 lines

net/can/gw.c +0 -0

 			return 0;
 		}
 	}
 
 	/* ifindex == 0 is not allowed for job creation */

net/can/gw.c static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh, · line 1233

Quoted 10 lines

net/can/gw.c +2 -2

@@ -1219,16 +1226,16 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 		if (gwj->limit_hops != limhops)
 			continue;
 
 		/* we have a match when uid is enabled and identical */
-		if (gwj->mod.uid || mod.uid) {
-			if (gwj->mod.uid != mod.uid)
+		if (gwj->mod->uid || mod.uid) {
+			if (gwj->mod->uid != mod.uid)
 				continue;

Also here you must use rcu_dereference() for gwj->mod. Since all add/remove jobs happen under the RTNL lock you could use

	rcu_dereference_protected(, lockdep_rtnl_is_held());

Sebastian

Oliver Hartkopp

author maintainer

2024-01-21 10:07 UTC #2 lore raw

Quoted 4 lines · Sebastian Andrzej Siewior

Why are you afraid of doing
	mod = kmalloc(sizeof(*mod), GFP_KERNEL);
before invoking cgw_parse_attr()?

The update of the modification content should be performed instantly and without any potential scheduling from kmalloc().

As you pointed out one of the problems may arise from changing the modification functions but not from changing the modification content.

So what about the below patch then?

Would a spin_lock() or spin_lock_bh() be an alternative to lock this update against the modification execution in can_can_gw_rcv()?

Best regards,
Oliver

net/can/gw.c +9 -3

index 37528826935e..79aeb380b66a 100644
@@ -1085,21 +1085,27 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (mod.uid) {
 		ASSERT_RTNL();
 
 		/* check for updating an existing job with identical uid */
 		hlist_for_each_entry(gwj, &net->can.cgw_list, list) {
+			int mi;
+
 			if (gwj->mod.uid != mod.uid)
 				continue;
 
 			/* interfaces & filters must be identical */
 			if (memcmp(&gwj->ccgw, &ccgw, sizeof(ccgw)))
 				return -EINVAL;
 
-			/* update modifications with disabled softirq & quit */
-			local_bh_disable();
+			/* modification functions must be identical */
+			for (mi = 0; mi < MAX_MODFUNCTIONS; mi++) {
+				if (gwj->mod.modfunc[mi] != mod.modfunc[mi])
+					return -EINVAL;
+			}
+
+			/* update only the modification content & quit */
 			memcpy(&gwj->mod, &mod, sizeof(mod));
-			local_bh_enable();
 			return 0;
 		}
 	}
 
 	/* ifindex == 0 is not allowed for job creation */

Sebastian Andrzej Siewior

2024-01-22 10:10 UTC #3 lore raw

Quoted 9 lines · Oliver Hartkopp

Quoted 4 lines · Sebastian Andrzej Siewior
Why are you afraid of doing
	mod = kmalloc(sizeof(*mod), GFP_KERNEL);
before invoking cgw_parse_attr()?
The update of the modification content should be performed instantly and without any potential scheduling from kmalloc().

In most cases, the memory would be acquired without a delay. But if the memory is tight it will get delayed over returning a NULL pointer and (depending on the size) the NULL pointer can still be returned. This code path is preemptible so the scheduler _can_ preempt the code in favour of another task.

Quoted 4 lines

As you pointed out one of the problems may arise from changing the modification functions but not from changing the modification content.
So what about the below patch then?

Well, you do change parts of the data structure while the other side can read it so I would say no. Also, in general there is no guarantee if the memcpy() implementation copies the data from front or the end.

Quoted 2 lines

Would a spin_lock() or spin_lock_bh() be an alternative to lock this update against the modification execution in can_can_gw_rcv()?

Yes, that is what locks are for. You would have to put the lock outside of ccgw and acquire it before reading or writting ccgw.

Quoted 2 lines

Best regards,
Oliver

Sebastian

Sebastian Andrzej Siewior

2024-04-29 09:47 UTC #4 lore raw

Quoted 5 lines · To Oliver Hartkopp

Quoted 2 lines
Would a spin_lock() or spin_lock_bh() be an alternative to lock this update against the modification execution in can_can_gw_rcv()?
Yes, that is what locks are for. You would have to put the lock outside of ccgw and acquire it before reading or writting ccgw.

Hi :)

Quoted 2 lines

Quoted 2 lines
Best regards,
Oliver

Sebastian