Skip to content

Commit 0fedc63

Browse files
congwangdavem330
authored andcommitted
net_sched: commit action insertions together
syzbot is able to trigger a failure case inside the loop in tcf_action_init(), and when this happens we clean up with tcf_action_destroy(). But, as these actions are already inserted into the global IDR, other parallel process could free them before tcf_action_destroy(), then we will trigger a use-after-free. Fix this by deferring the insertions even later, after the loop, and committing all the insertions in a separate loop, so we will never fail in the middle of the insertions any more. One side effect is that the window between alloction and final insertion becomes larger, now it is more likely that the loop in tcf_del_walker() sees the placeholder -EBUSY pointer. So we have to check for error pointer in tcf_del_walker(). Reported-and-tested-by: [email protected] Fixes: 0190c1d ("net: sched: atomically check-allocate action") Cc: Vlad Buslov <[email protected]> Cc: Jamal Hadi Salim <[email protected]> Cc: Jiri Pirko <[email protected]> Signed-off-by: Cong Wang <[email protected]> Signed-off-by: David S. Miller <[email protected]>
1 parent e49d8c2 commit 0fedc63

File tree

1 file changed

+23
-9
lines changed

1 file changed

+23
-9
lines changed

net/sched/act_api.c

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,8 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
307307

308308
mutex_lock(&idrinfo->lock);
309309
idr_for_each_entry_ul(idr, p, tmp, id) {
310+
if (IS_ERR(p))
311+
continue;
310312
ret = tcf_idr_release_unsafe(p);
311313
if (ret == ACT_P_DELETED) {
312314
module_put(ops->owner);
@@ -891,14 +893,24 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
891893
[TCA_ACT_HW_STATS] = NLA_POLICY_BITFIELD32(TCA_ACT_HW_STATS_ANY),
892894
};
893895

894-
static void tcf_idr_insert(struct tc_action *a)
896+
static void tcf_idr_insert_many(struct tc_action *actions[])
895897
{
896-
struct tcf_idrinfo *idrinfo = a->idrinfo;
898+
int i;
897899

898-
mutex_lock(&idrinfo->lock);
899-
/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
900-
WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
901-
mutex_unlock(&idrinfo->lock);
900+
for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
901+
struct tc_action *a = actions[i];
902+
struct tcf_idrinfo *idrinfo;
903+
904+
if (!a)
905+
continue;
906+
idrinfo = a->idrinfo;
907+
mutex_lock(&idrinfo->lock);
908+
/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc if
909+
* it is just created, otherwise this is just a nop.
910+
*/
911+
idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
912+
mutex_unlock(&idrinfo->lock);
913+
}
902914
}
903915

904916
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
@@ -995,9 +1007,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
9951007
return ERR_PTR(-EINVAL);
9961008
}
9971009

998-
if (err == ACT_P_CREATED)
999-
tcf_idr_insert(a);
1000-
10011010
if (!name && tb[TCA_ACT_COOKIE])
10021011
tcf_set_action_cookie(&a->act_cookie, cookie);
10031012

@@ -1053,6 +1062,11 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
10531062
actions[i - 1] = act;
10541063
}
10551064

1065+
/* We have to commit them all together, because if any error happened in
1066+
* between, we could not handle the failure gracefully.
1067+
*/
1068+
tcf_idr_insert_many(actions);
1069+
10561070
*attr_size = tcf_action_full_attrs_size(sz);
10571071
return i - 1;
10581072

0 commit comments

Comments
 (0)