Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-05 Thread Levente Kurusa
2013-12-05 03:57 keltezéssel, Chen, Gong írta:
> On Wed, Dec 04, 2013 at 07:39:07PM +0100, Levente Kurusa wrote:
>> Date:Wed, 04 Dec 2013 19:39:07 +0100
>> From: Levente Kurusa 
>> To: Borislav Petkov , Ingo Molnar , Thomas
>>  Gleixner , Tony Luck , "H. Peter
>>  Anvin" , x...@kernel.org, EDAC ,
>>  LKML 
>> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
>> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101
>>  Thunderbird/24.1.0
>>
>> 2013-12-04 08:38, Chen, Gong:
>>> On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
>>>> Date: Tue, 3 Dec 2013 18:01:50 +0100
>>>> From: Borislav Petkov 
>>>> To: "Chen, Gong" 
>>>> Cc: Levente Kurusa , Ingo Molnar ,
>>>>  Thomas Gleixner , Tony Luck , "H.
>>>>  Peter Anvin" , x...@kernel.org, EDAC
>>>>  , LKML 
>>>> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register 
>>>> failure
>>>> User-Agent: Mutt/1.5.21 (2010-09-15)
>>>>
>>>> Can you please fix your
>>>>
>>>> Mail-Followup-To:
>>>>
>>>> header? It is impossible to reply to your emails without fiddling with
>>>> the To: and Cc: by hand which gets very annoying over time.
>>>
>>> I add some configs in my muttrc. Hope it works.
>>>
>>>>
>>>> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
>>>>> I have some concerns about it. if device_register is failed, it will
>>>>> backtraces all kinds of conditions automatically, including put_device
>>>>> definately. So do we really need an extra put_device when it returns
>>>>> failure?
>>>>
>>>> Do you mean the "done:" label in device_add() which does put_device()
>>>> and which gets called by device_register()?
>>>>
>>>
>>> Not only. I noticed that another put_device under label "Error:".
>>>
>>
>> That label is called when we failed to add the kobject to its parent.
>> It just puts the parent of the device. I don't think it has anything
>> to do with us put_device()-ing the actual device too.
>>
> OK, you are right. I read some kobject related codes and get:
> 
> static inline void kref_init(struct kref *kref)
> {
> atomic_set(>refcount, 1);
> }
> 
> The init refcount is 1, which means even if we meet an error and put_device
> in device_add, we still need an extra put_device to make refcount = 0
> and then release the dev object.

Exactly. This is why the comment you have found later on. :-)
> 
> BTW, from the comments of device_register:
> 
> "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. "
> 
> Many caller don't follow this logic. For example:
> in arch/arm/common/locomo.c
> locomo_init_one_child
> ...
> ret = device_register(>dev);
> if (ret) {
> out:
> kfree(dev);

Umm, but it frees dev which is a container_of dev->dev so dev->dev is not
even freed. This is a memleak as well.
> }
> ...
>  
> in arch/parisc/kernel/drivers.c
> create_tree_node
> ...
> if (device_register(>dev)) {
> kfree(dev);

Same here.
> return NULL;
> }
> ...
> 
> etc.
> 
> Maybe we need one more patch to fix them all. :-)

Yes, I will post a series which fixes this during the weekend when I am not 
that busy. :-)

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-05 Thread Levente Kurusa
2013-12-05 03:57 keltezéssel, Chen, Gong írta:
 On Wed, Dec 04, 2013 at 07:39:07PM +0100, Levente Kurusa wrote:
 Date:Wed, 04 Dec 2013 19:39:07 +0100
 From: Levente Kurusa le...@linux.com
 To: Borislav Petkov b...@alien8.de, Ingo Molnar mi...@kernel.org, Thomas
  Gleixner t...@linutronix.de, Tony Luck tony.l...@intel.com, H. Peter
  Anvin h...@zytor.com, x...@kernel.org, EDAC linux-e...@vger.kernel.org,
  LKML linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101
  Thunderbird/24.1.0

 2013-12-04 08:38, Chen, Gong:
 On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
 Date: Tue, 3 Dec 2013 18:01:50 +0100
 From: Borislav Petkov b...@alien8.de
 To: Chen, Gong gong.c...@linux.intel.com
 Cc: Levente Kurusa le...@linux.com, Ingo Molnar mi...@kernel.org,
  Thomas Gleixner t...@linutronix.de, Tony Luck tony.l...@intel.com, H.
  Peter Anvin h...@zytor.com, x...@kernel.org, EDAC
  linux-e...@vger.kernel.org, LKML linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] x86: mcheck: call put_device on device_register 
 failure
 User-Agent: Mutt/1.5.21 (2010-09-15)

 Can you please fix your

 Mail-Followup-To:

 header? It is impossible to reply to your emails without fiddling with
 the To: and Cc: by hand which gets very annoying over time.

 I add some configs in my muttrc. Hope it works.


 On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
 I have some concerns about it. if device_register is failed, it will
 backtraces all kinds of conditions automatically, including put_device
 definately. So do we really need an extra put_device when it returns
 failure?

 Do you mean the done: label in device_add() which does put_device()
 and which gets called by device_register()?


 Not only. I noticed that another put_device under label Error:.


 That label is called when we failed to add the kobject to its parent.
 It just puts the parent of the device. I don't think it has anything
 to do with us put_device()-ing the actual device too.

 OK, you are right. I read some kobject related codes and get:
 
 static inline void kref_init(struct kref *kref)
 {
 atomic_set(kref-refcount, 1);
 }
 
 The init refcount is 1, which means even if we meet an error and put_device
 in device_add, we still need an extra put_device to make refcount = 0
 and then release the dev object.

