Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585414 > unrolled thread

[PATCH] sound: convert snd_seq_subscribers.ref_count from atomic_t to refcount_t

Back to article view | Back to linux.kernel

Contents

  [PATCH] sound: convert snd_seq_subscribers.ref_count from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:30 +0100
    Re: [PATCH] sound: convert snd_seq_subscribers.ref_count from  atomic_t to refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-02-21 16:50 +0100
    Re: [PATCH] sound: convert snd_seq_subscribers.ref_count from atomic_t to refcount_t Takashi Iwai <tiwai@suse.de> - 2017-02-23 16:00 +0100

#1585414 — [PATCH] sound: convert snd_seq_subscribers.ref_count from atomic_t to refcount_t

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 sound/core/seq/seq_clientmgr.c | 2 +-
 sound/core/seq/seq_ports.c     | 7 +++----
 sound/core/seq/seq_ports.h     | 3 ++-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index 4c93520..3440f76 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -666,7 +666,7 @@ static int deliver_to_subscribers(struct snd_seq_client *client,
 		down_read(&grp->list_mutex);
 	list_for_each_entry(subs, &grp->list_head, src_list) {
 		/* both ports ready? */
-		if (atomic_read(&subs->ref_count) != 2)
+		if (refcount_read(&subs->ref_count) != 2)
 			continue;
 		event->dest = subs->info.dest;
 		if (subs->info.flags & SNDRV_SEQ_PORT_SUBS_TIMESTAMP)
diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c
index fe686ee..1ca896b 100644
--- a/sound/core/seq/seq_ports.c
+++ b/sound/core/seq/seq_ports.c
@@ -241,7 +241,7 @@ static void clear_subscriber_list(struct snd_seq_client *client,
 			 * we decrease the counter, and when both ports are deleted
 			 * remove the subscriber info
 			 */
-			if (atomic_dec_and_test(&subs->ref_count))
+			if (refcount_dec_and_test(&subs->ref_count))
 				kfree(subs);
 			continue;
 		}
@@ -520,7 +520,6 @@ static int check_and_subscribe_port(struct snd_seq_client *client,
 	else
 		list_add_tail(&subs->dest_list, &grp->list_head);
 	grp->exclusive = exclusive;
-	atomic_inc(&subs->ref_count);
 	write_unlock_irq(&grp->list_lock);
 	err = 0;
 
@@ -570,7 +569,6 @@ int snd_seq_port_connect(struct snd_seq_client *connector,
 		return -ENOMEM;
 
 	subs->info = *info;
-	atomic_set(&subs->ref_count, 0);
 	INIT_LIST_HEAD(&subs->src_list);
 	INIT_LIST_HEAD(&subs->dest_list);
 
@@ -587,6 +585,7 @@ int snd_seq_port_connect(struct snd_seq_client *connector,
 	if (err < 0)
 		goto error_dest;
 
+	refcount_set(&subs->ref_count, 2);
 	return 0;
 
  error_dest:
@@ -613,7 +612,7 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector,
 	/* look for the connection */
 	list_for_each_entry(subs, &src->list_head, src_list) {
 		if (match_subs_info(info, &subs->info)) {
-			atomic_dec(&subs->ref_count); /* mark as not ready */
+			refcount_dec(&subs->ref_count); /* mark as not ready */
 			err = 0;
 			break;
 		}
diff --git a/sound/core/seq/seq_ports.h b/sound/core/seq/seq_ports.h
index 26bd71f..d09e396 100644
--- a/sound/core/seq/seq_ports.h
+++ b/sound/core/seq/seq_ports.h
@@ -21,6 +21,7 @@
 #ifndef __SND_SEQ_PORTS_H
 #define __SND_SEQ_PORTS_H
 
+#include <linux/refcount.h>
 #include <sound/seq_kernel.h>
 #include "seq_lock.h"
 
@@ -44,7 +45,7 @@ struct snd_seq_subscribers {
 	struct snd_seq_port_subscribe info;	/* additional info */
 	struct list_head src_list;	/* link of sources */
 	struct list_head dest_list;	/* link of destinations */
-	atomic_t ref_count;
+	refcount_t ref_count;
 };
 
 struct snd_seq_port_subs_info {
-- 
2.7.4

[toc] | [next] | [standalone]

#1585449 — Re: [PATCH] sound: convert snd_seq_subscribers.ref_count from atomic_t to refcount_t

On Tue, Feb 21, 2017 at 05:27:10PM +0200, Elena Reshetova wrote:
> diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c
> index fe686ee..1ca896b 100644
> --- a/sound/core/seq/seq_ports.c
> +++ b/sound/core/seq/seq_ports.c

> @@ -520,7 +520,6 @@ static int check_and_subscribe_port(struct snd_seq_client *client,
>  	else
>  		list_add_tail(&subs->dest_list, &grp->list_head);
>  	grp->exclusive = exclusive;
> -	atomic_inc(&subs->ref_count);
>  	write_unlock_irq(&grp->list_lock);
>  	err = 0;
>  
> @@ -570,7 +569,6 @@ int snd_seq_port_connect(struct snd_seq_client *connector,
>  		return -ENOMEM;
>  
>  	subs->info = *info;
> -	atomic_set(&subs->ref_count, 0);
>  	INIT_LIST_HEAD(&subs->src_list);
>  	INIT_LIST_HEAD(&subs->dest_list);
>  
> @@ -587,6 +585,7 @@ int snd_seq_port_connect(struct snd_seq_client *connector,
>  	if (err < 0)
>  		goto error_dest;
>  
> +	refcount_set(&subs->ref_count, 2);
>  	return 0;
>  
>   error_dest:
> @@ -613,7 +612,7 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector,
>  	/* look for the connection */
>  	list_for_each_entry(subs, &src->list_head, src_list) {
>  		if (match_subs_info(info, &subs->info)) {
> -			atomic_dec(&subs->ref_count); /* mark as not ready */
> +			refcount_dec(&subs->ref_count); /* mark as not ready */
>  			err = 0;
>  			break;
>  		}

This looks dodgy... someone should look hard at this.

[toc] | [prev] | [next] | [standalone]

#1586937

On Tue, 21 Feb 2017 16:27:10 +0100,
Elena Reshetova wrote:
> 
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.

Well, in this case, it cannot overflow since the highest refcount is
2, as you already figured out.


Takashi

> 
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: David Windsor <dwindsor@gmail.com>
> ---
>  sound/core/seq/seq_clientmgr.c | 2 +-
>  sound/core/seq/seq_ports.c     | 7 +++----
>  sound/core/seq/seq_ports.h     | 3 ++-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
> index 4c93520..3440f76 100644
> --- a/sound/core/seq/seq_clientmgr.c
> +++ b/sound/core/seq/seq_clientmgr.c
> @@ -666,7 +666,7 @@ static int deliver_to_subscribers(struct snd_seq_client *client,
>  		down_read(&grp->list_mutex);
>  	list_for_each_entry(subs, &grp->list_head, src_list) {
>  		/* both ports ready? */
> -		if (atomic_read(&subs->ref_count) != 2)
> +		if (refcount_read(&subs->ref_count) != 2)
>  			continue;
>  		event->dest = subs->info.dest;
>  		if (subs->info.flags & SNDRV_SEQ_PORT_SUBS_TIMESTAMP)
> diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c
> index fe686ee..1ca896b 100644
> --- a/sound/core/seq/seq_ports.c
> +++ b/sound/core/seq/seq_ports.c
> @@ -241,7 +241,7 @@ static void clear_subscriber_list(struct snd_seq_client *client,
>  			 * we decrease the counter, and when both ports are deleted
>  			 * remove the subscriber info
>  			 */
> -			if (atomic_dec_and_test(&subs->ref_count))
> +			if (refcount_dec_and_test(&subs->ref_count))
>  				kfree(subs);
>  			continue;
>  		}
> @@ -520,7 +520,6 @@ static int check_and_subscribe_port(struct snd_seq_client *client,
>  	else
>  		list_add_tail(&subs->dest_list, &grp->list_head);
>  	grp->exclusive = exclusive;
> -	atomic_inc(&subs->ref_count);
>  	write_unlock_irq(&grp->list_lock);
>  	err = 0;
>  
> @@ -570,7 +569,6 @@ int snd_seq_port_connect(struct snd_seq_client *connector,
>  		return -ENOMEM;
>  
>  	subs->info = *info;
> -	atomic_set(&subs->ref_count, 0);
>  	INIT_LIST_HEAD(&subs->src_list);
>  	INIT_LIST_HEAD(&subs->dest_list);
>  
> @@ -587,6 +585,7 @@ int snd_seq_port_connect(struct snd_seq_client *connector,
>  	if (err < 0)
>  		goto error_dest;
>  
> +	refcount_set(&subs->ref_count, 2);
>  	return 0;
>  
>   error_dest:
> @@ -613,7 +612,7 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector,
>  	/* look for the connection */
>  	list_for_each_entry(subs, &src->list_head, src_list) {
>  		if (match_subs_info(info, &subs->info)) {
> -			atomic_dec(&subs->ref_count); /* mark as not ready */
> +			refcount_dec(&subs->ref_count); /* mark as not ready */
>  			err = 0;
>  			break;
>  		}
> diff --git a/sound/core/seq/seq_ports.h b/sound/core/seq/seq_ports.h
> index 26bd71f..d09e396 100644
> --- a/sound/core/seq/seq_ports.h
> +++ b/sound/core/seq/seq_ports.h
> @@ -21,6 +21,7 @@
>  #ifndef __SND_SEQ_PORTS_H
>  #define __SND_SEQ_PORTS_H
>  
> +#include <linux/refcount.h>
>  #include <sound/seq_kernel.h>
>  #include "seq_lock.h"
>  
> @@ -44,7 +45,7 @@ struct snd_seq_subscribers {
>  	struct snd_seq_port_subscribe info;	/* additional info */
>  	struct list_head src_list;	/* link of sources */
>  	struct list_head dest_list;	/* link of destinations */
> -	atomic_t ref_count;
> +	refcount_t ref_count;
>  };
>  
>  struct snd_seq_port_subs_info {
> -- 
> 2.7.4
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web