diff mbox

[net-next] net_sched: fix regression in tc_action_ops

Message ID 1387571552.19078.468.camel@edumazet-glaptop2.roam.corp.google.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Dec. 20, 2013, 8:32 p.m. UTC 
From: Eric Dumazet <edumazet@google.com>

list_for_each_entry(a, &act_base, head) doesn't
exit with a = NULL if we reached the end of the list.

tcf_unregister_action(), tc_lookup_action_n() and tc_lookup_action()
need fixes.

Remove tc_lookup_action_id() as its unused and not worth 'fixing'

Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 1f747c26c48b ("net_sched: convert tc_action_ops to use struct list_head")
Cc: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_api.c |   53 ++++++++++--------------------------------
 1 file changed, 13 insertions(+), 40 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Cong Wang Dec. 20, 2013, 9:35 p.m. UTC | #1 
On Fri, Dec 20, 2013 at 12:32 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> list_for_each_entry(a, &act_base, head) doesn't
> exit with a = NULL if we reached the end of the list.
>
> tcf_unregister_action(), tc_lookup_action_n() and tc_lookup_action()
> need fixes.
>
> Remove tc_lookup_action_id() as its unused and not worth 'fixing'

I even don't want to touch the #if 0 code block. :)

>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 1f747c26c48b ("net_sched: convert tc_action_ops to use struct list_head")
> Cc: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Dec. 20, 2013, 10:07 p.m. UTC | #2 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 20 Dec 2013 12:32:32 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> list_for_each_entry(a, &act_base, head) doesn't
> exit with a = NULL if we reached the end of the list.
> 
> tcf_unregister_action(), tc_lookup_action_n() and tc_lookup_action()
> need fixes.
> 
> Remove tc_lookup_action_id() as its unused and not worth 'fixing'
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 1f747c26c48b ("net_sched: convert tc_action_ops to use struct list_head")

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8114fef308d9..dce2b6ecdbd8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -291,12 +291,12 @@  int tcf_unregister_action(struct tc_action_ops *act)
 	int err = -ENOENT;
 
 	write_lock(&act_mod_lock);
-	list_for_each_entry(a, &act_base, head)
-		if (a == act)
+	list_for_each_entry(a, &act_base, head) {
+		if (a == act) {
+			list_del(&act->head);
+			err = 0;
 			break;
-	if (a) {
-		list_del(&act->head);
-		err = 0;
+		}
 	}
 	write_unlock(&act_mod_lock);
 	return err;
@@ -306,68 +306,41 @@  EXPORT_SYMBOL(tcf_unregister_action);
 /* lookup by name */
 static struct tc_action_ops *tc_lookup_action_n(char *kind)
 {
-	struct tc_action_ops *a = NULL;
+	struct tc_action_ops *a, *res = NULL;
 
 	if (kind) {
 		read_lock(&act_mod_lock);
 		list_for_each_entry(a, &act_base, head) {
 			if (strcmp(kind, a->kind) == 0) {
-				if (!try_module_get(a->owner)) {
-					read_unlock(&act_mod_lock);
-					return NULL;
-				}
+				if (try_module_get(a->owner))
+					res = a;
 				break;
 			}
 		}
 		read_unlock(&act_mod_lock);
 	}
-	return a;
+	return res;
 }
 
 /* lookup by nlattr */
 static struct tc_action_ops *tc_lookup_action(struct nlattr *kind)
 {
-	struct tc_action_ops *a = NULL;
+	struct tc_action_ops *a, *res = NULL;
 
 	if (kind) {
 		read_lock(&act_mod_lock);
 		list_for_each_entry(a, &act_base, head) {
 			if (nla_strcmp(kind, a->kind) == 0) {
-				if (!try_module_get(a->owner)) {
-					read_unlock(&act_mod_lock);
-					return NULL;
-				}
+				if (try_module_get(a->owner))
+					res = a;
 				break;
 			}
 		}
 		read_unlock(&act_mod_lock);
 	}
-	return a;
+	return res;
 }
 
-#if 0
-/* lookup by id */
-static struct tc_action_ops *tc_lookup_action_id(u32 type)
-{
-	struct tc_action_ops *a = NULL;
-
-	if (type) {
-		read_lock(&act_mod_lock);
-		for (a = act_base; a; a = a->next) {
-			if (a->type == type) {
-				if (!try_module_get(a->owner)) {
-					read_unlock(&act_mod_lock);
-					return NULL;
-				}
-				break;
-			}
-		}
-		read_unlock(&act_mod_lock);
-	}
-	return a;
-}
-#endif
-
 int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
 		    struct tcf_result *res)
 {