Commit 511e64e1 authored by Oliver Hartkopp's avatar Oliver Hartkopp Committed by Marc Kleine-Budde
Browse files

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
description is changed under rcu protection.

Rename cgw_job::mod to cf_mod and make it a RCU pointer. Allocate in
cgw_create_job() and free it together with cgw_job in
cgw_job_free_rcu(). Update all users to dereference cgw_job::cf_mod with
a RCU accessor and if possible once.

[bigeasy: Replace mod1/mod2 from the Oliver's original patch with dynamic
allocation, use RCU annotation and accessor]

Reported-by: default avatarSebastian Andrzej Siewior <bigeasy@linutronix.de>
Closes: https://lore.kernel.org/linux-can/20231031112349.y0aLoBrz@linutronix.de/


Fixes: dd895d7f ("can: cangw: introduce optional uid to reference created routing jobs")
Tested-by: default avatarOliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: default avatarOliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: default avatarSebastian Andrzej Siewior <bigeasy@linutronix.de>
Link: https://patch.msgid.link/20250429070555.cs-7b_eZ@linutronix.de


Signed-off-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
parent f695e308

net/can/gw.c

+90 −59
Original line number Diff line number Diff line
@@ -130,7 +130,7 @@ struct cgw_job { 
	u32 handled_frames;

	u32 dropped_frames;

	u32 deleted_frames;

	struct cf_mod mod;

	struct cf_mod __rcu *cf_mod;

	union {

		/* CAN frame data source */

		struct net_device *dev;
@@ -459,6 +459,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) 
	struct cgw_job *gwj = (struct cgw_job *)data;

	struct canfd_frame *cf;

	struct sk_buff *nskb;

	struct cf_mod *mod;

	int modidx = 0;



	/* process strictly Classic CAN or CAN FD frames */
@@ -506,7 +507,8 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) 
	 * 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])

	mod = rcu_dereference(gwj->cf_mod);

	if (mod->modfunc[0])

		nskb = skb_copy(skb, GFP_ATOMIC);

	else

		nskb = skb_clone(skb, GFP_ATOMIC);
@@ -529,8 +531,8 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) 
	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 && mod->modfunc[modidx])

		(*mod->modfunc[modidx++])(cf, mod);



	/* Has the CAN frame been modified? */

	if (modidx) {
@@ -546,11 +548,11 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) 
		}



		/* check for checksum updates */

		if (gwj->mod.csumfunc.crc8)

			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);

		if (mod->csumfunc.crc8)

			(*mod->csumfunc.crc8)(cf, &mod->csum.crc8);



		if (gwj->mod.csumfunc.xor)

			(*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor);

		if (mod->csumfunc.xor)

			(*mod->csumfunc.xor)(cf, &mod->csum.xor);

	}



	/* clear the skb timestamp if not configured the other way */
@@ -581,9 +583,20 @@ static void cgw_job_free_rcu(struct rcu_head *rcu_head) 
{

	struct cgw_job *gwj = container_of(rcu_head, struct cgw_job, rcu);



	/* cgw_job::cf_mod is always accessed from the same cgw_job object within

	 * the same RCU read section. Once cgw_job is scheduled for removal,

	 * cf_mod can also be removed without mandating an additional grace period.

	 */

	kfree(rcu_access_pointer(gwj->cf_mod));

	kmem_cache_free(cgw_cache, gwj);

}



/* Return cgw_job::cf_mod with RTNL protected section */

static struct cf_mod *cgw_job_cf_mod(struct cgw_job *gwj)

{

	return rcu_dereference_protected(gwj->cf_mod, rtnl_is_locked());

}



static int cgw_notifier(struct notifier_block *nb,

			unsigned long msg, void *ptr)

