[PATCH] iommu: fix device remove

[PATCH] iommu: fix device remove

[Rob Clark]

It looks like it *used* to make sense to free the device.  But now it is
embedded in 'struct iommu' (which is allocated or embedded in something
that the device allocated).

Spotted when testing qcom_iommu with CONFIG_DEBUG_TEST_DRIVER_REMOVE.

Fixes: 39ab955 ("iommu: Add sysfs bindings for struct iommu_device")
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/iommu/iommu-sysfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index c58351e..ad19cbb 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -34,7 +34,6 @@ static const struct attribute_group *iommu_dev_groups[] = {
 
 static void iommu_release_device(struct device *dev)
 {
-	kfree(dev);
 }
 
 static struct class iommu_class = {
-- 
2.9.3

Re: [PATCH] iommu: fix device remove

[Greg KH]

On Fri, May 05, 2017 at 02:08:37PM -0400, Rob Clark wrote:
> It looks like it *used* to make sense to free the device.  But now it is
> embedded in 'struct iommu' (which is allocated or embedded in something
> that the device allocated).
> 
> Spotted when testing qcom_iommu with CONFIG_DEBUG_TEST_DRIVER_REMOVE.
> 
> Fixes: 39ab955 ("iommu: Add sysfs bindings for struct iommu_device")
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/iommu/iommu-sysfs.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
> index c58351e..ad19cbb 100644
> --- a/drivers/iommu/iommu-sysfs.c
> +++ b/drivers/iommu/iommu-sysfs.c
> @@ -34,7 +34,6 @@ static const struct attribute_group *iommu_dev_groups[] = {
>  
>  static void iommu_release_device(struct device *dev)
>  {
> -	kfree(dev);
>  }

As per the documentation in the kernel tree, I now get to make fun of
you for doing such a crazh and foolish thing!

Come on, don't do that, a release function _HAS_ to free the memory
involved.  If not, then it is really broken...

greg k-h

Re: [PATCH] iommu: fix device remove

[Rob Clark]

On Fri, May 5, 2017 at 2:24 PM, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Fri, May 05, 2017 at 02:08:37PM -0400, Rob Clark wrote:
>> It looks like it *used* to make sense to free the device.  But now it is
>> embedded in 'struct iommu' (which is allocated or embedded in something
>> that the device allocated).
>>
>> Spotted when testing qcom_iommu with CONFIG_DEBUG_TEST_DRIVER_REMOVE.
>>
>> Fixes: 39ab955 ("iommu: Add sysfs bindings for struct iommu_device")
>> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/iommu/iommu-sysfs.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
>> index c58351e..ad19cbb 100644
>> --- a/drivers/iommu/iommu-sysfs.c
>> +++ b/drivers/iommu/iommu-sysfs.c
>> @@ -34,7 +34,6 @@ static const struct attribute_group *iommu_dev_groups[] = {
>>
>>  static void iommu_release_device(struct device *dev)
>>  {
>> -     kfree(dev);
>>  }
>
> As per the documentation in the kernel tree, I now get to make fun of
> you for doing such a crazh and foolish thing!
>
> Come on, don't do that, a release function _HAS_ to free the memory
> involved.  If not, then it is really broken...

There are shenanigans going on.. so release isn't counterpoint to a
_probe() which allocates some memory.  See iommu_device_sysfs_add().
So I'm not the one you get to make fun of ;-)

This the correct thing to do.  Whether the way the extra fake device
embedded in something allocated in the iommu driver's probe (and
free'd it *it's* _release()) stuff for iommu sysfs stuff works is
bonkers or not, I'll let someone else decide..  it was like that when
I got here.

BR,
-R

> greg k-h

Re: [PATCH] iommu: fix device remove

[Greg KH]

On Fri, May 05, 2017 at 02:56:00PM -0400, Rob Clark wrote:
> On Fri, May 5, 2017 at 2:24 PM, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Fri, May 05, 2017 at 02:08:37PM -0400, Rob Clark wrote:
> >> It looks like it *used* to make sense to free the device.  But now it is
> >> embedded in 'struct iommu' (which is allocated or embedded in something
> >> that the device allocated).
> >>
> >> Spotted when testing qcom_iommu with CONFIG_DEBUG_TEST_DRIVER_REMOVE.
> >>
> >> Fixes: 39ab955 ("iommu: Add sysfs bindings for struct iommu_device")
> >> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/iommu/iommu-sysfs.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
> >> index c58351e..ad19cbb 100644
> >> --- a/drivers/iommu/iommu-sysfs.c
> >> +++ b/drivers/iommu/iommu-sysfs.c
> >> @@ -34,7 +34,6 @@ static const struct attribute_group *iommu_dev_groups[] = {
> >>
> >>  static void iommu_release_device(struct device *dev)
> >>  {
> >> -     kfree(dev);
> >>  }
> >
> > As per the documentation in the kernel tree, I now get to make fun of
> > you for doing such a crazh and foolish thing!
> >
> > Come on, don't do that, a release function _HAS_ to free the memory
> > involved.  If not, then it is really broken...
> 
> There are shenanigans going on.. so release isn't counterpoint to a
> _probe() which allocates some memory.  See iommu_device_sysfs_add().
> So I'm not the one you get to make fun of ;-)
> 
> This the correct thing to do.  Whether the way the extra fake device
> embedded in something allocated in the iommu driver's probe (and
> free'd it *it's* _release()) stuff for iommu sysfs stuff works is
> bonkers or not, I'll let someone else decide..  it was like that when
> I got here.

If you have multiple reference counts in the same structure, your code
is wrong.  That is the root issue here that needs to be resolved.  Yes,
your patch papers over that, but again, it isn't right either.

greg k-h

Re: [PATCH] iommu: fix device remove

[Rob Clark]

On Fri, May 5, 2017 at 3:58 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, May 05, 2017 at 02:56:00PM -0400, Rob Clark wrote:
>> On Fri, May 5, 2017 at 2:24 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, May 05, 2017 at 02:08:37PM -0400, Rob Clark wrote:
>> >> It looks like it *used* to make sense to free the device.  But now it is
>> >> embedded in 'struct iommu' (which is allocated or embedded in something
>> >> that the device allocated).
>> >>
>> >> Spotted when testing qcom_iommu with CONFIG_DEBUG_TEST_DRIVER_REMOVE.
>> >>
>> >> Fixes: 39ab955 ("iommu: Add sysfs bindings for struct iommu_device")
>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >> ---
>> >>  drivers/iommu/iommu-sysfs.c | 1 -
>> >>  1 file changed, 1 deletion(-)
>> >>
>> >> diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
>> >> index c58351e..ad19cbb 100644
>> >> --- a/drivers/iommu/iommu-sysfs.c
>> >> +++ b/drivers/iommu/iommu-sysfs.c
>> >> @@ -34,7 +34,6 @@ static const struct attribute_group *iommu_dev_groups[] = {
>> >>
>> >>  static void iommu_release_device(struct device *dev)
>> >>  {
>> >> -     kfree(dev);
>> >>  }
>> >
>> > As per the documentation in the kernel tree, I now get to make fun of
>> > you for doing such a crazh and foolish thing!
>> >
>> > Come on, don't do that, a release function _HAS_ to free the memory
>> > involved.  If not, then it is really broken...
>>
>> There are shenanigans going on.. so release isn't counterpoint to a
>> _probe() which allocates some memory.  See iommu_device_sysfs_add().
>> So I'm not the one you get to make fun of ;-)
>>
>> This the correct thing to do.  Whether the way the extra fake device
>> embedded in something allocated in the iommu driver's probe (and
>> free'd it *it's* _release()) stuff for iommu sysfs stuff works is
>> bonkers or not, I'll let someone else decide..  it was like that when
>> I got here.
>
> If you have multiple reference counts in the same structure, your code
> is wrong.  That is the root issue here that needs to be resolved.  Yes,
> your patch papers over that, but again, it isn't right either.
>

fair enough, I should have been more precise and said that this patch
is "the correct thing to do for how the code works now".. as far as
bigger refactoring, I'll leave that to someone who understands why the
code works the way it currently does.  My patch at least makes things
less wrong.  (But removing an iommu is kind of a crazy thing to do so
it's perhaps a rather theoretical problem.)

BR,
-R