[PATCH] devres: Freeing the drs after all release() are called

[PATCH] devres: Freeing the drs after all release() are called

[Chuansheng Liu]

In release_nodes(), it will call dr->node.release() and kfree
dr one by one.

But sometimes the previous dr maybe be used by next .release(),
such as:
[50314.855534]  [<c12b172f>] ? synchronize_irq+0x3f/0xb0
[50314.861193]  [<c12b18e9>] __free_irq+0x149/0x200
[50314.866367]  [<c12b19e3>] free_irq+0x43/0xa0
[50314.871152]  [<c12b4864>] devm_irq_release+0x14/0x20
[50314.876713]  [<c169e806>] release_nodes+0x136/0x1b0
[50314.882178]  [<c169ee79>] devres_release_all+0x39/0x60
[50314.887935]  [<c169b411>] __device_release_driver+0x71/0xd0

the free_irq() will sync the last irq handling, which maybe use
freed dr, then it will cause memory corruption.

Here split the dr kfreeing actions after all dr->node.release().

Signed-off-by: Liu, Chuansheng <chuansheng.liu@intel.com>
---
 drivers/base/devres.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 507379e..66ff0be 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -490,9 +490,11 @@ static int release_nodes(struct device *dev, struct list_head *first,
 	list_for_each_entry_safe_reverse(dr, tmp, &todo, node.entry) {
 		devres_log(dev, &dr->node, "REL");
 		dr->node.release(dev, dr->data);
-		kfree(dr);
 	}
 
+	list_for_each_entry_safe_reverse(dr, tmp, &todo, node.entry)
+		kfree(dr);
+
 	return cnt;
 }
 
-- 
1.7.9.5

Re: [PATCH] devres: Freeing the drs after all release() are called

[Greg KH]

On Wed, Nov 06, 2013 at 02:40:18PM +0800, Chuansheng Liu wrote:
> 
> In release_nodes(), it will call dr->node.release() and kfree
> dr one by one.
> 
> But sometimes the previous dr maybe be used by next .release(),
> such as:
> [50314.855534]  [<c12b172f>] ? synchronize_irq+0x3f/0xb0
> [50314.861193]  [<c12b18e9>] __free_irq+0x149/0x200
> [50314.866367]  [<c12b19e3>] free_irq+0x43/0xa0
> [50314.871152]  [<c12b4864>] devm_irq_release+0x14/0x20
> [50314.876713]  [<c169e806>] release_nodes+0x136/0x1b0
> [50314.882178]  [<c169ee79>] devres_release_all+0x39/0x60
> [50314.887935]  [<c169b411>] __device_release_driver+0x71/0xd0
> 
> the free_irq() will sync the last irq handling, which maybe use
> freed dr, then it will cause memory corruption.
> 
> Here split the dr kfreeing actions after all dr->node.release().

But you aren't really solving any problems here, how is dr being used
after the release function is called?  Who is doing it in such a way
that this change would really fix anything?

thanks,

greg k-h

RE: [PATCH] devres: Freeing the drs after all release() are called

[Liu, Chuansheng]

Hello Greg,

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, November 06, 2013 4:59 PM
> To: Liu, Chuansheng
> Cc: dmitry.torokhov@gmail.com; tj@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] devres: Freeing the drs after all release() are called
> 
> On Wed, Nov 06, 2013 at 02:40:18PM +0800, Chuansheng Liu wrote:
> >
> > In release_nodes(), it will call dr->node.release() and kfree
> > dr one by one.
> >
> > But sometimes the previous dr maybe be used by next .release(),
> > such as:
> > [50314.855534]  [<c12b172f>] ? synchronize_irq+0x3f/0xb0
> > [50314.861193]  [<c12b18e9>] __free_irq+0x149/0x200
> > [50314.866367]  [<c12b19e3>] free_irq+0x43/0xa0
> > [50314.871152]  [<c12b4864>] devm_irq_release+0x14/0x20
> > [50314.876713]  [<c169e806>] release_nodes+0x136/0x1b0
> > [50314.882178]  [<c169ee79>] devres_release_all+0x39/0x60
> > [50314.887935]  [<c169b411>] __device_release_driver+0x71/0xd0
> >
> > the free_irq() will sync the last irq handling, which maybe use
> > freed dr, then it will cause memory corruption.
> >
> > Here split the dr kfreeing actions after all dr->node.release().
> 
> But you aren't really solving any problems here, how is dr being used
> after the release function is called?  Who is doing it in such a way
> that this change would really fix anything?
> 
The driver code is as below:
_INIT() {

A = devm_kzalloc();
B= devm_request_threaded_irq(isr_handler);
C = devm_kzalloc();
}

