From 80b2ff6e6cbc6f06d9b695698b88e5960918f206 Mon Sep 17 00:00:00 2001 From: Radek Matejka Date: Fri, 17 Aug 2012 16:33:50 +0200 Subject: [PATCH] wrongly nested rcu_read_* fix Rcu lock replaced with mutex. --- kernel/canethgw.c | 69 ++++++++++++++++++++++++++++------------------- test/test | 10 +++++-- 2 files changed, 49 insertions(+), 30 deletions(-) diff --git a/kernel/canethgw.c b/kernel/canethgw.c index 27f7525..1d1e95e 100644 --- a/kernel/canethgw.c +++ b/kernel/canethgw.c @@ -57,9 +57,11 @@ static struct socket *can_sock = NULL, *udp_sock = NULL; static struct task_struct *eth_to_can = NULL, *can_to_eth = NULL; static struct notifier_block notifier; -HLIST_HEAD(rule_can_eth); -HLIST_HEAD(rule_eth_can); -DEFINE_MUTEX(cegw_mutex); +static HLIST_HEAD(rule_eth_can); +static HLIST_HEAD(rule_can_eth); +static DEFINE_MUTEX(rule_eth_can_mutex); +static DEFINE_MUTEX(rule_can_eth_mutex); +static DEFINE_MUTEX(cegw_mutex); static void cegw_udp_send(struct socket *udp_sock, struct can_frame *cf, struct in_addr ipaddr, u16 port) @@ -134,7 +136,7 @@ static int cegw_udp_can(void *data) break; vec.iov_base = &cf; vec.iov_len = sizeof(cf); - recv_size = kernel_recvmsg(udp_sock, &mh, &vec, 1, + recv_size = kernel_recvmsg(udp_sock, &mh, &vec, 1, sizeof(cf), 0); /* recv_size == 0 when shutting down */ if (recv_size != sizeof(cf) || recv_size == 0) @@ -142,13 +144,13 @@ static int cegw_udp_can(void *data) else if (recv_size < 0) return -1; - hlist_for_each_entry_rcu(rule, pos, &rule_eth_can, list) { - rcu_read_lock(); + mutex_lock(&rule_eth_can_mutex); + hlist_for_each_entry(rule, pos, &rule_eth_can, list) { can_ifidx = rule->can_ifindex; - rcu_read_unlock(); /* ToDo: from filter */ cegw_can_send(can_sock, &cf, can_ifidx); } + mutex_unlock(&rule_eth_can_mutex); } return 0; @@ -188,14 +190,14 @@ static int cegw_can_udp(void* data) else if (recv_size < 0) return -1; - hlist_for_each_entry_rcu(rule, pos, &rule_can_eth, list) { - rcu_read_lock(); + mutex_lock(&rule_can_eth_mutex); + hlist_for_each_entry(rule, pos, &rule_can_eth, list) { eth_ip = rule->eth_ip; eth_port = rule->eth_port; - rcu_read_unlock(); if (rule->can_ifindex == ca.can_ifindex) cegw_udp_send(udp_sock, &cf, eth_ip, eth_port); } + mutex_unlock(&rule_can_eth_mutex); } return 0; @@ -246,19 +248,19 @@ static int cegw_newroute(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) set->eth_port = *(unsigned short*)nla_data(tb[CEGW_ETH_PORT]); kthread_run(cegw_thread_start, set, "canethgw"); break; - case CEGW_RULE_CAN_ETH: + case CEGW_RULE_ETH_CAN: if (!tb[CEGW_ETH_IP] || !tb[CEGW_ETH_PORT] || !tb[CEGW_CAN_IFINDEX]) { - pr_devel("canethgw: missing attribute for " - "CEGW_RULE_CAN_ETH\n"); + pr_devel("canethgw: missing attribute for" + "CEGW_RULE_ETH_CAN\n"); return -EINVAL; } ifindex = *(int*)nla_data(tb[CEGW_CAN_IFINDEX]); ip = *(struct in_addr*)nla_data(tb[CEGW_ETH_IP]); port = *(unsigned short*)nla_data(tb[CEGW_ETH_PORT]); - pr_devel("canethgw: new can->eth rule - (%d)->(%x:%hu)\n", - ifindex, ip.s_addr, port); + pr_devel("canethgw: new eth->can rule - (%x:%hu)->(%d)\n", + ip.s_addr, port, ifindex); rule = kmalloc(sizeof(struct cegw_rule), GFP_KERNEL); if (rule == NULL) @@ -268,21 +270,23 @@ static int cegw_newroute(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) rule->eth_ip = ip; rule->eth_port = port; - hlist_add_head_rcu(&rule->list, &rule_can_eth); + mutex_lock(&rule_eth_can_mutex); + hlist_add_head(&rule->list, &rule_eth_can); + mutex_unlock(&rule_eth_can_mutex); break; - case CEGW_RULE_ETH_CAN: + case CEGW_RULE_CAN_ETH: if (!tb[CEGW_ETH_IP] || !tb[CEGW_ETH_PORT] || !tb[CEGW_CAN_IFINDEX]) { - pr_devel("canethgw: missing attribute for" - "CEGW_RULE_ETH_CAN\n"); + pr_devel("canethgw: missing attribute for " + "CEGW_RULE_CAN_ETH\n"); return -EINVAL; } ifindex = *(int*)nla_data(tb[CEGW_CAN_IFINDEX]); ip = *(struct in_addr*)nla_data(tb[CEGW_ETH_IP]); port = *(unsigned short*)nla_data(tb[CEGW_ETH_PORT]); - pr_devel("canethgw: new eth->can rule - (%x:%hu)->(%d)\n", - ip.s_addr, port, ifindex); + pr_devel("canethgw: new can->eth rule - (%d)->(%x:%hu)\n", + ifindex, ip.s_addr, port); rule = kmalloc(sizeof(struct cegw_rule), GFP_KERNEL); if (rule == NULL) @@ -292,7 +296,9 @@ static int cegw_newroute(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) rule->eth_ip = ip; rule->eth_port = port; - hlist_add_head_rcu(&rule->list, &rule_eth_can); + mutex_lock(&rule_can_eth_mutex); + hlist_add_head(&rule->list, &rule_can_eth); + mutex_unlock(&rule_can_eth_mutex); break; default: pr_devel("canethgw: unknown CEGW_CMD_INFO\n"); @@ -307,14 +313,19 @@ static void cegw_flush(void) struct cegw_rule *rule; struct hlist_node *pos, *n; - hlist_for_each_entry_safe(rule, pos, n, &rule_can_eth, list) { + mutex_lock(&rule_eth_can_mutex); + hlist_for_each_entry_safe(rule, pos, n, &rule_eth_can, list) { hlist_del(&rule->list); kfree(rule); } - hlist_for_each_entry_safe(rule, pos, n, &rule_eth_can, list) { + mutex_unlock(&rule_eth_can_mutex); + + mutex_lock(&rule_can_eth_mutex); + hlist_for_each_entry_safe(rule, pos, n, &rule_can_eth, list) { hlist_del(&rule->list); kfree(rule); } + mutex_unlock(&rule_can_eth_mutex); } static int cegw_delroute(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) @@ -402,8 +413,9 @@ static int cegw_getroute(struct sk_buff *skb, struct netlink_callback *cb) int idx = 0; int s_idx = cb->args[0]; - rcu_read_lock(); - hlist_for_each_entry_rcu(rule, pos, &rule_eth_can, list) { + mutex_lock(&rule_eth_can_mutex); + mutex_lock(&rule_can_eth_mutex); + hlist_for_each_entry(rule, pos, &rule_eth_can, list) { if (idx < s_idx) goto cont1; @@ -413,7 +425,7 @@ cont1: idx++; } - hlist_for_each_entry_rcu(rule, pos, &rule_can_eth, list) { + hlist_for_each_entry(rule, pos, &rule_can_eth, list) { if (idx < s_idx) goto cont2; @@ -424,7 +436,8 @@ cont2: } brk: - rcu_read_unlock(); + mutex_unlock(&rule_eth_can_mutex); + mutex_unlock(&rule_can_eth_mutex); cb->args[0] = idx; return skb->len; diff --git a/test/test b/test/test index 44b0ecd..419b1e5 100755 --- a/test/test +++ b/test/test @@ -1,4 +1,6 @@ #!/bin/sh +N=10 + evalmsg() { if test $1 -eq 0; then @@ -9,11 +11,15 @@ evalmsg() echo -ne "\033[0m" } +if test $# -ne 0; then + N=$1; +fi + echo -n "Testing eth->can: " -cegwbench -s udp@127.0.0.1:10501 -d can@vcan0 -n 10 -t 1 &> /dev/zero +cegwbench -s udp@127.0.0.1:10501 -d can@vcan0 -n $N -t 1 &> /dev/zero evalmsg $? echo -n "Testing can->eth: " -cegwbench -s can@vcan0 -d udp@127.0.0.1:10502 -n 10 -t 1 &> /dev/zero +cegwbench -s can@vcan0 -d udp@127.0.0.1:10502 -n $N -t 1 &> /dev/zero evalmsg $? -- 2.39.2