From edfaf94fa705181eeb2fe0c36c0b902dedbd40f1 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Sun, 19 Aug 2018 12:22:05 -0700 Subject: net_sched: improve and refactor tcf_action_put_many() tcf_action_put_many() is mostly called to clean up actions on failure path, but tcf_action_put_many(&actions[acts_deleted]) is used in the ugliest way: it passes a slice of the array and uses an additional NULL at the end to avoid out-of-bound access. acts_deleted is completely unnecessary since we can teach tcf_action_put_many() scan the whole array and checks against NULL pointer. Which also means tcf_action_delete() should set deleted action pointers to NULL to avoid double free. Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers to actions") Cc: Jiri Pirko Cc: Vlad Buslov Signed-off-by: Cong Wang Signed-off-by: David S. Miller --- net/sched/act_api.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 229d63c99be2..cd69a6afcf88 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -686,14 +686,18 @@ static int tcf_action_put(struct tc_action *p) return __tcf_action_put(p, false); } +/* Put all actions in this array, skip those NULL's. */ static void tcf_action_put_many(struct tc_action *actions[]) { int i; - for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { struct tc_action *a = actions[i]; - const struct tc_action_ops *ops = a->ops; + const struct tc_action_ops *ops; + if (!a) + continue; + ops = a->ops; if (tcf_action_put(a)) module_put(ops->owner); } @@ -1176,7 +1180,7 @@ err_out: } static int tcf_action_delete(struct net *net, struct tc_action *actions[], - int *acts_deleted, struct netlink_ext_ack *extack) + struct netlink_ext_ack *extack) { u32 act_index; int ret, i; @@ -1196,20 +1200,17 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[], } else { /* now do the delete */ ret = ops->delete(net, act_index); - if (ret < 0) { - *acts_deleted = i + 1; + if (ret < 0) return ret; - } } + actions[i] = NULL; } - *acts_deleted = i; return 0; } static int tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[], - int *acts_deleted, u32 portid, size_t attr_size, - struct netlink_ext_ack *extack) + u32 portid, size_t attr_size, struct netlink_ext_ack *extack) { int ret; struct sk_buff *skb; @@ -1227,7 +1228,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[], } /* now do the delete */ - ret = tcf_action_delete(net, actions, acts_deleted, extack); + ret = tcf_action_delete(net, actions, extack); if (ret < 0) { NL_SET_ERR_MSG(extack, "Failed to delete TC action"); kfree_skb(skb); @@ -1249,8 +1250,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, struct nlattr *tb[TCA_ACT_MAX_PRIO + 1]; struct tc_action *act; size_t attr_size = 0; - struct tc_action *actions[TCA_ACT_MAX_PRIO + 1] = {}; - int acts_deleted = 0; + struct tc_action *actions[TCA_ACT_MAX_PRIO] = {}; ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack); if (ret < 0) @@ -1280,14 +1280,13 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, if (event == RTM_GETACTION) ret = tcf_get_notify(net, portid, n, actions, event, extack); else { /* delete */ - ret = tcf_del_notify(net, n, actions, &acts_deleted, portid, - attr_size, extack); + ret = tcf_del_notify(net, n, actions, portid, attr_size, extack); if (ret) goto err; - return ret; + return 0; } err: - tcf_action_put_many(&actions[acts_deleted]); + tcf_action_put_many(actions); return ret; } -- cgit v1.2.3-59-g8ed1b