When driver _EXIT, the devres_release_all () will be called.
The C will be kfreed before B, but when freeing irq B, the pending isr_handler() possibly
will access the memory B which has been freed.
Then the memory corruption occurred.

This patch can solve this scenario.
Thanks.

Re: [PATCH] devres: Freeing the drs after all release() are called

[tj]

Hello, Liu.

On Thu, Nov 07, 2013 at 12:27:56AM +0000, Liu, Chuansheng wrote:
> The driver code is as below:
> _INIT() {
> 
> A = devm_kzalloc();
> B= devm_request_threaded_irq(isr_handler);
> C = devm_kzalloc();
> }
> 
> When driver _EXIT, the devres_release_all () will be called.
> The C will be kfreed before B, but when freeing irq B, the pending isr_handler() possibly
> will access the memory B which has been freed.
> Then the memory corruption occurred.
> 
> This patch can solve this scenario.

Isn't the bug there IRQ being requested before all its resources are
allocated?  The proposed change just masks the underlying issue or
incorrectly ordered operations.

Thanks.

-- 
tejun

RE: [PATCH] devres: Freeing the drs after all release() are called

[Liu, Chuansheng]

> -----Original Message-----
> From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of tj@kernel.org
> Sent: Thursday, November 07, 2013 8:30 AM
> To: Liu, Chuansheng
> Cc: Greg KH; dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] devres: Freeing the drs after all release() are called
> 
> Hello, Liu.
> 
> On Thu, Nov 07, 2013 at 12:27:56AM +0000, Liu, Chuansheng wrote:
> > The driver code is as below:
> > _INIT() {
> >
> > A = devm_kzalloc();
> > B= devm_request_threaded_irq(isr_handler);
> > C = devm_kzalloc();
> > }
> >
> > When driver _EXIT, the devres_release_all () will be called.
> > The C will be kfreed before B, but when freeing irq B, the pending isr_handler()
> possibly
> > will access the memory B which has been freed.
> > Then the memory corruption occurred.
> >
> > This patch can solve this scenario.
> 
> Isn't the bug there IRQ being requested before all its resources are
> allocated?  The proposed change just masks the underlying issue or
> incorrectly ordered operations.
Yes, I knew I can put the code always like below:
A = devm_kzalloc();
C = devm_kzalloc();
...
B= devm_request_threaded_irq(isr_handler);

But, the above is just one simple coding prototype, if there are many calling:
E -- > F -- > D -- >... then to devm_kzalloc().

To be honest, it will make code too hard to always adapt the rule?
And I trying to find out every potential devm_kzalloc() before irq requesting.

Re: [PATCH] devres: Freeing the drs after all release() are called

[Dmitry Torokhov]

On Thu, Nov 07, 2013 at 12:36:56AM +0000, Liu, Chuansheng wrote:
> 
> 
> > -----Original Message-----
> > From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of tj@kernel.org
> > Sent: Thursday, November 07, 2013 8:30 AM
> > To: Liu, Chuansheng
> > Cc: Greg KH; dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] devres: Freeing the drs after all release() are called
> > 
> > Hello, Liu.
> > 
> > On Thu, Nov 07, 2013 at 12:27:56AM +0000, Liu, Chuansheng wrote:
> > > The driver code is as below:
> > > _INIT() {
> > >
> > > A = devm_kzalloc();
> > > B= devm_request_threaded_irq(isr_handler);
> > > C = devm_kzalloc();
> > > }
> > >
> > > When driver _EXIT, the devres_release_all () will be called.
> > > The C will be kfreed before B, but when freeing irq B, the pending isr_handler()
> > possibly
> > > will access the memory B which has been freed.
> > > Then the memory corruption occurred.
> > >
> > > This patch can solve this scenario.
> > 
> > Isn't the bug there IRQ being requested before all its resources are
> > allocated?  The proposed change just masks the underlying issue or
> > incorrectly ordered operations.
> Yes, I knew I can put the code always like below:
> A = devm_kzalloc();
> C = devm_kzalloc();
> ...
> B= devm_request_threaded_irq(isr_handler);
> 
> But, the above is just one simple coding prototype, if there are many calling:
> E -- > F -- > D -- >... then to devm_kzalloc().
> 
> To be honest, it will make code too hard to always adapt the rule?
> And I trying to find out every potential devm_kzalloc() before irq requesting.