Exactly. This is why the comment you have found later on. :-)
 
 BTW, from the comments of device_register:
 
 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. 
 
 Many caller don't follow this logic. For example:
 in arch/arm/common/locomo.c
 locomo_init_one_child
 ...
 ret = device_register(dev-dev);
 if (ret) {
 out:
 kfree(dev);

Umm, but it frees dev which is a container_of dev-dev so dev-dev is not
even freed. This is a memleak as well.
 }
 ...
  
 in arch/parisc/kernel/drivers.c
 create_tree_node
 ...
 if (device_register(dev-dev)) {
 kfree(dev);

Same here.
 return NULL;
 }
 ...
 
 etc.
 
 Maybe we need one more patch to fix them all. :-)

Yes, I will post a series which fixes this during the weekend when I am not 
that busy. :-)

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-04 Thread Chen, Gong
On Wed, Dec 04, 2013 at 07:39:07PM +0100, Levente Kurusa wrote:
> Date: Wed, 04 Dec 2013 19:39:07 +0100
> From: Levente Kurusa 
> To: Borislav Petkov , Ingo Molnar , Thomas
>  Gleixner , Tony Luck , "H. Peter
>  Anvin" , x...@kernel.org, EDAC ,
>  LKML 
> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101
>  Thunderbird/24.1.0
> 
> 2013-12-04 08:38, Chen, Gong:
> > On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
> >> Date: Tue, 3 Dec 2013 18:01:50 +0100
> >> From: Borislav Petkov 
> >> To: "Chen, Gong" 
> >> Cc: Levente Kurusa , Ingo Molnar ,
> >>  Thomas Gleixner , Tony Luck , "H.
> >>  Peter Anvin" , x...@kernel.org, EDAC
> >>  , LKML 
> >> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register 
> >> failure
> >> User-Agent: Mutt/1.5.21 (2010-09-15)
> >>
> >> Can you please fix your
> >>
> >> Mail-Followup-To:
> >>
> >> header? It is impossible to reply to your emails without fiddling with
> >> the To: and Cc: by hand which gets very annoying over time.
> > 
> > I add some configs in my muttrc. Hope it works.
> > 
> >>
> >> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
> >>> I have some concerns about it. if device_register is failed, it will
> >>> backtraces all kinds of conditions automatically, including put_device
> >>> definately. So do we really need an extra put_device when it returns
> >>> failure?
> >>
> >> Do you mean the "done:" label in device_add() which does put_device()
> >> and which gets called by device_register()?
> >>
> > 
> > Not only. I noticed that another put_device under label "Error:".
> > 
> 
> That label is called when we failed to add the kobject to its parent.
> It just puts the parent of the device. I don't think it has anything
> to do with us put_device()-ing the actual device too.
> 
OK, you are right. I read some kobject related codes and get:

static inline void kref_init(struct kref *kref)
{
atomic_set(>refcount, 1);
}

The init refcount is 1, which means even if we meet an error and put_device
in device_add, we still need an extra put_device to make refcount = 0
and then release the dev object.

BTW, from the comments of device_register:

"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. "

Many caller don't follow this logic. For example:
in arch/arm/common/locomo.c
locomo_init_one_child
...
ret = device_register(>dev);
if (ret) {
out:
kfree(dev);
}
...
 
in arch/parisc/kernel/drivers.c
create_tree_node
...
if (device_register(>dev)) {
kfree(dev);
return NULL;
}
...

etc.

Maybe we need one more patch to fix them all. :-)
> -- 
> Regards,
> Levente Kurusa
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-04 Thread Levente Kurusa
2013-12-04 08:38, Chen, Gong:
> On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
>> Date: Tue, 3 Dec 2013 18:01:50 +0100
>> From: Borislav Petkov 
>> To: "Chen, Gong" 
>> Cc: Levente Kurusa , Ingo Molnar ,
>>  Thomas Gleixner , Tony Luck , "H.
>>  Peter Anvin" , x...@kernel.org, EDAC
>>  , LKML 
>> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
>> User-Agent: Mutt/1.5.21 (2010-09-15)
>>
>> Can you please fix your
>>
>> Mail-Followup-To:
>>
>> header? It is impossible to reply to your emails without fiddling with
>> the To: and Cc: by hand which gets very annoying over time.
> 
> I add some configs in my muttrc. Hope it works.
> 
>>
>> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
>>> I have some concerns about it. if device_register is failed, it will
>>> backtraces all kinds of conditions automatically, including put_device
>>> definately. So do we really need an extra put_device when it returns
>>> failure?
>>
>> Do you mean the "done:" label in device_add() which does put_device()
>> and which gets called by device_register()?
>>
> 
> Not only. I noticed that another put_device under label "Error:".
> 

