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:   Wed, 17 Jun 2020 00:03:27 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Cong Wang <xiyou.wangcong@...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 Tue, 16 Jun 2020 at 06:33, Cong Wang <xiyou.wangcong@...il.com> wrote:
>

Hi Cong,

> 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?
>

I agree with that.
And, do you have any plan to replace netif_addr_lock_bh() with
netif_addr_lock_nested()?
(Of course, it needs BH handling code)
I'm not sure but I think it would be needed.

> >
> > >>>        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.
>

Yes, you're right.

Thanks!
Taehee Yoo