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]

Date:   Mon, 15 Jun 2020 14:33:04 -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 Sat, Jun 13, 2020 at 9:03 AM Taehee Yoo <ap420073@...il.com> wrote:
>
> On Thu, 11 Jun 2020 at 08:21, Cong Wang <xiyou.wangcong@...il.com> wrote:
> >
>
> Hi Cong :)
>
> > 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:
> > > >
> > > > +       lockdep_set_class_and_subclass(&dev->addr_list_lock,
> > > > +                                      &vlan_netdev_addr_lock_key,
> > > > +                                      subclass);
>
> In this patch, lockdep_set_class_and_subclass() is used.
> As far as I know, this function initializes lockdep key and subclass
> value with a given variable.
> A dev->lower_level variable is used as a subclass value in this patch.
> When dev->lower_level value is changed, the subclass value of this
> lockdep key is not changed automatically.
> If this value has to be changed, additional function is needed.

Hmm, but we pass a dynamic subclass to spin_lock_nested().

So I guess I should just remove all the
lockdep_set_class_and_subclass() and leave subclass to 0?

>
> >>>        netif_addr_lock_bh(from);
> In this function, internally spin_lock_bh() is used and this function
> might use an 'initialized subclass value' not a current dev->lower_level.
> At this point, I think the lockdep splat might occur.
>
> +static inline void netif_addr_lock_nested(struct net_device *dev)
> +{
> +       spin_lock_nested(&dev->addr_list_lock, dev->lower_level);
> +}
> In this patch, you used netif_addr_lock_nested() too.
> These two subclass values could be different.
> But I'm not sure whether using spin_lock_nested with two different
> subclass values are the right way or not.

Yeah, as long as dev->lower_level is different, it should be different
subclass. I assume dev->lower_level is automatically adjusted
whenever the topology changes, like the vlan over bond case above.

Thanks.