[alsa-devel] [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Takashi Iwai]

At Wed, 24 Aug 2011 08:26:47 +0200,
Takashi Iwai wrote:
> 
> At Wed, 24 Aug 2011 14:17:57 +0800,
> Lu Guanqun wrote:
> > 
> > On Wed, Aug 24, 2011 at 02:06:34PM +0800, Takashi Iwai wrote:
> > > At Wed, 24 Aug 2011 11:12:43 +0800,
> > > Lu Guanqun wrote:
> > > > 
> > > > I don't see how info's owner field relates to kcontrol's count field. It should
> > > > assign to info's count instead.
> > > > 
> > > > Let's assume this scenario:
> > > > 
> > > > 1. user reads the control element from kernel (the owner field is set)
> > > > 2. user changes some values
> > > > 3. user issues 'SNDRV_CTL_IOCTL_ELEM_REPLACE' ioctl.
> > > > 
> > > > With the original code, 'kctl' here gets a large count number due to its
> > > > non-empty owner field. Therefore it fails on subsequent call snd_ctl_new().
> > > 
> > > In IOCTL_ELEM_ADD and REPLACE, count and owner struct fields have
> > > different meanings from others.  The count contains the array size,
> > > not the number of identical elements.  And the owner contains the
> > > number of elements to be created.  So, the current code is correct.
> > 
> > This sounds tricky...
> > 
> > in snd_ctl_elem_info():
> > 	info->owner = pid_vnr(vd->owner->pid);
> > } else {
> > 	info->owner = -1;
> > 
> > This is where owner of info get assigned, but it's not number of
> > elements to be created.
> 
> The kcontrol.count field is never exposed to user-space.
> 
> > I'm a bit unclear on this.
> > 
> > > 
> > > What we need is the documentation of this feature...
> > 
> > So my test case seems to be wrong, but how should I change it? Do I need
> > to clear the owner field when user space application sends down the
> > REPLACE ioctl?
> 
> Yes.  Maybe it'd be better to define a new struct and use it in
> ADD/REPLACE ioctls so that user sees a clear difference.  The struct
> will be identical except for renaming from owner to elem_count or
> such.

I mean a patch like below (untested).


Takashi

---
diff --git a/include/sound/asound.h b/include/sound/asound.h
index 5d6074f..9f5f55d 100644
--- a/include/sound/asound.h
+++ b/include/sound/asound.h
@@ -813,6 +813,33 @@ struct snd_ctl_elem_info {
 	unsigned char reserved[64-4*sizeof(unsigned short)];
 };
 
+/* The struct used for ELEM_ADD and ELEM_REPLACE ioctls;
+ * This is is identical with snd_ctl_elem_info, but just owner field is
+ * different, keeping the number of elements to be created.
+ * The unused fields are stripped for simplicity.
+ */
+struct snd_ctl_elem_add_info {
+	struct snd_ctl_elem_id id;	/* element ID */
+	snd_ctl_elem_type_t type;	/* value type */
+	unsigned int access;		/* value access (bitmask) */
+	unsigned int count;		/* count of values */
+	__kernel_pid_t elem_count;	/* # of elements to create (0=1) */
+	union {
+		struct {
+			long min;		/* R: minimum value */
+			long max;		/* R: maximum value */
+			long step;		/* R: step (0 variable) */
+		} integer;
+		struct {
+			long long min;		/* R: minimum value */
+			long long max;		/* R: maximum value */
+			long long step;		/* R: step (0 variable) */
+		} integer64;
+		unsigned char reserved[128];
+	} value;
+	unsigned char reserved[64];
+};
+
 struct snd_ctl_elem_value {
 	struct snd_ctl_elem_id id;	/* W: element ID */
 	unsigned int indirect: 1;	/* W: indirect access - obsoleted */
@@ -854,8 +881,8 @@ struct snd_ctl_tlv {
 #define SNDRV_CTL_IOCTL_ELEM_LOCK	_IOW('U', 0x14, struct snd_ctl_elem_id)
 #define SNDRV_CTL_IOCTL_ELEM_UNLOCK	_IOW('U', 0x15, struct snd_ctl_elem_id)
 #define SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS _IOWR('U', 0x16, int)
-#define SNDRV_CTL_IOCTL_ELEM_ADD	_IOWR('U', 0x17, struct snd_ctl_elem_info)
-#define SNDRV_CTL_IOCTL_ELEM_REPLACE	_IOWR('U', 0x18, struct snd_ctl_elem_info)
+#define SNDRV_CTL_IOCTL_ELEM_ADD	_IOWR('U', 0x17, struct snd_ctl_elem_add_info)
+#define SNDRV_CTL_IOCTL_ELEM_REPLACE	_IOWR('U', 0x18, struct snd_ctl_elem_add_info)
 #define SNDRV_CTL_IOCTL_ELEM_REMOVE	_IOWR('U', 0x19, struct snd_ctl_elem_id)
 #define SNDRV_CTL_IOCTL_TLV_READ	_IOWR('U', 0x1a, struct snd_ctl_tlv)
 #define SNDRV_CTL_IOCTL_TLV_WRITE	_IOWR('U', 0x1b, struct snd_ctl_tlv)
diff --git a/sound/core/control.c b/sound/core/control.c
index dc2a440..15c171d 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1064,7 +1064,7 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
 }
 
 static int snd_ctl_elem_add(struct snd_ctl_file *file,
-			    struct snd_ctl_elem_info *info, int replace)
+			    struct snd_ctl_elem_add_info *info, int replace)
 {
 	struct snd_card *card = file->card;
 	struct snd_kcontrol kctl, *_kctl;
@@ -1099,7 +1099,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (err < 0)
 		return err;
 	memcpy(&kctl.id, &info->id, sizeof(info->id));
-	kctl.count = info->owner ? info->owner : 1;
+	kctl.count = info->elem_count ? info->elem_count : 1;
 	access |= SNDRV_CTL_ELEM_ACCESS_USER;
 	kctl.info = snd_ctl_elem_user_info;
 	if (access & SNDRV_CTL_ELEM_ACCESS_READ)
@@ -1139,7 +1139,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL);
 	if (ue == NULL)
 		return -ENOMEM;
-	ue->info = *info;
+	memcpy(&ue->info, info, sizeof(*info));
 	ue->info.access = 0;
 	ue->elem_data = (char *)ue + sizeof(*ue);
 	ue->elem_data_size = private_size;
@@ -1164,9 +1164,10 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 }
 
 static int snd_ctl_elem_add_user(struct snd_ctl_file *file,
-				 struct snd_ctl_elem_info __user *_info, int replace)
+				 struct snd_ctl_elem_add_info __user *_info,
+				 int replace)
 {
-	struct snd_ctl_elem_info info;
+	struct snd_ctl_elem_add_info info;
 	if (copy_from_user(&info, _info, sizeof(info)))
 		return -EFAULT;
 	return snd_ctl_elem_add(file, &info, replace);