linux-kernel - Re: [watchdog] watchdog: mei_wdt: request stop on unregister

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: <20210123184744.GA61339@roeck-us.net>
Date:   Sat, 23 Jan 2021 10:47:44 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Tomas Winkler <tomas.winkler@...el.com>
Cc:     Wim Van Sebroeck <wim@...ana.be>, linux-watchdog@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Alexander Usyskin <alexander.usyskin@...el.com>,
        stable@...r.kernel.org
Subject: Re: [watchdog] watchdog: mei_wdt: request stop on unregister

Tomas,

On Thu, Jan 07, 2021 at 04:12:15PM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Jan 07, 2021 at 09:57:30PM +0200, Tomas Winkler wrote:
> > From: Alexander Usyskin <alexander.usyskin@...el.com>
> > 
> > Send the stop command to the firmware on watchdog unregister
> > to eleminate false event on suspend.
> > 
> 
> Normally the watchdog driver would not be expected to unregister
> during suspend, only when the driver is manually unloaded.
> To support suspend/resume, other watchdog drivers implement
> suspend/resume functions to stop the watchdog on suspend and
> to restart it on resume. Unloading a watchdog driver on suspend
> would also have odd implications for userspace watchdog daemons.
> 
> On top of that, it should not actually be possible to unregister
> a watchdog while it is in use (because it is open in that case
> and should be marked as busy). watchdog_stop_on_unregister()
> only serves as backup in case someone actually manages to unload
> the driver while the watchdog is running. The function was
> implemented to avoid calls to stop the watchdog in the remove
> function because I can not mathematically prove that there are
> no situations where the watchdog is unloaded while running.
> However, I have not actually been able to do that.
> 
> Are you sure this patch is doing what you expect it to do ?
> 

I have not heard anything back. I tried to understand how this
patch would resolve a problem during suspend/resume, but
I didn't find anything.

Can you maybe add a log message showing the false event that is
prevented with this patch, and some context explaining how the
patch fixes the problem ?

Thanks,
Guenter

> Thanks,
> Guenter
> 
> > Cc: <stable@...r.kernel.org>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@...el.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@...el.com>
> > ---
> >  drivers/watchdog/mei_wdt.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> > index 5391bf3e6b11..c5967d8b4256 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -382,6 +382,7 @@ static int mei_wdt_register(struct mei_wdt *wdt)
> >  
> >  	watchdog_set_drvdata(&wdt->wdd, wdt);
> >  	watchdog_stop_on_reboot(&wdt->wdd);
> > +	watchdog_stop_on_unregister(&wdt->wdd);
> >  
> >  	ret = watchdog_register_device(&wdt->wdd);
> >  	if (ret)
> > -- 
> > 2.26.2
> >