lkml.org 
[lkml]   [2015]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.1 148/159] net: sched: fix refcount imbalance in actions
    Date
    4.1-stable review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Daniel Borkmann <daniel@iogearbox.net>

    [ Upstream commit 28e6b67f0b292f557468c139085303b15f1a678f ]

    Since commit 55334a5db5cd ("net_sched: act: refuse to remove bound action
    outside"), we end up with a wrong reference count for a tc action.

    Test case 1:

    FOO="1,6 0 0 4294967295,"
    BAR="1,6 0 0 4294967294,"
    tc filter add dev foo parent 1: bpf bytecode "$FOO" flowid 1:1 \
    action bpf bytecode "$FOO"
    tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967295' default-action pipe
    index 1 ref 1 bind 1
    tc actions replace action bpf bytecode "$BAR" index 1
    tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967294' default-action pipe
    index 1 ref 2 bind 1
    tc actions replace action bpf bytecode "$FOO" index 1
    tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967295' default-action pipe
    index 1 ref 3 bind 1

    Test case 2:

    FOO="1,6 0 0 4294967295,"
    tc filter add dev foo parent 1: bpf bytecode "$FOO" flowid 1:1 action ok
    tc actions show action gact
    action order 0: gact action pass
    random type none pass val 0
    index 1 ref 1 bind 1
    tc actions add action drop index 1
    RTNETLINK answers: File exists [...]
    tc actions show action gact
    action order 0: gact action pass
    random type none pass val 0
    index 1 ref 2 bind 1
    tc actions add action drop index 1
    RTNETLINK answers: File exists [...]
    tc actions show action gact
    action order 0: gact action pass
    random type none pass val 0
    index 1 ref 3 bind 1

    What happens is that in tcf_hash_check(), we check tcf_common for a given
    index and increase tcfc_refcnt and conditionally tcfc_bindcnt when we've
    found an existing action. Now there are the following cases:

    1) We do a late binding of an action. In that case, we leave the
    tcfc_refcnt/tcfc_bindcnt increased and are done with the ->init()
    handler. This is correctly handeled.

    2) We replace the given action, or we try to add one without replacing
    and find out that the action at a specific index already exists
    (thus, we go out with error in that case).

    In case of 2), we have to undo the reference count increase from
    tcf_hash_check() in the tcf_hash_check() function. Currently, we fail to
    do so because of the 'tcfc_bindcnt > 0' check which bails out early with
    an -EPERM error.

    Now, while commit 55334a5db5cd prevents 'tc actions del action ...' on an
    already classifier-bound action to drop the reference count (which could
    then become negative, wrap around etc), this restriction only accounts for
    invocations outside a specific action's ->init() handler.

    One possible solution would be to add a flag thus we possibly trigger
    the -EPERM ony in situations where it is indeed relevant.

    After the patch, above test cases have correct reference count again.

    Fixes: 55334a5db5cd ("net_sched: act: refuse to remove bound action outside")
    Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
    Reviewed-by: Cong Wang <cwang@twopensource.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    include/net/act_api.h | 8 +++++++-
    net/sched/act_api.c | 11 ++++++-----
    2 files changed, 13 insertions(+), 6 deletions(-)

    --- a/include/net/act_api.h
    +++ b/include/net/act_api.h
    @@ -99,7 +99,6 @@ struct tc_action_ops {

    int tcf_hash_search(struct tc_action *a, u32 index);
    void tcf_hash_destroy(struct tc_action *a);
    -int tcf_hash_release(struct tc_action *a, int bind);
    u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo);
    int tcf_hash_check(u32 index, struct tc_action *a, int bind);
    int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
    @@ -107,6 +106,13 @@ int tcf_hash_create(u32 index, struct nl
    void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
    void tcf_hash_insert(struct tc_action *a);

    +int __tcf_hash_release(struct tc_action *a, bool bind, bool strict);
    +
    +static inline int tcf_hash_release(struct tc_action *a, bool bind)
    +{
    + return __tcf_hash_release(a, bind, false);
    +}
    +
    int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
    int tcf_unregister_action(struct tc_action_ops *a);
    int tcf_action_destroy(struct list_head *actions, int bind);
    --- a/net/sched/act_api.c
    +++ b/net/sched/act_api.c
    @@ -45,7 +45,7 @@ void tcf_hash_destroy(struct tc_action *
    }
    EXPORT_SYMBOL(tcf_hash_destroy);

    -int tcf_hash_release(struct tc_action *a, int bind)
    +int __tcf_hash_release(struct tc_action *a, bool bind, bool strict)
    {
    struct tcf_common *p = a->priv;
    int ret = 0;
    @@ -53,7 +53,7 @@ int tcf_hash_release(struct tc_action *a
    if (p) {
    if (bind)
    p->tcfc_bindcnt--;
    - else if (p->tcfc_bindcnt > 0)
    + else if (strict && p->tcfc_bindcnt > 0)
    return -EPERM;

    p->tcfc_refcnt--;
    @@ -64,9 +64,10 @@ int tcf_hash_release(struct tc_action *a
    ret = 1;
    }
    }
    +
    return ret;
    }
    -EXPORT_SYMBOL(tcf_hash_release);
    +EXPORT_SYMBOL(__tcf_hash_release);

    static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
    struct tc_action *a)
    @@ -136,7 +137,7 @@ static int tcf_del_walker(struct sk_buff
    head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
    hlist_for_each_entry_safe(p, n, head, tcfc_head) {
    a->priv = p;
    - ret = tcf_hash_release(a, 0);
    + ret = __tcf_hash_release(a, false, true);
    if (ret == ACT_P_DELETED) {
    module_put(a->ops->owner);
    n_i++;
    @@ -413,7 +414,7 @@ int tcf_action_destroy(struct list_head
    int ret = 0;

    list_for_each_entry_safe(a, tmp, actions, list) {
    - ret = tcf_hash_release(a, bind);
    + ret = __tcf_hash_release(a, bind, true);
    if (ret == ACT_P_DELETED)
    module_put(a->ops->owner);
    else if (ret < 0)



    \
     
     \ /
      Last update: 2015-09-26 23:41    [W:4.118 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site