[patch 0/4] kref debugging

[patch 0/4] kref debugging

[Akinobu Mita]

This patch series introduces the DEBUG_KREF config option.

It adds another debug check to detect kref_put() with unreferenced
kref object, and splits all kref debug checks  into this config option.
Also it can be disable or enable in run-time by sysctl.
--

[patch 1/4] kref: warn kref_put() with unreferenced kref

[Akinobu Mita]

[-- Attachment #1: kref-more-warn.patch --]
[-- Type: text/plain, Size: 1156 bytes --]

This patch adds warning to detect kref_put() with unreferenced kref.

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>

 lib/kref.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

Index: 2.6-git/lib/kref.c
===================================================================
--- 2.6-git.orig/lib/kref.c
+++ 2.6-git/lib/kref.c
@@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
  */
 int kref_put(struct kref *kref, void (*release)(struct kref *kref))
 {
+	WARN_ON(atomic_read(&kref->refcount) < 1);
 	WARN_ON(release == NULL);
 	WARN_ON(release == (void (*)(struct kref *))kfree);
 
@@ -56,12 +57,13 @@ int kref_put(struct kref *kref, void (*r
 	 * if current count is one, we are the last user and can release object
 	 * right now, avoiding an atomic operation on 'refcount'
 	 */
-	if ((atomic_read(&kref->refcount) == 1) ||
-	    (atomic_dec_and_test(&kref->refcount))) {
-		release(kref);
-		return 1;
-	}
-	return 0;
+	if (atomic_read(&kref->refcount) == 1)
+		atomic_set(&kref->refcount, 0);
+	else if (!atomic_dec_and_test(&kref->refcount))
+		return 0;
+
+	release(kref);
+	return 1;
 }
 
 EXPORT_SYMBOL(kref_init);

--

[patch 2/4] kref debugging config option

[Akinobu Mita]

[-- Attachment #1: kref-debug.patch --]
[-- Type: text/plain, Size: 2307 bytes --]

This patch converts all WARN_ON() in kref code to BUG_ON().
And split them into new DEBUG_KREF config option.

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>

 lib/Kconfig.debug |    7 +++++++
 lib/kref.c        |   30 +++++++++++++++++++++++++-----
 2 files changed, 32 insertions(+), 5 deletions(-)

Index: 2.6-git/lib/Kconfig.debug
===================================================================
--- 2.6-git.orig/lib/Kconfig.debug
+++ 2.6-git/lib/Kconfig.debug
@@ -130,6 +130,13 @@ config DEBUG_KOBJECT
 	  If you say Y here, some extra kobject debugging messages will be sent
 	  to the syslog. 
 
+config DEBUG_KREF
+	bool "kref debugging"
+	depends on DEBUG_KERNEL
+	help
+	  This option enables addition error checking for kref,
+	  library routines for handling generic reference counted objects.
+
 config DEBUG_HIGHMEM
 	bool "Highmem debugging"
 	depends on DEBUG_KERNEL && HIGHMEM
Index: 2.6-git/lib/kref.c
===================================================================
--- 2.6-git.orig/lib/kref.c
+++ 2.6-git/lib/kref.c
@@ -20,16 +20,38 @@
  */
 void kref_init(struct kref *kref)
 {
-	atomic_set(&kref->refcount,1);
+	atomic_set(&kref->refcount, 1);
 }
 
+#ifdef CONFIG_DEBUG_KREF
+static void kref_get_debug_check(struct kref *kref)
+{
+	BUG_ON(!atomic_read(&kref->refcount));
+}
+static void kref_put_debug_check(struct kref *kref,
+				void (*release)(struct kref *kref))
+{
+	BUG_ON(atomic_read(&kref->refcount) < 1);
+	BUG_ON(release == NULL);
+	BUG_ON(release == (void (*)(struct kref *))kfree);
+}
+#else
+static void kref_get_debug_check(struct kref *kref)
+{
+}
+static void kref_put_debug_check(struct kref *kref,
+				void (*release)(struct kref *kref))
+{
+}
+#endif
+
 /**
  * kref_get - increment refcount for object.
  * @kref: object.
  */
 void kref_get(struct kref *kref)
 {
-	WARN_ON(!atomic_read(&kref->refcount));
+	kref_get_debug_check(kref);
 	atomic_inc(&kref->refcount);
 }
 
@@ -49,9 +71,7 @@ void kref_get(struct kref *kref)
  */
 int kref_put(struct kref *kref, void (*release)(struct kref *kref))
 {
-	WARN_ON(atomic_read(&kref->refcount) < 1);
-	WARN_ON(release == NULL);
-	WARN_ON(release == (void (*)(struct kref *))kfree);
+	kref_put_debug_check(kref, release);
 
 	/*
 	 * if current count is one, we are the last user and can release object

--

[patch 3/4] dynamic configurable kref debugging

[Akinobu Mita]

[-- Attachment #1: kref-debug-dynamic-off.patch --]
[-- Type: text/plain, Size: 3242 bytes --]

This patch makes kref-debugging dynamically configurable
by sysctl kernel.kref_debug

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>

 Documentation/sysctl/kernel.txt |    8 ++++++++
 include/linux/sysctl.h          |    1 +
 kernel/sysctl.c                 |   14 ++++++++++++++
 lib/kref.c                      |    9 +++++++++
 4 files changed, 32 insertions(+)

Index: 2.6-git/lib/kref.c
===================================================================
--- 2.6-git.orig/lib/kref.c
+++ 2.6-git/lib/kref.c
@@ -24,13 +24,22 @@ void kref_init(struct kref *kref)
 }
 
 #ifdef CONFIG_DEBUG_KREF
+
+int kref_debug = 1;
+
 static void kref_get_debug_check(struct kref *kref)
 {
+	if (!kref_debug)
+		return;
+
 	BUG_ON(!atomic_read(&kref->refcount));
 }
 static void kref_put_debug_check(struct kref *kref,
 				void (*release)(struct kref *kref))
 {
+	if (!kref_debug)
+		return;
+
 	BUG_ON(atomic_read(&kref->refcount) < 1);
 	BUG_ON(release == NULL);
 	BUG_ON(release == (void (*)(struct kref *))kfree);
Index: 2.6-git/include/linux/sysctl.h
===================================================================
--- 2.6-git.orig/include/linux/sysctl.h
+++ 2.6-git/include/linux/sysctl.h
@@ -148,6 +148,7 @@ enum
 	KERN_SPIN_RETRY=70,	/* int: number of spinlock retries */
 	KERN_ACPI_VIDEO_FLAGS=71, /* int: flags for setting up video after ACPI sleep */
 	KERN_IA64_UNALIGNED=72, /* int: ia64 unaligned userland trap enable */
+	KERN_KREF_DEBUG=73,	/* int: kref debug on or off */
 };
 
 
Index: 2.6-git/kernel/sysctl.c
===================================================================
--- 2.6-git.orig/kernel/sysctl.c
+++ 2.6-git/kernel/sysctl.c
@@ -131,6 +131,10 @@ extern int acct_parm[];
 extern int no_unaligned_warning;
 #endif
 
+#ifdef CONFIG_DEBUG_KREF
+extern int kref_debug;
+#endif
+
 static int parse_table(int __user *, int, void __user *, size_t __user *, void __user *, size_t,
 		       ctl_table *, void **);
 static int proc_doutsstring(ctl_table *table, int write, struct file *filp,
@@ -683,6 +687,16 @@ static ctl_table kern_table[] = {
 		.proc_handler	= &proc_dointvec,
 	},
 #endif
+#ifdef CONFIG_DEBUG_KREF
+	{
+		.ctl_name	= KERN_KREF_DEBUG,
+		.procname	= "kref_debug",
+		.data		= &kref_debug,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
+#endif
 	{ .ctl_name = 0 }
 };
 
Index: 2.6-git/Documentation/sysctl/kernel.txt
===================================================================
--- 2.6-git.orig/Documentation/sysctl/kernel.txt
+++ 2.6-git/Documentation/sysctl/kernel.txt
@@ -27,6 +27,7 @@ show up in /proc/sys/kernel:
 - hotplug
 - java-appletviewer           [ binfmt_java, obsolete ]
 - java-interpreter            [ binfmt_java, obsolete ]
+- kref_debug
 - l2cr                        [ PPC only ]
 - modprobe                    ==> Documentation/kmod.txt
 - msgmax
@@ -161,6 +162,13 @@ Default value is "/sbin/hotplug".
 
 ==============================================================
 
+kref_debug:
+
+This flag controls the kref debugging. If 0, kref debugging is
+disabled.  Enabled if nonzero.
+
+==============================================================
+
 l2cr: (PPC only)
 
 This flag controls the L2 cache of G3 processor boards. If

--

[patch 4/4] change slab poison pattern

[Akinobu Mita]

[-- Attachment #1: poison-pattern.patch --]
[-- Type: text/plain, Size: 1205 bytes --]

kref debugging cannot detect kref_put() with unreferenced object,
when the structure including kref is allocated by slab
and slab debugging config is enabled.

Because use-after-free poisoning make kref counter signed value.
So this patch prevents it by changing poisoning pattern.

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
CC: Matt Mackall <mpm@selenic.com>
CC: Manfred Spraul <manfred@colorfullife.com>
CC: Pekka J Enberg <penberg@cs.helsinki.fi>

 mm/slab.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Index: 2.6-git/mm/slab.c
===================================================================
--- 2.6-git.orig/mm/slab.c
+++ 2.6-git/mm/slab.c
@@ -503,9 +503,9 @@ struct kmem_cache {
 #define	RED_ACTIVE	0x170FC2A5UL	/* when obj is active */
 
 /* ...and for poisoning */
-#define	POISON_INUSE	0x5a	/* for use-uninitialised poisoning */
-#define POISON_FREE	0x6b	/* for use-after-free poisoning */
-#define	POISON_END	0xa5	/* end-byte of poisoning */
+#define	POISON_INUSE	0xa5	/* for use-uninitialised poisoning */
+#define POISON_FREE	0xb6	/* for use-after-free poisoning */
+#define	POISON_END	0xab	/* end-byte of poisoning */
 
 /*
  * memory layout of objects:

--

Re: [patch 4/4] change slab poison pattern

[Pekka Enberg]

Hi,

On Mon, 2006-04-24 at 16:33 +0800, Akinobu Mita wrote:
> kref debugging cannot detect kref_put() with unreferenced object,
> when the structure including kref is allocated by slab
> and slab debugging config is enabled.
> 
> Because use-after-free poisoning make kref counter signed value.
> So this patch prevents it by changing poisoning pattern.

Then why not check against POISON_INUSE when CONFIG_SLAB_DEBUG in the
kref debugging code? I would prefer you didn't change the slab constants
(they're well known by everyone now) but if you must, at least stick a
big fat comment there.

			Pekka

Re: [patch 4/4] change slab poison pattern

[Akinobu Mita]

On Mon, Apr 24, 2006 at 12:20:03PM +0300, Pekka Enberg wrote:
> > Because use-after-free poisoning make kref counter signed value.
> > So this patch prevents it by changing poisoning pattern.
> 
> Then why not check against POISON_INUSE when CONFIG_SLAB_DEBUG in the
> kref debugging code? I would prefer you didn't change the slab constants
> (they're well known by everyone now) but if you must, at least stick a
> big fat comment there.

This slab poisoning pattern change is not very important.

Because even if slab debugging is enalbed, kref_put() with unreferenced
kref object will be detected as slab corruption.

Because kref_put() decrements the kref counter in freed slab object.

Slab corruption: start=e0e8b698, len=32
Redzone: 0x5a2cf071/0x5a2cf071.
Last user: [<f08ea008>](release_test_kref+0x8/0x14 [test])
000: 6b 6b 6b 6b 6a 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
                 ^^
The main reason I want to propose this kref debugging is because
I can find many places where we can replace from atomic_t to
struct kref by doing "grep -r atomic_dec_and_test .".

But I warried that replacing by kref will just increase atomic operations
because of debugging code in kref (WARN_ON()es in kref.c)

So patch 4/4 is not very important, I want to drop patch 4/4.

Re: [patch 4/4] change slab poison pattern

[Matt Mackall]

On Mon, Apr 24, 2006 at 06:23:52PM +0800, Akinobu Mita wrote:
> On Mon, Apr 24, 2006 at 12:20:03PM +0300, Pekka Enberg wrote:
> > > Because use-after-free poisoning make kref counter signed value.
> > > So this patch prevents it by changing poisoning pattern.
> > 
> > Then why not check against POISON_INUSE when CONFIG_SLAB_DEBUG in the
> > kref debugging code? I would prefer you didn't change the slab constants
> > (they're well known by everyone now) but if you must, at least stick a
> > big fat comment there.
> 
> This slab poisoning pattern change is not very important.
> 
> Because even if slab debugging is enalbed, kref_put() with unreferenced
> kref object will be detected as slab corruption.
> 
> Because kref_put() decrements the kref counter in freed slab object.
> 
> Slab corruption: start=e0e8b698, len=32
> Redzone: 0x5a2cf071/0x5a2cf071.
> Last user: [<f08ea008>](release_test_kref+0x8/0x14 [test])
> 000: 6b 6b 6b 6b 6a 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>                  ^^
> The main reason I want to propose this kref debugging is because
> I can find many places where we can replace from atomic_t to
> struct kref by doing "grep -r atomic_dec_and_test .".
> 
> But I warried that replacing by kref will just increase atomic operations
> because of debugging code in kref (WARN_ON()es in kref.c)
> 
> So patch 4/4 is not very important, I want to drop patch 4/4.

It would have conflicted with this, btw:

http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.17-rc1/2.6.17-rc1-mm3/broken-out/add-poisonh-and-patch-primary-users.patch

-- 
Mathematics is the supreme nostalgia of our time.

Re: [patch 2/4] kref debugging config option

[Andrew Morton]

Akinobu Mita <mita@miraclelinux.com> wrote:
>
> This patch converts all WARN_ON() in kref code to BUG_ON().

Why?  This change will irritate testers and will decrease their ability to
capture (and hence report) diagnostic info.

Re: [patch 0/4] kref debugging

[Greg KH]

On Mon, Apr 24, 2006 at 04:33:33PM +0800, Akinobu Mita wrote:
> This patch series introduces the DEBUG_KREF config option.

Any reason you didn't CC: the kref author and maintainer?  :(

thanks,

greg k-h

Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Greg KH]

On Mon, Apr 24, 2006 at 04:33:34PM +0800, Akinobu Mita wrote:
> This patch adds warning to detect kref_put() with unreferenced kref.
> 
> Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
> 
>  lib/kref.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> Index: 2.6-git/lib/kref.c
> ===================================================================
> --- 2.6-git.orig/lib/kref.c
> +++ 2.6-git/lib/kref.c
> @@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
>   */
>  int kref_put(struct kref *kref, void (*release)(struct kref *kref))
>  {
> +	WARN_ON(atomic_read(&kref->refcount) < 1);

How can this ever be true?  If the refcount _ever_ goes below 1, the
object is freed.

> @@ -56,12 +57,13 @@ int kref_put(struct kref *kref, void (*r
>  	 * if current count is one, we are the last user and can release object
>  	 * right now, avoiding an atomic operation on 'refcount'
>  	 */
> -	if ((atomic_read(&kref->refcount) == 1) ||
> -	    (atomic_dec_and_test(&kref->refcount))) {
> -		release(kref);
> -		return 1;
> -	}
> -	return 0;
> +	if (atomic_read(&kref->refcount) == 1)
> +		atomic_set(&kref->refcount, 0);
> +	else if (!atomic_dec_and_test(&kref->refcount))
> +		return 0;
> +
> +	release(kref);
> +	return 1;

What does this change do?  What are you trying to help out with?
CONFIG_DEBUG_SLAB will check to see if you reference krefs that have
already been freed.

thanks,

greg k -h

Re: [patch 2/4] kref debugging config option

[Greg KH]

On Mon, Apr 24, 2006 at 04:33:35PM +0800, Akinobu Mita wrote:
> This patch converts all WARN_ON() in kref code to BUG_ON().
> And split them into new DEBUG_KREF config option.
> 
> Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
> 
>  lib/Kconfig.debug |    7 +++++++
>  lib/kref.c        |   30 +++++++++++++++++++++++++-----
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> Index: 2.6-git/lib/Kconfig.debug
> ===================================================================
> --- 2.6-git.orig/lib/Kconfig.debug
> +++ 2.6-git/lib/Kconfig.debug
> @@ -130,6 +130,13 @@ config DEBUG_KOBJECT
>  	  If you say Y here, some extra kobject debugging messages will be sent
>  	  to the syslog. 
>  
> +config DEBUG_KREF
> +	bool "kref debugging"
> +	depends on DEBUG_KERNEL
> +	help
> +	  This option enables addition error checking for kref,
> +	  library routines for handling generic reference counted objects.
> +
>  config DEBUG_HIGHMEM
>  	bool "Highmem debugging"
>  	depends on DEBUG_KERNEL && HIGHMEM
> Index: 2.6-git/lib/kref.c
> ===================================================================
> --- 2.6-git.orig/lib/kref.c
> +++ 2.6-git/lib/kref.c
> @@ -20,16 +20,38 @@
>   */
>  void kref_init(struct kref *kref)
>  {
> -	atomic_set(&kref->refcount,1);
> +	atomic_set(&kref->refcount, 1);
>  }
>  
> +#ifdef CONFIG_DEBUG_KREF
> +static void kref_get_debug_check(struct kref *kref)
> +{
> +	BUG_ON(!atomic_read(&kref->refcount));
> +}

Eeek, no, you do not want to do this.  Unless you never want to get a
bug report with this enabled :)

thanks,

greg k-h

Re: [patch 3/4] dynamic configurable kref debugging

[Greg KH]

On Mon, Apr 24, 2006 at 04:33:36PM +0800, Akinobu Mita wrote:
> This patch makes kref-debugging dynamically configurable
> by sysctl kernel.kref_debug

No new sysctls please, if anything, make this a sysfs file instead.

But even then, this should be a build option, not something you enable
at runtime.

thanks,

greg k-h

Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Valdis.Kletnieks]

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

On Mon, 24 Apr 2006 20:51:28 PDT, Greg KH said:
> On Mon, Apr 24, 2006 at 04:33:34PM +0800, Akinobu Mita wrote:

> > --- 2.6-git.orig/lib/kref.c
> > +++ 2.6-git/lib/kref.c
> > @@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
> >   */
> >  int kref_put(struct kref *kref, void (*release)(struct kref *kref))
> >  {
> > +	WARN_ON(atomic_read(&kref->refcount) < 1);
> 
> How can this ever be true?  If the refcount _ever_ goes below 1, the
> object is freed.

Maybe it should BUG_ON instead in that case. ;)

And strictly speaking, as long as the kref.c stuff is the only stuff to
play with ->refcount, that *should* be true.  On the other hand, if somebody
has a bad pointer that just did a fandango on core, it would be a nice thing
to know that.  Looking at the *next* few lines:

        if ((atomic_read(&kref->refcount) == 1) ||
            (atomic_dec_and_test(&kref->refcount))) {
                release(kref); 
                return 1; 
        }    
        return 0;

If we managed to get a -1 smashed in there, this won't actually trigger
for another 2**32-2 or so kref_put calls - the first test is for "exactly 1",
and the dec_and_test is for "exactly zero"...

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Akinobu Mita]

On Mon, Apr 24, 2006 at 08:51:28PM -0700, Greg KH wrote:
> > @@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
> >   */
> >  int kref_put(struct kref *kref, void (*release)(struct kref *kref))
> >  {
> > +	WARN_ON(atomic_read(&kref->refcount) < 1);
> 
> How can this ever be true?  If the refcount _ever_ goes below 1, the
> object is freed.

The idea of detection kref_put() with unreferenced object was stolen
from BUG_ON()es in blocks/ll_rw_blk.c and fs/bio.c

ll_rw_blk.c:    BUG_ON(atomic_read(&ioc->refcount) == 0);

bio.c:          BIO_BUG_ON(!atomic_read(&bio->bi_cnt));

But the kref counter usually does not become zero. Because kref is
trying to reduce the number of atomic_dec_and_test()

So this patch also set kref counter to zero here:

> > +	if (atomic_read(&kref->refcount) == 1)
> > +		atomic_set(&kref->refcount, 0);

Re: [patch 2/4] kref debugging config option

[Akinobu Mita]

On Mon, Apr 24, 2006 at 02:38:45PM -0700, Andrew Morton wrote:
> Akinobu Mita <mita@miraclelinux.com> wrote:
> >
> > This patch converts all WARN_ON() in kref code to BUG_ON().
> 
> Why?  This change will irritate testers and will decrease their ability to
> capture (and hence report) diagnostic info.

I have no grudge against this BUG_ON().

But BUG_ON() is more prominent than WARN_ON().
Because I often could not realized whether WARN_ON() happned or not.

Should we make warn_counter which will be increment when WARN_ON()
happens, and export it as /proc/warn-counter? (also export die_counter
as /proc/die-counter) Then I'll make pretty gnome applet.

Or put the shell script which just do "dmesg | grep $warn_on_pattern".

Re: [patch 2/4] kref debugging config option

[Greg KH]

On Tue, Apr 25, 2006 at 12:53:37PM +0800, Akinobu Mita wrote:
> On Mon, Apr 24, 2006 at 02:38:45PM -0700, Andrew Morton wrote:
> > Akinobu Mita <mita@miraclelinux.com> wrote:
> > >
> > > This patch converts all WARN_ON() in kref code to BUG_ON().
> > 
> > Why?  This change will irritate testers and will decrease their ability to
> > capture (and hence report) diagnostic info.
> 
> I have no grudge against this BUG_ON().
> 
> But BUG_ON() is more prominent than WARN_ON().

Sure, it crashes your kernel, hopefully you notice that better :)

thanks,

greg k-h

Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Greg KH]

On Tue, Apr 25, 2006 at 12:34:10PM +0800, Akinobu Mita wrote:
> On Mon, Apr 24, 2006 at 08:51:28PM -0700, Greg KH wrote:
> > > @@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
> > >   */
> > >  int kref_put(struct kref *kref, void (*release)(struct kref *kref))
> > >  {
> > > +	WARN_ON(atomic_read(&kref->refcount) < 1);
> > 
> > How can this ever be true?  If the refcount _ever_ goes below 1, the
> > object is freed.
> 
> The idea of detection kref_put() with unreferenced object was stolen
> from BUG_ON()es in blocks/ll_rw_blk.c and fs/bio.c
> 
> ll_rw_blk.c:    BUG_ON(atomic_read(&ioc->refcount) == 0);
> 
> bio.c:          BIO_BUG_ON(!atomic_read(&bio->bi_cnt));
> 
> But the kref counter usually does not become zero. Because kref is
> trying to reduce the number of atomic_dec_and_test()
> 
> So this patch also set kref counter to zero here:
> 
> > > +	if (atomic_read(&kref->refcount) == 1)
> > > +		atomic_set(&kref->refcount, 0);

But again, if this happens the memory will be freed.  So again,
CONFIG_DEBUG_SLAB will catch this kind of thing.

Is there some reason you created these patches?  Were you trying to
debug something that was tracked down to a kref issue?

thanks,

greg k-h

Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Greg KH]

On Tue, Apr 25, 2006 at 12:19:15AM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 24 Apr 2006 20:51:28 PDT, Greg KH said:
> > On Mon, Apr 24, 2006 at 04:33:34PM +0800, Akinobu Mita wrote:
> 
> > > --- 2.6-git.orig/lib/kref.c
> > > +++ 2.6-git/lib/kref.c
> > > @@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
> > >   */
> > >  int kref_put(struct kref *kref, void (*release)(struct kref *kref))
> > >  {
> > > +	WARN_ON(atomic_read(&kref->refcount) < 1);
> > 
> > How can this ever be true?  If the refcount _ever_ goes below 1, the
> > object is freed.
> 
> Maybe it should BUG_ON instead in that case. ;)
> 
> And strictly speaking, as long as the kref.c stuff is the only stuff to
> play with ->refcount, that *should* be true.  On the other hand, if somebody
> has a bad pointer that just did a fandango on core, it would be a nice thing
> to know that.  Looking at the *next* few lines:
> 
>         if ((atomic_read(&kref->refcount) == 1) ||
>             (atomic_dec_and_test(&kref->refcount))) {
>                 release(kref); 
>                 return 1; 
>         }    
>         return 0;
> 
> If we managed to get a -1 smashed in there, this won't actually trigger
> for another 2**32-2 or so kref_put calls - the first test is for "exactly 1",
> and the dec_and_test is for "exactly zero"...

Those two lines were recently added to make this test faster.  See the
archives for details.  If you are really worried about some memory
getting clobbered in there, we should worry about this for the entire
kernel :)

thanks,

greg k-h

Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Akinobu Mita]

> Is there some reason you created these patches?  Were you trying to
> debug something that was tracked down to a kref issue?

I can find many places where I can replace refcounter with kref by doing
"grep -r atomic_dec_and_test linux/".

If we have this detection of kref_put() with unreferenced object,
The work of kref convertions would be more than code cleanup and
consolidation.

Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Rogier Wolff]

On Mon, Apr 24, 2006 at 04:33:34PM +0800, Akinobu Mita wrote:
> @@ -56,12 +57,13 @@ int kref_put(struct kref *kref, void (*r
>  	 * if current count is one, we are the last user and can release object
>  	 * right now, avoiding an atomic operation on 'refcount'
>  	 */
> -	if ((atomic_read(&kref->refcount) == 1) ||
> -	    (atomic_dec_and_test(&kref->refcount))) {
> -		release(kref);
> -		return 1;
> -	}
> -	return 0;
> +	if (atomic_read(&kref->refcount) == 1)
> +		atomic_set(&kref->refcount, 0);
> +	else if (!atomic_dec_and_test(&kref->refcount))
> +		return 0;

This is WRONG. The refcount can be incremented (to two) AFTER the
atomic_read ==1 and the "atomic_set". The original code seems 
ok in this respect. 

	Roger. 


-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