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/20231031112349.y0aLoBrz@xxxxxxxxxxxxx/ Fixes: dd895d7f21b2 ("can: cangw: introduce optional uid to reference created routing jobs") Reported-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx> --- net/can/gw.c | 99 ++++++++++++++++++++++++++++------------------------ 1 file changed, 53 insertions(+), 46 deletions(-) diff --git a/net/can/gw.c b/net/can/gw.c index 37528826935e..6411f1c60f48 100644 --- a/net/can/gw.c +++ b/net/can/gw.c @@ -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))) -- 2.39.2
