linux-kernel - RE: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver

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] [day] [month] [year] [list]

Date:	Wed, 2 Dec 2015 07:41:21 +0000
From:	"Winkler, Tomas" <tomas.winkler@...el.com>
To:	Guenter Roeck <linux@...ck-us.net>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"wim@...ana.be" <wim@...ana.be>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Usyskin, Alexander" <alexander.usyskin@...el.com>,
	"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>
Subject: RE: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver


> >>
> >> Any special reason for having two data structures instead of one ?
> >> You could just move the variables from struct mei_wdt_dev into
> >> struct mei_wdt, no ?
> >
> > Yes, on newer platform mei_wdt_dev might be not present in case the the
> > device is not provisioned. This came to action in the following
> > patches.
> >
> 
> It is still an implementation choice to keep the data structures separate.
> That mei_wdt_dev can be unregistered doesn't mean that the data structure
> has to be destroyed or allocated on registration.

That's correct, the issue is that if the MEI device resets or go through suspend/resume the mei_wdt will be removed destroyed
Why watchdog daemon will still holds open handler to watchdog_device and will releases it upon ping failure. This is where reference counter will come in.
Currently we do not support seamless reset. 

> >>> +
> >>> +	/* valid value is already checked by the caller */
> >>> +	wdt->timeout = timeout;
> >>> +	wdd->timeout = timeout;
> >>
> >> One of those seems unnecessary. Why keep the timeout twice ?
> >
> > We need two as wdd may not exists and we still need to send ping to
> > detect if the device is provisioned.
> >
> 
> Ok, makes sense.
> 
> >>> +
> >>> +static void mei_wdt_unregister(struct mei_wdt *wdt)
> >>> +{
> >>> +	if (!wdt->mwd)
> >>> +		return;
> >>> +
> >>> +	watchdog_unregister_device(&wdt->mwd->wdd);
> >>> +	kref_put(&wdt->mwd->refcnt, mei_wdt_release);
> >>> +	wdt->mwd = NULL;
> 
> If setting this to NULL was really needed, you would have a race condition here:
> wdt->mwd is set to NULL only after the pointer it points to was freed, leaving
> a small window where the code above could still access it.


Yes, good catch will fix that.

> 
> >>> +}
> >>
> >> Seems to me that using two separate data structures instead of one
> >> adds a lot of complexity.
> >
> > It might be reduced but I'm not sure it can be significantly simpler.
> > It the reference counter will be part of watchdog_device it would be
> > simpler.
> >
> 
> It would be there if struct watchdog_device was allocated by the
> watchdog core, which is not the case. I am still trying to create
> a situation where the local data structure (struct mei_wdt in this case)
> can be released while the watchdog device is still open
> (ie how to unprovision the watchdog device while in use).

This happen on device suspend/resume or device reset, this is not the case of provisioning.
Currently the our bus layer doesn't support seamless reconnection. 

> Once I figure that out, I hope I can find a way to protect it
> differently and drop the ref/unref callbacks. I suspect it may
> be necessary to separate struct watchdog_device into two parts:
> one used by drivers and one used by the watchdog core.
> The watchdog core would then manage its own data structure, and
> no longer depend on struct watchdog_device (owned by the driver)
> to actually be there. Different story, though.


I think that would somehow fit also our driver. 

Thanks
Tomas