Skip to content

Commit

Permalink
act_ife: load meta modules before tcf_idr_check_alloc()
Browse files Browse the repository at this point in the history
The following deadlock scenario is triggered by syzbot:

Thread A:				Thread B:
tcf_idr_check_alloc()
...
populate_metalist()
  rtnl_unlock()
					rtnl_lock()
					...
  request_module()			tcf_idr_check_alloc()
  rtnl_lock()

At this point, thread A is waiting for thread B to release RTNL
lock, while thread B is waiting for thread A to commit the IDR
change with tcf_idr_insert() later.

Break this deadlock situation by preloading ife modules earlier,
before tcf_idr_check_alloc(), this is fine because we only need
to load modules we need potentially.

Reported-and-tested-by: [email protected]
Fixes: 0190c1d ("net: sched: atomically check-allocate action")
Cc: Jamal Hadi Salim <[email protected]>
Cc: Vlad Buslov <[email protected]>
Cc: Jiri Pirko <[email protected]>
Signed-off-by: Cong Wang <[email protected]>
Signed-off-by: Jakub Kicinski <[email protected]>
  • Loading branch information
congwang authored and kuba-moo committed Sep 5, 2020
1 parent c2b9478 commit cc8e58f
Showing 1 changed file with 34 additions and 10 deletions.
44 changes: 34 additions & 10 deletions net/sched/act_ife.c
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,25 @@ static void tcf_ife_cleanup(struct tc_action *a)
kfree_rcu(p, rcu);
}

static int load_metalist(struct nlattr **tb, bool rtnl_held)
{
int i;

for (i = 1; i < max_metacnt; i++) {
if (tb[i]) {
void *val = nla_data(tb[i]);
int len = nla_len(tb[i]);
int rc;

rc = load_metaops_and_vet(i, val, len, rtnl_held);
if (rc != 0)
return rc;
}
}

return 0;
}

static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
bool exists, bool rtnl_held)
{
Expand All @@ -449,10 +468,6 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
val = nla_data(tb[i]);
len = nla_len(tb[i]);

rc = load_metaops_and_vet(i, val, len, rtnl_held);
if (rc != 0)
return rc;

rc = add_metainfo(ife, i, val, len, exists);
if (rc)
return rc;
Expand Down Expand Up @@ -509,6 +524,21 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
if (!p)
return -ENOMEM;

if (tb[TCA_IFE_METALST]) {
err = nla_parse_nested_deprecated(tb2, IFE_META_MAX,
tb[TCA_IFE_METALST], NULL,
NULL);
if (err) {
kfree(p);
return err;
}
err = load_metalist(tb2, rtnl_held);
if (err) {
kfree(p);
return err;
}
}

index = parm->index;
err = tcf_idr_check_alloc(tn, &index, a, bind);
if (err < 0) {
Expand Down Expand Up @@ -570,15 +600,9 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
}

if (tb[TCA_IFE_METALST]) {
err = nla_parse_nested_deprecated(tb2, IFE_META_MAX,
tb[TCA_IFE_METALST], NULL,
NULL);
if (err)
goto metadata_parse_err;
err = populate_metalist(ife, tb2, exists, rtnl_held);
if (err)
goto metadata_parse_err;

} else {
/* if no passed metadata allow list or passed allow-all
* then here we process by adding as many supported metadatum
Expand Down

0 comments on commit cc8e58f

Please sign in to comment.