question regarding drivers/staging/iio/adc/ad7280a.c

question regarding drivers/staging/iio/adc/ad7280a.c

[Himangi Saraogi]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

Hi,

The code seems to have a memory leak. The function ad7280_attr_init calls
kasprintf a number of times, which calls kmalloc (or more precisely
kmalloc_track_caller), but this data does not ever seem to be freed. I
propose to introduce a devm_  version of kasprintf, which will be useful
for other files also. I am not very sure that will it be useful to
introduce a bunch of kfrees, just to remove the memory leaks immediately,
but I think it would be safer just to devm
everything, so then one is sure that everything is freed as it should be,
in the right order.

Thanks
Himangi

[-- Attachment #2: Type: text/html, Size: 673 bytes --]

Re: question regarding drivers/staging/iio/adc/ad7280a.c

[Jonathan Cameron]

On 14/07/14 21:31, Himangi Saraogi wrote:
> Hi,
>
> The code seems to have a memory leak. The function ad7280_attr_init
> calls kasprintf a number of times, which calls kmalloc (or more
> precisely kmalloc_track_caller), but this data does not ever seem to
> be freed. I propose to introduce a devm_  version of kasprintf, which
> will be useful for other files also. I am not very sure that will it
> be useful to introduce a bunch of kfrees, just to remove the memory
> leaks immediately, but I think it would be safer just to devm
> everything, so then one is sure that everything is freed as it should
> be, in the right order.
>
The question here is whether such a memory leak squashing would be
worth applying to stable.  Personally I'd go with no. In which case
feel free to fix it via the introduction of a devm version.


Jonathan
> Thanks
> Himangi

Re: question regarding drivers/staging/iio/adc/ad7280a.c

[Lars-Peter Clausen]

On 07/15/2014 07:31 PM, Jonathan Cameron wrote:
> On 14/07/14 21:31, Himangi Saraogi wrote:
>> Hi,
>>
>> The code seems to have a memory leak. The function ad7280_attr_init
>> calls kasprintf a number of times, which calls kmalloc (or more
>> precisely kmalloc_track_caller), but this data does not ever seem to
>> be freed. I propose to introduce a devm_  version of kasprintf, which
>> will be useful for other files also. I am not very sure that will it
>> be useful to introduce a bunch of kfrees, just to remove the memory
>> leaks immediately, but I think it would be safer just to devm
>> everything, so then one is sure that everything is freed as it should
>> be, in the right order.
>>
> The question here is whether such a memory leak squashing would be
> worth applying to stable.  Personally I'd go with no. In which case
> feel free to fix it via the introduction of a devm version.

devm is probably fine here.

The long term fix should be to switch the driver to use 
iio_chan_spec_ext_info infrastructure which will handle the allocation and 
freeing internally in the IIO framework.

- Lars

Re: question regarding drivers/staging/iio/adc/ad7280a.c

[Himangi Saraogi]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

Hi,

I have sent in a patch for adding devm_kasprintf
https://lkml.org/lkml/2014/7/16/667.
I will be updating this file and send in a patch once it is accepted. Was
it the right thing
to do to send the devm_kasprintf patch separately from the patch on the
file to use it for,
because the person applying the later may not see the dependency?

Thanks
Himangi


On 16 July 2014 16:44, Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 07/15/2014 07:31 PM, Jonathan Cameron wrote:
>
>> On 14/07/14 21:31, Himangi Saraogi wrote:
>>
>>> Hi,
>>>
>>> The code seems to have a memory leak. The function ad7280_attr_init
>>> calls kasprintf a number of times, which calls kmalloc (or more
>>> precisely kmalloc_track_caller), but this data does not ever seem to
>>> be freed. I propose to introduce a devm_  version of kasprintf, which
>>> will be useful for other files also. I am not very sure that will it
>>> be useful to introduce a bunch of kfrees, just to remove the memory
>>> leaks immediately, but I think it would be safer just to devm
>>> everything, so then one is sure that everything is freed as it should
>>> be, in the right order.
>>>
>>>  The question here is whether such a memory leak squashing would be
>> worth applying to stable.  Personally I'd go with no. In which case
>> feel free to fix it via the introduction of a devm version.
>>
>
> devm is probably fine here.
>
> The long term fix should be to switch the driver to use
> iio_chan_spec_ext_info infrastructure which will handle the allocation and
> freeing internally in the IIO framework.
>
> - Lars
>
>

[-- Attachment #2: Type: text/html, Size: 2374 bytes --]

Re: question regarding drivers/staging/iio/adc/ad7280a.c

[Greg Kroah-Hartman]

On Thu, Jul 17, 2014 at 11:40:04PM +0530, Himangi Saraogi wrote:
> Hi,
> 
> I have sent in a patch for adding devm_kasprintf https://lkml.org/lkml/2014/7/
> 16/667.
> I will be updating this file and send in a patch once it is accepted. Was it
> the right thing
> to do to send the devm_kasprintf patch separately from the patch on the file to
> use it for,
> because the person applying the later may not see the dependency?

Well, you should have told me there was a dependancy :)

As I'll end up taking both of these patches in the end, I'll queue your
devm_kasprintf patch up in my staging-next tree so that it will work
properly with your patch.

thanks,

greg k-h

Re: question regarding drivers/staging/iio/adc/ad7280a.c

[Greg Kroah-Hartman]

On Thu, Jul 17, 2014 at 01:58:15PM -0700, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2014 at 11:40:04PM +0530, Himangi Saraogi wrote:
> > Hi,
> > 
> > I have sent in a patch for adding devm_kasprintf https://lkml.org/lkml/2014/7/
> > 16/667.
> > I will be updating this file and send in a patch once it is accepted. Was it
> > the right thing
> > to do to send the devm_kasprintf patch separately from the patch on the file to
> > use it for,
> > because the person applying the later may not see the dependency?
> 
> Well, you should have told me there was a dependancy :)
> 
> As I'll end up taking both of these patches in the end, I'll queue your
> devm_kasprintf patch up in my staging-next tree so that it will work
> properly with your patch.

Now applied there, so all should be good, right?

thanks,

greg k-h