{
@@ -616,6 +629,7 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type, 
{

	struct rtcanmsg *rtcan;

	struct nlmsghdr *nlh;

	struct cf_mod *mod;



	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);

	if (!nlh)
@@ -650,82 +664,83 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type, 
			goto cancel;

	}



	mod = cgw_job_cf_mod(gwj);

	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 (mod->modtype.and) {

			memcpy(&mb.cf, &mod->modframe.and, sizeof(mb.cf));

			mb.modtype = 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 (mod->modtype.or) {

			memcpy(&mb.cf, &mod->modframe.or, sizeof(mb.cf));

			mb.modtype = 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 (mod->modtype.xor) {

			memcpy(&mb.cf, &mod->modframe.xor, sizeof(mb.cf));

			mb.modtype = 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 (mod->modtype.set) {

			memcpy(&mb.cf, &mod->modframe.set, sizeof(mb.cf));

			mb.modtype = 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 (mod->modtype.and) {

			memcpy(&mb.cf, &mod->modframe.and, sizeof(mb.cf));

			mb.modtype = 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 (mod->modtype.or) {

			memcpy(&mb.cf, &mod->modframe.or, sizeof(mb.cf));

			mb.modtype = 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 (mod->modtype.xor) {

			memcpy(&mb.cf, &mod->modframe.xor, sizeof(mb.cf));

			mb.modtype = 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 (mod->modtype.set) {

			memcpy(&mb.cf, &mod->modframe.set, sizeof(mb.cf));

			mb.modtype = 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 (mod->uid) {

		if (nla_put_u32(skb, CGW_MOD_UID, mod->uid) < 0)

			goto cancel;

	}



	if (gwj->mod.csumfunc.crc8) {

	if (mod->csumfunc.crc8) {

		if (nla_put(skb, CGW_CS_CRC8, CGW_CS_CRC8_LEN,

			    &gwj->mod.csum.crc8) < 0)

			    &mod->csum.crc8) < 0)

			goto cancel;

	}



	if (gwj->mod.csumfunc.xor) {

	if (mod->csumfunc.xor) {

		if (nla_put(skb, CGW_CS_XOR, CGW_CS_XOR_LEN,

			    &gwj->mod.csum.xor) < 0)

			    &mod->csum.xor) < 0)

			goto cancel;

	}
@@ -1059,7 +1074,7 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh, 
	struct net *net = sock_net(skb->sk);

	struct rtcanmsg *r;

	struct cgw_job *gwj;

	struct cf_mod mod;

	struct cf_mod *mod;

	struct can_can_gw ccgw;

	u8 limhops = 0;

	int err = 0;
@@ -1078,37 +1093,48 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh, 
	if (r->gwtype != CGW_TYPE_CAN_CAN)

		return -EINVAL;



	err = cgw_parse_attr(nlh, &mod, CGW_TYPE_CAN_CAN, &ccgw, &limhops);

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

	if (!mod)

		return -ENOMEM;



	err = cgw_parse_attr(nlh, mod, CGW_TYPE_CAN_CAN, &ccgw, &limhops);

	if (err < 0)

		return err;

		goto out_free_cf;



	if (mod.uid) {

	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)

			struct cf_mod *old_cf;



			old_cf = cgw_job_cf_mod(gwj);

			if (old_cf->uid != mod->uid)

				continue;



			/* interfaces & filters must be identical */

			if (memcmp(&gwj->ccgw, &ccgw, sizeof(ccgw)))

				return -EINVAL;

			if (memcmp(&gwj->ccgw, &ccgw, sizeof(ccgw))) {

				err = -EINVAL;

				goto out_free_cf;

			}



			/* update modifications with disabled softirq & quit */

			local_bh_disable();

			memcpy(&gwj->mod, &mod, sizeof(mod));

			local_bh_enable();

			rcu_assign_pointer(gwj->cf_mod, mod);

			kfree_rcu_mightsleep(old_cf);

			return 0;

		}

	}



	/* ifindex == 0 is not allowed for job creation */

	if (!ccgw.src_idx || !ccgw.dst_idx)

		return -ENODEV;

	if (!ccgw.src_idx || !ccgw.dst_idx) {

		err = -ENODEV;

		goto out_free_cf;

	}



	gwj = kmem_cache_alloc(cgw_cache, GFP_KERNEL);

	if (!gwj)

		return -ENOMEM;

	if (!gwj) {

		err = -ENOMEM;

		goto out_free_cf;

	}



	gwj->handled_frames = 0;

	gwj->dropped_frames = 0;
@@ -1118,7 +1144,7 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh, 
	gwj->limit_hops = limhops;



	/* insert already parsed information */

	memcpy(&gwj->mod, &mod, sizeof(mod));

	RCU_INIT_POINTER(gwj->cf_mod, mod);

	memcpy(&gwj->ccgw, &ccgw, sizeof(ccgw));



	err = -ENODEV;
@@ -1152,9 +1178,11 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh, 
	if (!err)

		hlist_add_head_rcu(&gwj->list, &net->can.cgw_list);

out:

	if (err)

	if (err) {

		kmem_cache_free(cgw_cache, gwj);



out_free_cf:

		kfree(mod);

	}

	return err;

}
@@ -1214,19 +1242,22 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh, 


	/* remove only the first matching entry */

	hlist_for_each_entry_safe(gwj, nx, &net->can.cgw_list, list) {

		struct cf_mod *cf_mod;



		if (gwj->flags != r->flags)

			continue;



		if (gwj->limit_hops != limhops)

			continue;



		cf_mod = cgw_job_cf_mod(gwj);

		/* we have a match when uid is enabled and identical */

		if (gwj->mod.uid || mod.uid) {

			if (gwj->mod.uid != mod.uid)

		if (cf_mod->uid || mod.uid) {

			if (cf_mod->uid != mod.uid)

				continue;

		} else {

			/* no uid => check for identical modifications */

			if (memcmp(&gwj->mod, &mod, sizeof(mod)))

			if (memcmp(cf_mod, &mod, sizeof(mod)))

				continue;

		}