netdev - Re: [Patch net] net: change addr_list

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAM_iQpWM5Bxj-oEuF_mYBL9Qf-eWmoVbfPCo7a=SjOJ0LnMjAA@mail.gmail.com>
Date:   Wed, 10 Jun 2020 16:21:24 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        syzbot+f3a0e80c34b3fc28ac5e@...kaller.appspotmail.com,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [Patch net] net: change addr_list_lock back to static key

On Wed, Jun 10, 2020 at 7:48 AM Taehee Yoo <ap420073@...il.com> wrote:
>
> On Tue, 9 Jun 2020 at 06:53, Cong Wang <xiyou.wangcong@...il.com> wrote:
> >
>
> Hi Cong,
> Thank you for this work!
>
> > The dynamic key update for addr_list_lock still causes troubles,
> > for example the following race condition still exists:
> >
> > CPU 0:                          CPU 1:
> > (RCU read lock)                 (RTNL lock)
> > dev_mc_seq_show()               netdev_update_lockdep_key()
> >                                   -> lockdep_unregister_key()
> >  -> netif_addr_lock_bh()
> >
> > because lockdep doesn't provide an API to update it atomically.
> > Therefore, we have to move it back to static keys and use subclass
> > for nest locking like before.
> >
>
> I'm sorry for the late reply.
> I agree that using subclass mechanism to avoid too many lockdep keys.

Avoiding too many lockdep keys is not the real goal of my patch,
its main purpose is to fix a race condition shown above. Just FYI.


> But the subclass mechanism is also not updated its subclass key
> automatically. So, if upper/lower relationship is changed,
> interface would have incorrect subclass key.
> It eventually results in lockdep warning.

So dev->lower_level is not updated accordingly? I just blindly trust
dev->lower_level, as you use it in other places too.

> And, I think this patch doesn't contain bonding and team module part.
> So, an additional patch is needed.

Hmm, dev->lower_level is generic, so is addr_list_lock.

Again, I just assume you already update dev->lower_level each time
the topology changes. I added some printk() to verify it too for my
simple bond over bond case. So, I can't immediately see what is
wrong with dev->lower_level here. Do you mind to be more specific?
Or I misunderstand your point?

Thanks!