Commit f4eaed28 authored by Daniel Borkmann's avatar Daniel Borkmann Committed by David S. Miller

act_bpf: fix memory leaks when replacing bpf programs

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: d23b8ad8 ("tc: add BPF based action")
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent f68b1231
...@@ -27,9 +27,10 @@ ...@@ -27,9 +27,10 @@
struct tcf_bpf_cfg { struct tcf_bpf_cfg {
struct bpf_prog *filter; struct bpf_prog *filter;
struct sock_filter *bpf_ops; struct sock_filter *bpf_ops;
char *bpf_name; const char *bpf_name;
u32 bpf_fd; u32 bpf_fd;
u16 bpf_num_ops; u16 bpf_num_ops;
bool is_ebpf;
}; };
static int tcf_bpf(struct sk_buff *skb, const struct tc_action *act, static int tcf_bpf(struct sk_buff *skb, const struct tc_action *act,
...@@ -207,6 +208,7 @@ static int tcf_bpf_init_from_ops(struct nlattr **tb, struct tcf_bpf_cfg *cfg) ...@@ -207,6 +208,7 @@ static int tcf_bpf_init_from_ops(struct nlattr **tb, struct tcf_bpf_cfg *cfg)
cfg->bpf_ops = bpf_ops; cfg->bpf_ops = bpf_ops;
cfg->bpf_num_ops = bpf_num_ops; cfg->bpf_num_ops = bpf_num_ops;
cfg->filter = fp; cfg->filter = fp;
cfg->is_ebpf = false;
return 0; return 0;
} }
...@@ -241,18 +243,40 @@ static int tcf_bpf_init_from_efd(struct nlattr **tb, struct tcf_bpf_cfg *cfg) ...@@ -241,18 +243,40 @@ static int tcf_bpf_init_from_efd(struct nlattr **tb, struct tcf_bpf_cfg *cfg)
cfg->bpf_fd = bpf_fd; cfg->bpf_fd = bpf_fd;
cfg->bpf_name = name; cfg->bpf_name = name;
cfg->filter = fp; cfg->filter = fp;
cfg->is_ebpf = true;
return 0; 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, static int tcf_bpf_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action *act, struct nlattr *est, struct tc_action *act,
int replace, int bind) int replace, int bind)
{ {
struct nlattr *tb[TCA_ACT_BPF_MAX + 1]; struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
struct tcf_bpf_cfg cfg, old;
struct tc_act_bpf *parm; struct tc_act_bpf *parm;
struct tcf_bpf *prog; struct tcf_bpf *prog;
struct tcf_bpf_cfg cfg;
bool is_bpf, is_ebpf; bool is_bpf, is_ebpf;
int ret; int ret;
...@@ -301,6 +325,9 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla, ...@@ -301,6 +325,9 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
prog = to_bpf(act); prog = to_bpf(act);
spin_lock_bh(&prog->tcf_lock); 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_ops = cfg.bpf_ops;
prog->bpf_name = cfg.bpf_name; prog->bpf_name = cfg.bpf_name;
...@@ -316,32 +343,22 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla, ...@@ -316,32 +343,22 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED) if (ret == ACT_P_CREATED)
tcf_hash_insert(act); tcf_hash_insert(act);
else
tcf_bpf_cfg_cleanup(&old);
return ret; return ret;
destroy_fp: destroy_fp:
if (is_ebpf) tcf_bpf_cfg_cleanup(&cfg);
bpf_prog_put(cfg.filter);
else
bpf_prog_destroy(cfg.filter);
kfree(cfg.bpf_ops);
kfree(cfg.bpf_name);
return ret; return ret;
} }
static void tcf_bpf_cleanup(struct tc_action *act, int bind) 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);
kfree(prog->bpf_ops); tcf_bpf_prog_fill_cfg(act->priv, &tmp);
kfree(prog->bpf_name); tcf_bpf_cfg_cleanup(&tmp);
} }
static struct tc_action_ops act_bpf_ops __read_mostly = { static struct tc_action_ops act_bpf_ops __read_mostly = {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment