[PATCH 1/3] ALSA: core: trivial code style fix

[PATCH 1/3] ALSA: core: trivial code style fix

[Lu Guanqun]

remove trailing tab on the line.

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 sound/core/control.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index f8c5be4..7f2b3a7 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1072,7 +1072,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	long private_size;
 	struct user_element *ue;
 	int idx, err;
-	
+
 	if (card->user_ctl_count >= MAX_USER_CONTROLS)
 		return -ENOMEM;
 	if (info->count < 1)

[PATCH 2/3] ALSA: core: release the constraint check for replace ops

[Lu Guanqun]

Suppose the ALSA card already has a number of MAX_USER_CONTROLS controls, and
the user wants to replace one, it should not fail at this condition check.

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 sound/core/control.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 7f2b3a7..98b7fc7 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1073,7 +1073,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	struct user_element *ue;
 	int idx, err;
 
-	if (card->user_ctl_count >= MAX_USER_CONTROLS)
+	if (card->user_ctl_count >= MAX_USER_CONTROLS + !!replace)
 		return -ENOMEM;
 	if (info->count < 1)
 		return -EINVAL;

Re: [PATCH 2/3] ALSA: core: release the constraint check for replace ops

[Takashi Iwai]

At Wed, 24 Aug 2011 11:12:39 +0800,
Lu Guanqun wrote:
> 
> Suppose the ALSA card already has a number of MAX_USER_CONTROLS controls, and
> the user wants to replace one, it should not fail at this condition check.

It'd be simpler to ignore the check when replace flag is set?
The proposed change here looks too tricky, IMO...


thanks,

Takashi


> Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
> ---
>  sound/core/control.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 7f2b3a7..98b7fc7 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1073,7 +1073,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	struct user_element *ue;
>  	int idx, err;
>  
> -	if (card->user_ctl_count >= MAX_USER_CONTROLS)
> +	if (card->user_ctl_count >= MAX_USER_CONTROLS + !!replace)
>  		return -ENOMEM;
>  	if (info->count < 1)
>  		return -EINVAL;
> 

Re: [PATCH 2/3] ALSA: core: release the constraint check for replace ops

[Lu Guanqun]

On Wed, Aug 24, 2011 at 02:01:08PM +0800, Takashi Iwai wrote:
> At Wed, 24 Aug 2011 11:12:39 +0800,
> Lu Guanqun wrote:
> > 
> > Suppose the ALSA card already has a number of MAX_USER_CONTROLS controls, and
> > the user wants to replace one, it should not fail at this condition check.
> 
> It'd be simpler to ignore the check when replace flag is set?
> The proposed change here looks too tricky, IMO...

Agreed. I'll cook another one.

-- 
guanqun

[PATCH v2] ALSA: core: release the constraint check for replace ops

[Lu Guanqun]

Suppose the ALSA card already has a number of MAX_USER_CONTROLS controls, and
the user wants to replace one, it should not fail at this condition check.

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 sound/core/control.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 7f2b3a7..dc2a440 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1073,7 +1073,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	struct user_element *ue;
 	int idx, err;
 
-	if (card->user_ctl_count >= MAX_USER_CONTROLS)
+	if (!replace && card->user_ctl_count >= MAX_USER_CONTROLS)
 		return -ENOMEM;
 	if (info->count < 1)
 		return -EINVAL;

Re: [PATCH v2] ALSA: core: release the constraint check for replace ops

[Takashi Iwai]

At Wed, 24 Aug 2011 14:45:10 +0800,
Lu Guanqun wrote:
> 
> Suppose the ALSA card already has a number of MAX_USER_CONTROLS controls, and
> the user wants to replace one, it should not fail at this condition check.
> 
> Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>

Thanks, applied.


Takashi

> ---
>  sound/core/control.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 7f2b3a7..dc2a440 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1073,7 +1073,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	struct user_element *ue;
>  	int idx, err;
>  
> -	if (card->user_ctl_count >= MAX_USER_CONTROLS)
> +	if (!replace && card->user_ctl_count >= MAX_USER_CONTROLS)
>  		return -ENOMEM;
>  	if (info->count < 1)
>  		return -EINVAL;
> 

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

[Lu Guanqun]

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().

This patch fixes this issue.

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 sound/core/control.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 98b7fc7..30e1483 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -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->count;
 	access |= SNDRV_CTL_ELEM_ACCESS_USER;
 	kctl.info = snd_ctl_elem_user_info;
 	if (access & SNDRV_CTL_ELEM_ACCESS_READ)

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Takashi Iwai]

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.

What we need is the documentation of this feature...


thanks,

Takashi


> This patch fixes this issue.
> 
> Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
> ---
>  sound/core/control.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 98b7fc7..30e1483 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -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->count;
>  	access |= SNDRV_CTL_ELEM_ACCESS_USER;
>  	kctl.info = snd_ctl_elem_user_info;
>  	if (access & SNDRV_CTL_ELEM_ACCESS_READ)
> 

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Lu Guanqun]

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. 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?

