From: Oliver Hartkopp <socketcan@hartkopp.net>
To: linux-can@vger.kernel.org
Cc: Oliver Hartkopp <socketcan@hartkopp.net>,
Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Subject: [RFC PATCH] can: gw: fix RCU/BH usage in cgw_create_job()
Date: Thu, 21 Dec 2023 13:37:03 +0100 [thread overview]
Message-ID: <20231221123703.8170-1-socketcan@hartkopp.net> (raw)
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@linutronix.de/
Fixes: dd895d7f21b2 ("can: cangw: introduce optional uid to reference created routing jobs")
Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
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
next reply other threads:[~2023-12-21 12:40 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-21 12:37 Oliver Hartkopp [this message]
2024-01-11 12:14 ` [RFC PATCH] can: gw: fix RCU/BH usage in cgw_create_job() Sebastian Andrzej Siewior
2024-01-21 10:07 ` Oliver Hartkopp
2024-01-22 10:10 ` Sebastian Andrzej Siewior
2024-04-29 9:47 ` Sebastian Andrzej Siewior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231221123703.8170-1-socketcan@hartkopp.net \
--to=socketcan@hartkopp.net \
--cc=bigeasy@linutronix.de \
--cc=linux-can@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.