Re: [PATCH] x86: mcheck: call put_device on device_register failure
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 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
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 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 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
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
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
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
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
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
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
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 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
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 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
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 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
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
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 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
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 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
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 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 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
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/
Re: [PATCH] x86: mcheck: call put_device on device_register failure
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 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/