netdev - Re: [patchlet] r8169: fix napi_schedule_irqoff() called with irqs enabled warning

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: <e65ea1a0-fa97-5d07-fbbf-4071f91e2429@gmail.com>
Date:   Fri, 16 Oct 2020 21:15:32 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Mike Galbraith <efault@....de>, Vladimir Oltean <olteanv@...il.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>
Subject: Re: [patchlet] r8169: fix napi_schedule_irqoff() called with irqs
 enabled warning

On 16.10.2020 19:11, Mike Galbraith wrote:
> On Fri, 2020-10-16 at 17:26 +0300, Vladimir Oltean wrote:
>> On Fri, Oct 16, 2020 at 01:34:55PM +0200, Heiner Kallweit wrote:
>>> I'm aware of the topic, but missing the benefits of the irqoff version
>>> unconditionally doesn't seem to be the best option.
>>
>> What are the benefits of the irqoff version? As far as I see it, the
>> only use case for that function is when the caller has _explicitly_
>> disabled interrupts.
> 
> Yeah, it's a straight up correctness issue as it sits.  There is a
> dinky bit of overhead added to the general case when using the correct
> function though, at least on x86.  I personally don't see why we should
> care deeply enough to want to add more code to avoid it given there are
> about a zillions places where we do the same for the same reason, but
> that's a maintainer call.
> 
Of course switching back to napi_schedule() is the easiest solution,
and also for r8169 we may come to the conclusion that it's the best one.
(or, considering full RT, we may even remove the irqoff version completely)
But we should spend at least a few thoughts on whether and how the
irqoff version could be improved. This would have two benefits:
- avoid the local_irq_save/local_irq_restore overhead (architecture-dependent)
- automatically fix all drivers using the irqoff version
If others go the easy way, then this doesn't mean that we must not think
about a better way.