Thomas Graf wrote:
+int
+tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+ struct rtattr *rate_tlv, struct tcf_exts *exts,
+ struct tcf_ext_map *map)
+{
+ memset(exts, 0, sizeof(*exts));
+
+#ifdef CONFIG_NET_CLS_ACT
+ int err;
+ struct tc_action *act;
+
+ if (map->police && tb[map->police-1] && rate_tlv) {
+ act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
+ TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+ if (NULL == act)
Please use act == NULL
+void
+tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+ struct tcf_exts *src)
+{
+#ifdef CONFIG_NET_CLS_ACT
+ if (src->action) {
+ if (dst->action) {
+ struct tc_action *act;
+
+ tcf_tree_lock(tp);
+ act = xchg(&dst->action, src->action);
+ tcf_tree_unlock(tp);
+
+ tcf_action_destroy(act, TCA_ACT_UNBIND);
+ } else
+ dst->action = src->action;
This isn't right (its also wrong in the current code). If the
CPU reorders stores and another CPU looks at dst->action at
the wrong time it might see an inconsistent structure. So at
least smp_wmb is required for the unlocked case, but I think
you should just remove it completely. I also wonder if anyone
actually knows why we need the xchg (here and in all the other
places), it looks totally useless.
Regards
Patrick
|