On Sun 20 May 2018 at 06:22, Jiri Pirko [off-list ref] wrote:

Sat, May 19, 2018 at 11:43:27PM CEST, marcelo.leitner@gmail.com wrote:

quoted

On Mon, May 14, 2018 at 05:27:07PM +0300, Vlad Buslov wrote:
...

quoted

@@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 	return err;
 }

+static int tcf_action_delete(struct net *net, struct list_head *actions,
+			     struct netlink_ext_ack *extack)
+{
+	int ret;

Reverse christmass tree.. this line should be the last in variable
declarations.

quoted

+	struct tc_action *a, *tmp;
+	char kind[IFNAMSIZ];
+	u32 act_index;
+
+	list_for_each_entry_safe(a, tmp, actions, list) {
+		const struct tc_action_ops *ops = a->ops;
+
+		/* Actions can be deleted concurrently
+		 * so we must save their type and id to search again
+		 * after reference is released.
+		 */
+		strncpy(kind, a->ops->kind, sizeof(kind) - 1);

This may be problematic. Why strncpy here?

This is not necessary if Vlad is going to hold module referece, ops
cannot disappear.

Yes, I've already refactored this code to reuse ops.

quoted

a->ops->kind is also of size IFNAMSIZ. If a->ops->kind is actually
IFNAMSIZ-1 long, kind here won't be NULL terminated, as kind is not
initialized and strncpy won't add the NULL.

quoted

+		act_index = a->tcfa_index;
+
+		list_del(&a->list);
+		if (tcf_action_put(a))
+			module_put(ops->owner);
+
+		/* now do the delete */
+		ret = tcf_action_del_1(net, kind, act_index, extack);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}