That label is called when we failed to add the kobject to its parent.
It just puts the parent of the device. I don't think it has anything
to do with us put_device()-ing the actual device too.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-04 Thread Levente Kurusa
2013-12-04 08:38, Chen, Gong:
 On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
 Date: Tue, 3 Dec 2013 18:01:50 +0100
 From: Borislav Petkov b...@alien8.de
 To: Chen, Gong gong.c...@linux.intel.com
 Cc: Levente Kurusa le...@linux.com, Ingo Molnar mi...@kernel.org,
  Thomas Gleixner t...@linutronix.de, Tony Luck tony.l...@intel.com, H.
  Peter Anvin h...@zytor.com, x...@kernel.org, EDAC
  linux-e...@vger.kernel.org, LKML linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
 User-Agent: Mutt/1.5.21 (2010-09-15)

 Can you please fix your

 Mail-Followup-To:

 header? It is impossible to reply to your emails without fiddling with
 the To: and Cc: by hand which gets very annoying over time.
 
 I add some configs in my muttrc. Hope it works.
 

 On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
 I have some concerns about it. if device_register is failed, it will
 backtraces all kinds of conditions automatically, including put_device
 definately. So do we really need an extra put_device when it returns
 failure?

 Do you mean the done: label in device_add() which does put_device()
 and which gets called by device_register()?

 
 Not only. I noticed that another put_device under label Error:.
 

That label is called when we failed to add the kobject to its parent.
It just puts the parent of the device. I don't think it has anything
to do with us put_device()-ing the actual device too.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-04 Thread Chen, Gong
On Wed, Dec 04, 2013 at 07:39:07PM +0100, Levente Kurusa wrote:
 Date: Wed, 04 Dec 2013 19:39:07 +0100
 From: Levente Kurusa le...@linux.com
 To: Borislav Petkov b...@alien8.de, Ingo Molnar mi...@kernel.org, Thomas
  Gleixner t...@linutronix.de, Tony Luck tony.l...@intel.com, H. Peter
  Anvin h...@zytor.com, x...@kernel.org, EDAC linux-e...@vger.kernel.org,
  LKML linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101
  Thunderbird/24.1.0
 
 2013-12-04 08:38, Chen, Gong:
  On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
  Date: Tue, 3 Dec 2013 18:01:50 +0100
  From: Borislav Petkov b...@alien8.de
  To: Chen, Gong gong.c...@linux.intel.com
  Cc: Levente Kurusa le...@linux.com, Ingo Molnar mi...@kernel.org,
   Thomas Gleixner t...@linutronix.de, Tony Luck tony.l...@intel.com, H.
   Peter Anvin h...@zytor.com, x...@kernel.org, EDAC
   linux-e...@vger.kernel.org, LKML linux-kernel@vger.kernel.org
  Subject: Re: [PATCH] x86: mcheck: call put_device on device_register 
  failure
  User-Agent: Mutt/1.5.21 (2010-09-15)
 
  Can you please fix your
 
  Mail-Followup-To:
 
  header? It is impossible to reply to your emails without fiddling with
  the To: and Cc: by hand which gets very annoying over time.
  
  I add some configs in my muttrc. Hope it works.
  
 
  On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
  I have some concerns about it. if device_register is failed, it will
  backtraces all kinds of conditions automatically, including put_device
  definately. So do we really need an extra put_device when it returns
  failure?
 
  Do you mean the done: label in device_add() which does put_device()
  and which gets called by device_register()?
 
  
  Not only. I noticed that another put_device under label Error:.
  
 
 That label is called when we failed to add the kobject to its parent.
 It just puts the parent of the device. I don't think it has anything
 to do with us put_device()-ing the actual device too.
 
OK, you are right. I read some kobject related codes and get:

static inline void kref_init(struct kref *kref)
{
atomic_set(kref-refcount, 1);
}

The init refcount is 1, which means even if we meet an error and put_device
in device_add, we still need an extra put_device to make refcount = 0
and then release the dev object.

BTW, from the comments of device_register:

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. 

Many caller don't follow this logic. For example:
in arch/arm/common/locomo.c
locomo_init_one_child
...
ret = device_register(dev-dev);
if (ret) {
out:
kfree(dev);
}
...
 
in arch/parisc/kernel/drivers.c
create_tree_node
...
if (device_register(dev-dev)) {
kfree(dev);
return NULL;
}
...

etc.

Maybe we need one more patch to fix them all. :-)
 -- 
 Regards,
 Levente Kurusa
 --
 To unsubscribe from this list: send the line unsubscribe linux-edac in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-03 Thread Chen, Gong
