RFC: Fix "tc filter show" for basic filters

RFC: Fix "tc filter show" for basic filters

[Ignacy Gawedzki]

Hi everyone,

I have a problem with tc filter and very simple "basic" filters with no
associated action.

The thing is I have an HTB qdisc with handle 1: with a single class 1:1
attached to it.  When I add a basic filter that does nothing,

  tc filter add dev eth0 prio 1 handle 1 parent 1: basic classid 1:1

it doesn't completely appear when I type

  tc filter show dev eth0

When debugging basic_dump() in net/sched/cls_basic.c, I noticed that
tcf_exts_dump_stats() returns -1, which triggers a jump to nla_put_failure
which ends up removing all the added attributes so far.  When looking at
tcf_action_copy_stats() in net/sched/act_api.c, which is ultimately called by
tcf_exts_dump_stats(), I see that the returned error is triggered by a->priv
being NULL.

Although I confess I didn't make the effort of understanding all the workings
of a->priv in this context, it seems to me that if that is NULL, this is
probably not a good reason to return an error.  I made the change below and so
far it works for me.

Can anyone take a critical look at this?

Thanks.

Ignacy

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3d43e49..6dd46be 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -602,7 +602,7 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct
tc_action *a,
        struct tcf_common *p = a->priv;
 
        if (p == NULL)
