netdev - Re: Strangeness in ehea network driver's shutdown

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: <Y491kVZdw2lLB3yU@quatroqueijos.cascardo.eti.br>
Date:   Tue, 6 Dec 2022 14:02:09 -0300
From:   Thadeu Lima de Souza Cascardo <cascardo@...onical.com>
To:     "Guilherme G. Piccoli" <kernel@...ccoli.net>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Douglas Miller <dougmill@...ux.ibm.com>,
        netdev@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>, kernel@...gutronix.de,
        gpiccoli@...lia.com
Subject: Re: Strangeness in ehea network driver's shutdown

On Tue, Dec 06, 2022 at 01:49:01PM -0300, Guilherme G. Piccoli wrote:
> On Mon, Oct 3, 2022 at 1:36 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > On Sat, 1 Oct 2022 16:31:31 +0200 Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > while doing some cleanup I stumbled over a problem in the ehea network
> > > driver.
> > >
> > > In the driver's probe function (ehea_probe_adapter() via
> > > ehea_register_memory_hooks()) a reboot notifier is registered. When this
> > > notifier is triggered (ehea_reboot_notifier()) it unregisters the
> > > driver. I'm unsure what is the order of the actions triggered by that.
> > > Maybe the driver is unregistered twice if there are two bound devices?

I see how you would think it might be called for every bound device. That's
because ehea_register_memory_hooks is called by ehea_probe_adapter. However,
there is this test here that leads it the reboot_notifier to be registered only
once:

[...]
static int ehea_register_memory_hooks(void)
{
	int ret = 0;

	if (atomic_inc_return(&ehea_memory_hooks_registered) > 1)
	^^^^^^^^^^^^^^^^^^^^^^
		return 0;
[...]


> > > Or the reboot notifier is called under a lock and unregistering the
> > > driver (and so the devices) tries to unregister the notifier that is
> > > currently locked and so results in a deadlock? Maybe Greg or Rafael can
> > > tell about the details here?
> > >
> > > Whatever the effect is, it's strange. It makes me wonder why it's
> > > necessary to free all the resources of the driver on reboot?! I don't

As for why:

commit 2a6f4e4983918b18fe5d3fb364afe33db7139870
Author: Jan-Bernd Themann <ossthema@...ibm.com>
Date:   Fri Oct 26 14:37:28 2007 +0200

    ehea: add kexec support
    
    eHEA resources that are allocated via H_CALLs have a unique identifier each.
    These identifiers are necessary to free the resources. A reboot notifier
    is used to free all eHEA resources before the indentifiers get lost, i.e
    before kexec starts a new kernel.
    
    Signed-off-by: Jan-Bernd Themann <themann@...ibm.com>
    Signed-off-by: Jeff Garzik <jeff@...zik.org>

> > > know anything about the specifics of the affected machines, but I guess
> > > doing just the necessary stuff on reboot would be easier to understand,
> > > quicker to execute and doesn't have such strange side effects.
> > >
> > > With my lack of knowledge about the machine, the best I can do is report
> > > my findings. So don't expect a patch or testing from my side.
> >
> > Last meaningful commit to this driver FWIW:
> >
> > commit 29ab5a3b94c87382da06db88e96119911d557293
> > Author: Guilherme G. Piccoli <kernel@...ccoli.net>
> > Date:   Thu Nov 3 08:16:20 2016 -0200
> >
> > Also that's the last time we heard from Douglas AFAICT..
> 
> Hey folks, thanks for CCing me.
> 
> I've worked a bit with ehea some time ago, will need to dig up a bit
> to understand things again.
> But I'm cc'ing Cascardo - which have(/had?) a more deep knowledge on
> that - in the hopes he can discuss the issue without requiring that
> much study heh
> 
> Cheers,
> 
> 
> Guilherme