On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
> Date: Tue, 3 Dec 2013 18:01:50 +0100
> From: Borislav Petkov 
> To: "Chen, Gong" 
> Cc: Levente Kurusa , Ingo Molnar ,
>  Thomas Gleixner , Tony Luck , "H.
>  Peter Anvin" , x...@kernel.org, EDAC
>  , LKML 
> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> Can you please fix your
> 
> Mail-Followup-To:
> 
> header? It is impossible to reply to your emails without fiddling with
> the To: and Cc: by hand which gets very annoying over time.

I add some configs in my muttrc. Hope it works.

> 
> On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
> > I have some concerns about it. if device_register is failed, it will
> > backtraces all kinds of conditions automatically, including put_device
> > definately. So do we really need an extra put_device when it returns
> > failure?
> 
> Do you mean the "done:" label in device_add() which does put_device()
> and which gets called by device_register()?
> 

Not only. I noticed that another put_device under label "Error:".


signature.asc
Description: Digital signature


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-03 Thread Borislav Petkov
Can you please fix your

Mail-Followup-To:

header? It is impossible to reply to your emails without fiddling with
the To: and Cc: by hand which gets very annoying over time.

On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
> I have some concerns about it. if device_register is failed, it will
> backtraces all kinds of conditions automatically, including put_device
> definately. So do we really need an extra put_device when it returns
> failure?

Do you mean the "done:" label in device_add() which does put_device()
and which gets called by device_register()?

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-03 Thread Borislav Petkov
Can you please fix your

Mail-Followup-To:

header? It is impossible to reply to your emails without fiddling with
the To: and Cc: by hand which gets very annoying over time.

On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
 I have some concerns about it. if device_register is failed, it will
 backtraces all kinds of conditions automatically, including put_device
 definately. So do we really need an extra put_device when it returns
 failure?

Do you mean the done: label in device_add() which does put_device()
and which gets called by device_register()?

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-03 Thread Chen, Gong
On Tue, Dec 03, 2013 at 06:01:50PM +0100, Borislav Petkov wrote:
 Date: Tue, 3 Dec 2013 18:01:50 +0100
 From: Borislav Petkov b...@alien8.de
 To: Chen, Gong gong.c...@linux.intel.com
 Cc: Levente Kurusa le...@linux.com, Ingo Molnar mi...@kernel.org,
  Thomas Gleixner t...@linutronix.de, Tony Luck tony.l...@intel.com, H.
  Peter Anvin h...@zytor.com, x...@kernel.org, EDAC
  linux-e...@vger.kernel.org, LKML linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
 User-Agent: Mutt/1.5.21 (2010-09-15)
 
 Can you please fix your
 
 Mail-Followup-To:
 
 header? It is impossible to reply to your emails without fiddling with
 the To: and Cc: by hand which gets very annoying over time.

I add some configs in my muttrc. Hope it works.

 
 On Mon, Dec 02, 2013 at 09:23:30PM -0500, Chen, Gong wrote:
  I have some concerns about it. if device_register is failed, it will
  backtraces all kinds of conditions automatically, including put_device
  definately. So do we really need an extra put_device when it returns
  failure?
 
 Do you mean the done: label in device_add() which does put_device()
 and which gets called by device_register()?
 

Not only. I noticed that another put_device under label Error:.


signature.asc
Description: Digital signature


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-02 Thread Chen, Gong
On Sat, Nov 30, 2013 at 12:12:14PM +0100, Borislav Petkov wrote:
> Date: Sat, 30 Nov 2013 12:12:14 +0100
> From: Borislav Petkov 
> To: Levente Kurusa 
> Cc: Ingo Molnar , Thomas Gleixner ,
>  Tony Luck , "H. Peter Anvin" ,
>  x...@kernel.org, EDAC , LKML
>  
> Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
> > No, if the call to put_device gives up the last reference to the
> > device, then device_release gets called which in turn frees the memory
> > associated with it. In this case, mce_device_release() will get
> > called, which is just a simple kfree call.
> 
> Aah, that's that delayed freeing the driver core does, right. Now you
> made me go and look into detail:
> 
> device_unregister
> |->put_device
>   |->kobject_put
>  |->kref_put(>kref, kobject_release)
>   |->kref_sub(kref, 1, release)
>  |->release
>  |->kobject_release
> |->kobject_cleanup
>|->t->release
>|->device_release
>   |->mce_device_release
> 
> 
> Ok, I see it now. :-) :-)
> 
> Thanks, I'll take your patch as-is.
> 

I have some concerns about it. if device_register is failed, it will
backtraces all kinds of conditions automatically, including put_device
definately. So do we really need an extra put_device when it returns
failure?


signature.asc
Description: Digital signature


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-12-02 Thread Chen, Gong
On Sat, Nov 30, 2013 at 12:12:14PM +0100, Borislav Petkov wrote:
 Date: Sat, 30 Nov 2013 12:12:14 +0100
 From: Borislav Petkov b...@alien8.de
 To: Levente Kurusa le...@linux.com
 Cc: Ingo Molnar mi...@kernel.org, Thomas Gleixner t...@linutronix.de,
  Tony Luck tony.l...@intel.com, H. Peter Anvin h...@zytor.com,
  x...@kernel.org, EDAC linux-e...@vger.kernel.org, LKML
  linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] x86: mcheck: call put_device on device_register failure
 User-Agent: Mutt/1.5.21 (2010-09-15)
 
 On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
  No, if the call to put_device gives up the last reference to the
  device, then device_release gets called which in turn frees the memory
  associated with it. In this case, mce_device_release() will get
  called, which is just a simple kfree call.
 
 Aah, that's that delayed freeing the driver core does, right. Now you
 made me go and look into detail:
 
 device_unregister
 |-put_device
   |-kobject_put
  |-kref_put(kobj-kref, kobject_release)
   |-kref_sub(kref, 1, release)
  |-release
  |-kobject_release
 |-kobject_cleanup
|-t-release
|-device_release
   |-mce_device_release
 
 
 Ok, I see it now. :-) :-)
 
 Thanks, I'll take your patch as-is.
 

I have some concerns about it. if device_register is failed, it will
backtraces all kinds of conditions automatically, including put_device
definately. So do we really need an extra put_device when it returns
failure?


signature.asc
Description: Digital signature


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Levente Kurusa
2013-11-30 13:08 keltezéssel, Borislav Petkov írta:
> On Sat, Nov 30, 2013 at 12:44:59PM +0100, Levente Kurusa wrote:
>> Yes, I saw that as well. By that I meant that by doing some identifier
>> searches for device_register and then checking whether they call
>> put_device and have device_release registered. Also, I wonder if it
>> would be beneficial to have a generic device_release? Most of the
>> drivers I quickly swept through only call kfree(). Wouldn't a generic
>> one save some space?
> 
> Again, I wouldn't waste my time with hypothetical issues which never
> happen - there are many other, real issues which would rather need
> attention than what-if ones.
> 
> About saving space, that could be worth a try. If you can actually do
> that and show numbers to back it up, I'm sure people will have a look.
> And if you can't show any space savings, you'll still have learned stuff
> along the way.
> 
> But don't ask me about whether it makes sense to have a generic
> device_release - I'm no driver core and am not even trying. You could
> try to answer that question yourself, btw. :)

Okay, I will, thanks for the tips! :)
> 
>> Yes, I do that daily usually, but most of the time I only get some
>> uninitialized warnings. :-)
> 
> You can always try to understand why such warnings get issued and maybe
> even fix them if they're legit and the compiler is right. Also, look
> through git log for examples how others have fixed such warnings.
Yes, but there are some fake one where for example it doesn't recognize the
pci_read_config_byte call as something which could write to its args. So most
of them are fake warnings.

> 
> For example, sometimes changing code flow instead of simply shutting
> up the variable is much better. But in order to do that, you'd need to
> understand the code first and try to change it so that it doesn't break
> and the warning disappears. This is a very good way, IMO, to get to
> really understand what the code does and learn from others.
> 
> Another good exercise is trying to boot those random kernels with kvm -
> that can catch a bunch of issues too.
> 
> The save-space experiment you can also quickly test with kvm. By now you
> probably are catching my drift: testing kernels with kvm is awesome! :-)
I will try kvm, didn't use that before. :p

> 
>> What does that do? Never heard of it yet.
> 
> Well, you can have a look: scripts/Makefile.build

Ahh, now I see. Just didn't know where to look.

> 
> :-)
> 
> Good luck!
Thank you and for your time! :)

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Borislav Petkov
On Sat, Nov 30, 2013 at 12:44:59PM +0100, Levente Kurusa wrote:
> Yes, I saw that as well. By that I meant that by doing some identifier
> searches for device_register and then checking whether they call
> put_device and have device_release registered. Also, I wonder if it
> would be beneficial to have a generic device_release? Most of the
> drivers I quickly swept through only call kfree(). Wouldn't a generic
> one save some space?

Again, I wouldn't waste my time with hypothetical issues which never
happen - there are many other, real issues which would rather need
attention than what-if ones.

About saving space, that could be worth a try. If you can actually do
that and show numbers to back it up, I'm sure people will have a look.
And if you can't show any space savings, you'll still have learned stuff
along the way.

But don't ask me about whether it makes sense to have a generic
device_release - I'm no driver core and am not even trying. You could
try to answer that question yourself, btw. :)

> Yes, I do that daily usually, but most of the time I only get some
> uninitialized warnings. :-)

You can always try to understand why such warnings get issued and maybe
even fix them if they're legit and the compiler is right. Also, look
through git log for examples how others have fixed such warnings.

For example, sometimes changing code flow instead of simply shutting
up the variable is much better. But in order to do that, you'd need to
understand the code first and try to change it so that it doesn't break
and the warning disappears. This is a very good way, IMO, to get to
really understand what the code does and learn from others.

Another good exercise is trying to boot those random kernels with kvm -
that can catch a bunch of issues too.

The save-space experiment you can also quickly test with kvm. By now you
probably are catching my drift: testing kernels with kvm is awesome! :-)

> What does that do? Never heard of it yet.

Well, you can have a look: scripts/Makefile.build

:-)

