• Daniel Borkmann's avatar
    net: sched: fix refcount imbalance in actions · 28e6b67f
    Daniel Borkmann authored
    Since commit 55334a5d ("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 55334a5d 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: 55334a5d ("net_sched: act: refuse to remove bound action outside")
    Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
    Reviewed-by: default avatarCong Wang <cwang@twopensource.com>
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    28e6b67f
act_api.h 3.88 KB