Such code is already broken anyway and has to be fixed - if your IRQ
handler accesses data allocated by devm_kzalloc that you call only
_after_ you called devm_request_irq() then interrupt arriving at
unfortunate moment will crash your driver at the probing stage.

Thanks. 

-- 
Dmitry

RE: [PATCH] devres: Freeing the drs after all release() are called

[Liu, Chuansheng]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Thursday, November 07, 2013 8:47 AM
> To: Liu, Chuansheng
> Cc: tj@kernel.org; Greg KH; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] devres: Freeing the drs after all release() are called
> 
> On Thu, Nov 07, 2013 at 12:36:56AM +0000, Liu, Chuansheng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of tj@kernel.org
> > > Sent: Thursday, November 07, 2013 8:30 AM
> > > To: Liu, Chuansheng
> > > Cc: Greg KH; dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] devres: Freeing the drs after all release() are called
> > >
> > > Hello, Liu.
> > >
> > > On Thu, Nov 07, 2013 at 12:27:56AM +0000, Liu, Chuansheng wrote:
> > > > The driver code is as below:
> > > > _INIT() {
> > > >
> > > > A = devm_kzalloc();
> > > > B= devm_request_threaded_irq(isr_handler);
> > > > C = devm_kzalloc();
> > > > }
> > > >
> > > > When driver _EXIT, the devres_release_all () will be called.
> > > > The C will be kfreed before B, but when freeing irq B, the pending
> isr_handler()
> > > possibly
> > > > will access the memory B which has been freed.
> > > > Then the memory corruption occurred.
> > > >
> > > > This patch can solve this scenario.
> > >
> > > Isn't the bug there IRQ being requested before all its resources are
> > > allocated?  The proposed change just masks the underlying issue or
> > > incorrectly ordered operations.
> > Yes, I knew I can put the code always like below:
> > A = devm_kzalloc();
> > C = devm_kzalloc();
> > ...
> > B= devm_request_threaded_irq(isr_handler);
> >
> > But, the above is just one simple coding prototype, if there are many calling:
> > E -- > F -- > D -- >... then to devm_kzalloc().
> >
> > To be honest, it will make code too hard to always adapt the rule?
> > And I trying to find out every potential devm_kzalloc() before irq requesting.
> 
> Such code is already broken anyway and has to be fixed - if your IRQ
> handler accesses data allocated by devm_kzalloc that you call only
> _after_ you called devm_request_irq() then interrupt arriving at
> unfortunate moment will crash your driver at the probing stage.

Not exactly, request_irq() does not means the device irq is enabled indeed.

After using devres_release_all() to free the device resources, it hiddenly
force all drivers probe() to put the request_irq() at the end of probe() function.
Moreover, you can not call devres_alloc* again after probe()....
Otherwise, there are potential memory corruption.

And except for irq related, maybe there are other things.

So I just feel this rule is too restricted?

Re: [PATCH] devres: Freeing the drs after all release() are called

[tj]

Hello,

On Thu, Nov 07, 2013 at 12:36:56AM +0000, Liu, Chuansheng wrote:
> Yes, I knew I can put the code always like below:
> A = devm_kzalloc();
> C = devm_kzalloc();
> ...
> B= devm_request_threaded_irq(isr_handler);
> 
> But, the above is just one simple coding prototype, if there are many calling:
> E -- > F -- > D -- >... then to devm_kzalloc().
> 
> To be honest, it will make code too hard to always adapt the rule?
> And I trying to find out every potential devm_kzalloc() before irq requesting.