Good luck!

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Levente Kurusa
2013-11-30 12:32, Borislav Petkov:
> On Sat, Nov 30, 2013 at 12:25:44PM +0100, Levente Kurusa wrote:
>> Now this tree makes me wonder if there are devices where the author
>> forgot to set a device_release or when the put_device is not called. I
>> will take a look into this.
> 
> kobject_cleanup() warns about !t->release already.

Yes, I saw that as well. By that I meant that by doing some identifier searches
for device_register and then checking whether they call put_device and have 
device_release
registered. Also, I wonder if it would be beneficial to have a generic 
device_release? Most
of the drivers I quickly swept through only call kfree(). Wouldn't a generic 
one save
some space?

> 
> If you want to fix actual issues and not waste time with potential
> issues which have never actually triggered, try building a couple of
> randconfigs and look at the output :-)

Yes, I do that daily usually, but most of the time I only get some 
uninitialized warnings. :-)

> 
> Also, we have "make W=" which gives you even more :-)

What does that do? Never heard of it yet.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Borislav Petkov
On Sat, Nov 30, 2013 at 12:25:44PM +0100, Levente Kurusa wrote:
> Now this tree makes me wonder if there are devices where the author
> forgot to set a device_release or when the put_device is not called. I
> will take a look into this.

kobject_cleanup() warns about !t->release already.

If you want to fix actual issues and not waste time with potential
issues which have never actually triggered, try building a couple of
randconfigs and look at the output :-)

Also, we have "make W=" which gives you even more :-)

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Levente Kurusa
2013-11-30 12:12, Borislav Petkov:
> On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
>> No, if the call to put_device gives up the last reference to the
>> device, then device_release gets called which in turn frees the memory
>> associated with it. In this case, mce_device_release() will get
>> called, which is just a simple kfree call.
> 
> Aah, that's that delayed freeing the driver core does, right. Now you
> made me go and look into detail:
> 
> device_unregister
> |->put_device
>   |->kobject_put
>  |->kref_put(>kref, kobject_release)
>   |->kref_sub(kref, 1, release)
>  |->release
>  |->kobject_release
> |->kobject_cleanup
>|->t->release
>|->device_release
>   |->mce_device_release
> 
Now this tree makes me wonder if there are devices where
the author forgot to set a device_release or when the put_device
is not called. I will take a look into this.
> 
> Ok, I see it now. :-) :-)
> 
> Thanks, I'll take your patch as-is.
> 
Awesome, thanks! :-)


-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Borislav Petkov
On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
> No, if the call to put_device gives up the last reference to the
> device, then device_release gets called which in turn frees the memory
> associated with it. In this case, mce_device_release() will get
> called, which is just a simple kfree call.

Aah, that's that delayed freeing the driver core does, right. Now you
made me go and look into detail:

device_unregister
|->put_device
  |->kobject_put
 |->kref_put(>kref, kobject_release)
|->kref_sub(kref, 1, release)
   |->release
   |->kobject_release
  |->kobject_cleanup
 |->t->release
 |->device_release
|->mce_device_release


Ok, I see it now. :-) :-)

Thanks, I'll take your patch as-is.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Borislav Petkov
On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
 No, if the call to put_device gives up the last reference to the
 device, then device_release gets called which in turn frees the memory
 associated with it. In this case, mce_device_release() will get
 called, which is just a simple kfree call.

Aah, that's that delayed freeing the driver core does, right. Now you
made me go and look into detail:

device_unregister
|-put_device
  |-kobject_put
 |-kref_put(kobj-kref, kobject_release)
|-kref_sub(kref, 1, release)
   |-release
   |-kobject_release
  |-kobject_cleanup
 |-t-release
 |-device_release
|-mce_device_release


Ok, I see it now. :-) :-)

Thanks, I'll take your patch as-is.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Levente Kurusa
2013-11-30 12:12, Borislav Petkov:
 On Sat, Nov 30, 2013 at 08:30:33AM +0100, Levente Kurusa wrote:
 No, if the call to put_device gives up the last reference to the
 device, then device_release gets called which in turn frees the memory
 associated with it. In this case, mce_device_release() will get
 called, which is just a simple kfree call.
 
 Aah, that's that delayed freeing the driver core does, right. Now you
 made me go and look into detail:
 
 device_unregister
 |-put_device
   |-kobject_put
  |-kref_put(kobj-kref, kobject_release)
   |-kref_sub(kref, 1, release)
  |-release
  |-kobject_release
 |-kobject_cleanup
|-t-release
|-device_release
   |-mce_device_release
 
Now this tree makes me wonder if there are devices where
the author forgot to set a device_release or when the put_device
is not called. I will take a look into this.
 
 Ok, I see it now. :-) :-)
 
 Thanks, I'll take your patch as-is.
 
Awesome, thanks! :-)


-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Borislav Petkov
On Sat, Nov 30, 2013 at 12:25:44PM +0100, Levente Kurusa wrote:
 Now this tree makes me wonder if there are devices where the author
 forgot to set a device_release or when the put_device is not called. I
 will take a look into this.

kobject_cleanup() warns about !t-release already.

If you want to fix actual issues and not waste time with potential
issues which have never actually triggered, try building a couple of
randconfigs and look at the output :-)

