net: sched: ensure tc flower reoffload takes filter ref

Recent changes to TC flower remove the requirement for rtnl lock when accessing and modifying filters. Refcounts now ensure access and deletion do not happen concurrently. However, the reoffload function which cycles through all filters and replays them to registered hw drivers is not protected. Use the fl_get_next_filter() function to cycle the filters for reoffload and ensure the ref taken by this function is put when done with each filter. Signed-off-by: John Hurley <john.hurley@netronome.com> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> Reviewed-by: Vlad Buslov <vladbu@mellanox.com> Signed-off-by: David S. Miller <davem@davemloft.net>

net: sched: ensure tc flower reoffload takes filter ref
Recent changes to TC flower remove the requirement for rtnl lock when accessing and modifying filters. Refcounts now ensure access and deletion do not happen concurrently. However, the reoffload function which cycles through all filters and replays them to registered hw drivers is not protected. Use the fl_get_next_filter() function to cycle the filters for reoffload and ensure the ref taken by this function is put when done with each filter. Signed-off-by: John Hurley <john.hurley@netronome.com> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> Reviewed-by: Vlad Buslov <vladbu@mellanox.com> Signed-off-by: David S. Miller <davem@davemloft.net>
John Hurley · David S. Miller
1 parent 847d44efad
Showing 1 changed file with 46 additions and 42 deletions Side-by-side Diff
net/sched/cls_flower.c
@@ -1683,59 +1683,63 @@
 static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
 			void *cb_priv, struct netlink_ext_ack *extack)
 {
-	struct cls_fl_head *head = fl_head_dereference(tp);
 	struct tc_cls_flower_offload cls_flower = {};
 	struct tcf_block *block = tp->chain->block;
-	struct fl_flow_mask *mask;
+	unsigned long handle = 0;
 	struct cls_fl_filter *f;
 	int err;
  
-	list_for_each_entry(mask, &head->masks, list) {
-		list_for_each_entry(f, &mask->filters, list) {
-			if (tc_skip_hw(f->flags))
-				continue;
+	while ((f = fl_get_next_filter(tp, &handle))) {
+		if (tc_skip_hw(f->flags))
+			goto next_flow;
  
-			cls_flower.rule =
-				flow_rule_alloc(tcf_exts_num_actions(&f->exts));
-			if (!cls_flower.rule)
-				return -ENOMEM;
+		cls_flower.rule =
+			flow_rule_alloc(tcf_exts_num_actions(&f->exts));
+		if (!cls_flower.rule) {
+			__fl_put(f);
+			return -ENOMEM;
+		}
  
-			tc_cls_common_offload_init(&cls_flower.common, tp,
-						   f->flags, extack);
-			cls_flower.command = add ?
-				TC_CLSFLOWER_REPLACE : TC_CLSFLOWER_DESTROY;
-			cls_flower.cookie = (unsigned long)f;
-			cls_flower.rule->match.dissector = &mask->dissector;
-			cls_flower.rule->match.mask = &mask->key;
-			cls_flower.rule->match.key = &f->mkey;
+		tc_cls_common_offload_init(&cls_flower.common, tp, f->flags,
+					   extack);
+		cls_flower.command = add ?
+			TC_CLSFLOWER_REPLACE : TC_CLSFLOWER_DESTROY;
+		cls_flower.cookie = (unsigned long)f;
+		cls_flower.rule->match.dissector = &f->mask->dissector;
+		cls_flower.rule->match.mask = &f->mask->key;
+		cls_flower.rule->match.key = &f->mkey;
  
-			err = tc_setup_flow_action(&cls_flower.rule->action,
-						   &f->exts);
-			if (err) {
-				kfree(cls_flower.rule);
-				if (tc_skip_sw(f->flags)) {
-					NL_SET_ERR_MSG_MOD(extack, "Failed to setup flow action");
-					return err;
-				}
-				continue;
+		err = tc_setup_flow_action(&cls_flower.rule->action, &f->exts);
+		if (err) {
+			kfree(cls_flower.rule);
+			if (tc_skip_sw(f->flags)) {
+				NL_SET_ERR_MSG_MOD(extack, "Failed to setup flow action");
+				__fl_put(f);
+				return err;
 			}
+			goto next_flow;
+		}
  
-			cls_flower.classid = f->res.classid;
+		cls_flower.classid = f->res.classid;
  
-			err = cb(TC_SETUP_CLSFLOWER, &cls_flower, cb_priv);
-			kfree(cls_flower.rule);
+		err = cb(TC_SETUP_CLSFLOWER, &cls_flower, cb_priv);
+		kfree(cls_flower.rule);
  
-			if (err) {
-				if (add && tc_skip_sw(f->flags))
-					return err;
-				continue;
+		if (err) {
+			if (add && tc_skip_sw(f->flags)) {
+				__fl_put(f);
+				return err;
 			}
-
-			spin_lock(&tp->lock);
-			tc_cls_offload_cnt_update(block, &f->in_hw_count,
-						  &f->flags, add);
-			spin_unlock(&tp->lock);
+			goto next_flow;
 		}
+
+		spin_lock(&tp->lock);
+		tc_cls_offload_cnt_update(block, &f->in_hw_count, &f->flags,
+					  add);
+		spin_unlock(&tp->lock);
+next_flow:
+		handle++;
+		__fl_put(f);
 	}
  
 	return 0;
...	...	@@ -1683,59 +1683,63 @@
1683	1683	static int fl_reoffload(struct tcf_proto tp, bool add, tc_setup_cb_t cb,
1684	1684	void cb_priv, struct netlink_ext_ack extack)
1685	1685	{
1686		- struct cls_fl_head *head = fl_head_dereference(tp);
1687	1686	struct tc_cls_flower_offload cls_flower = {};
1688	1687	struct tcf_block *block = tp->chain->block;
1689		- struct fl_flow_mask *mask;
	1688	+ unsigned long handle = 0;
1690	1689	struct cls_fl_filter *f;
1691	1690	int err;
1692	1691
1693		- list_for_each_entry(mask, &head->masks, list) {
1694		- list_for_each_entry(f, &mask->filters, list) {
1695		- if (tc_skip_hw(f->flags))
1696		- continue;
	1692	+ while ((f = fl_get_next_filter(tp, &handle))) {
	1693	+ if (tc_skip_hw(f->flags))
	1694	+ goto next_flow;
1697	1695
1698		- cls_flower.rule =
1699		- flow_rule_alloc(tcf_exts_num_actions(&f->exts));
1700		- if (!cls_flower.rule)
1701		- return -ENOMEM;
	1696	+ cls_flower.rule =
	1697	+ flow_rule_alloc(tcf_exts_num_actions(&f->exts));
	1698	+ if (!cls_flower.rule) {
	1699	+ __fl_put(f);
	1700	+ return -ENOMEM;
	1701	+ }
1702	1702
1703		- tc_cls_common_offload_init(&cls_flower.common, tp,
1704		- f->flags, extack);
1705		- cls_flower.command = add ?
1706		- TC_CLSFLOWER_REPLACE : TC_CLSFLOWER_DESTROY;
1707		- cls_flower.cookie = (unsigned long)f;
1708		- cls_flower.rule->match.dissector = &mask->dissector;
1709		- cls_flower.rule->match.mask = &mask->key;
1710		- cls_flower.rule->match.key = &f->mkey;
	1703	+ tc_cls_common_offload_init(&cls_flower.common, tp, f->flags,
	1704	+ extack);
	1705	+ cls_flower.command = add ?
	1706	+ TC_CLSFLOWER_REPLACE : TC_CLSFLOWER_DESTROY;
	1707	+ cls_flower.cookie = (unsigned long)f;
	1708	+ cls_flower.rule->match.dissector = &f->mask->dissector;
	1709	+ cls_flower.rule->match.mask = &f->mask->key;
	1710	+ cls_flower.rule->match.key = &f->mkey;
1711	1711
1712		- err = tc_setup_flow_action(&cls_flower.rule->action,
1713		- &f->exts);
1714		- if (err) {
1715		- kfree(cls_flower.rule);
1716		- if (tc_skip_sw(f->flags)) {
1717		- NL_SET_ERR_MSG_MOD(extack, "Failed to setup flow action");
1718		- return err;
1719		- }
1720		- continue;
	1712	+ err = tc_setup_flow_action(&cls_flower.rule->action, &f->exts);
	1713	+ if (err) {
	1714	+ kfree(cls_flower.rule);
	1715	+ if (tc_skip_sw(f->flags)) {
	1716	+ NL_SET_ERR_MSG_MOD(extack, "Failed to setup flow action");
	1717	+ __fl_put(f);
	1718	+ return err;
1721	1719	}
	1720	+ goto next_flow;
	1721	+ }
1722	1722
1723		- cls_flower.classid = f->res.classid;
	1723	+ cls_flower.classid = f->res.classid;
1724	1724
1725		- err = cb(TC_SETUP_CLSFLOWER, &cls_flower, cb_priv);
1726		- kfree(cls_flower.rule);
	1725	+ err = cb(TC_SETUP_CLSFLOWER, &cls_flower, cb_priv);
	1726	+ kfree(cls_flower.rule);
1727	1727
1728		- if (err) {
1729		- if (add && tc_skip_sw(f->flags))
1730		- return err;
1731		- continue;
	1728	+ if (err) {
	1729	+ if (add && tc_skip_sw(f->flags)) {
	1730	+ __fl_put(f);
	1731	+ return err;
1732	1732	}
1733		-
1734		- spin_lock(&tp->lock);
1735		- tc_cls_offload_cnt_update(block, &f->in_hw_count,
1736		- &f->flags, add);
1737		- spin_unlock(&tp->lock);
	1733	+ goto next_flow;
1738	1734	}
	1735	+
	1736	+ spin_lock(&tp->lock);
	1737	+ tc_cls_offload_cnt_update(block, &f->in_hw_count, &f->flags,
	1738	+ add);
	1739	+ spin_unlock(&tp->lock);
	1740	+next_flow:
	1741	+ handle++;
	1742	+ __fl_put(f);
1739	1743	}
1740	1744
1741	1745	return 0;