Free memory, if afiucv_iucv_init is not successful and
removing a IUCV driver.
Signed-off-by: Arvind Yadav <[email protected]>
---
net/iucv/af_iucv.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 1e8cc7b..eb0995a 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2433,9 +2433,11 @@ static int afiucv_iucv_init(void)
af_iucv_dev->driver = &af_iucv_driver;
err = device_register(af_iucv_dev);
if (err)
- goto out_driver;
+ goto out_iucv_dev;
return 0;
+out_iucv_dev:
+ kfree(af_iucv_dev);
out_driver:
driver_unregister(&af_iucv_driver);
out_iucv:
@@ -2496,6 +2498,7 @@ static void __exit afiucv_exit(void)
{
if (pr_iucv) {
device_unregister(af_iucv_dev);
+ kfree(af_iucv_dev);
driver_unregister(&af_iucv_driver);
pr_iucv->iucv_unregister(&af_iucv_handler, 0);
symbol_put(iucv_if);
--
1.9.1
On Wed, 28 Feb 2018 15:24:16 +0530
Arvind Yadav <[email protected]> wrote:
> Free memory, if afiucv_iucv_init is not successful and
> removing a IUCV driver.
>
> Signed-off-by: Arvind Yadav <[email protected]>
> ---
> net/iucv/af_iucv.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 1e8cc7b..eb0995a 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2433,9 +2433,11 @@ static int afiucv_iucv_init(void)
> af_iucv_dev->driver = &af_iucv_driver;
> err = device_register(af_iucv_dev);
> if (err)
> - goto out_driver;
> + goto out_iucv_dev;
> return 0;
>
> +out_iucv_dev:
> + kfree(af_iucv_dev);
> out_driver:
> driver_unregister(&af_iucv_driver);
> out_iucv:
> @@ -2496,6 +2498,7 @@ static void __exit afiucv_exit(void)
> {
> if (pr_iucv) {
> device_unregister(af_iucv_dev);
> + kfree(af_iucv_dev);
> driver_unregister(&af_iucv_driver);
> pr_iucv->iucv_unregister(&af_iucv_handler, 0);
> symbol_put(iucv_if);
No, you must not use kfree() after you called device_register() (even
if it was not successful!) -- see the comment for device_register().
On Wednesday 28 February 2018 04:00 PM, Cornelia Huck wrote:
> On Wed, 28 Feb 2018 15:24:16 +0530
> Arvind Yadav <[email protected]> wrote:
>
>> Free memory, if afiucv_iucv_init is not successful and
>> removing a IUCV driver.
>>
>> Signed-off-by: Arvind Yadav <[email protected]>
>> ---
>> net/iucv/af_iucv.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>> index 1e8cc7b..eb0995a 100644
>> --- a/net/iucv/af_iucv.c
>> +++ b/net/iucv/af_iucv.c
>> @@ -2433,9 +2433,11 @@ static int afiucv_iucv_init(void)
>> af_iucv_dev->driver = &af_iucv_driver;
>> err = device_register(af_iucv_dev);
>> if (err)
>> - goto out_driver;
>> + goto out_iucv_dev;
>> return 0;
>>
>> +out_iucv_dev:
>> + kfree(af_iucv_dev);
>> out_driver:
>> driver_unregister(&af_iucv_driver);
>> out_iucv:
>> @@ -2496,6 +2498,7 @@ static void __exit afiucv_exit(void)
>> {
>> if (pr_iucv) {
>> device_unregister(af_iucv_dev);
>> + kfree(af_iucv_dev);
>> driver_unregister(&af_iucv_driver);
>> pr_iucv->iucv_unregister(&af_iucv_handler, 0);
>> symbol_put(iucv_if);
> No, you must not use kfree() after you called device_register() (even
> if it was not successful!) -- see the comment for device_register().
Yes, Your are right. First we need to call put_device() then kfree().
I will send updated patch.
~arvind
On Wed, 28 Feb 2018 17:14:55 +0530
Arvind Yadav <[email protected]> wrote:
> On Wednesday 28 February 2018 04:00 PM, Cornelia Huck wrote:
> > On Wed, 28 Feb 2018 15:24:16 +0530
> > Arvind Yadav <[email protected]> wrote:
> >
> >> Free memory, if afiucv_iucv_init is not successful and
> >> removing a IUCV driver.
> >>
> >> Signed-off-by: Arvind Yadav <[email protected]>
> >> ---
> >> net/iucv/af_iucv.c | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> >> index 1e8cc7b..eb0995a 100644
> >> --- a/net/iucv/af_iucv.c
> >> +++ b/net/iucv/af_iucv.c
> >> @@ -2433,9 +2433,11 @@ static int afiucv_iucv_init(void)
> >> af_iucv_dev->driver = &af_iucv_driver;
> >> err = device_register(af_iucv_dev);
> >> if (err)
> >> - goto out_driver;
> >> + goto out_iucv_dev;
> >> return 0;
> >>
> >> +out_iucv_dev:
> >> + kfree(af_iucv_dev);
> >> out_driver:
> >> driver_unregister(&af_iucv_driver);
> >> out_iucv:
> >> @@ -2496,6 +2498,7 @@ static void __exit afiucv_exit(void)
> >> {
> >> if (pr_iucv) {
> >> device_unregister(af_iucv_dev);
> >> + kfree(af_iucv_dev);
> >> driver_unregister(&af_iucv_driver);
> >> pr_iucv->iucv_unregister(&af_iucv_handler, 0);
> >> symbol_put(iucv_if);
> > No, you must not use kfree() after you called device_register() (even
> > if it was not successful!) -- see the comment for device_register().
> Yes, Your are right. First we need to call put_device() then kfree().
> I will send updated patch.
No, that's not correct, either. device_register() will give up any
reference it obtained, and the caller did not obtain any additional
reference, so a put_device() would be wrong. A kfree() on a refcounted
structure is wrong as well.
On Wednesday 28 February 2018 05:26 PM, Cornelia Huck wrote:
> On Wed, 28 Feb 2018 17:14:55 +0530
> Arvind Yadav <[email protected]> wrote:
>
>> On Wednesday 28 February 2018 04:00 PM, Cornelia Huck wrote:
>>> On Wed, 28 Feb 2018 15:24:16 +0530
>>> Arvind Yadav <[email protected]> wrote:
>>>
>>>> Free memory, if afiucv_iucv_init is not successful and
>>>> removing a IUCV driver.
>>>>
>>>> Signed-off-by: Arvind Yadav <[email protected]>
>>>> ---
>>>> net/iucv/af_iucv.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>>>> index 1e8cc7b..eb0995a 100644
>>>> --- a/net/iucv/af_iucv.c
>>>> +++ b/net/iucv/af_iucv.c
>>>> @@ -2433,9 +2433,11 @@ static int afiucv_iucv_init(void)
>>>> af_iucv_dev->driver = &af_iucv_driver;
>>>> err = device_register(af_iucv_dev);
>>>> if (err)
>>>> - goto out_driver;
>>>> + goto out_iucv_dev;
>>>> return 0;
>>>>
>>>> +out_iucv_dev:
>>>> + kfree(af_iucv_dev);
>>>> out_driver:
>>>> driver_unregister(&af_iucv_driver);
>>>> out_iucv:
>>>> @@ -2496,6 +2498,7 @@ static void __exit afiucv_exit(void)
>>>> {
>>>> if (pr_iucv) {
>>>> device_unregister(af_iucv_dev);
>>>> + kfree(af_iucv_dev);
>>>> driver_unregister(&af_iucv_driver);
>>>> pr_iucv->iucv_unregister(&af_iucv_handler, 0);
>>>> symbol_put(iucv_if);
>>> No, you must not use kfree() after you called device_register() (even
>>> if it was not successful!) -- see the comment for device_register().
>> Yes, Your are right. First we need to call put_device() then kfree().
>> I will send updated patch.
> No, that's not correct, either. device_register() will give up any
> reference it obtained, and the caller did not obtain any additional
> reference, so a put_device() would be wrong. A kfree() on a refcounted
> structure is wrong as well.
If you will see the comment for device_register() (drivers/base/core.c)
there is mentioned that
'NOTE: _Never_ directly free @dev after calling this function, even
if it returned an error! Always use put_device() to give up the
reference initialized in this function instead.'
But as per you comment. we should not use.
Thanks,
Hi,
> On Wednesday 28 February 2018 05:26 PM, Cornelia Huck wrote:
> > On Wed, 28 Feb 2018 17:14:55 +0530
> > Arvind Yadav <[email protected]> wrote:
> >
> >> On Wednesday 28 February 2018 04:00 PM, Cornelia Huck wrote:
> >>> On Wed, 28 Feb 2018 15:24:16 +0530
> >>> Arvind Yadav <[email protected]> wrote:
> >>>
> >>>> Free memory, if afiucv_iucv_init is not successful and
> >>>> removing a IUCV driver.
> >>>>
> >>>> Signed-off-by: Arvind Yadav <[email protected]>
> >>>> ---
> >>>> net/iucv/af_iucv.c | 5 ++++-
> >>>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> >>>> index 1e8cc7b..eb0995a 100644
> >>>> --- a/net/iucv/af_iucv.c
> >>>> +++ b/net/iucv/af_iucv.c
> >>>> @@ -2433,9 +2433,11 @@ static int afiucv_iucv_init(void)
> >>>> af_iucv_dev->driver = &af_iucv_driver;
> >>>> err = device_register(af_iucv_dev);
> >>>> if (err)
> >>>> - goto out_driver;
> >>>> + goto out_iucv_dev;
> >>>> return 0;
> >>>>
> >>>> +out_iucv_dev:
> >>>> + kfree(af_iucv_dev);
> >>>> out_driver:
> >>>> driver_unregister(&af_iucv_driver);
> >>>> out_iucv:
> >>>> @@ -2496,6 +2498,7 @@ static void __exit afiucv_exit(void)
> >>>> {
> >>>> if (pr_iucv) {
> >>>> device_unregister(af_iucv_dev);
> >>>> + kfree(af_iucv_dev);
> >>>> driver_unregister(&af_iucv_driver);
> >>>> pr_iucv->iucv_unregister(&af_iucv_handler, 0);
> >>>> symbol_put(iucv_if);
> >>> No, you must not use kfree() after you called device_register() (even
> >>> if it was not successful!) -- see the comment for device_register().
> >> Yes, Your are right. First we need to call put_device() then kfree().
> >> I will send updated patch.
> > No, that's not correct, either. device_register() will give up any
> > reference it obtained, and the caller did not obtain any additional
> > reference, so a put_device() would be wrong. A kfree() on a refcounted
> > structure is wrong as well.
> If you will see the comment for device_register() (drivers/base/core.c)
> there is mentioned that
> 'NOTE: _Never_ directly free @dev after calling this function, even
> if it returned an error! Always use put_device() to give up the
> reference initialized in this function instead.'
> But as per you comment. we should not use.
> Thanks,
>
In case that device_register() fails simply call device_put(). This will decrement
the last reference and then free the memory.
Regards,
Lino
On Wed, 28 Feb 2018 17:39:53 +0530
Arvind Yadav <[email protected]> wrote:
> On Wednesday 28 February 2018 05:26 PM, Cornelia Huck wrote:
> > On Wed, 28 Feb 2018 17:14:55 +0530
> > Arvind Yadav <[email protected]> wrote:
> >
> >> On Wednesday 28 February 2018 04:00 PM, Cornelia Huck wrote:
> >>> On Wed, 28 Feb 2018 15:24:16 +0530
> >>> Arvind Yadav <[email protected]> wrote:
> >>>
> >>>> Free memory, if afiucv_iucv_init is not successful and
> >>>> removing a IUCV driver.
> >>>>
> >>>> Signed-off-by: Arvind Yadav <[email protected]>
> >>>> ---
> >>>> net/iucv/af_iucv.c | 5 ++++-
> >>>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> >>>> index 1e8cc7b..eb0995a 100644
> >>>> --- a/net/iucv/af_iucv.c
> >>>> +++ b/net/iucv/af_iucv.c
> >>>> @@ -2433,9 +2433,11 @@ static int afiucv_iucv_init(void)
> >>>> af_iucv_dev->driver = &af_iucv_driver;
> >>>> err = device_register(af_iucv_dev);
> >>>> if (err)
> >>>> - goto out_driver;
> >>>> + goto out_iucv_dev;
> >>>> return 0;
> >>>>
> >>>> +out_iucv_dev:
> >>>> + kfree(af_iucv_dev);
> >>>> out_driver:
> >>>> driver_unregister(&af_iucv_driver);
> >>>> out_iucv:
> >>>> @@ -2496,6 +2498,7 @@ static void __exit afiucv_exit(void)
> >>>> {
> >>>> if (pr_iucv) {
> >>>> device_unregister(af_iucv_dev);
> >>>> + kfree(af_iucv_dev);
> >>>> driver_unregister(&af_iucv_driver);
> >>>> pr_iucv->iucv_unregister(&af_iucv_handler, 0);
> >>>> symbol_put(iucv_if);
> >>> No, you must not use kfree() after you called device_register() (even
> >>> if it was not successful!) -- see the comment for device_register().
> >> Yes, Your are right. First we need to call put_device() then kfree().
> >> I will send updated patch.
> > No, that's not correct, either. device_register() will give up any
> > reference it obtained, and the caller did not obtain any additional
> > reference, so a put_device() would be wrong. A kfree() on a refcounted
> > structure is wrong as well.
> If you will see the comment for device_register() (drivers/base/core.c)
> there is mentioned that
> 'NOTE: _Never_ directly free @dev after calling this function, even
> if it returned an error! Always use put_device() to give up the
> reference initialized in this function instead.'
> But as per you comment. we should not use.
You don't need to do a put_device() after your device_unregister(),
that's all managed by the driver core.