Also, we have make W= which gives you even more :-)

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Levente Kurusa
2013-11-30 12:32, Borislav Petkov:
 On Sat, Nov 30, 2013 at 12:25:44PM +0100, Levente Kurusa wrote:
 Now this tree makes me wonder if there are devices where the author
 forgot to set a device_release or when the put_device is not called. I
 will take a look into this.
 
 kobject_cleanup() warns about !t-release already.

Yes, I saw that as well. By that I meant that by doing some identifier searches
for device_register and then checking whether they call put_device and have 
device_release
registered. Also, I wonder if it would be beneficial to have a generic 
device_release? Most
of the drivers I quickly swept through only call kfree(). Wouldn't a generic 
one save
some space?

 
 If you want to fix actual issues and not waste time with potential
 issues which have never actually triggered, try building a couple of
 randconfigs and look at the output :-)

Yes, I do that daily usually, but most of the time I only get some 
uninitialized warnings. :-)

 
 Also, we have make W= which gives you even more :-)

What does that do? Never heard of it yet.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Borislav Petkov
On Sat, Nov 30, 2013 at 12:44:59PM +0100, Levente Kurusa wrote:
 Yes, I saw that as well. By that I meant that by doing some identifier
 searches for device_register and then checking whether they call
 put_device and have device_release registered. Also, I wonder if it
 would be beneficial to have a generic device_release? Most of the
 drivers I quickly swept through only call kfree(). Wouldn't a generic
 one save some space?

Again, I wouldn't waste my time with hypothetical issues which never
happen - there are many other, real issues which would rather need
attention than what-if ones.

About saving space, that could be worth a try. If you can actually do
that and show numbers to back it up, I'm sure people will have a look.
And if you can't show any space savings, you'll still have learned stuff
along the way.

But don't ask me about whether it makes sense to have a generic
device_release - I'm no driver core and am not even trying. You could
try to answer that question yourself, btw. :)

 Yes, I do that daily usually, but most of the time I only get some
 uninitialized warnings. :-)

You can always try to understand why such warnings get issued and maybe
even fix them if they're legit and the compiler is right. Also, look
through git log for examples how others have fixed such warnings.

For example, sometimes changing code flow instead of simply shutting
up the variable is much better. But in order to do that, you'd need to
understand the code first and try to change it so that it doesn't break
and the warning disappears. This is a very good way, IMO, to get to
really understand what the code does and learn from others.

Another good exercise is trying to boot those random kernels with kvm -
that can catch a bunch of issues too.

The save-space experiment you can also quickly test with kvm. By now you
probably are catching my drift: testing kernels with kvm is awesome! :-)

 What does that do? Never heard of it yet.

Well, you can have a look: scripts/Makefile.build

:-)

Good luck!

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-30 Thread Levente Kurusa
2013-11-30 13:08 keltezéssel, Borislav Petkov írta:
 On Sat, Nov 30, 2013 at 12:44:59PM +0100, Levente Kurusa wrote:
 Yes, I saw that as well. By that I meant that by doing some identifier
 searches for device_register and then checking whether they call
 put_device and have device_release registered. Also, I wonder if it
 would be beneficial to have a generic device_release? Most of the
 drivers I quickly swept through only call kfree(). Wouldn't a generic
 one save some space?
 
 Again, I wouldn't waste my time with hypothetical issues which never
 happen - there are many other, real issues which would rather need
 attention than what-if ones.
 
 About saving space, that could be worth a try. If you can actually do
 that and show numbers to back it up, I'm sure people will have a look.
 And if you can't show any space savings, you'll still have learned stuff
 along the way.
 
 But don't ask me about whether it makes sense to have a generic
 device_release - I'm no driver core and am not even trying. You could
 try to answer that question yourself, btw. :)

Okay, I will, thanks for the tips! :)
 
 Yes, I do that daily usually, but most of the time I only get some
 uninitialized warnings. :-)
 
 You can always try to understand why such warnings get issued and maybe
 even fix them if they're legit and the compiler is right. Also, look
 through git log for examples how others have fixed such warnings.
Yes, but there are some fake one where for example it doesn't recognize the
pci_read_config_byte call as something which could write to its args. So most
of them are fake warnings.

 
 For example, sometimes changing code flow instead of simply shutting
 up the variable is much better. But in order to do that, you'd need to
 understand the code first and try to change it so that it doesn't break
 and the warning disappears. This is a very good way, IMO, to get to
 really understand what the code does and learn from others.
 
 Another good exercise is trying to boot those random kernels with kvm -
 that can catch a bunch of issues too.
 
 The save-space experiment you can also quickly test with kvm. By now you
 probably are catching my drift: testing kernels with kvm is awesome! :-)
I will try kvm, didn't use that before. :p

 
 What does that do? Never heard of it yet.
 
 Well, you can have a look: scripts/Makefile.build

Ahh, now I see. Just didn't know where to look.

 
 :-)
 
 Good luck!
