netdev - Re: [RFC PATCH v2 net-next 00/15] tc-taprio offload for SJA1105 DSA

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:   Sat, 31 Aug 2019 20:03:36 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        vedang.patel@...el.com, Richard Cochran <richardcochran@...il.com>,
        weifeng.voon@...el.com, jiri@...lanox.com, m-karicheri2@...com,
        Jose.Abreu@...opsys.com,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        xiyou.wangcong@...il.com, netdev <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH v2 net-next 00/15] tc-taprio offload for SJA1105 DSA

On Sat, 31 Aug 2019 at 01:29, Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
>
> On Fri, 30 Aug 2019 13:11:11 +0300, Vladimir Oltean wrote:
> > On Fri, 30 Aug 2019 at 04:21, Jakub Kicinski wrote:
> > > On Fri, 30 Aug 2019 03:46:20 +0300, Vladimir Oltean wrote:
> > > > - Configuring the switch over SPI cannot apparently be done from this
> > > >   ndo_setup_tc callback because it runs in atomic context. I also have
> > > >   some downstream patches to offload tc clsact matchall with mirred
> > > >   action, but in that case it looks like the atomic context restriction
> > > >   does not apply.
> > >
> > > This sounds really surprising ndo_setup_tc should always be allowed to
> > > sleep. Can the taprio limitation be lifted somehow?
> >
> > I need to get more familiar with the taprio internal data structures.
> > I think you're suggesting to get those updated to a consistent state
> > while under spin_lock_bh(qdisc_lock(sch)), then call ndo_setup_tc from
> > outside that critical section?
>
> I'm not 100% sure how taprio handles locking TBH, it just seems naive
> that HW callback will not need to sleep, so the kernel should make sure
> that callback can sleep. Otherwise we'll end up with 3/4 of drivers
> implementing some async work routine...
>
> Sorry, I know that's quite general and not that helpful.
>
> > Also, I just noticed that I introduced a bug in taprio_disable_offload
> > with my reference counting addition. The qdisc can't just pass stack
> > memory to the driver, now that it's allowing it to keep it. So
> > definitely the patch needs more refactoring.
>
> Ah, a slight deja vu, I think someone else has done it in the past :)

Ok, I think I managed to move the ndo_setup_tc callback outside of
atomic context and nothing broke so far... Any other comments or
should I go ahead and send a first proper patchset?

Thanks,
-Vladimir