-               goto errout;
+               return 0;
 
        /* compat_mode being true specifies a call that is supposed
         * to add additional backward compatibility statistic TLVs.

-- 
Ignacy Gawędzki
R&D Engineer
Green Communications

[PATCH net 0/1] Re: RFC: Fix "tc filter show" for basic filters

[Ignacy Gawędzki]

Hi again,

After some playing around, I found that my quick-and-dirty fix didn't actually
do the job.  I eventually traced the problem to come from the fact that
tcf_exts_dump_stats() just assumed that the list of actions in exts->actions
contains at least one element and accessed it using tcf_exts_first_act().
This is clearly not true in the case of filters with no associated action in
particular, as in the case of my "basic" filter.

Simply ensuring that the list is not empty beforehand is enough to fix the
problem, just as is also done above in tcf_exts_dump().

Ignacy Gawędzki (1):
  cls_api.c: Fix dumping of non-existing actions' stats.

 net/sched/cls_api.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.1.0

[PATCH net 1/1] cls_api.c: Fix dumping of non-existing actions' stats.

[Ignacy Gawędzki]

In tcf_exts_dump_stats(), ensure that exts->actions is not empty before
accessing the first element of that list and calling tcf_action_copy_stats()
on it.  This fixes some random segvs when adding filters of type "basic" with
no particular action.

This also fixes the dumping of those "no-action" filters, which more often
than not made calls to tcf_action_copy_stats() fail and consequently netlink
attributes added by the caller to be removed by a call to nla_nest_cancel().

Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
 net/sched/cls_api.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index aad6a67..30e6967 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -602,9 +602,12 @@ EXPORT_SYMBOL(tcf_exts_dump);
 int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	struct tc_action *a = tcf_exts_first_act(exts);
-	if (tcf_action_copy_stats(skb, a, 1) < 0)
-		return -1;
+	struct tc_action *a;
+	if (!list_empty(&exts->actions)) {
+		a = tcf_exts_first_act(exts);
+		if (tcf_action_copy_stats(skb, a, 1) < 0)
+			return -1;
+	}
 #endif
 	return 0;
 }
-- 
2.1.0

Re: [PATCH net 1/1] cls_api.c: Fix dumping of non-existing actions' stats.

[Cong Wang]

On Tue, Feb 3, 2015 at 9:32 AM, Ignacy Gawędzki
<ignacy.gawedzki@green-communications.fr> wrote:
> In tcf_exts_dump_stats(), ensure that exts->actions is not empty before
> accessing the first element of that list and calling tcf_action_copy_stats()
> on it.  This fixes some random segvs when adding filters of type "basic" with
> no particular action.
>
> This also fixes the dumping of those "no-action" filters, which more often
> than not made calls to tcf_action_copy_stats() fail and consequently netlink
> attributes added by the caller to be removed by a call to nla_nest_cancel().
>
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> ---
>  net/sched/cls_api.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index aad6a67..30e6967 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -602,9 +602,12 @@ EXPORT_SYMBOL(tcf_exts_dump);
>  int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts)
>  {
>  #ifdef CONFIG_NET_CLS_ACT
> -       struct tc_action *a = tcf_exts_first_act(exts);
> -       if (tcf_action_copy_stats(skb, a, 1) < 0)
> -               return -1;
> +       struct tc_action *a;
> +       if (!list_empty(&exts->actions)) {
> +               a = tcf_exts_first_act(exts);
> +               if (tcf_action_copy_stats(skb, a, 1) < 0)
> +                       return -1;
> +       }

Hmm, or just fix tcf_exts_first_act()? Let it call list_first_entry_or_null().

Also, please add Fixes: tag.

Fixes: commit 33be627159913b094bb578e83e9a7fdc66c10208

Re: [PATCH net 1/1] cls_api.c: Fix dumping of non-existing actions' stats.

[Eric Dumazet]

On Tue, 2015-02-03 at 09:39 -0800, Cong Wang wrote:

> Also, please add Fixes: tag.
> 
> Fixes: commit 33be627159913b094bb578e83e9a7fdc66c10208

Or more exactly use this format for Fixes tag :

Fixes: 33be62715991 ("net_sched: act: use standard struct list_head")

(assuming this commit is indeed the one introducing the problem)

[PATCH net v2] cls_api.c: Fix dumping of non-existing actions' stats.

[Ignacy Gawędzki]

In tcf_exts_dump_stats(), ensure that exts->actions is not empty before
accessing the first element of that list and calling tcf_action_copy_stats()
on it.  This fixes some random segvs when adding filters of type "basic" with
no particular action.

This also fixes the dumping of those "no-action" filters, which more often
than not made calls to tcf_action_copy_stats() fail and consequently netlink
attributes added by the caller to be removed by a call to nla_nest_cancel().

Fixes: 33be62715991 ("net_sched: act: use standard struct list_head")
Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
 net/sched/cls_api.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index aad6a67..baef987 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -556,8 +556,9 @@ void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
 }
 EXPORT_SYMBOL(tcf_exts_change);
 
-#define tcf_exts_first_act(ext) \
-		list_first_entry(&(exts)->actions, struct tc_action, list)
+#define tcf_exts_first_act(ext)					\
+	list_first_entry_or_null(&(exts)->actions,		\
+				 struct tc_action, list)
 
 int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 {
@@ -603,7 +604,7 @@ int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	struct tc_action *a = tcf_exts_first_act(exts);
-	if (tcf_action_copy_stats(skb, a, 1) < 0)
+	if (a != NULL && tcf_action_copy_stats(skb, a, 1) < 0)
 		return -1;
 #endif
 	return 0;
-- 
2.1.0

Re: [PATCH net v2] cls_api.c: Fix dumping of non-existing actions' stats.

[Cong Wang]

On Tue, Feb 3, 2015 at 10:05 AM, Ignacy Gawędzki
<ignacy.gawedzki@green-communications.fr> wrote:
> In tcf_exts_dump_stats(), ensure that exts->actions is not empty before
> accessing the first element of that list and calling tcf_action_copy_stats()
> on it.  This fixes some random segvs when adding filters of type "basic" with
> no particular action.
>
> This also fixes the dumping of those "no-action" filters, which more often
> than not made calls to tcf_action_copy_stats() fail and consequently netlink
> attributes added by the caller to be removed by a call to nla_nest_cancel().
>
> Fixes: 33be62715991 ("net_sched: act: use standard struct list_head")
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>

Acked-by: Cong Wang <cwang@twopensource.com>

Thanks!

Re: [PATCH net 1/1] cls_api.c: Fix dumping of non-existing actions' stats.

[David Miller]

From: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
Date: Tue, 3 Feb 2015 18:32:51 +0100

> In tcf_exts_dump_stats(), ensure that exts->actions is not empty before
> accessing the first element of that list and calling tcf_action_copy_stats()
> on it.  This fixes some random segvs when adding filters of type "basic" with
> no particular action.
> 
> This also fixes the dumping of those "no-action" filters, which more often
> than not made calls to tcf_action_copy_stats() fail and consequently netlink
> attributes added by the caller to be removed by a call to nla_nest_cancel().
> 
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>

Applied, thanks.

Re: [PATCH net 1/1] cls_api.c: Fix dumping of non-existing actions' stats.

[Cong Wang]

On Wed, Feb 4, 2015 at 8:26 PM, David Miller <davem@davemloft.net> wrote:
> From: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> Date: Tue, 3 Feb 2015 18:32:51 +0100
>
>> In tcf_exts_dump_stats(), ensure that exts->actions is not empty before
>> accessing the first element of that list and calling tcf_action_copy_stats()
>> on it.  This fixes some random segvs when adding filters of type "basic" with
>> no particular action.
>>
>> This also fixes the dumping of those "no-action" filters, which more often
>> than not made calls to tcf_action_copy_stats() fail and consequently netlink
>> attributes added by the caller to be removed by a call to nla_nest_cancel().
>>
>> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
>
> Applied, thanks.

Dave, please queue this patch for -stable as well, if you haven't done yet.
It needs to backport back to 3.14.

Thanks!

Re: [PATCH net 1/1] cls_api.c: Fix dumping of non-existing actions' stats.

[David Miller]

From: Cong Wang <cwang@twopensource.com>
Date: Tue, 24 Feb 2015 16:20:42 -0800

> On Wed, Feb 4, 2015 at 8:26 PM, David Miller <davem@davemloft.net> wrote:
>> From: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
>> Date: Tue, 3 Feb 2015 18:32:51 +0100
>>
>>> In tcf_exts_dump_stats(), ensure that exts->actions is not empty before
>>> accessing the first element of that list and calling tcf_action_copy_stats()
>>> on it.  This fixes some random segvs when adding filters of type "basic" with
>>> no particular action.
>>>
>>> This also fixes the dumping of those "no-action" filters, which more often
>>> than not made calls to tcf_action_copy_stats() fail and consequently netlink
>>> attributes added by the caller to be removed by a call to nla_nest_cancel().
>>>
>>> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
>>
>> Applied, thanks.
> 
> Dave, please queue this patch for -stable as well, if you haven't done yet.
> It needs to backport back to 3.14.

Done.