linux-kernel - Re: [PATCH 1/2] staging/comedi/drivers: release allocated I/O region if alloc

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:	Tue, 3 Jul 2012 11:18:45 +0100
From:	Ian Abbott <abbotti@....co.uk>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
CC:	Ian Abbott <ian.abbott@....co.uk>,
	Devendra Naga <devendra.aaru@...il.com>,
	Mori Hess <fmhess@...rs.sourceforge.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] staging/comedi/drivers: release allocated I/O region
 if alloc_private fails

On 2012-07-02 16:57, H Hartley Sweeten wrote:
> On Monday, July 02, 2012 1:41 AM, Ian Abbott wrote:
>> No.  The I/O region will be deallocated in fl512_detach() because
>> dev->iobase has been set non-zero.  fl512_detach() will be called by the
>> comedi core if fl512_attach() returns an error.  This is an unusual
>> aspect of the comedi drivers.
>
> I have been wondering if that aspect should be "fixed".
>
> It's more typical for kernel drivers to clean up after themselves if
> the probe/attach/init/etc. fails. And the release/detach/exit/etc.
> is only called if the driver has successfully loaded.
>
> With the comedi drivers, the "detach" is always called if the "attach"
> failed. And, of course the "detach" is called when the device is removed.
>
> Because of this the "detach" routines need to do all the checks to
> see what needs to be cleaned up. Not a big deal but it does create
> some confusion as this patch shows.

At the very least, this behaviour should be documented with a comment at 
the appropriate place in comedidev.h.

I think Comedi's current clean-up model is based on the open/close model 
of TTY driver operations, where the 'close' tty_operation is called when 
the 'open' tty_operation fails (although typically these operations 
don't do much allocation or deallocation).

> Ian, what's your opinion on this? Do you think we should refactor all
> the driver "attach" routines so they clean up on failure and fix the
> core so the "detach" is only called after a successful "attach"?

It would be nicer, although the existing mechanism does have the slight 
advantage of using less code.

If this mechanism is adopted, drivers using the new mechanism should 
also be responsible for freeing their subdevices (which will need a new 
function comedi_free_subdevices()) and their comedi_device private data. 
  (They're already responsible for freeing comedi_subdevice private data 
in their 'detach' routines.)

> This would be a pretty big patch since it affects every driver as well
> as the core.
>
> We could break it up by introducing a temporary flag in the comedi_driver
> struct that indicates if the driver has been "fixed". The core could then
> work as-is for non-updated drivers. Once all the drivers have been updated
> we then fix the core and remove the flag from all the drivers.

That sounds like a reasonable plan.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@....co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/