-- 
guanqun

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Takashi Iwai]

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.


Takashi

Re: [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);

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Lu Guanqun]

On Wed, Aug 24, 2011 at 04:42:05PM +0800, Takashi Iwai wrote:
> ---
> 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;

Hi Takashi,

I don't how this approach can solve this problem. There's still an
implicit assumption here that user space application will clear
info->elem_count to zero before it issues the REPLACE ioctl, right?

>  	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));

Should we add some assertion here checking the size of these two
structure should be the same?

>  	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);

-- 
guanqun

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Takashi Iwai]

At Wed, 24 Aug 2011 19:46:13 +0800,
Lu Guanqun wrote:
> 
> On Wed, Aug 24, 2011 at 04:42:05PM +0800, Takashi Iwai wrote:
> > ---
> > 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;
> 
> Hi Takashi,
> 
> I don't how this approach can solve this problem.

There is no problem.  You can't copy the data from ELEM_INFO for
ELEM_ADD/REPLACE.  It was a wrong assumption to reuse the data.
Instead, you had to set up the struct field manually.
That is, the test procedure was simply wrong.

> There's still an
> implicit assumption here that user space application will clear
> info->elem_count to zero before it issues the REPLACE ioctl, right?

Yes, the point is that it's no longer same structure.  So, you can't
copy from the result from ELEM_INFO ioctl any more.  Instead, you need
to set up the struct field manually.

> >  	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));
> 
> Should we add some assertion here checking the size of these two
> structure should be the same?

It could, at most by BUILD_BUG_ON().


thanks,

Takashi

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Lu Guanqun]

On Wed, Aug 24, 2011 at 08:05:56PM +0800, Takashi Iwai wrote:
> > Hi Takashi,
> > 
> > I don't how this approach can solve this problem.
> 
> There is no problem.  You can't copy the data from ELEM_INFO for
> ELEM_ADD/REPLACE.  It was a wrong assumption to reuse the data.
> Instead, you had to set up the struct field manually.
> That is, the test procedure was simply wrong.
> 
> > There's still an
> > implicit assumption here that user space application will clear
> > info->elem_count to zero before it issues the REPLACE ioctl, right?
> 
> Yes, the point is that it's no longer same structure.  So, you can't
> copy from the result from ELEM_INFO ioctl any more.  Instead, you need
> to set up the struct field manually.

Thanks Takashi, that's the info I'm looking for. We need to state this
requirements explicitly, it seems we lack these kind of documentation and
thus it results in poorly written test cases in user space.

Is there any man pages or documentation we can state this clearly? I can
help to contribute some documentation or even add some sample programs
there.

-- 
guanqun

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Takashi Iwai]

At Wed, 24 Aug 2011 20:46:38 +0800,
Lu Guanqun wrote:
> 
> On Wed, Aug 24, 2011 at 08:05:56PM +0800, Takashi Iwai wrote:
> > > Hi Takashi,
> > > 
> > > I don't how this approach can solve this problem.
> > 
> > There is no problem.  You can't copy the data from ELEM_INFO for
> > ELEM_ADD/REPLACE.  It was a wrong assumption to reuse the data.
> > Instead, you had to set up the struct field manually.
> > That is, the test procedure was simply wrong.
> > 
> > > There's still an
> > > implicit assumption here that user space application will clear
> > > info->elem_count to zero before it issues the REPLACE ioctl, right?
> > 
> > Yes, the point is that it's no longer same structure.  So, you can't
> > copy from the result from ELEM_INFO ioctl any more.  Instead, you need
> > to set up the struct field manually.
> 
> Thanks Takashi, that's the info I'm looking for. We need to state this
> requirements explicitly, it seems we lack these kind of documentation and
> thus it results in poorly written test cases in user space.

Yep.

> Is there any man pages or documentation we can state this clearly?

No.  Partly because the user-space apps are supposed to use alsa-lib.

> I can
> help to contribute some documentation or even add some sample programs
> there.

Yes, please, that'll be helpful.


thanks,

Takashi

Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly

[Takashi Iwai]

At Wed, 24 Aug 2011 08:06:34 +0200,
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.

Correction: the count field means same as others.  The info.count
specifies the array size in IOCTL_ELEM_INFO, too.
The only difference is an (ab)use of owner field.


Takashi

Re: [PATCH 1/3] ALSA: core: trivial code style fix

[Takashi Iwai]

At Wed, 24 Aug 2011 11:12:34 +0800,
Lu Guanqun wrote:
> 
> remove trailing tab on the line.
> 
> Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>

Applied now.  Thanks.


Takashi

> ---
>  sound/core/control.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index f8c5be4..7f2b3a7 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1072,7 +1072,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	long private_size;
>  	struct user_element *ue;
>  	int idx, err;
> -	
> +
>  	if (card->user_ctl_count >= MAX_USER_CONTROLS)
>  		return -ENOMEM;
>  	if (info->count < 1)
>