Syzkaller reports a memory leak as follows:
====================================
BUG: memory leak
unreferenced object 0xffff88810c287f00 (size 256):
comm "syz-executor105", pid 3600, jiffies 4294943292 (age 12.990s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff814cf9f0>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1046
[<ffffffff839c9e07>] kmalloc include/linux/slab.h:576 [inline]
[<ffffffff839c9e07>] kmalloc_array include/linux/slab.h:627 [inline]
[<ffffffff839c9e07>] kcalloc include/linux/slab.h:659 [inline]
[<ffffffff839c9e07>] tcf_exts_init include/net/pkt_cls.h:250 [inline]
[<ffffffff839c9e07>] tcindex_set_parms+0xa7/0xbe0 net/sched/cls_tcindex.c:342
[<ffffffff839caa1f>] tcindex_change+0xdf/0x120 net/sched/cls_tcindex.c:553
[<ffffffff8394db62>] tc_new_tfilter+0x4f2/0x1100 net/sched/cls_api.c:2147
[<ffffffff8389e91c>] rtnetlink_rcv_msg+0x4dc/0x5d0 net/core/rtnetlink.c:6082
[<ffffffff839eba67>] netlink_rcv_skb+0x87/0x1d0 net/netlink/af_netlink.c:2540
[<ffffffff839eab87>] netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
[<ffffffff839eab87>] netlink_unicast+0x397/0x4c0 net/netlink/af_netlink.c:1345
[<ffffffff839eb046>] netlink_sendmsg+0x396/0x710 net/netlink/af_netlink.c:1921
[<ffffffff8383e796>] sock_sendmsg_nosec net/socket.c:714 [inline]
[<ffffffff8383e796>] sock_sendmsg+0x56/0x80 net/socket.c:734
[<ffffffff8383eb08>] ____sys_sendmsg+0x178/0x410 net/socket.c:2482
[<ffffffff83843678>] ___sys_sendmsg+0xa8/0x110 net/socket.c:2536
[<ffffffff838439c5>] __sys_sendmmsg+0x105/0x330 net/socket.c:2622
[<ffffffff83843c14>] __do_sys_sendmmsg net/socket.c:2651 [inline]
[<ffffffff83843c14>] __se_sys_sendmmsg net/socket.c:2648 [inline]
[<ffffffff83843c14>] __x64_sys_sendmmsg+0x24/0x30 net/socket.c:2648
[<ffffffff84605fd5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
[<ffffffff84605fd5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
[<ffffffff84800087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
====================================
Kernel uses tcindex_change() to change an existing
filter properties. During the process of changing,
kernel uses tcindex_alloc_perfect_hash() to newly
allocate filter results, uses tcindex_filter_result_init()
to clear the old filter result.
Yet the problem is that, kernel clears the old
filter result, without destroying its tcf_exts structure,
which triggers the above memory leak.
Considering that there already extis a tc_filter_wq workqueue
to destroy the old tcindex_data by tcindex_partial_destroy_work()
at the end of tcindex_set_parms(), this patch solves this memory
leak bug by removing this old filter result clearing part,
and delegating it to the tc_filter_wq workqueue.
[Thanks to the suggestion from Jakub Kicinski, Cong Wang, Paolo Abeni
and Dmitry Vyukov]
Fixes: b9a24bb76bf6 ("net_sched: properly handle failure case of tcf_exts_init()")
Link: https://lore.kernel.org/all/[email protected]/
Reported-by: [email protected]
Tested-by: [email protected]
Cc: Cong Wang <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Signed-off-by: Hawkins Jiawei <[email protected]>
---
v3:
- refactor the commit message
- delegate the tcf_exts_destroy() to tc_filter_wq workqueue,
suggested by Paolo Abeni and Dmitry Vyukov
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
net/sched/cls_tcindex.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 1c9eeb98d826..3f4e7a6cdd96 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -478,14 +478,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
tcf_bind_filter(tp, &cr, base);
}
- if (old_r && old_r != r) {
- err = tcindex_filter_result_init(old_r, cp, net);
- if (err < 0) {
- kfree(f);
- goto errout_alloc;
- }
- }
-
oldp = p;
r->res = cr;
tcf_exts_change(&r->exts, &e);
--
2.34.1
On Tue, 2022-11-29 at 10:52 +0800, Hawkins Jiawei wrote:
> Syzkaller reports a memory leak as follows:
> ====================================
> BUG: memory leak
> unreferenced object 0xffff88810c287f00 (size 256):
> comm "syz-executor105", pid 3600, jiffies 4294943292 (age 12.990s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff814cf9f0>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1046
> [<ffffffff839c9e07>] kmalloc include/linux/slab.h:576 [inline]
> [<ffffffff839c9e07>] kmalloc_array include/linux/slab.h:627 [inline]
> [<ffffffff839c9e07>] kcalloc include/linux/slab.h:659 [inline]
> [<ffffffff839c9e07>] tcf_exts_init include/net/pkt_cls.h:250 [inline]
> [<ffffffff839c9e07>] tcindex_set_parms+0xa7/0xbe0 net/sched/cls_tcindex.c:342
> [<ffffffff839caa1f>] tcindex_change+0xdf/0x120 net/sched/cls_tcindex.c:553
> [<ffffffff8394db62>] tc_new_tfilter+0x4f2/0x1100 net/sched/cls_api.c:2147
> [<ffffffff8389e91c>] rtnetlink_rcv_msg+0x4dc/0x5d0 net/core/rtnetlink.c:6082
> [<ffffffff839eba67>] netlink_rcv_skb+0x87/0x1d0 net/netlink/af_netlink.c:2540
> [<ffffffff839eab87>] netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
> [<ffffffff839eab87>] netlink_unicast+0x397/0x4c0 net/netlink/af_netlink.c:1345
> [<ffffffff839eb046>] netlink_sendmsg+0x396/0x710 net/netlink/af_netlink.c:1921
> [<ffffffff8383e796>] sock_sendmsg_nosec net/socket.c:714 [inline]
> [<ffffffff8383e796>] sock_sendmsg+0x56/0x80 net/socket.c:734
> [<ffffffff8383eb08>] ____sys_sendmsg+0x178/0x410 net/socket.c:2482
> [<ffffffff83843678>] ___sys_sendmsg+0xa8/0x110 net/socket.c:2536
> [<ffffffff838439c5>] __sys_sendmmsg+0x105/0x330 net/socket.c:2622
> [<ffffffff83843c14>] __do_sys_sendmmsg net/socket.c:2651 [inline]
> [<ffffffff83843c14>] __se_sys_sendmmsg net/socket.c:2648 [inline]
> [<ffffffff83843c14>] __x64_sys_sendmmsg+0x24/0x30 net/socket.c:2648
> [<ffffffff84605fd5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> [<ffffffff84605fd5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> [<ffffffff84800087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> ====================================
>
> Kernel uses tcindex_change() to change an existing
> filter properties. During the process of changing,
> kernel uses tcindex_alloc_perfect_hash() to newly
> allocate filter results, uses tcindex_filter_result_init()
> to clear the old filter result.
>
> Yet the problem is that, kernel clears the old
> filter result, without destroying its tcf_exts structure,
> which triggers the above memory leak.
>
> Considering that there already extis a tc_filter_wq workqueue
> to destroy the old tcindex_data by tcindex_partial_destroy_work()
> at the end of tcindex_set_parms(), this patch solves this memory
> leak bug by removing this old filter result clearing part,
> and delegating it to the tc_filter_wq workqueue.
>
> [Thanks to the suggestion from Jakub Kicinski, Cong Wang, Paolo Abeni
> and Dmitry Vyukov]
>
> Fixes: b9a24bb76bf6 ("net_sched: properly handle failure case of tcf_exts_init()")
> Link: https://lore.kernel.org/all/[email protected]/
> Reported-by: [email protected]
> Tested-by: [email protected]
> Cc: Cong Wang <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Signed-off-by: Hawkins Jiawei <[email protected]>
The patch looks correct to me, but we are very late in this release
cycle, and I fear there is a chance of introducing some regression. The
issue addressed here is present since quite some time, I suggest to
postpone this fix to the beginning of the next release cycle.
Please, repost this patch after that 6.1 is released, thanks! (And feel
free to add my Acked-by).
Paolo
On Thu, 1 Dec 2022 at 18:24, Paolo Abeni <[email protected]> wrote:
>
> On Tue, 2022-11-29 at 10:52 +0800, Hawkins Jiawei wrote:
> > Syzkaller reports a memory leak as follows:
> > ====================================
> > BUG: memory leak
> > unreferenced object 0xffff88810c287f00 (size 256):
> > comm "syz-executor105", pid 3600, jiffies 4294943292 (age 12.990s)
> > hex dump (first 32 bytes):
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > backtrace:
> > [<ffffffff814cf9f0>] kmalloc_trace+0x20/0x90 mm/slab_common.c:1046
> > [<ffffffff839c9e07>] kmalloc include/linux/slab.h:576 [inline]
> > [<ffffffff839c9e07>] kmalloc_array include/linux/slab.h:627 [inline]
> > [<ffffffff839c9e07>] kcalloc include/linux/slab.h:659 [inline]
> > [<ffffffff839c9e07>] tcf_exts_init include/net/pkt_cls.h:250 [inline]
> > [<ffffffff839c9e07>] tcindex_set_parms+0xa7/0xbe0 net/sched/cls_tcindex.c:342
> > [<ffffffff839caa1f>] tcindex_change+0xdf/0x120 net/sched/cls_tcindex.c:553
> > [<ffffffff8394db62>] tc_new_tfilter+0x4f2/0x1100 net/sched/cls_api.c:2147
> > [<ffffffff8389e91c>] rtnetlink_rcv_msg+0x4dc/0x5d0 net/core/rtnetlink.c:6082
> > [<ffffffff839eba67>] netlink_rcv_skb+0x87/0x1d0 net/netlink/af_netlink.c:2540
> > [<ffffffff839eab87>] netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
> > [<ffffffff839eab87>] netlink_unicast+0x397/0x4c0 net/netlink/af_netlink.c:1345
> > [<ffffffff839eb046>] netlink_sendmsg+0x396/0x710 net/netlink/af_netlink.c:1921
> > [<ffffffff8383e796>] sock_sendmsg_nosec net/socket.c:714 [inline]
> > [<ffffffff8383e796>] sock_sendmsg+0x56/0x80 net/socket.c:734
> > [<ffffffff8383eb08>] ____sys_sendmsg+0x178/0x410 net/socket.c:2482
> > [<ffffffff83843678>] ___sys_sendmsg+0xa8/0x110 net/socket.c:2536
> > [<ffffffff838439c5>] __sys_sendmmsg+0x105/0x330 net/socket.c:2622
> > [<ffffffff83843c14>] __do_sys_sendmmsg net/socket.c:2651 [inline]
> > [<ffffffff83843c14>] __se_sys_sendmmsg net/socket.c:2648 [inline]
> > [<ffffffff83843c14>] __x64_sys_sendmmsg+0x24/0x30 net/socket.c:2648
> > [<ffffffff84605fd5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > [<ffffffff84605fd5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > [<ffffffff84800087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > ====================================
> >
> > Kernel uses tcindex_change() to change an existing
> > filter properties. During the process of changing,
> > kernel uses tcindex_alloc_perfect_hash() to newly
> > allocate filter results, uses tcindex_filter_result_init()
> > to clear the old filter result.
> >
> > Yet the problem is that, kernel clears the old
> > filter result, without destroying its tcf_exts structure,
> > which triggers the above memory leak.
> >
> > Considering that there already extis a tc_filter_wq workqueue
> > to destroy the old tcindex_data by tcindex_partial_destroy_work()
> > at the end of tcindex_set_parms(), this patch solves this memory
> > leak bug by removing this old filter result clearing part,
> > and delegating it to the tc_filter_wq workqueue.
> >
> > [Thanks to the suggestion from Jakub Kicinski, Cong Wang, Paolo Abeni
> > and Dmitry Vyukov]
> >
> > Fixes: b9a24bb76bf6 ("net_sched: properly handle failure case of tcf_exts_init()")
> > Link: https://lore.kernel.org/all/[email protected]/
> > Reported-by: [email protected]
> > Tested-by: [email protected]
> > Cc: Cong Wang <[email protected]>
> > Cc: Jakub Kicinski <[email protected]>
> > Cc: Paolo Abeni <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > Signed-off-by: Hawkins Jiawei <[email protected]>
>
> The patch looks correct to me, but we are very late in this release
> cycle, and I fear there is a chance of introducing some regression. The
> issue addressed here is present since quite some time, I suggest to
> postpone this fix to the beginning of the next release cycle.
>
> Please, repost this patch after that 6.1 is released, thanks! (And feel
> free to add my Acked-by).
Thanks for your review.
I will retest this patch after 6.1, and repost this patch
if the patch works fine.
>
> Paolo
>
On Tue, Nov 29, 2022 at 10:52:49AM +0800, Hawkins Jiawei wrote:
> Kernel uses tcindex_change() to change an existing
> filter properties. During the process of changing,
> kernel uses tcindex_alloc_perfect_hash() to newly
> allocate filter results, uses tcindex_filter_result_init()
> to clear the old filter result.
>
> Yet the problem is that, kernel clears the old
> filter result, without destroying its tcf_exts structure,
> which triggers the above memory leak.
>
> Considering that there already extis a tc_filter_wq workqueue
> to destroy the old tcindex_data by tcindex_partial_destroy_work()
> at the end of tcindex_set_parms(), this patch solves this memory
> leak bug by removing this old filter result clearing part,
> and delegating it to the tc_filter_wq workqueue.
Hmm?? The tcindex_partial_destroy_work() is to destroy 'oldp' which is
different from 'old_r'. I mean, you seem assuming that struct
tcindex_filter_result is always from struct tcindex_data, which is not
true, check the following tcindex_lookup() which retrieves tcindex_filter_result
from struct tcindex_filter.
static struct tcindex_filter_result *tcindex_lookup(struct tcindex_data *p,
u16 key)
{
if (p->perfect) {
struct tcindex_filter_result *f = p->perfect + key;
return tcindex_filter_is_set(f) ? f : NULL;
} else if (p->h) {
struct tcindex_filter __rcu **fp;
struct tcindex_filter *f;
fp = &p->h[key % p->hash];
for (f = rcu_dereference_bh_rtnl(*fp);
f;
fp = &f->next, f = rcu_dereference_bh_rtnl(*fp))
if (f->key == key)
return &f->result;
}
return NULL;
}
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index 1c9eeb98d826..3f4e7a6cdd96 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -478,14 +478,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> tcf_bind_filter(tp, &cr, base);
> }
>
> - if (old_r && old_r != r) {
> - err = tcindex_filter_result_init(old_r, cp, net);
> - if (err < 0) {
> - kfree(f);
> - goto errout_alloc;
> - }
> - }
> -
Even if your above analysis is correct, 'old_r' becomes unused (set but not used)
now, I think you should get some compiler warning.
Thanks.
On Sun, 4 Dec 2022 at 04:19, Cong Wang <[email protected]> wrote:
>
> On Tue, Nov 29, 2022 at 10:52:49AM +0800, Hawkins Jiawei wrote:
> > Kernel uses tcindex_change() to change an existing
> > filter properties. During the process of changing,
> > kernel uses tcindex_alloc_perfect_hash() to newly
> > allocate filter results, uses tcindex_filter_result_init()
> > to clear the old filter result.
> >
> > Yet the problem is that, kernel clears the old
> > filter result, without destroying its tcf_exts structure,
> > which triggers the above memory leak.
> >
> > Considering that there already extis a tc_filter_wq workqueue
> > to destroy the old tcindex_data by tcindex_partial_destroy_work()
> > at the end of tcindex_set_parms(), this patch solves this memory
> > leak bug by removing this old filter result clearing part,
> > and delegating it to the tc_filter_wq workqueue.
>
> Hmm?? The tcindex_partial_destroy_work() is to destroy 'oldp' which is
> different from 'old_r'. I mean, you seem assuming that struct
> tcindex_filter_result is always from struct tcindex_data, which is not
> true, check the following tcindex_lookup() which retrieves tcindex_filter_result
> from struct tcindex_filter.
>
> static struct tcindex_filter_result *tcindex_lookup(struct tcindex_data *p,
> u16 key)
> {
> if (p->perfect) {
> struct tcindex_filter_result *f = p->perfect + key;
>
> return tcindex_filter_is_set(f) ? f : NULL;
> } else if (p->h) {
> struct tcindex_filter __rcu **fp;
> struct tcindex_filter *f;
>
> fp = &p->h[key % p->hash];
> for (f = rcu_dereference_bh_rtnl(*fp);
> f;
> fp = &f->next, f = rcu_dereference_bh_rtnl(*fp))
> if (f->key == key)
> return &f->result;
> }
>
> return NULL;
> }
Oh, thanks for correcting me! You are right, I wrongly assuming that
struct tcindex_filter_result is always from struct tcindex_data
`perfect` field.
But I think this patch still can fix this problem, after reviewing
the tcindex_set_parms(). Because only the `tcindex_filter_result` is
from `struct tcindex_data`, can the code reaches the deleted part
in this patch.
To be more specific, the simplified logic about original
tcindex_set_parms() is as below:
static int
tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
u32 handle, struct tcindex_data *p,
struct tcindex_filter_result *r, struct nlattr **tb,
struct nlattr *est, u32 flags, struct netlink_ext_ack *extack)
{
...
if (p->perfect) {
int i;
if (tcindex_alloc_perfect_hash(net, cp) < 0)
goto errout;
cp->alloc_hash = cp->hash;
for (i = 0; i < min(cp->hash, p->hash); i++)
cp->perfect[i].res = p->perfect[i].res;
balloc = 1;
}
cp->h = p->h;
...
if (cp->perfect)
r = cp->perfect + handle;
else
r = tcindex_lookup(cp, handle) ? : &new_filter_result;
if (old_r && old_r != r) {
err = tcindex_filter_result_init(old_r, cp, net);
if (err < 0) {
kfree(f);
goto errout_alloc;
}
}
...
}
- cp's h field is directly copied from p's h field
- if `old_r` is retrieved from struct tcindex_filter, in other word,
is retrieved from p's h field. Then the `r` should get the same value
from `tcindex_loopup(cp, handle)`.
- so `old_r == r` is true, code will never uses tcindex_filter_result_init()
to clear the old_r in such case.
So I think this patch still can fix this memory leak caused by
tcindex_filter_result_init(), But maybe I need to improve my
commit message.
Please correct me If I am wrong.
> > diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> > index 1c9eeb98d826..3f4e7a6cdd96 100644
> > --- a/net/sched/cls_tcindex.c
> > +++ b/net/sched/cls_tcindex.c
> > @@ -478,14 +478,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> > tcf_bind_filter(tp, &cr, base);
> > }
> >
> > - if (old_r && old_r != r) {
> > - err = tcindex_filter_result_init(old_r, cp, net);
> > - if (err < 0) {
> > - kfree(f);
> > - goto errout_alloc;
> > - }
> > - }
> > -
>
> Even if your above analysis is correct, 'old_r' becomes unused (set but not used)
> now, I think you should get some compiler warning.
Oh, it actually didn't trigger any compiler warning,
because there is still a used place as below:
static int
tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
u32 handle, struct tcindex_data *p,
struct tcindex_filter_result *r, struct nlattr **tb,
struct nlattr *est, u32 flags, struct netlink_ext_ack *extack)
{
struct tcindex_filter_result new_filter_result, *old_r = r;
...
err = tcindex_filter_result_init(&new_filter_result, cp, net);
if (err < 0)
goto errout_alloc;
if (old_r)
cr = r->res;
...
}
But the `old_r` and `r` has the same value here, so we can just replace
the `old_r` with `r` here, and delete the `old_r` as you suggested.
Thanks for your suggestion!
>
> Thanks.
On Mon, Dec 05, 2022 at 11:19:56PM +0800, Hawkins Jiawei wrote:
> To be more specific, the simplified logic about original
> tcindex_set_parms() is as below:
>
> static int
> tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> u32 handle, struct tcindex_data *p,
> struct tcindex_filter_result *r, struct nlattr **tb,
> struct nlattr *est, u32 flags, struct netlink_ext_ack *extack)
> {
> ...
> if (p->perfect) {
> int i;
>
> if (tcindex_alloc_perfect_hash(net, cp) < 0)
> goto errout;
> cp->alloc_hash = cp->hash;
> for (i = 0; i < min(cp->hash, p->hash); i++)
> cp->perfect[i].res = p->perfect[i].res;
> balloc = 1;
> }
> cp->h = p->h;
>
> ...
>
> if (cp->perfect)
> r = cp->perfect + handle;
We can reach here if p->perfect is non-NULL.
> else
> r = tcindex_lookup(cp, handle) ? : &new_filter_result;
>
> if (old_r && old_r != r) {
> err = tcindex_filter_result_init(old_r, cp, net);
> if (err < 0) {
> kfree(f);
> goto errout_alloc;
> }
> }
> ...
> }
>
> - cp's h field is directly copied from p's h field
>
> - if `old_r` is retrieved from struct tcindex_filter, in other word,
> is retrieved from p's h field. Then the `r` should get the same value
> from `tcindex_loopup(cp, handle)`.
See above, 'r' can be 'cp->perfect + handle' which is newly allocated,
hence different from 'old_r'.
>
> - so `old_r == r` is true, code will never uses tcindex_filter_result_init()
> to clear the old_r in such case.
Not always.
>
> So I think this patch still can fix this memory leak caused by
> tcindex_filter_result_init(), But maybe I need to improve my
> commit message.
>
I think your patch may introduce other memory leaks and 'old_r' may
be left as obsoleted too.
Thanks.
On Sun, 11 Dec 2022 at 05:29, Cong Wang <[email protected]> wrote:
>
> On Mon, Dec 05, 2022 at 11:19:56PM +0800, Hawkins Jiawei wrote:
> > To be more specific, the simplified logic about original
> > tcindex_set_parms() is as below:
> >
> > static int
> > tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> > u32 handle, struct tcindex_data *p,
> > struct tcindex_filter_result *r, struct nlattr **tb,
> > struct nlattr *est, u32 flags, struct netlink_ext_ack *extack)
> > {
> > ...
> > if (p->perfect) {
> > int i;
> >
> > if (tcindex_alloc_perfect_hash(net, cp) < 0)
> > goto errout;
> > cp->alloc_hash = cp->hash;
> > for (i = 0; i < min(cp->hash, p->hash); i++)
> > cp->perfect[i].res = p->perfect[i].res;
> > balloc = 1;
> > }
> > cp->h = p->h;
> >
> > ...
> >
> > if (cp->perfect)
> > r = cp->perfect + handle;
>
> We can reach here if p->perfect is non-NULL.
>
> > else
> > r = tcindex_lookup(cp, handle) ? : &new_filter_result;
> >
> > if (old_r && old_r != r) {
> > err = tcindex_filter_result_init(old_r, cp, net);
> > if (err < 0) {
> > kfree(f);
> > goto errout_alloc;
> > }
> > }
> > ...
> > }
> >
> > - cp's h field is directly copied from p's h field
> >
> > - if `old_r` is retrieved from struct tcindex_filter, in other word,
> > is retrieved from p's h field. Then the `r` should get the same value
> > from `tcindex_loopup(cp, handle)`.
>
> See above, 'r' can be 'cp->perfect + handle' which is newly allocated,
> hence different from 'old_r'.
But if `r` is `cp->perfect + handle`, this means `cp->perfect` is not
NULL. So `p->perfect` should not be NULL, which means `old_r` should be
`p->perfect + handle`, according to tcindex_lookup(). This is not
correct with the assumption that `old_r` is retrieved from p's h field.
>
> >
> > - so `old_r == r` is true, code will never uses tcindex_filter_result_init()
> > to clear the old_r in such case.
>
> Not always.
>
> >
> > So I think this patch still can fix this memory leak caused by
> > tcindex_filter_result_init(), But maybe I need to improve my
> > commit message.
> >
>
> I think your patch may introduce other memory leaks and 'old_r' may
> be left as obsoleted too.
I still think this patch should not introduce any memory leaks.
* If the `old_r` is not NULL, it should have only two source according
to the tcindex_lookup() - `old_r` is retrieved from `p->perfect`; or
`old_r` is retrieved from `p->h`. And if `old_r` is retrieved from `p->h`,
this means `p->perfect` is NULL.
* If the `old_r` is retrieved from `p->perfect`, kernel uses
tcindex_alloc_perfect_hash() to newly allocate the filter results.
And `r` should be `cp->perfect + handle`, which is newly allocated.
So `r != old_r` in this situation, but kernel will clears the `old_r`
at tc_filter_wq workqueue in tcindex_partial_destroy_work(), by
destroying the p->perfect. So here kernel doesn't need
tcindex_filter_result_init() to clear the old filter result, and
there is no memory leak.
* If the `old_r` is retrieved from `p->h`, then `p->perfect` is NULL
discussed above. Considering that `cp->h` is directly copied from
`p->h`, `r` should get the same value as `old_r` from tcindex_lookup().
So `r == old_r`, it will ignore the part that kernel uses
tcindex_filter_result_init() to clear the old filter result. So removing
this part of code should have no effect in this situation.
It seems that whether `old_r` is retrived from `p->h` or `p->perfect`,
it is okay to directly deleting the part that kernel uses
tcindex_filter_result_init() to clear the old filter result, without any
memory leak. But this can fix the memory leak caused by
tcindex_filter_result_init().
As for `old_r` may be left as obsoleted, do you mean `old_r` becomes
unused(set but not used)? I think we can directly removing `old_r`.
>
> Thanks.