Re: [BUG] act_ife: sleeping functions called in atomic context

From: Cong Wang
Date: Fri Jun 17 2016 - 01:38:53 EST

On Thu, Jun 16, 2016 at 7:14 PM, Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote:
>
> I think we can just remove that tcf_lock, I am testing a patch now.

Please try the attached patch, I will do more tests tomorrow.

Thanks!
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 658046d..859fb02 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -240,8 +240,7 @@ static int ife_validate_metatype(struct tcf_meta_ops *ops, void *val, int len)
}

/* called when adding new meta information
- * under ife->tcf_lock
-*/
+ */
static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
void *val, int len)
{
@@ -251,11 +250,9 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
if (!ops) {
ret = -ENOENT;
#ifdef CONFIG_MODULES
- spin_unlock_bh(&ife->tcf_lock);
rtnl_unlock();
request_module("ifemeta%u", metaid);
rtnl_lock();
- spin_lock_bh(&ife->tcf_lock);
ops = find_ife_oplist(metaid);
#endif
}
@@ -272,8 +269,8 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
}

/* called when adding new meta information
- * under ife->tcf_lock
-*/
+ * under RTNL lock
+ */
static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
int len)
{
@@ -284,7 +281,7 @@ static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
if (!ops)
return -ENOENT;

- mi = kzalloc(sizeof(*mi), GFP_KERNEL);
+ mi = kzalloc(sizeof(*mi), GFP_ATOMIC);
if (!mi) {
/*put back what find_ife_oplist took */
module_put(ops->owner);
@@ -302,8 +299,7 @@ static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
}
}

- list_add_tail(&mi->metalist, &ife->metalist);
-
+ list_add_tail_rcu(&mi->metalist, &ife->metalist);
return ret;
}

@@ -313,11 +309,13 @@ static int use_all_metadata(struct tcf_ife_info *ife)
int rc = 0;
int installed = 0;

+ read_lock(&ife_mod_lock);
list_for_each_entry(o, &ifeoplist, list) {
rc = add_metainfo(ife, o->metaid, NULL, 0);
if (rc == 0)
installed += 1;
}
+ read_unlock(&ife_mod_lock);

if (installed)
return 0;
@@ -340,10 +338,12 @@ static int dump_metalist(struct sk_buff *skb, struct tcf_ife_info *ife)
if (!nest)
goto out_nlmsg_trim;

- list_for_each_entry(e, &ife->metalist, metalist) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(e, &ife->metalist, metalist) {
if (!e->ops->get(skb, e))
total_encoded += 1;
}
+ rcu_read_unlock();

if (!total_encoded)
goto out_nlmsg_trim;
@@ -357,15 +357,14 @@ out_nlmsg_trim:
return -1;
}

-/* under ife->tcf_lock */
-static void _tcf_ife_cleanup(struct tc_action *a, int bind)
+static void tcf_ife_cleanup(struct tc_action *a, int bind)
{
struct tcf_ife_info *ife = a->priv;
struct tcf_meta_info *e, *n;

list_for_each_entry_safe(e, n, &ife->metalist, metalist) {
module_put(e->ops->owner);
- list_del(&e->metalist);
+ list_del_rcu(&e->metalist);
if (e->metaval) {
if (e->ops->release)
e->ops->release(e);
@@ -376,16 +375,6 @@ static void _tcf_ife_cleanup(struct tc_action *a, int bind)
}
}

-static void tcf_ife_cleanup(struct tc_action *a, int bind)
-{
- struct tcf_ife_info *ife = a->priv;
-
- spin_lock_bh(&ife->tcf_lock);
- _tcf_ife_cleanup(a, bind);
- spin_unlock_bh(&ife->tcf_lock);
-}
-
-/* under ife->tcf_lock */
static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb)
{
int len = 0;
@@ -474,7 +463,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
saddr = nla_data(tb[TCA_IFE_SMAC]);
}

- spin_lock_bh(&ife->tcf_lock);
ife->tcf_action = parm->action;

if (parm->flags & IFE_ENCODE) {
@@ -502,9 +490,8 @@ metadata_parse_err:
if (exists)
tcf_hash_release(a, bind);
if (ret == ACT_P_CREATED)
- _tcf_ife_cleanup(a, bind);
+ tcf_ife_cleanup(a, bind);

- spin_unlock_bh(&ife->tcf_lock);
return err;
}

@@ -521,15 +508,12 @@ metadata_parse_err:
err = use_all_metadata(ife);
if (err) {
if (ret == ACT_P_CREATED)
- _tcf_ife_cleanup(a, bind);
+ tcf_ife_cleanup(a, bind);

- spin_unlock_bh(&ife->tcf_lock);
return err;
}
}

- spin_unlock_bh(&ife->tcf_lock);
-
if (ret == ACT_P_CREATED)
tcf_hash_insert(tn, a);

@@ -589,16 +573,19 @@ int find_decode_metaid(struct sk_buff *skb, struct tcf_ife_info *ife,
{
struct tcf_meta_info *e;

+ rcu_read_lock();
/* XXX: use hash to speed up */
- list_for_each_entry(e, &ife->metalist, metalist) {
+ list_for_each_entry_rcu(e, &ife->metalist, metalist) {
if (metaid == e->metaid) {
if (e->ops) {
+ rcu_read_unlock();
/* We check for decode presence already */
return e->ops->decode(skb, mdata, mlen);
}
}
}

+ rcu_read_unlock();
return 0;
}

@@ -621,16 +608,12 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
u16 ifehdrln = ifehdr->metalen;
struct meta_tlvhdr *tlv = (struct meta_tlvhdr *)(ifehdr->tlv_data);

- spin_lock(&ife->tcf_lock);
- bstats_update(&ife->tcf_bstats, skb);
- ife->tcf_tm.lastuse = jiffies;
- spin_unlock(&ife->tcf_lock);
+ tcf_lastuse_update(&ife->tcf_tm);
+ bstats_cpu_update(this_cpu_ptr(ife->common.cpu_bstats), skb);

ifehdrln = ntohs(ifehdrln);
if (unlikely(!pskb_may_pull(skb, ifehdrln))) {
- spin_lock(&ife->tcf_lock);
- ife->tcf_qstats.drops++;
- spin_unlock(&ife->tcf_lock);
+ qstats_drop_inc(this_cpu_ptr(ife->common.cpu_qstats));
return TC_ACT_SHOT;
}

@@ -654,7 +637,7 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
*/
pr_info_ratelimited("Unknown metaid %d alnlen %d\n",
mtype, mlen);
- ife->tcf_qstats.overlimits++;
+ qstats_overlimit_inc(this_cpu_ptr(ife->common.cpu_qstats));
}

tlvdata += mlen;
@@ -671,16 +654,17 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
**/
static int ife_get_sz(struct sk_buff *skb, struct tcf_ife_info *ife)
{
- struct tcf_meta_info *e, *n;
+ struct tcf_meta_info *e;
int tot_run_sz = 0, run_sz = 0;

- list_for_each_entry_safe(e, n, &ife->metalist, metalist) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(e, &ife->metalist, metalist) {
if (e->ops->check_presence) {
run_sz = e->ops->check_presence(skb, e);
tot_run_sz += run_sz;
}
}
-
+ rcu_read_unlock();
return tot_run_sz;
}

@@ -709,34 +693,28 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
exceed_mtu = true;
}

- spin_lock(&ife->tcf_lock);
- bstats_update(&ife->tcf_bstats, skb);
- ife->tcf_tm.lastuse = jiffies;
+ tcf_lastuse_update(&ife->tcf_tm);
+ bstats_cpu_update(this_cpu_ptr(ife->common.cpu_bstats), skb);

+ rcu_read_lock();
if (!metalen) { /* no metadata to send */
/* abuse overlimits to count when we allow packet
* with no metadata
*/
- ife->tcf_qstats.overlimits++;
- spin_unlock(&ife->tcf_lock);
+ qstats_overlimit_inc(this_cpu_ptr(ife->common.cpu_qstats));
+ rcu_read_unlock();
return action;
}
/* could be stupid policy setup or mtu config
* so lets be conservative.. */
- if ((action == TC_ACT_SHOT) || exceed_mtu) {
- ife->tcf_qstats.drops++;
- spin_unlock(&ife->tcf_lock);
- return TC_ACT_SHOT;
- }
+ if ((action == TC_ACT_SHOT) || exceed_mtu)
+ goto drop;

iethh = eth_hdr(skb);

err = skb_cow_head(skb, hdrm);
- if (unlikely(err)) {
- ife->tcf_qstats.drops++;
- spin_unlock(&ife->tcf_lock);
- return TC_ACT_SHOT;
- }
+ if (unlikely(err))
+ goto drop;

if (!(at & AT_EGRESS))
skb_push(skb, skb->dev->hard_header_len);
@@ -756,17 +734,13 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
* not repeat some of the computations that are done by
* ops->presence_check...
*/
- list_for_each_entry(e, &ife->metalist, metalist) {
+ list_for_each_entry_rcu(e, &ife->metalist, metalist) {
if (e->ops->encode) {
err = e->ops->encode(skb, (void *)(skb->data + skboff),
e);
}
- if (err < 0) {
- /* too corrupt to keep around if overwritten */
- ife->tcf_qstats.drops++;
- spin_unlock(&ife->tcf_lock);
- return TC_ACT_SHOT;
- }
+ if (err < 0)
+ goto drop;
skboff += err;
}

@@ -783,9 +757,14 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
if (!(at & AT_EGRESS))
skb_pull(skb, skb->dev->hard_header_len);

- spin_unlock(&ife->tcf_lock);
+ rcu_read_unlock();

return action;
+
+drop:
+ qstats_drop_inc(this_cpu_ptr(ife->common.cpu_qstats));
+ rcu_read_unlock();
+ return TC_ACT_SHOT;
}

static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
@@ -800,11 +779,10 @@ static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
return tcf_ife_decode(skb, a, res);

pr_info_ratelimited("unknown failure(policy neither de/encode\n");
- spin_lock(&ife->tcf_lock);
- bstats_update(&ife->tcf_bstats, skb);
- ife->tcf_tm.lastuse = jiffies;
- ife->tcf_qstats.drops++;
- spin_unlock(&ife->tcf_lock);
+
+ tcf_lastuse_update(&ife->tcf_tm);
+ bstats_cpu_update(this_cpu_ptr(ife->common.cpu_bstats), skb);
+ qstats_drop_inc(this_cpu_ptr(ife->common.cpu_qstats));

return TC_ACT_SHOT;
}