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 147/159] act_bpf: fix memory leaks when replacing bpf programs
    Date
    4.1-stable review patch.  If anyone has any objections, please let me know.

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

    From: Daniel Borkmann <daniel@iogearbox.net>

    [ Upstream commit f4eaed28c7834fc049c754f63e6988bbd73778d9 ]

    We currently trigger multiple memory leaks when replacing bpf
    actions, besides others:

    comm "tc", pid 1909, jiffies 4294851310 (age 1602.796s)
    hex dump (first 32 bytes):
    01 00 00 00 03 00 00 00 00 00 00 00 00 00 00 00 ................
    18 b0 98 6d 00 88 ff ff 00 00 00 00 00 00 00 00 ...m............
    backtrace:
    [<ffffffff817e623e>] kmemleak_alloc+0x4e/0xb0
    [<ffffffff8120a22d>] __vmalloc_node_range+0x1bd/0x2c0
    [<ffffffff8120a37a>] __vmalloc+0x4a/0x50
    [<ffffffff811a8d0a>] bpf_prog_alloc+0x3a/0xa0
    [<ffffffff816c0684>] bpf_prog_create+0x44/0xa0
    [<ffffffffa09ba4eb>] tcf_bpf_init+0x28b/0x3c0 [act_bpf]
    [<ffffffff816d7001>] tcf_action_init_1+0x191/0x1b0
    [<ffffffff816d70a2>] tcf_action_init+0x82/0xf0
    [<ffffffff816d4d12>] tcf_exts_validate+0xb2/0xc0
    [<ffffffffa09b5838>] cls_bpf_modify_existing+0x98/0x340 [cls_bpf]
    [<ffffffffa09b5cd6>] cls_bpf_change+0x1a6/0x274 [cls_bpf]
    [<ffffffff816d56e5>] tc_ctl_tfilter+0x335/0x910
    [<ffffffff816b9145>] rtnetlink_rcv_msg+0x95/0x240
    [<ffffffff816df34f>] netlink_rcv_skb+0xaf/0xc0
    [<ffffffff816b909e>] rtnetlink_rcv+0x2e/0x40
    [<ffffffff816deaaf>] netlink_unicast+0xef/0x1b0

    Issue is that the old content from tcf_bpf is allocated and needs
    to be released when we replace it. We seem to do that since the
    beginning of act_bpf on the filter and insns, later on the name as
    well.

    Example test case, after patch:

    # FOO="1,6 0 0 4294967295,"
    # BAR="1,6 0 0 4294967294,"
    # tc actions add action bpf bytecode "$FOO" index 2
    # tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967295' default-action pipe
    index 2 ref 1 bind 0
    # tc actions replace action bpf bytecode "$BAR" index 2
    # tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967294' default-action pipe
    index 2 ref 1 bind 0
    # tc actions replace action bpf bytecode "$FOO" index 2
    # tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967295' default-action pipe
    index 2 ref 1 bind 0
    # tc actions del action bpf index 2
    [...]
    # echo "scan" > /sys/kernel/debug/kmemleak
    # cat /sys/kernel/debug/kmemleak | grep "comm \"tc\"" | wc -l
    0

    Fixes: d23b8ad8ab23 ("tc: add BPF based action")
    Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    net/sched/act_bpf.c | 50 +++++++++++++++++++++++++++++++++++---------------
    1 file changed, 35 insertions(+), 15 deletions(-)

    --- a/net/sched/act_bpf.c
    +++ b/net/sched/act_bpf.c
    @@ -27,9 +27,10 @@
    struct tcf_bpf_cfg {
    struct bpf_prog *filter;
    struct sock_filter *bpf_ops;
    - char *bpf_name;
    + const char *bpf_name;
    u32 bpf_fd;
    u16 bpf_num_ops;
    + bool is_ebpf;
    };

    static int tcf_bpf(struct sk_buff *skb, const struct tc_action *act,
    @@ -200,6 +201,7 @@ static int tcf_bpf_init_from_ops(struct
    cfg->bpf_ops = bpf_ops;
    cfg->bpf_num_ops = bpf_num_ops;
    cfg->filter = fp;
    + cfg->is_ebpf = false;

    return 0;
    }
    @@ -234,18 +236,40 @@ static int tcf_bpf_init_from_efd(struct
    cfg->bpf_fd = bpf_fd;
    cfg->bpf_name = name;
    cfg->filter = fp;
    + cfg->is_ebpf = true;

    return 0;
    }

    +static void tcf_bpf_cfg_cleanup(const struct tcf_bpf_cfg *cfg)
    +{
    + if (cfg->is_ebpf)
    + bpf_prog_put(cfg->filter);
    + else
    + bpf_prog_destroy(cfg->filter);
    +
    + kfree(cfg->bpf_ops);
    + kfree(cfg->bpf_name);
    +}
    +
    +static void tcf_bpf_prog_fill_cfg(const struct tcf_bpf *prog,
    + struct tcf_bpf_cfg *cfg)
    +{
    + cfg->is_ebpf = tcf_bpf_is_ebpf(prog);
    + cfg->filter = prog->filter;
    +
    + cfg->bpf_ops = prog->bpf_ops;
    + cfg->bpf_name = prog->bpf_name;
    +}
    +
    static int tcf_bpf_init(struct net *net, struct nlattr *nla,
    struct nlattr *est, struct tc_action *act,
    int replace, int bind)
    {
    struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
    + struct tcf_bpf_cfg cfg, old;
    struct tc_act_bpf *parm;
    struct tcf_bpf *prog;
    - struct tcf_bpf_cfg cfg;
    bool is_bpf, is_ebpf;
    int ret;

    @@ -294,6 +318,9 @@ static int tcf_bpf_init(struct net *net,
    prog = to_bpf(act);
    spin_lock_bh(&prog->tcf_lock);

    + if (ret != ACT_P_CREATED)
    + tcf_bpf_prog_fill_cfg(prog, &old);
    +
    prog->bpf_ops = cfg.bpf_ops;
    prog->bpf_name = cfg.bpf_name;

    @@ -309,29 +336,22 @@ static int tcf_bpf_init(struct net *net,

    if (ret == ACT_P_CREATED)
    tcf_hash_insert(act);
    + else
    + tcf_bpf_cfg_cleanup(&old);

    return ret;

    destroy_fp:
    - if (is_ebpf)
    - bpf_prog_put(cfg.filter);
    - else
    - bpf_prog_destroy(cfg.filter);
    -
    - kfree(cfg.bpf_ops);
    - kfree(cfg.bpf_name);
    -
    + tcf_bpf_cfg_cleanup(&cfg);
    return ret;
    }

    static void tcf_bpf_cleanup(struct tc_action *act, int bind)
    {
    - const struct tcf_bpf *prog = act->priv;
    + struct tcf_bpf_cfg tmp;

    - if (tcf_bpf_is_ebpf(prog))
    - bpf_prog_put(prog->filter);
    - else
    - bpf_prog_destroy(prog->filter);
    + tcf_bpf_prog_fill_cfg(act->priv, &tmp);
    + tcf_bpf_cfg_cleanup(&tmp);
    }

    static struct tc_action_ops act_bpf_ops __read_mostly = {



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