netdev - Re: [Patch net] net_sched: add a temporary refcnt for struct tcindex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpVrERPYoNNK+hywxLONEv3mF7f0Y37uMQ0gqVeR8E8kPQ@mail.gmail.com>
Date:   Mon, 30 Mar 2020 19:54:09 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     "Paul E . McKenney" <paulmck@...nel.org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        syzbot <syzbot+46f513c3033d592409d2@...kaller.appspotmail.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net] net_sched: add a temporary refcnt for struct tcindex_data

On Mon, Mar 30, 2020 at 7:30 PM Paul E. McKenney <paulmck@...nel.org> wrote:
>
> On Mon, Mar 30, 2020 at 04:24:42PM -0700, Cong Wang wrote:
> > On Mon, Mar 30, 2020 at 2:35 PM Paul E. McKenney <paulmck@...nel.org> wrote:
> > >
> > > On Sat, Mar 28, 2020 at 12:12:59PM -0700, Cong Wang wrote:
> > > > Although we intentionally use an ordered workqueue for all tc
> > > > filter works, the ordering is not guaranteed by RCU work,
> > > > given that tcf_queue_work() is esstenially a call_rcu().
> > > >
> > > > This problem is demostrated by Thomas:
> > > >
> > > >   CPU 0:
> > > >     tcf_queue_work()
> > > >       tcf_queue_work(&r->rwork, tcindex_destroy_rexts_work);
> > > >
> > > >   -> Migration to CPU 1
> > > >
> > > >   CPU 1:
> > > >      tcf_queue_work(&p->rwork, tcindex_destroy_work);
> > > >
> > > > so the 2nd work could be queued before the 1st one, which leads
> > > > to a free-after-free.
> > > >
> > > > Enforcing this order in RCU work is hard as it requires to change
> > > > RCU code too. Fortunately we can workaround this problem in tcindex
> > > > filter by taking a temporary refcnt, we only refcnt it right before
> > > > we begin to destroy it. This simplifies the code a lot as a full
> > > > refcnt requires much more changes in tcindex_set_parms().
> > > >
> > > > Reported-by: syzbot+46f513c3033d592409d2@...kaller.appspotmail.com
> > > > Fixes: 3d210534cc93 ("net_sched: fix a race condition in tcindex_destroy()")
> > > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > > Cc: Paul E. McKenney <paulmck@...nel.org>
> > > > Cc: Jamal Hadi Salim <jhs@...atatu.com>
> > > > Cc: Jiri Pirko <jiri@...nulli.us>
> > > > Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> > >
> > > Looks plausible, but what did you do to verify that the structures
> > > were in fact being freed?  See below for more detail.
> >
> > I ran the syzbot reproducer for about 20 minutes, there was no
> > memory leak reported after scanning.
>
> And if you (say) set the initial reference count to two instead of one,
> there is a memory leak reported, correct?

No, I didn't do an A/B test. I just added a printk right before the kfree(),
if it helps to convince you, here is one portion of the kernel log:

[   39.159298] a.out (703) used greatest stack depth: 11624 bytes left
[   39.166365] a.out (701) used greatest stack depth: 11352 bytes left
[   39.453257] freeing struct tcindex_data.
[   39.573554] freeing struct tcindex_data.
[   39.681540] freeing struct tcindex_data.
[   39.781158] freeing struct tcindex_data.
[   39.877726] freeing struct tcindex_data.
[   39.985515] freeing struct tcindex_data.
[   40.097687] freeing struct tcindex_data.
[   40.213691] freeing struct tcindex_data.
[   40.271465] device bridge_slave_1 left promiscuous mode
[   40.274078] bridge0: port 2(bridge_slave_1) entered disabled state
[   40.297258] device bridge_slave_0 left promiscuous mode
[   40.299377] bridge0: port 1(bridge_slave_0) entered disabled state
[   40.733355] device hsr_slave_0 left promiscuous mode
[   40.749322] device hsr_slave_1 left promiscuous mode
[   40.784220] team0 (unregistering): Port device team_slave_1 removed
[   40.792641] team0 (unregistering): Port device team_slave_0 removed
[   40.806302] bond0 (unregistering): (slave bond_slave_1): Releasing
backup interface
[   40.836972] bond0 (unregistering): (slave bond_slave_0): Releasing
backup interface
[   40.931688] bond0 (unregistering): Released all slaves
[   44.149970] freeing struct tcindex_data.
[   44.159568] freeing struct tcindex_data.
[   44.172786] freeing struct tcindex_data.
[   44.813214] freeing struct tcindex_data.
[   44.821857] freeing struct tcindex_data.
[   44.825064] freeing struct tcindex_data.
[   44.826889] freeing struct tcindex_data.
[   45.294254] freeing struct tcindex_data.
[   45.297980] freeing struct tcindex_data.
[   45.300623] freeing struct tcindex_data.

And no memory leak of course:

[root@...alhost tmp]# echo scan > /sys/kernel/debug/kmemleak
[root@...alhost tmp]# echo scan > /sys/kernel/debug/kmemleak
[root@...alhost tmp]# cat /sys/kernel/debug/kmemleak

Thanks.