netdev - Re: [PATCH net v4 07/12] macvlan: use dynamic lockdep key instead of subclass

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: <bb48fca5a5ffb0a877b2bff8de07ec8090b63427.camel@sipsolutions.net>
Date:   Mon, 07 Oct 2019 13:41:14 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>, linux-wireless@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        j.vosburgh@...il.com, vfalico@...il.com,
        Andy Gospodarek <andy@...yhouse.net>,
        Jiří Pírko <jiri@...nulli.us>,
        sd@...asysnail.net, Roopa Prabhu <roopa@...ulusnetworks.com>,
        saeedm@...lanox.com, manishc@...vell.com, rahulv@...vell.com,
        kys@...rosoft.com, haiyangz@...rosoft.com,
        Stephen Hemminger <stephen@...workplumber.org>,
        sashal@...nel.org, hare@...e.de, varun@...lsio.com,
        ubraun@...ux.ibm.com, kgraul@...ux.ibm.com,
        Jay Vosburgh <jay.vosburgh@...onical.com>,
        Cody Schuffelen <schuffelen@...gle.com>, bjorn@...k.no
Subject: Re: [PATCH net v4 07/12] macvlan: use dynamic lockdep key instead
 of subclass

On Sat, 2019-10-05 at 18:13 +0900, Taehee Yoo wrote:
> 
> If we place lockdep keys into "struct net_device", this macro would be a
> little bit modified and reused. And driver code shape will not be huge
> changed. I think this way is better than this v4 way.
> So I will try it.

What I was thinking was that if we can do this for every VLAN netdev,
why shouldn't we do it for *every* netdev unconditionally? Some code
could perhaps even be simplified if this was just a general part of
netdev allocation.

> > But it seems to me the whole nesting also has to be applied here?
> > 
> > __dev_xmit_skb:
> >  * qdisc_run_begin()
> >  * sch_direct_xmit()
> >    * HARD_TX_LOCK(dev, txq, smp_processor_id());
> >    * dev_hard_start_xmit() // say this is VLAN
> >      * dev_queue_xmit() // on real_dev
> >        * __dev_xmit_skb // recursion on another netdev
> > 
> > Now if you have VLAN-in-VLAN the whole thing will recurse right?
> > 
> 
> I have checked on this routine.
> Only xmit_lock(HARD_TX_LOCK) could be nested. other
> qdisc locks(runinng, busylock) will not be nested. 

OK, I still didn't check it too closely I guess, or got confused which
lock I should look at.

> This patch already
> handles the _xmit_lock key. so I think there is no problem.

Right

> But I would like to place four lockdep keys(busylock, address,
> running, _xmit_lock) into "struct net_device" because of code complexity.
> 
> Let me know if I misunderstood anything.

Nothing to misunderstand - I was just asking/wondering why the qdisc
locks were not treated the same way.

johannes