Thank you and for your time! :)

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-29 Thread Levente Kurusa
2013-11-29 21:56, Borislav Petkov:
> On Fri, Nov 29, 2013 at 09:28:48PM +0100, Levente Kurusa wrote:
>> This patch adds a call to put_device() when the device_register()
>> call has failed. This is required so that the last reference to the
>> device is given up.
> 
> I'd assume this is not something you're actually hitting but have caught
> this by code staring...?
> 

Yup, I have been staring at it.

> If we're going to do this, I'd also like to see you add another label
> after device_unregister(dev) which also kfrees dev because apparently
> we're not doing that either.
> 

No, if the call to put_device gives up the last reference to the device,
then device_release gets called which in turn frees the memory associated with 
it.
In this case, mce_device_release() will get called, which is just a simple 
kfree call.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-29 Thread Borislav Petkov
On Fri, Nov 29, 2013 at 09:28:48PM +0100, Levente Kurusa wrote:
> This patch adds a call to put_device() when the device_register()
> call has failed. This is required so that the last reference to the
> device is given up.

I'd assume this is not something you're actually hitting but have caught
this by code staring...?

> Signed-off-by: Levente Kurusa 
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c 
> b/arch/x86/kernel/cpu/mcheck/mce.c
> index b3218cd..a389c1d 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -2272,8 +2272,10 @@ static int mce_device_create(unsigned int cpu)
>   dev->release = _device_release;
> 
>   err = device_register(dev);
> - if (err)
> + if (err) {
> + put_device(dev);
>   return err;
> + }

If we're going to do this, I'd also like to see you add another label
after device_unregister(dev) which also kfrees dev because apparently
we're not doing that either.

Thanks.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] x86: mcheck: call put_device on device_register failure

2013-11-29 Thread Levente Kurusa
This patch adds a call to put_device() when the device_register()
call has failed. This is required so that the last reference to the
device is given up.

Signed-off-by: Levente Kurusa 
---
 arch/x86/kernel/cpu/mcheck/mce.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index b3218cd..a389c1d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2272,8 +2272,10 @@ static int mce_device_create(unsigned int cpu)
dev->release = _device_release;

err = device_register(dev);
-   if (err)
+   if (err) {
+   put_device(dev);
return err;
+   }

for (i = 0; mce_device_attrs[i]; i++) {
err = device_create_file(dev, mce_device_attrs[i]);
-- 
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] x86: mcheck: call put_device on device_register failure

2013-11-29 Thread Levente Kurusa
This patch adds a call to put_device() when the device_register()
call has failed. This is required so that the last reference to the
device is given up.

Signed-off-by: Levente Kurusa le...@linux.com
---
 arch/x86/kernel/cpu/mcheck/mce.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index b3218cd..a389c1d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2272,8 +2272,10 @@ static int mce_device_create(unsigned int cpu)
dev-release = mce_device_release;

err = device_register(dev);
-   if (err)
+   if (err) {
+   put_device(dev);
return err;
+   }

for (i = 0; mce_device_attrs[i]; i++) {
err = device_create_file(dev, mce_device_attrs[i]);
-- 
1.8.1.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-29 Thread Borislav Petkov
On Fri, Nov 29, 2013 at 09:28:48PM +0100, Levente Kurusa wrote:
 This patch adds a call to put_device() when the device_register()
 call has failed. This is required so that the last reference to the
 device is given up.

I'd assume this is not something you're actually hitting but have caught
this by code staring...?

 Signed-off-by: Levente Kurusa le...@linux.com
 ---
  arch/x86/kernel/cpu/mcheck/mce.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/arch/x86/kernel/cpu/mcheck/mce.c 
 b/arch/x86/kernel/cpu/mcheck/mce.c
 index b3218cd..a389c1d 100644
 --- a/arch/x86/kernel/cpu/mcheck/mce.c
 +++ b/arch/x86/kernel/cpu/mcheck/mce.c
 @@ -2272,8 +2272,10 @@ static int mce_device_create(unsigned int cpu)
   dev-release = mce_device_release;
 
   err = device_register(dev);
 - if (err)
 + if (err) {
 + put_device(dev);
   return err;
 + }

If we're going to do this, I'd also like to see you add another label
after device_unregister(dev) which also kfrees dev because apparently
we're not doing that either.

Thanks.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] x86: mcheck: call put_device on device_register failure

2013-11-29 Thread Levente Kurusa
2013-11-29 21:56, Borislav Petkov:
 On Fri, Nov 29, 2013 at 09:28:48PM +0100, Levente Kurusa wrote:
 This patch adds a call to put_device() when the device_register()
 call has failed. This is required so that the last reference to the
 device is given up.
 
 I'd assume this is not something you're actually hitting but have caught
 this by code staring...?
 

Yup, I have been staring at it.

 If we're going to do this, I'd also like to see you add another label
 after device_unregister(dev) which also kfrees dev because apparently
 we're not doing that either.
 

No, if the call to put_device gives up the last reference to the device,
then device_release gets called which in turn frees the memory associated with 
it.
In this case, mce_device_release() will get called, which is just a simple 
kfree call.

-- 
Regards,
Levente Kurusa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/