It isn't a good idea to paper over existing bugs from upper layer.
You realize that the above code sequence is already buggy during init
unless there's something explicitly blocking generation of irqs until
init is complete, right?  The right thing to do would be either
reordering the operations or wrapping the operation which unblocks irq
at the end of init with devres so that irq gets blocked before the
rest of release proceeds.

What we must *NOT* do is working around existing bugs in a half-assed
way from midlayer.  What if some of the devres resources are more
complex and actually need to be released by the devres release
callback instead of being freed along with @dr?  What if devres blocks
are in use and releases are done in multiple steps and irq release
belongs to a later group?

-- 
tejun

RE: [PATCH] devres: Freeing the drs after all release() are called

[Liu, Chuansheng]

Hello,

> -----Original Message-----
> From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of tj@kernel.org
> Sent: Thursday, November 07, 2013 8:52 AM
> To: Liu, Chuansheng
> Cc: Greg KH; dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] devres: Freeing the drs after all release() are called
> 
> Hello,
> 
> On Thu, Nov 07, 2013 at 12:36:56AM +0000, Liu, Chuansheng wrote:
> > Yes, I knew I can put the code always like below:
> > A = devm_kzalloc();
> > C = devm_kzalloc();
> > ...
> > B= devm_request_threaded_irq(isr_handler);
> >
> > But, the above is just one simple coding prototype, if there are many calling:
> > E -- > F -- > D -- >... then to devm_kzalloc().
> >
> > To be honest, it will make code too hard to always adapt the rule?
> > And I trying to find out every potential devm_kzalloc() before irq requesting.
> 
> It isn't a good idea to paper over existing bugs from upper layer.
> You realize that the above code sequence is already buggy during init
> unless there's something explicitly blocking generation of irqs until
> init is complete, right?  The right thing to do would be either
> reordering the operations or wrapping the operation which unblocks irq
> at the end of init with devres so that irq gets blocked before the
> rest of release proceeds.
> 
> What we must *NOT* do is working around existing bugs in a half-assed
> way from midlayer.  

Yes, doing the right order initialization is always right thing.
But normally when we hit the panic during shutdown/reboot like below:
PAGE FAULT XXX 0x12345678

It is really difficult to debug.
So at least, could we have method to expose these hidden issues?

Thanks.

I am reviewing other codes' usage of devm_request_threaded_irq() also.

Re: [PATCH] devres: Freeing the drs after all release() are called

[Greg KH]

On Thu, Nov 07, 2013 at 01:18:54AM +0000, Liu, Chuansheng wrote:
> Hello,
> 
> > -----Original Message-----
> > From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of tj@kernel.org
> > Sent: Thursday, November 07, 2013 8:52 AM
> > To: Liu, Chuansheng
> > Cc: Greg KH; dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] devres: Freeing the drs after all release() are called
> > 
> > Hello,
> > 
> > On Thu, Nov 07, 2013 at 12:36:56AM +0000, Liu, Chuansheng wrote:
> > > Yes, I knew I can put the code always like below:
> > > A = devm_kzalloc();
> > > C = devm_kzalloc();
> > > ...
> > > B= devm_request_threaded_irq(isr_handler);
> > >
> > > But, the above is just one simple coding prototype, if there are many calling:
> > > E -- > F -- > D -- >... then to devm_kzalloc().
> > >
> > > To be honest, it will make code too hard to always adapt the rule?
> > > And I trying to find out every potential devm_kzalloc() before irq requesting.
> > 
> > It isn't a good idea to paper over existing bugs from upper layer.
> > You realize that the above code sequence is already buggy during init
> > unless there's something explicitly blocking generation of irqs until
> > init is complete, right?  The right thing to do would be either
> > reordering the operations or wrapping the operation which unblocks irq
> > at the end of init with devres so that irq gets blocked before the
> > rest of release proceeds.
> > 
> > What we must *NOT* do is working around existing bugs in a half-assed
> > way from midlayer.  
> 
> Yes, doing the right order initialization is always right thing.
> But normally when we hit the panic during shutdown/reboot like below:
> PAGE FAULT XXX 0x12345678
> 
> It is really difficult to debug.
> So at least, could we have method to expose these hidden issues?

Have you enabled timer debugging?  I think there's an irq debugging
option as well.

We aren't going to paper over driver bugs by changing the kernel core,
sorry.  Consider this patch dropped.

greg k-h