[PATCH] target: fix memory leak on XCOPY

[PATCH] target: fix memory leak on XCOPY

[Mikulas Patocka]

On each processed XCOPY command, two "kmalloc-512" memory objects are
leaked. These represent two allocations of struct xcopy_pt_cmd in
target_core_xcopy.c.

The reason for the memory leak is that the cmd_kref field is not
initialized (thus, it is zero because the allocations were done with
kzalloc). When we decrement zero kref in target_put_sess_cmd, the result
is not zero, thus target_release_cmd_kref is not called.

This patch fixes the bug by moving kref initialization from
target_get_sess_cmd to transport_init_se_cmd (this function is called from
target_core_xcopy.c, so it will correctly initialize cmd_kref). It can be
easily verified that all code that calls target_get_sess_cmd also calls
transport_init_se_cmd earlier, thus moving kref_init shouldn't introduce
any new problems.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org	# 3.12+

---
 drivers/target/target_core_transport.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-3.15-rc5/drivers/target/target_core_transport.c
===================================================================
--- linux-3.15-rc5.orig/drivers/target/target_core_transport.c	2014-04-14 16:08:15.000000000 +0200
+++ linux-3.15-rc5/drivers/target/target_core_transport.c	2014-05-16 18:24:57.000000000 +0200
@@ -1113,6 +1113,7 @@ void transport_init_se_cmd(
 	init_completion(&cmd->cmd_wait_comp);
 	init_completion(&cmd->task_stop_comp);
 	spin_lock_init(&cmd->t_state_lock);
+	kref_init(&cmd->cmd_kref);
 	cmd->transport_state = CMD_T_DEV_ACTIVE;
 
 	cmd->se_tfo = tfo;
@@ -2357,7 +2358,6 @@ int target_get_sess_cmd(struct se_sessio
 	unsigned long flags;
 	int ret = 0;
 
-	kref_init(&se_cmd->cmd_kref);
 	/*
 	 * Add a second kref if the fabric caller is expecting to handle
 	 * fabric acknowledgement that requires two target_put_sess_cmd()

[PATCH] kref: warn on uninitialized kref

[Mikulas Patocka]

I found a memory leak in iSCSI target that was caused by kref initialized
to zero (the memory object was allocated with kzalloc, kref_init was not
called and kref_put_spinlock_irqsave was called which changed "0" to "-1"
and didn't free the object).

Similar bugs may exist in other kernel areas, so I submit this patch that
adds a check to kref.h. If the value is zero or negative, we can assume
that it is uninitialized and we warn about it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 include/linux/kref.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-3.15-rc5/include/linux/kref.h
===================================================================
--- linux-3.15-rc5.orig/include/linux/kref.h	2013-07-02 22:23:38.000000000 +0200
+++ linux-3.15-rc5/include/linux/kref.h	2014-05-16 18:56:18.000000000 +0200
@@ -69,7 +69,7 @@ static inline int kref_sub(struct kref *
 	     void (*release)(struct kref *kref))
 {
 	WARN_ON(release == NULL);
-
+	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
 	if (atomic_sub_and_test((int) count, &kref->refcount)) {
 		release(kref);
 		return 1;
@@ -119,6 +119,7 @@ static inline int kref_put_spinlock_irqs
 	unsigned long flags;
 
 	WARN_ON(release == NULL);
+	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
 	if (atomic_add_unless(&kref->refcount, -1, 1))
 		return 0;
 	spin_lock_irqsave(lock, flags);
@@ -136,6 +137,7 @@ static inline int kref_put_mutex(struct 
 				 struct mutex *lock)
 {
 	WARN_ON(release == NULL);
+	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
 	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
 		mutex_lock(lock);
 		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {

Re: [PATCH] kref: warn on uninitialized kref

[Mateusz Guzik]

On Sat, May 17, 2014 at 06:53:17AM -0400, Mikulas Patocka wrote:
> I found a memory leak in iSCSI target that was caused by kref initialized
> to zero (the memory object was allocated with kzalloc, kref_init was not
> called and kref_put_spinlock_irqsave was called which changed "0" to "-1"
> and didn't free the object).
> 
> Similar bugs may exist in other kernel areas, so I submit this patch that
> adds a check to kref.h. If the value is zero or negative, we can assume
> that it is uninitialized and we warn about it.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  include/linux/kref.h |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux-3.15-rc5/include/linux/kref.h
> ===================================================================
> --- linux-3.15-rc5.orig/include/linux/kref.h	2013-07-02 22:23:38.000000000 +0200
> +++ linux-3.15-rc5/include/linux/kref.h	2014-05-16 18:56:18.000000000 +0200
> @@ -69,7 +69,7 @@ static inline int kref_sub(struct kref *
>  	     void (*release)(struct kref *kref))
>  {
>  	WARN_ON(release == NULL);
> -
> +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
>  	if (atomic_sub_and_test((int) count, &kref->refcount)) {
>  		release(kref);
>  		return 1;
> @@ -119,6 +119,7 @@ static inline int kref_put_spinlock_irqs
>  	unsigned long flags;
>  
>  	WARN_ON(release == NULL);
> +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
>  	if (atomic_add_unless(&kref->refcount, -1, 1))
>  		return 0;
>  	spin_lock_irqsave(lock, flags);
> @@ -136,6 +137,7 @@ static inline int kref_put_mutex(struct 
>  				 struct mutex *lock)
>  {
>  	WARN_ON(release == NULL);
> +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
>  	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
>  		mutex_lock(lock);
>  		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {

This has a side effect of detecting some overputs, which is nice.

However, could be made better if kref_sub checked that refs you want to
take don't put the count below 0.

i.e.
WARN_ON(count > atomic_read(&kref->refcount));

this also detects your original problem.

-- 
Mateusz Guzik

Re: [PATCH] kref: warn on uninitialized kref

[Mikulas Patocka]

On Sat, 17 May 2014, Mateusz Guzik wrote:

> On Sat, May 17, 2014 at 06:53:17AM -0400, Mikulas Patocka wrote:
> > I found a memory leak in iSCSI target that was caused by kref initialized
> > to zero (the memory object was allocated with kzalloc, kref_init was not
> > called and kref_put_spinlock_irqsave was called which changed "0" to "-1"
> > and didn't free the object).
> > 
> > Similar bugs may exist in other kernel areas, so I submit this patch that
> > adds a check to kref.h. If the value is zero or negative, we can assume
> > that it is uninitialized and we warn about it.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  include/linux/kref.h |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > Index: linux-3.15-rc5/include/linux/kref.h
> > ===================================================================
> > --- linux-3.15-rc5.orig/include/linux/kref.h	2013-07-02 22:23:38.000000000 +0200
> > +++ linux-3.15-rc5/include/linux/kref.h	2014-05-16 18:56:18.000000000 +0200
> > @@ -69,7 +69,7 @@ static inline int kref_sub(struct kref *
> >  	     void (*release)(struct kref *kref))
> >  {
> >  	WARN_ON(release == NULL);
> > -
> > +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
> >  	if (atomic_sub_and_test((int) count, &kref->refcount)) {
> >  		release(kref);
> >  		return 1;
> > @@ -119,6 +119,7 @@ static inline int kref_put_spinlock_irqs
> >  	unsigned long flags;
> >  
> >  	WARN_ON(release == NULL);
> > +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
> >  	if (atomic_add_unless(&kref->refcount, -1, 1))
> >  		return 0;
> >  	spin_lock_irqsave(lock, flags);
> > @@ -136,6 +137,7 @@ static inline int kref_put_mutex(struct 
> >  				 struct mutex *lock)
> >  {
> >  	WARN_ON(release == NULL);
> > +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
> >  	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
> >  		mutex_lock(lock);
> >  		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
> 
> This has a side effect of detecting some overputs, which is nice.
> 
> However, could be made better if kref_sub checked that refs you want to
> take don't put the count below 0.
> 
> i.e.
> WARN_ON(count > atomic_read(&kref->refcount));
> 
> this also detects your original problem.
> 
> -- 
> Mateusz Guzik

Good point. I'm sending the updated patch.

Mikulas

[PATCH v2] kref: warn on uninitialized kref

[Mikulas Patocka]

I found a memory leak in iSCSI target that was caused by kref initialized
to zero (the memory object was allocated with kzalloc, kref_init was not
called and kref_put_spinlock_irqsave was called which changed "0" to "-1"
and didn't free the object).

Similar bugs may exist in other kernel areas, so I submit this patch that
adds a check to kref.h. If the value is zero or negative, we can assume
that it is uninitialized and we warn about it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 include/linux/kref.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-3.15-rc5/include/linux/kref.h
===================================================================
--- linux-3.15-rc5.orig/include/linux/kref.h	2014-05-16 19:00:18.000000000 +0200
+++ linux-3.15-rc5/include/linux/kref.h	2014-05-17 13:19:31.000000000 +0200
@@ -69,7 +69,7 @@ static inline int kref_sub(struct kref *
 	     void (*release)(struct kref *kref))
 {
 	WARN_ON(release == NULL);
-
+	WARN_ON_ONCE(atomic_read(&kref->refcount) < (int)count);
 	if (atomic_sub_and_test((int) count, &kref->refcount)) {
 		release(kref);
 		return 1;
@@ -119,6 +119,7 @@ static inline int kref_put_spinlock_irqs
 	unsigned long flags;
 
 	WARN_ON(release == NULL);
+	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
 	if (atomic_add_unless(&kref->refcount, -1, 1))
 		return 0;
 	spin_lock_irqsave(lock, flags);
@@ -136,6 +137,7 @@ static inline int kref_put_mutex(struct 
 				 struct mutex *lock)
 {
 	WARN_ON(release == NULL);
+	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
 	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
 		mutex_lock(lock);
 		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {

Re: [PATCH v2] kref: warn on uninitialized kref

[Bart Van Assche]

On 05/17/14 14:38, Mikulas Patocka wrote:
> I found a memory leak in iSCSI target that was caused by kref initialized
> to zero (the memory object was allocated with kzalloc, kref_init was not
> called and kref_put_spinlock_irqsave was called which changed "0" to "-1"
> and didn't free the object).
> 
> Similar bugs may exist in other kernel areas, so I submit this patch that
> adds a check to kref.h. If the value is zero or negative, we can assume
> that it is uninitialized and we warn about it.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  include/linux/kref.h |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux-3.15-rc5/include/linux/kref.h
> ===================================================================
> --- linux-3.15-rc5.orig/include/linux/kref.h	2014-05-16 19:00:18.000000000 +0200
> +++ linux-3.15-rc5/include/linux/kref.h	2014-05-17 13:19:31.000000000 +0200
> @@ -69,7 +69,7 @@ static inline int kref_sub(struct kref *
>  	     void (*release)(struct kref *kref))
>  {
>  	WARN_ON(release == NULL);
> -
> +	WARN_ON_ONCE(atomic_read(&kref->refcount) < (int)count);
>  	if (atomic_sub_and_test((int) count, &kref->refcount)) {
>  		release(kref);
>  		return 1;
> @@ -119,6 +119,7 @@ static inline int kref_put_spinlock_irqs
>  	unsigned long flags;
>  
>  	WARN_ON(release == NULL);
> +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
>  	if (atomic_add_unless(&kref->refcount, -1, 1))
>  		return 0;
>  	spin_lock_irqsave(lock, flags);
> @@ -136,6 +137,7 @@ static inline int kref_put_mutex(struct 
>  				 struct mutex *lock)
>  {
>  	WARN_ON(release == NULL);
> +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
>  	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
>  		mutex_lock(lock);
>  		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {

This patch adds two conditional branches and one atomic read to
kref_sub(). What is the performance impact of this patch on kernel code
that uses kref_put() in the hot path ? Has it been considered to enable
the newly added code only if a CONFIG_DEBUG_* macro has been set ?

Bart.

Re: [PATCH v2] kref: warn on uninitialized kref

[Mikulas Patocka]

On Sat, 17 May 2014, Bart Van Assche wrote:

> On 05/17/14 14:38, Mikulas Patocka wrote:
> > I found a memory leak in iSCSI target that was caused by kref initialized
> > to zero (the memory object was allocated with kzalloc, kref_init was not
> > called and kref_put_spinlock_irqsave was called which changed "0" to "-1"
> > and didn't free the object).
> > 
> > Similar bugs may exist in other kernel areas, so I submit this patch that
> > adds a check to kref.h. If the value is zero or negative, we can assume
> > that it is uninitialized and we warn about it.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  include/linux/kref.h |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > Index: linux-3.15-rc5/include/linux/kref.h
> > ===================================================================
> > --- linux-3.15-rc5.orig/include/linux/kref.h	2014-05-16 19:00:18.000000000 +0200
> > +++ linux-3.15-rc5/include/linux/kref.h	2014-05-17 13:19:31.000000000 +0200
> > @@ -69,7 +69,7 @@ static inline int kref_sub(struct kref *
> >  	     void (*release)(struct kref *kref))
> >  {
> >  	WARN_ON(release == NULL);
> > -
> > +	WARN_ON_ONCE(atomic_read(&kref->refcount) < (int)count);
> >  	if (atomic_sub_and_test((int) count, &kref->refcount)) {
> >  		release(kref);
> >  		return 1;
> > @@ -119,6 +119,7 @@ static inline int kref_put_spinlock_irqs
> >  	unsigned long flags;
> >  
> >  	WARN_ON(release == NULL);
> > +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
> >  	if (atomic_add_unless(&kref->refcount, -1, 1))
> >  		return 0;
> >  	spin_lock_irqsave(lock, flags);
> > @@ -136,6 +137,7 @@ static inline int kref_put_mutex(struct 
> >  				 struct mutex *lock)
> >  {
> >  	WARN_ON(release == NULL);
> > +	WARN_ON_ONCE(atomic_read(&kref->refcount) <= 0);
> >  	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
> >  		mutex_lock(lock);
> >  		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
> 
> This patch adds two conditional branches and one atomic read to
> kref_sub(). What is the performance impact of this patch on kernel code
> that uses kref_put() in the hot path ? Has it been considered to enable
> the newly added code only if a CONFIG_DEBUG_* macro has been set ?
> 
> Bart.

The atomic modification takes several tens of ticks, the atomic read and 
compare is just one tick. I don't exepct any performance problem with 
this.

BTW. if we talk about performance - what about replacing:

	if (atomic_dec_and_test(&variable)) {
		... release(object);
	}

with this:

	if (atomic_read(&variable) == 1 || atomic_dec_and_test(&variable)) {
		barrier();
		... release(object);
	}

It avoids the heavy atomic instruction if there is just one reference. Is 
there any problem with this? At least on x86 we could do this always 
(there is no read reordering in hardware, so barrier() is sufficient to 
prevent reads from being reordered with atomic_read). On the architectures 
that reorder reads, we could do it only if the release method doesn't 
contain any reads of the object being released.

Mikulas

Re: [PATCH] target: fix memory leak on XCOPY

[Nicholas A. Bellinger]

On Sat, 2014-05-17 at 06:49 -0400, Mikulas Patocka wrote:
> On each processed XCOPY command, two "kmalloc-512" memory objects are
> leaked. These represent two allocations of struct xcopy_pt_cmd in
> target_core_xcopy.c.
> 
> The reason for the memory leak is that the cmd_kref field is not
> initialized (thus, it is zero because the allocations were done with
> kzalloc). When we decrement zero kref in target_put_sess_cmd, the result
> is not zero, thus target_release_cmd_kref is not called.
> 
> This patch fixes the bug by moving kref initialization from
> target_get_sess_cmd to transport_init_se_cmd (this function is called from
> target_core_xcopy.c, so it will correctly initialize cmd_kref). It can be
> easily verified that all code that calls target_get_sess_cmd also calls
> transport_init_se_cmd earlier, thus moving kref_init shouldn't introduce
> any new problems.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org	# 3.12+
> 
> ---
>  drivers/target/target_core_transport.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-3.15-rc5/drivers/target/target_core_transport.c
> ===================================================================
> --- linux-3.15-rc5.orig/drivers/target/target_core_transport.c	2014-04-14 16:08:15.000000000 +0200
> +++ linux-3.15-rc5/drivers/target/target_core_transport.c	2014-05-16 18:24:57.000000000 +0200
> @@ -1113,6 +1113,7 @@ void transport_init_se_cmd(
>  	init_completion(&cmd->cmd_wait_comp);
>  	init_completion(&cmd->task_stop_comp);
>  	spin_lock_init(&cmd->t_state_lock);
> +	kref_init(&cmd->cmd_kref);
>  	cmd->transport_state = CMD_T_DEV_ACTIVE;
>  
>  	cmd->se_tfo = tfo;
> @@ -2357,7 +2358,6 @@ int target_get_sess_cmd(struct se_sessio
>  	unsigned long flags;
>  	int ret = 0;
>  
> -	kref_init(&se_cmd->cmd_kref);
>  	/*
>  	 * Add a second kref if the fabric caller is expecting to handle
>  	 * fabric acknowledgement that requires two target_put_sess_cmd()
> --

Applied.  Thanks alot Mikulas!

--nab

Re: [PATCH v2] kref: warn on uninitialized kref

[Bart Van Assche]

On 05/17/14 23:14, Mikulas Patocka wrote:
> BTW. if we talk about performance - what about replacing:
> 
> 	if (atomic_dec_and_test(&variable)) {
> 		... release(object);
> 	}
> 
> with this:
> 
> 	if (atomic_read(&variable) == 1 || atomic_dec_and_test(&variable)) {
> 		barrier();
> 		... release(object);
> 	}
> 
> It avoids the heavy atomic instruction if there is just one reference. Is 
> there any problem with this? At least on x86 we could do this always 
> (there is no read reordering in hardware, so barrier() is sufficient to 
> prevent reads from being reordered with atomic_read). On the architectures 
> that reorder reads, we could do it only if the release method doesn't 
> contain any reads of the object being released.

Although I'm not sure how big the performance impact is in this context,
this change has a performance impact if variable > 1. The
atomic_dec_and_test() triggers at most one cache line state transition.
The atomic_read() + atomic_dec_and_test() triggers two cache line state
transitions if "variable" is not in the local cache, namely first from
invalid to shared and then from shared to exclusive. See also section
"11.4 CACHE CONTROL PROTOCOL" and "Table 11-4 MESI Cache Line States" in
the Intel Software Developer Manual, Volume 3 for more information.

Bart.

Re: [PATCH v2] kref: warn on uninitialized kref

[Peter Zijlstra]

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

On Sat, May 17, 2014 at 05:14:13PM -0400, Mikulas Patocka wrote:
> BTW. if we talk about performance - what about replacing:
> 
> 	if (atomic_dec_and_test(&variable)) {
> 		... release(object);
> 	}
> 
> with this:
> 
> 	if (atomic_read(&variable) == 1 || atomic_dec_and_test(&variable)) {
> 		barrier();
> 		... release(object);
> 	}

That would completely wreck kref_get_unless_zero().

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