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: <CAMArcTVeFGqA2W26=rBD5KkjRpFB6gjSgXj8dp+WWrrwJ7pr-A@mail.gmail.com>
Date:   Sat, 5 Oct 2019 18:13:30 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Johannes Berg <johannes@...solutions.net>
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 Tue, 1 Oct 2019 at 16:25, Johannes Berg <johannes@...solutions.net> wrote:
>
> Hi,
>

Hi,

> > > I didn't see any discussion on this, but perhaps I missed it? The cost
> > > would be a bigger netdev struct (when lockdep is enabled), but we
> > > already have that for all the VLANs etc. it's just in the private data,
> > > so it's not a _huge_ difference really I'd think, and this is quite a
> > > bit of code for each device type now.
> >
> > Actually I agree with your opinion.
> > The benefits of this way are to be able to make common helper functions.
> > That would reduce duplicate codes and we can maintain this more easily.
> > But I'm not sure about the overhead of this way. So I would like to ask
> > maintainers and more reviewers about this.
>
> :-)
>
> > Using "struct nested_netdev_lockdep" looks really good.
> > I will make common codes such as "struct nested_netdev_lockdep"
> > and "netdev_devinit_nested_lockdep" and others in a v5 patch.
>
> That makes *sense*, but it seems to me that for example in virt_wifi we
> just missed this part completely, so addressing it in the generic code
> would still reduce overall code and complexity?
>

Yes, you're right,
Virt_wifi has the same problem. I will fix this in a v5 patch!

> Actually, looking at net-next, we already have
> netdev_lockdep_set_classes() as a macro there that handles all this. I
> guess having it as a macro makes some sense so it "evaporates" when
> lockdep isn't enabled.
>
>
> I'd probably try that but maybe somebody else can chime in and say what
> they think about applying that to *every* netdev instead though.
>

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's not really clear to me is why the qdisc locks can actually stay
> > > the same at all levels? Can they just never nest? But then why are they
> > > different per device type?
> >
> > I didn't test about qdisc so I didn't modify code related to qdisc code.
> > If someone reviews this, I would really appreciate.
>
> I didn't really think hard about it when I wrote this ...
>
> 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. This patch already
handles the _xmit_lock key. so I think there is no problem.
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.

> johannes
>

Thank you,
Taehee