linux-kernel - Re: [PATCH v2 13/13] nubus: Add support for the driver model

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]

Date:   Fri, 24 Nov 2017 10:40:20 +1100 (AEDT)
From:   Finn Thain <fthain@...egraphics.com.au>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-m68k@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/13] nubus: Add support for the driver model

On Thu, 23 Nov 2017, Greg Kroah-Hartman wrote:

> On Thu, Nov 23, 2017 at 11:24:38AM +1100, Finn Thain wrote:
> > On Mon, 20 Nov 2017, I wrote:
> > 
> > > > You need to free up the memory allocated, and I don't see that 
> > > > happening here ... The kernel should yell at you ...
> > 
> > > 
> > >                 WARN(1, KERN_ERR "Device '%s' does not have a release() "
> > >                         "function, it is broken and must be fixed.\n",
> > >                         dev_name(dev));
> > > 
> > > This won't fire unless device_del() is called, right?
> > 
> > Sorry, I should have written, "This won't fire unless 
> > device_unregister() is called, right?" -- though I guess it could be 
> > any call to put_device().
> > 
> > If need be I can add code to cleanly tear down the bus devices and the 
> > associated linked lists and procfs structures, just prior to kernel 
> > termination, as a kernel exitcall. But I don't see this pattern in 
> > use.
> 
> When the kernel shuts down, no, the devices are not removed.
> 
> But what happens when the bus code is unloaded if it is built as a 
> module?  The devices will be removed then.  Or they should be.
> 

This bus driver is not a module.

> So please implement the remove device code path,

OK.

> just because some other busses are buggy that way does not mean you need 
> to duplicate their incorrect behavior.
> 

Actually, I think the bug is in porting.txt, when it says "Optionally, the 
bus driver may set the device's name and release fields."

> > 
> > It's not clear to me that the extra complexity is worth it. This may 
> > explain the other devices which never get unregistered (e.g. 
> > rtc_device, rtc_efi_dev, etc.)
> > 
> > I've read Documentation/driver-model/ and watched your presentations 
> > on this topic but it's unclear to me whether you are saying in this 
> > thread that calling device_unregister() is mandatory.
> > 
> > It sounds like you are saying that a non-NULL device.release method is 
> > mandatory (which is easily solved with an empty function). But 
> > Documentation/driver-model/porting.txt says the release method is 
> > optional.
> 
> If you provide a non-NULL empty release function, you get to be made fun 
> of, as per the in-kernel kobject documentation.  The kernel is trying to 
> save you from yourself, that warning is not there just to try to work 
> around.
> 

That warning never shows up at all, because it would only ever appear at 
device_unregister() time, rather than at device_register() time.

Anyway, I will read the in-kernel comments in the kobject code. Thanks for 
the tip.

-- 

> thanks,
> 
> greg k-h