[PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Hi

It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
but its not. bug showed again reliably under memory-pressure.

      Karsten

===

Reset file->f_op in snd_card_file_remove(). Take 2


i think what happens here is:

  us428control runs, kernel has allocated a struct file for /dev/hwC1D0.

  usb disconnect

  snd_usb_usx2y calls snd_card_disconnect,
  tells us428control to exit.

  snd_card_disconnect replaces /dev/hwC1D0's file->f_op
  with a kmalloc()ed version, that would only allow releases.

  us428control starts exiting

  __fput is called with struct file for /dev/hwC1D0.

  snd_card_file_remove() is called, alsa notices struct file
  for /dev/hwC1D0 is about to be closed.
  with patch below, file->f_op would be set NULL now.

  snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
  file->f_ops, those that would only allow releases.

  for reason I would like to know,
  __fput is called again with struct file for /dev/hwC1D0
  from us428control's do_exit().
  __fput see's file->f_op is still set.
  Without patch and under memory pressure, file->f_op can
  point to anything now.


Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>


diff -pur ../alsa/1.0.13/alsa-driver-1.0.13rc3/alsa-kernel/core/init.c rt4-kw/sound/core/init.c
--- ../alsa/1.0.13/alsa-driver-1.0.13rc3/alsa-kernel/core/init.c	2006-09-25 15:33:19.000000000 +0200
+++ rt4-kw/sound/core/init.c	2006-09-28 18:48:15.000000000 +0200
@@ -707,6 +707,8 @@ int snd_card_file_remove(struct snd_card
 	mfile = card->files;
 	while (mfile) {
 		if (mfile->file == file) {
+			fops_put(file->f_op);
+			file->f_op = NULL;
 			if (pfile)
 				pfile->next = mfile->next;
 			else

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Hi

It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
but its not. bug showed again reliably under memory-pressure.

      Karsten

===

Reset file->f_op in snd_card_file_remove(). Take 2


i think what happens here is:

  us428control runs, kernel has allocated a struct file for /dev/hwC1D0.

  usb disconnect

  snd_usb_usx2y calls snd_card_disconnect,
  tells us428control to exit.

  snd_card_disconnect replaces /dev/hwC1D0's file->f_op
  with a kmalloc()ed version, that would only allow releases.

  us428control starts exiting

  __fput is called with struct file for /dev/hwC1D0.

  snd_card_file_remove() is called, alsa notices struct file
  for /dev/hwC1D0 is about to be closed.
  with patch below, file->f_op would be set NULL now.

  snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
  file->f_ops, those that would only allow releases.

  for reason I would like to know,
  __fput is called again with struct file for /dev/hwC1D0
  from us428control's do_exit().
  __fput see's file->f_op is still set.
  Without patch and under memory pressure, file->f_op can
  point to anything now.


Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>


diff -pur ../alsa/1.0.13/alsa-driver-1.0.13rc3/alsa-kernel/core/init.c rt4-kw/sound/core/init.c
--- ../alsa/1.0.13/alsa-driver-1.0.13rc3/alsa-kernel/core/init.c	2006-09-25 15:33:19.000000000 +0200
+++ rt4-kw/sound/core/init.c	2006-09-28 18:48:15.000000000 +0200
@@ -707,6 +707,8 @@ int snd_card_file_remove(struct snd_card
 	mfile = card->files;
 	while (mfile) {
 		if (mfile->file == file) {
+			fops_put(file->f_op);
+			file->f_op = NULL;
 			if (pfile)
 				pfile->next = mfile->next;
 			else

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Thu, 28 Sep 2006 22:28:02 +0200,
Karsten Wiese wrote:
> 
> Hi
> 
> It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> but its not. bug showed again reliably under memory-pressure.
> 
>       Karsten
> 
> ===
> 
> Reset file->f_op in snd_card_file_remove(). Take 2
> 
> 
> i think what happens here is:
> 
>   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> 
>   usb disconnect
> 
>   snd_usb_usx2y calls snd_card_disconnect,
>   tells us428control to exit.
> 
>   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
>   with a kmalloc()ed version, that would only allow releases.
> 
>   us428control starts exiting
> 
>   __fput is called with struct file for /dev/hwC1D0.
> 
>   snd_card_file_remove() is called, alsa notices struct file
>   for /dev/hwC1D0 is about to be closed.
>   with patch below, file->f_op would be set NULL now.
> 
>   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
>   file->f_ops, those that would only allow releases.
> 
>   for reason I would like to know,
>   __fput is called again with struct file for /dev/hwC1D0
>   from us428control's do_exit().
>   __fput see's file->f_op is still set.
>   Without patch and under memory pressure, file->f_op can
>   point to anything now.
> 
> 
> Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>

I guess this bug is fixed by Florin's patch below, juding from your
explanation.  Could you check it?


Takashi

Subject: [PATCH] Dereference after free in snd_hwdep_release()
From: Florin Malita <fmalita@gmail.com>
Date: Thu, 28 Sep 2006 19:45:50 -0400

snd_card_file_remove() may free hw->card so we can't dereference
hw->card->module after that.

Coverity ID 1420.

Signed-off-by: Florin Malita <fmalita@gmail.com>
---

diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c
index 9aa9d94..46b4768 100644
--- a/sound/core/hwdep.c
+++ b/sound/core/hwdep.c
@@ -158,6 +158,7 @@ static int snd_hwdep_release(struct inod
 {
 	int err = -ENXIO;
 	struct snd_hwdep *hw = file->private_data;
+	struct module *mod = hw->card->module;
 	mutex_lock(&hw->open_mutex);
 	if (hw->ops.release) {
 		err = hw->ops.release(hw, file);
@@ -167,7 +168,7 @@ static int snd_hwdep_release(struct inod
 		hw->used--;
 	snd_card_file_remove(hw->card, file);
 	mutex_unlock(&hw->open_mutex);
-	module_put(hw->card->module);
+	module_put(mod);
 	return err;
 }
 

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Thu, 28 Sep 2006 22:28:02 +0200,
Karsten Wiese wrote:
> 
> Hi
> 
> It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> but its not. bug showed again reliably under memory-pressure.
> 
>       Karsten
> 
> ===
> 
> Reset file->f_op in snd_card_file_remove(). Take 2
> 
> 
> i think what happens here is:
> 
>   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> 
>   usb disconnect
> 
>   snd_usb_usx2y calls snd_card_disconnect,
>   tells us428control to exit.
> 
>   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
>   with a kmalloc()ed version, that would only allow releases.
> 
>   us428control starts exiting
> 
>   __fput is called with struct file for /dev/hwC1D0.
> 
>   snd_card_file_remove() is called, alsa notices struct file
>   for /dev/hwC1D0 is about to be closed.
>   with patch below, file->f_op would be set NULL now.
> 
>   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
>   file->f_ops, those that would only allow releases.
> 
>   for reason I would like to know,
>   __fput is called again with struct file for /dev/hwC1D0
>   from us428control's do_exit().
>   __fput see's file->f_op is still set.
>   Without patch and under memory pressure, file->f_op can
>   point to anything now.
> 
> 
> Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>

I guess this bug is fixed by Florin's patch below, juding from your
explanation.  Could you check it?


Takashi

Subject: [PATCH] Dereference after free in snd_hwdep_release()
From: Florin Malita <fmalita@gmail.com>
Date: Thu, 28 Sep 2006 19:45:50 -0400

snd_card_file_remove() may free hw->card so we can't dereference
hw->card->module after that.

Coverity ID 1420.

Signed-off-by: Florin Malita <fmalita@gmail.com>
---

diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c
index 9aa9d94..46b4768 100644
--- a/sound/core/hwdep.c
+++ b/sound/core/hwdep.c
@@ -158,6 +158,7 @@ static int snd_hwdep_release(struct inod
 {
 	int err = -ENXIO;
 	struct snd_hwdep *hw = file->private_data;
+	struct module *mod = hw->card->module;
 	mutex_lock(&hw->open_mutex);
 	if (hw->ops.release) {
 		err = hw->ops.release(hw, file);
@@ -167,7 +168,7 @@ static int snd_hwdep_release(struct inod
 		hw->used--;
 	snd_card_file_remove(hw->card, file);
 	mutex_unlock(&hw->open_mutex);
-	module_put(hw->card->module);
+	module_put(mod);
 	return err;
 }
 

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> At Thu, 28 Sep 2006 22:28:02 +0200,
> Karsten Wiese wrote:
> > 
> > Hi
> > 
> > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > but its not. bug showed again reliably under memory-pressure.
> > 
> >       Karsten
> > 
> > ===
> > 
> > Reset file->f_op in snd_card_file_remove(). Take 2
> > 
> > 
> > i think what happens here is:
> > 
> >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > 
> >   usb disconnect
> > 
> >   snd_usb_usx2y calls snd_card_disconnect,
> >   tells us428control to exit.
> > 
> >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> >   with a kmalloc()ed version, that would only allow releases.
> > 
> >   us428control starts exiting
> > 
> >   __fput is called with struct file for /dev/hwC1D0.
> > 
> >   snd_card_file_remove() is called, alsa notices struct file
> >   for /dev/hwC1D0 is about to be closed.
> >   with patch below, file->f_op would be set NULL now.
> > 
> >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> >   file->f_ops, those that would only allow releases.
> > 
> >   for reason I would like to know,
> >   __fput is called again with struct file for /dev/hwC1D0
> >   from us428control's do_exit().
> >   __fput see's file->f_op is still set.
> >   Without patch and under memory pressure, file->f_op can
> >   point to anything now.
> > 
> > 
> > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> 
> I guess this bug is fixed by Florin's patch below, juding from your
> explanation.  Could you check it?
> 
Florin's patch fixes it.

This one for immediate consumation by mainline, mm, rt,
and the stable teams, hmm?

      Karsten
> 
> Takashi
> 
> Subject: [PATCH] Dereference after free in snd_hwdep_release()
> From: Florin Malita <fmalita@gmail.com>
> Date: Thu, 28 Sep 2006 19:45:50 -0400
> 
> snd_card_file_remove() may free hw->card so we can't dereference
> hw->card->module after that.
> 
> Coverity ID 1420.
> 
> Signed-off-by: Florin Malita <fmalita@gmail.com>
> ---
> 
> diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c
> index 9aa9d94..46b4768 100644
> --- a/sound/core/hwdep.c
> +++ b/sound/core/hwdep.c
> @@ -158,6 +158,7 @@ static int snd_hwdep_release(struct inod
>  {
>  	int err = -ENXIO;
>  	struct snd_hwdep *hw = file->private_data;
> +	struct module *mod = hw->card->module;
>  	mutex_lock(&hw->open_mutex);
>  	if (hw->ops.release) {
>  		err = hw->ops.release(hw, file);
> @@ -167,7 +168,7 @@ static int snd_hwdep_release(struct inod
>  		hw->used--;
>  	snd_card_file_remove(hw->card, file);
>  	mutex_unlock(&hw->open_mutex);
> -	module_put(hw->card->module);
> +	module_put(mod);
>  	return err;
>  }
>  
> 
> 

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> At Thu, 28 Sep 2006 22:28:02 +0200,
> Karsten Wiese wrote:
> > 
> > Hi
> > 
> > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > but its not. bug showed again reliably under memory-pressure.
> > 
> >       Karsten
> > 
> > ===
> > 
> > Reset file->f_op in snd_card_file_remove(). Take 2
> > 
> > 
> > i think what happens here is:
> > 
> >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > 
> >   usb disconnect
> > 
> >   snd_usb_usx2y calls snd_card_disconnect,
> >   tells us428control to exit.
> > 
> >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> >   with a kmalloc()ed version, that would only allow releases.
> > 
> >   us428control starts exiting
> > 
> >   __fput is called with struct file for /dev/hwC1D0.
> > 
> >   snd_card_file_remove() is called, alsa notices struct file
> >   for /dev/hwC1D0 is about to be closed.
> >   with patch below, file->f_op would be set NULL now.
> > 
> >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> >   file->f_ops, those that would only allow releases.
> > 
> >   for reason I would like to know,
> >   __fput is called again with struct file for /dev/hwC1D0
> >   from us428control's do_exit().
> >   __fput see's file->f_op is still set.
> >   Without patch and under memory pressure, file->f_op can
> >   point to anything now.
> > 
> > 
> > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> 
> I guess this bug is fixed by Florin's patch below, juding from your
> explanation.  Could you check it?
> 
Florin's patch fixes it.

This one for immediate consumation by mainline, mm, rt,
and the stable teams, hmm?

      Karsten
> 
> Takashi
> 
> Subject: [PATCH] Dereference after free in snd_hwdep_release()
> From: Florin Malita <fmalita@gmail.com>
> Date: Thu, 28 Sep 2006 19:45:50 -0400
> 
> snd_card_file_remove() may free hw->card so we can't dereference
> hw->card->module after that.
> 
> Coverity ID 1420.
> 
> Signed-off-by: Florin Malita <fmalita@gmail.com>
> ---
> 
> diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c
> index 9aa9d94..46b4768 100644
> --- a/sound/core/hwdep.c
> +++ b/sound/core/hwdep.c
> @@ -158,6 +158,7 @@ static int snd_hwdep_release(struct inod
>  {
>  	int err = -ENXIO;
>  	struct snd_hwdep *hw = file->private_data;
> +	struct module *mod = hw->card->module;
>  	mutex_lock(&hw->open_mutex);
>  	if (hw->ops.release) {
>  		err = hw->ops.release(hw, file);
> @@ -167,7 +168,7 @@ static int snd_hwdep_release(struct inod
>  		hw->used--;
>  	snd_card_file_remove(hw->card, file);
>  	mutex_unlock(&hw->open_mutex);
> -	module_put(hw->card->module);
> +	module_put(mod);
>  	return err;
>  }
>  
> 
> 

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Ingo Molnar]

* Karsten Wiese <annabellesgarden@yahoo.de> wrote:

> Florin's patch fixes it.

great - i've applied Florin's patch to -rt.

	Ingo

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Fri, 29 Sep 2006 14:29:20 +0200,
Karsten Wiese wrote:
> 
> Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> > At Thu, 28 Sep 2006 22:28:02 +0200,
> > Karsten Wiese wrote:
> > > 
> > > Hi
> > > 
> > > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > > but its not. bug showed again reliably under memory-pressure.
> > > 
> > >       Karsten
> > > 
> > > ===
> > > 
> > > Reset file->f_op in snd_card_file_remove(). Take 2
> > > 
> > > 
> > > i think what happens here is:
> > > 
> > >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > > 
> > >   usb disconnect
> > > 
> > >   snd_usb_usx2y calls snd_card_disconnect,
> > >   tells us428control to exit.
> > > 
> > >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> > >   with a kmalloc()ed version, that would only allow releases.
> > > 
> > >   us428control starts exiting
> > > 
> > >   __fput is called with struct file for /dev/hwC1D0.
> > > 
> > >   snd_card_file_remove() is called, alsa notices struct file
> > >   for /dev/hwC1D0 is about to be closed.
> > >   with patch below, file->f_op would be set NULL now.
> > > 
> > >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> > >   file->f_ops, those that would only allow releases.
> > > 
> > >   for reason I would like to know,
> > >   __fput is called again with struct file for /dev/hwC1D0
> > >   from us428control's do_exit().
> > >   __fput see's file->f_op is still set.
> > >   Without patch and under memory pressure, file->f_op can
> > >   point to anything now.
> > > 
> > > 
> > > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> > 
> > I guess this bug is fixed by Florin's patch below, juding from your
> > explanation.  Could you check it?
> > 
> Florin's patch fixes it.
> 
> This one for immediate consumation by mainline, mm, rt,
> and the stable teams, hmm?

It'll be pushed to mainline soon together with other ALSA fixes.  Then
I'll forward to stable, too.


Takashi

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Fri, 29 Sep 2006 14:29:20 +0200,
Karsten Wiese wrote:
> 
> Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> > At Thu, 28 Sep 2006 22:28:02 +0200,
> > Karsten Wiese wrote:
> > > 
> > > Hi
> > > 
> > > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > > but its not. bug showed again reliably under memory-pressure.
> > > 
> > >       Karsten
> > > 
> > > ===
> > > 
> > > Reset file->f_op in snd_card_file_remove(). Take 2
> > > 
> > > 
> > > i think what happens here is:
> > > 
> > >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > > 
> > >   usb disconnect
> > > 
> > >   snd_usb_usx2y calls snd_card_disconnect,
> > >   tells us428control to exit.
> > > 
> > >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> > >   with a kmalloc()ed version, that would only allow releases.
> > > 
> > >   us428control starts exiting
> > > 
> > >   __fput is called with struct file for /dev/hwC1D0.
> > > 
> > >   snd_card_file_remove() is called, alsa notices struct file
> > >   for /dev/hwC1D0 is about to be closed.
> > >   with patch below, file->f_op would be set NULL now.
> > > 
> > >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> > >   file->f_ops, those that would only allow releases.
> > > 
> > >   for reason I would like to know,
> > >   __fput is called again with struct file for /dev/hwC1D0
> > >   from us428control's do_exit().
> > >   __fput see's file->f_op is still set.
> > >   Without patch and under memory pressure, file->f_op can
> > >   point to anything now.
> > > 
> > > 
> > > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> > 
> > I guess this bug is fixed by Florin's patch below, juding from your
> > explanation.  Could you check it?
> > 
> Florin's patch fixes it.
> 
> This one for immediate consumation by mainline, mm, rt,
> and the stable teams, hmm?

It'll be pushed to mainline soon together with other ALSA fixes.  Then
I'll forward to stable, too.


Takashi

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

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

Am Freitag, 29. September 2006 14:45 schrieb Takashi Iwai:
> At Fri, 29 Sep 2006 14:29:20 +0200,
> Karsten Wiese wrote:
> > 
> > Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> > > At Thu, 28 Sep 2006 22:28:02 +0200,
> > > Karsten Wiese wrote:
> > > > 
> > > > Hi
> > > > 
> > > > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > > > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > > > but its not. bug showed again reliably under memory-pressure.
> > > > 
> > > >       Karsten
> > > > 
> > > > ===
> > > > 
> > > > Reset file->f_op in snd_card_file_remove(). Take 2
> > > > 
> > > > 
> > > > i think what happens here is:
> > > > 
> > > >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > > > 
> > > >   usb disconnect
> > > > 
> > > >   snd_usb_usx2y calls snd_card_disconnect,
> > > >   tells us428control to exit.
> > > > 
> > > >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> > > >   with a kmalloc()ed version, that would only allow releases.
> > > > 
> > > >   us428control starts exiting
> > > > 
> > > >   __fput is called with struct file for /dev/hwC1D0.
> > > > 
> > > >   snd_card_file_remove() is called, alsa notices struct file
> > > >   for /dev/hwC1D0 is about to be closed.
> > > >   with patch below, file->f_op would be set NULL now.
> > > > 
> > > >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> > > >   file->f_ops, those that would only allow releases.
> > > > 
> > > >   for reason I would like to know,
> > > >   __fput is called again with struct file for /dev/hwC1D0
> > > >   from us428control's do_exit().
> > > >   __fput see's file->f_op is still set.
> > > >   Without patch and under memory pressure, file->f_op can
> > > >   point to anything now.
> > > > 
> > > > 
> > > > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> > > 
> > > I guess this bug is fixed by Florin's patch below, juding from your
> > > explanation.  Could you check it?
> > > 
> > Florin's patch fixes it.
> > 
> > This one for immediate consumation by mainline, mm, rt,
> > and the stable teams, hmm?
> 
> It'll be pushed to mainline soon together with other ALSA fixes.  Then
> I'll forward to stable, too.
> 
bug is back. Florin's patch _is_ right.
And installed, unless im getting confuzed.
To help me proove,
please consider this controll of flow:

	usb disconnect

	usX2Y_usb_disconnect() calls snd_card_free(card);

	snd_card_free waits:
		wait_event(card->shutdown_sleep, card->files == NULL);

	us428control starts exiting, closes /dev/hwC1D0, calls __fput.
	__fput calls snd_hwdep_release,
	snd_hwdep_release calls snd_card_file_remove.
	snd_card_file_remove sees lastclose is set, does
		wake_up(&card->shutdown_sleep);

lets assume, snd_card_free's thread prio is FIFO, we are on UP
and us428control' prio is not FIFO:

What keeps snd_card_free from waking up now, deleting /dev/hwC1D0's
file->f_op _before_  __fput is rescheduled,
seeing a set but freeed file->f_op?

Attached crash dmesg also has "BUG: time warp detected!", but that BUG
is propably unrelated.


      Karsten






[-- Attachment #2: crash9.bz2 --]
[-- Type: application/x-bzip2, Size: 8395 bytes --]

[-- Attachment #3: Type: text/plain, Size: 348 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

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

Am Freitag, 29. September 2006 14:45 schrieb Takashi Iwai:
> At Fri, 29 Sep 2006 14:29:20 +0200,
> Karsten Wiese wrote:
> > 
> > Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> > > At Thu, 28 Sep 2006 22:28:02 +0200,
> > > Karsten Wiese wrote:
> > > > 
> > > > Hi
> > > > 
> > > > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > > > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > > > but its not. bug showed again reliably under memory-pressure.
> > > > 
> > > >       Karsten
> > > > 
> > > > ===
> > > > 
> > > > Reset file->f_op in snd_card_file_remove(). Take 2
> > > > 
> > > > 
> > > > i think what happens here is:
> > > > 
> > > >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > > > 
> > > >   usb disconnect
> > > > 
> > > >   snd_usb_usx2y calls snd_card_disconnect,
> > > >   tells us428control to exit.
> > > > 
> > > >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> > > >   with a kmalloc()ed version, that would only allow releases.
> > > > 
> > > >   us428control starts exiting
> > > > 
> > > >   __fput is called with struct file for /dev/hwC1D0.
> > > > 
> > > >   snd_card_file_remove() is called, alsa notices struct file
> > > >   for /dev/hwC1D0 is about to be closed.
> > > >   with patch below, file->f_op would be set NULL now.
> > > > 
> > > >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> > > >   file->f_ops, those that would only allow releases.
> > > > 
> > > >   for reason I would like to know,
> > > >   __fput is called again with struct file for /dev/hwC1D0
> > > >   from us428control's do_exit().
> > > >   __fput see's file->f_op is still set.
> > > >   Without patch and under memory pressure, file->f_op can
> > > >   point to anything now.
> > > > 
> > > > 
> > > > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> > > 
> > > I guess this bug is fixed by Florin's patch below, juding from your
> > > explanation.  Could you check it?
> > > 
> > Florin's patch fixes it.
> > 
> > This one for immediate consumation by mainline, mm, rt,
> > and the stable teams, hmm?
> 
> It'll be pushed to mainline soon together with other ALSA fixes.  Then
> I'll forward to stable, too.
> 
bug is back. Florin's patch _is_ right.
And installed, unless im getting confuzed.
To help me proove,
please consider this controll of flow:

	usb disconnect

	usX2Y_usb_disconnect() calls snd_card_free(card);

	snd_card_free waits:
		wait_event(card->shutdown_sleep, card->files == NULL);

	us428control starts exiting, closes /dev/hwC1D0, calls __fput.
	__fput calls snd_hwdep_release,
	snd_hwdep_release calls snd_card_file_remove.
	snd_card_file_remove sees lastclose is set, does
		wake_up(&card->shutdown_sleep);

lets assume, snd_card_free's thread prio is FIFO, we are on UP
and us428control' prio is not FIFO:

What keeps snd_card_free from waking up now, deleting /dev/hwC1D0's
file->f_op _before_  __fput is rescheduled,
seeing a set but freeed file->f_op?

Attached crash dmesg also has "BUG: time warp detected!", but that BUG
is propably unrelated.


      Karsten






[-- Attachment #2: crash9.bz2 --]
[-- Type: application/x-bzip2, Size: 8395 bytes --]

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Sun, 1 Oct 2006 20:29:36 +0200,
Karsten Wiese wrote:
> 
> Am Freitag, 29. September 2006 14:45 schrieb Takashi Iwai:
> > At Fri, 29 Sep 2006 14:29:20 +0200,
> > Karsten Wiese wrote:
> > > 
> > > Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> > > > At Thu, 28 Sep 2006 22:28:02 +0200,
> > > > Karsten Wiese wrote:
> > > > > 
> > > > > Hi
> > > > > 
> > > > > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > > > > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > > > > but its not. bug showed again reliably under memory-pressure.
> > > > > 
> > > > >       Karsten
> > > > > 
> > > > > ===
> > > > > 
> > > > > Reset file->f_op in snd_card_file_remove(). Take 2
> > > > > 
> > > > > 
> > > > > i think what happens here is:
> > > > > 
> > > > >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > > > > 
> > > > >   usb disconnect
> > > > > 
> > > > >   snd_usb_usx2y calls snd_card_disconnect,
> > > > >   tells us428control to exit.
> > > > > 
> > > > >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> > > > >   with a kmalloc()ed version, that would only allow releases.
> > > > > 
> > > > >   us428control starts exiting
> > > > > 
> > > > >   __fput is called with struct file for /dev/hwC1D0.
> > > > > 
> > > > >   snd_card_file_remove() is called, alsa notices struct file
> > > > >   for /dev/hwC1D0 is about to be closed.
> > > > >   with patch below, file->f_op would be set NULL now.
> > > > > 
> > > > >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> > > > >   file->f_ops, those that would only allow releases.
> > > > > 
> > > > >   for reason I would like to know,
> > > > >   __fput is called again with struct file for /dev/hwC1D0
> > > > >   from us428control's do_exit().
> > > > >   __fput see's file->f_op is still set.
> > > > >   Without patch and under memory pressure, file->f_op can
> > > > >   point to anything now.
> > > > > 
> > > > > 
> > > > > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> > > > 
> > > > I guess this bug is fixed by Florin's patch below, juding from your
> > > > explanation.  Could you check it?
> > > > 
> > > Florin's patch fixes it.
> > > 
> > > This one for immediate consumation by mainline, mm, rt,
> > > and the stable teams, hmm?
> > 
> > It'll be pushed to mainline soon together with other ALSA fixes.  Then
> > I'll forward to stable, too.
> > 
> bug is back. Florin's patch _is_ right.
> And installed, unless im getting confuzed.
> To help me proove,
> please consider this controll of flow:
> 
> 	usb disconnect
> 
> 	usX2Y_usb_disconnect() calls snd_card_free(card);

It should call snd_card_free_when_closed() instead.


Takashi

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Sun, 1 Oct 2006 20:29:36 +0200,
Karsten Wiese wrote:
> 
> Am Freitag, 29. September 2006 14:45 schrieb Takashi Iwai:
> > At Fri, 29 Sep 2006 14:29:20 +0200,
> > Karsten Wiese wrote:
> > > 
> > > Am Freitag, 29. September 2006 12:48 schrieb Takashi Iwai:
> > > > At Thu, 28 Sep 2006 22:28:02 +0200,
> > > > Karsten Wiese wrote:
> > > > > 
> > > > > Hi
> > > > > 
> > > > > It oopses with 2.6.18-rt4 + alsa-kernel-1.0.13rc3 now.
> > > > > I wrote before, 2.6.18-rt3 + alsa-driver-1.0.13rc3 would be ok,
> > > > > but its not. bug showed again reliably under memory-pressure.
> > > > > 
> > > > >       Karsten
> > > > > 
> > > > > ===
> > > > > 
> > > > > Reset file->f_op in snd_card_file_remove(). Take 2
> > > > > 
> > > > > 
> > > > > i think what happens here is:
> > > > > 
> > > > >   us428control runs, kernel has allocated a struct file for /dev/hwC1D0.
> > > > > 
> > > > >   usb disconnect
> > > > > 
> > > > >   snd_usb_usx2y calls snd_card_disconnect,
> > > > >   tells us428control to exit.
> > > > > 
> > > > >   snd_card_disconnect replaces /dev/hwC1D0's file->f_op
> > > > >   with a kmalloc()ed version, that would only allow releases.
> > > > > 
> > > > >   us428control starts exiting
> > > > > 
> > > > >   __fput is called with struct file for /dev/hwC1D0.
> > > > > 
> > > > >   snd_card_file_remove() is called, alsa notices struct file
> > > > >   for /dev/hwC1D0 is about to be closed.
> > > > >   with patch below, file->f_op would be set NULL now.
> > > > > 
> > > > >   snd_usb_usx2y's free()s snd_card instance and /dev/hwC1D0's
> > > > >   file->f_ops, those that would only allow releases.
> > > > > 
> > > > >   for reason I would like to know,
> > > > >   __fput is called again with struct file for /dev/hwC1D0
> > > > >   from us428control's do_exit().
> > > > >   __fput see's file->f_op is still set.
> > > > >   Without patch and under memory pressure, file->f_op can
> > > > >   point to anything now.
> > > > > 
> > > > > 
> > > > > Signed-off-by: Karsten Wiese <annabellesgarden@yahoo.de>
> > > > 
> > > > I guess this bug is fixed by Florin's patch below, juding from your
> > > > explanation.  Could you check it?
> > > > 
> > > Florin's patch fixes it.
> > > 
> > > This one for immediate consumation by mainline, mm, rt,
> > > and the stable teams, hmm?
> > 
> > It'll be pushed to mainline soon together with other ALSA fixes.  Then
> > I'll forward to stable, too.
> > 
> bug is back. Florin's patch _is_ right.
> And installed, unless im getting confuzed.
> To help me proove,
> please consider this controll of flow:
> 
> 	usb disconnect
> 
> 	usX2Y_usb_disconnect() calls snd_card_free(card);

It should call snd_card_free_when_closed() instead.


Takashi

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Mittwoch, 4. Oktober 2006 11:22 schrieb Takashi Iwai:
> 
> It should call snd_card_free_when_closed() instead.
> 
IMHO, that would just make sure that the bug happens.
Please see my annotations, starting with // in:

void fastcall __fput(struct file *file)
{
	struct dentry *dentry = file->f_dentry;
	struct vfsmount *mnt = file->f_vfsmnt;
	struct inode *inode = dentry->d_inode;

	might_sleep();

	fsnotify_close(file);
	/*
	 * The function eventpoll_release() should be the first called
	 * in the file cleanup chain.
	 */
	eventpoll_release(file);
	locks_remove_flock(file);

	if (file->f_op && file->f_op->release)
		file->f_op->release(inode, file);
// Here snd_hwdep_release() is called.
// snd_hwdep_release() calls snd_card_file_remove().
// snd_card_file_remove() sees card->free_on_last_close ist set,
// calls snd_card_do_free().
// snd_card_do_free frees file->f_op but doesn't set it NULL.
//
	security_file_free(file);
	if (unlikely(inode->i_cdev != NULL))
		cdev_put(inode->i_cdev);
	fops_put(file->f_op);
// file->f_op has already been freeed!
// fops_put(file->f_op) is likely to oops.
//
	if (file->f_mode & FMODE_WRITE)
		put_write_access(inode);
	file_kill(file);
	file->f_dentry = NULL;
	file->f_vfsmnt = NULL;
	file_free(file);
	dput(dentry);
	mntput(mnt);
}

Right or wrong?

      Karsten

	

	
		
___________________________________________________________ 
Der frühe Vogel fängt den Wurm. Hier gelangen Sie zum neuen Yahoo! Mail: http://mail.yahoo.de


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Wed, 4 Oct 2006 12:47:19 +0200,
Karsten Wiese wrote:
> 
> Am Mittwoch, 4. Oktober 2006 11:22 schrieb Takashi Iwai:
> > 
> > It should call snd_card_free_when_closed() instead.
> > 
> IMHO, that would just make sure that the bug happens.
> Please see my annotations, starting with // in:
> 
> void fastcall __fput(struct file *file)
> {
> 	struct dentry *dentry = file->f_dentry;
> 	struct vfsmount *mnt = file->f_vfsmnt;
> 	struct inode *inode = dentry->d_inode;
> 
> 	might_sleep();
> 
> 	fsnotify_close(file);
> 	/*
> 	 * The function eventpoll_release() should be the first called
> 	 * in the file cleanup chain.
> 	 */
> 	eventpoll_release(file);
> 	locks_remove_flock(file);
> 
> 	if (file->f_op && file->f_op->release)
> 		file->f_op->release(inode, file);
> // Here snd_hwdep_release() is called.
> // snd_hwdep_release() calls snd_card_file_remove().
> // snd_card_file_remove() sees card->free_on_last_close ist set,
> // calls snd_card_do_free().
> // snd_card_do_free frees file->f_op but doesn't set it NULL.
> //
> 	security_file_free(file);
> 	if (unlikely(inode->i_cdev != NULL))
> 		cdev_put(inode->i_cdev);
> 	fops_put(file->f_op);
> // file->f_op has already been freeed!
> // fops_put(file->f_op) is likely to oops.

Yes, this looks like an invalid access.

The problem is that we use kmalloc for allocating a dummy f_op.
IMO, the simlest solution is to use a static dummy f_op.


Takashi

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Mittwoch, 4. Oktober 2006 16:18 schrieb Takashi Iwai:
> At Wed, 4 Oct 2006 12:47:19 +0200,
> Karsten Wiese wrote:
> > 
> > Am Mittwoch, 4. Oktober 2006 11:22 schrieb Takashi Iwai:
> > > 
> > > It should call snd_card_free_when_closed() instead.
> > > 
> > IMHO, that would just make sure that the bug happens.
> > Please see my annotations, starting with // in:
> > 
> > void fastcall __fput(struct file *file)
> > {
> > 	struct dentry *dentry = file->f_dentry;
> > 	struct vfsmount *mnt = file->f_vfsmnt;
> > 	struct inode *inode = dentry->d_inode;
> > 
> > 	might_sleep();
> > 
> > 	fsnotify_close(file);
> > 	/*
> > 	 * The function eventpoll_release() should be the first called
> > 	 * in the file cleanup chain.
> > 	 */
> > 	eventpoll_release(file);
> > 	locks_remove_flock(file);
> > 
> > 	if (file->f_op && file->f_op->release)
> > 		file->f_op->release(inode, file);
> > // Here snd_hwdep_release() is called.
> > // snd_hwdep_release() calls snd_card_file_remove().
> > // snd_card_file_remove() sees card->free_on_last_close ist set,
> > // calls snd_card_do_free().
> > // snd_card_do_free frees file->f_op but doesn't set it NULL.
> > //
> > 	security_file_free(file);
> > 	if (unlikely(inode->i_cdev != NULL))
> > 		cdev_put(inode->i_cdev);
> > 	fops_put(file->f_op);
> > // file->f_op has already been freeed!
> > // fops_put(file->f_op) is likely to oops.
> 
> Yes, this looks like an invalid access.
> 
> The problem is that we use kmalloc for allocating a dummy f_op.
> IMO, the simlest solution is to use a static dummy f_op.
> 
That'd take 1 static dummy f_op per snd_*_release().
I prefer the patch at the start of this thread :-)

      Karsten

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Mittwoch, 4. Oktober 2006 16:18 schrieb Takashi Iwai:
> At Wed, 4 Oct 2006 12:47:19 +0200,
> Karsten Wiese wrote:
> > 
> > Am Mittwoch, 4. Oktober 2006 11:22 schrieb Takashi Iwai:
> > > 
> > > It should call snd_card_free_when_closed() instead.
> > > 
> > IMHO, that would just make sure that the bug happens.
> > Please see my annotations, starting with // in:
> > 
> > void fastcall __fput(struct file *file)
> > {
> > 	struct dentry *dentry = file->f_dentry;
> > 	struct vfsmount *mnt = file->f_vfsmnt;
> > 	struct inode *inode = dentry->d_inode;
> > 
> > 	might_sleep();
> > 
> > 	fsnotify_close(file);
> > 	/*
> > 	 * The function eventpoll_release() should be the first called
> > 	 * in the file cleanup chain.
> > 	 */
> > 	eventpoll_release(file);
> > 	locks_remove_flock(file);
> > 
> > 	if (file->f_op && file->f_op->release)
> > 		file->f_op->release(inode, file);
> > // Here snd_hwdep_release() is called.
> > // snd_hwdep_release() calls snd_card_file_remove().
> > // snd_card_file_remove() sees card->free_on_last_close ist set,
> > // calls snd_card_do_free().
> > // snd_card_do_free frees file->f_op but doesn't set it NULL.
> > //
> > 	security_file_free(file);
> > 	if (unlikely(inode->i_cdev != NULL))
> > 		cdev_put(inode->i_cdev);
> > 	fops_put(file->f_op);
> > // file->f_op has already been freeed!
> > // fops_put(file->f_op) is likely to oops.
> 
> Yes, this looks like an invalid access.
> 
> The problem is that we use kmalloc for allocating a dummy f_op.
> IMO, the simlest solution is to use a static dummy f_op.
> 
That'd take 1 static dummy f_op per snd_*_release().
I prefer the patch at the start of this thread :-)

      Karsten

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Wed, 4 Oct 2006 17:36:37 +0200,
Karsten Wiese wrote:
> 
> Am Mittwoch, 4. Oktober 2006 16:18 schrieb Takashi Iwai:
> > At Wed, 4 Oct 2006 12:47:19 +0200,
> > Karsten Wiese wrote:
> > > 
> > > Am Mittwoch, 4. Oktober 2006 11:22 schrieb Takashi Iwai:
> > > > 
> > > > It should call snd_card_free_when_closed() instead.
> > > > 
> > > IMHO, that would just make sure that the bug happens.
> > > Please see my annotations, starting with // in:
> > > 
> > > void fastcall __fput(struct file *file)
> > > {
> > > 	struct dentry *dentry = file->f_dentry;
> > > 	struct vfsmount *mnt = file->f_vfsmnt;
> > > 	struct inode *inode = dentry->d_inode;
> > > 
> > > 	might_sleep();
> > > 
> > > 	fsnotify_close(file);
> > > 	/*
> > > 	 * The function eventpoll_release() should be the first called
> > > 	 * in the file cleanup chain.
> > > 	 */
> > > 	eventpoll_release(file);
> > > 	locks_remove_flock(file);
> > > 
> > > 	if (file->f_op && file->f_op->release)
> > > 		file->f_op->release(inode, file);
> > > // Here snd_hwdep_release() is called.
> > > // snd_hwdep_release() calls snd_card_file_remove().
> > > // snd_card_file_remove() sees card->free_on_last_close ist set,
> > > // calls snd_card_do_free().
> > > // snd_card_do_free frees file->f_op but doesn't set it NULL.
> > > //
> > > 	security_file_free(file);
> > > 	if (unlikely(inode->i_cdev != NULL))
> > > 		cdev_put(inode->i_cdev);
> > > 	fops_put(file->f_op);
> > > // file->f_op has already been freeed!
> > > // fops_put(file->f_op) is likely to oops.
> > 
> > Yes, this looks like an invalid access.
> > 
> > The problem is that we use kmalloc for allocating a dummy f_op.
> > IMO, the simlest solution is to use a static dummy f_op.
> > 
> That'd take 1 static dummy f_op per snd_*_release().

Yes, but it'll remove extra codes at the same time, too.
Well, OTOH, it requires more additions for assignment of dummy ops...

> I prefer the patch at the start of this thread :-)

I think it's not good to set NULL always there.  The NULL is necessary
only when the card is freed.  So, I prefer the patch like below.  Is
it OK?


Takashi

diff -r f38b12373137 sound/core/init.c
--- a/sound/core/init.c	Wed Oct 04 17:17:32 2006 +0200
+++ b/sound/core/init.c	Wed Oct 04 17:50:11 2006 +0200
@@ -721,8 +721,14 @@ int snd_card_file_remove(struct snd_card
 	spin_unlock(&card->files_lock);
 	if (last_close) {
 		wake_up(&card->shutdown_sleep);
-		if (card->free_on_last_close)
+		if (card->free_on_last_close) {
+			/* release and clear f_op here since the dummy f_ops will
+			 * be freed in snd_card_do_free().
+			 */
+			fops_put(file->f_op);
+			file->f_op = NULL;
 			snd_card_do_free(card);
+		}
 	}
 	if (!mfile) {
 		snd_printk(KERN_ERR "ALSA card file remove problem (%p)\n", file);

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Wed, 4 Oct 2006 17:36:37 +0200,
Karsten Wiese wrote:
> 
> Am Mittwoch, 4. Oktober 2006 16:18 schrieb Takashi Iwai:
> > At Wed, 4 Oct 2006 12:47:19 +0200,
> > Karsten Wiese wrote:
> > > 
> > > Am Mittwoch, 4. Oktober 2006 11:22 schrieb Takashi Iwai:
> > > > 
> > > > It should call snd_card_free_when_closed() instead.
> > > > 
> > > IMHO, that would just make sure that the bug happens.
> > > Please see my annotations, starting with // in:
> > > 
> > > void fastcall __fput(struct file *file)
> > > {
> > > 	struct dentry *dentry = file->f_dentry;
> > > 	struct vfsmount *mnt = file->f_vfsmnt;
> > > 	struct inode *inode = dentry->d_inode;
> > > 
> > > 	might_sleep();
> > > 
> > > 	fsnotify_close(file);
> > > 	/*
> > > 	 * The function eventpoll_release() should be the first called
> > > 	 * in the file cleanup chain.
> > > 	 */
> > > 	eventpoll_release(file);
> > > 	locks_remove_flock(file);
> > > 
> > > 	if (file->f_op && file->f_op->release)
> > > 		file->f_op->release(inode, file);
> > > // Here snd_hwdep_release() is called.
> > > // snd_hwdep_release() calls snd_card_file_remove().
> > > // snd_card_file_remove() sees card->free_on_last_close ist set,
> > > // calls snd_card_do_free().
> > > // snd_card_do_free frees file->f_op but doesn't set it NULL.
> > > //
> > > 	security_file_free(file);
> > > 	if (unlikely(inode->i_cdev != NULL))
> > > 		cdev_put(inode->i_cdev);
> > > 	fops_put(file->f_op);
> > > // file->f_op has already been freeed!
> > > // fops_put(file->f_op) is likely to oops.
> > 
> > Yes, this looks like an invalid access.
> > 
> > The problem is that we use kmalloc for allocating a dummy f_op.
> > IMO, the simlest solution is to use a static dummy f_op.
> > 
> That'd take 1 static dummy f_op per snd_*_release().

Yes, but it'll remove extra codes at the same time, too.
Well, OTOH, it requires more additions for assignment of dummy ops...

> I prefer the patch at the start of this thread :-)

I think it's not good to set NULL always there.  The NULL is necessary
only when the card is freed.  So, I prefer the patch like below.  Is
it OK?


Takashi

diff -r f38b12373137 sound/core/init.c
--- a/sound/core/init.c	Wed Oct 04 17:17:32 2006 +0200
+++ b/sound/core/init.c	Wed Oct 04 17:50:11 2006 +0200
@@ -721,8 +721,14 @@ int snd_card_file_remove(struct snd_card
 	spin_unlock(&card->files_lock);
 	if (last_close) {
 		wake_up(&card->shutdown_sleep);
-		if (card->free_on_last_close)
+		if (card->free_on_last_close) {
+			/* release and clear f_op here since the dummy f_ops will
+			 * be freed in snd_card_do_free().
+			 */
+			fops_put(file->f_op);
+			file->f_op = NULL;
 			snd_card_do_free(card);
+		}
 	}
 	if (!mfile) {
 		snd_printk(KERN_ERR "ALSA card file remove problem (%p)\n", file);

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Mittwoch, 4. Oktober 2006 17:57 schrieb Takashi Iwai:
> > > 
> > > The problem is that we use kmalloc for allocating a dummy f_op.
> > > IMO, the simlest solution is to use a static dummy f_op.
> > > 
> > That'd take 1 static dummy f_op per snd_*_release().
> 
> Yes, but it'll remove extra codes at the same time, too.
> Well, OTOH, it requires more additions for assignment of dummy ops...
> 
> > I prefer the patch at the start of this thread :-)
> 
> I think it's not good to set NULL always there.  The NULL is necessary

I disagree. In snd_card_file_remove() the file has already been
released. Is file->f_op still used anywhere later on
except from __fput()'s call to fops_put(file->f_op); ?
First glance didn't show.... 

> only when the card is freed.  So, I prefer the patch like below.  Is
> it OK?
> 
IMO not:
Lets assume 2 cpus CA, CB and two processes PA, PB,
closing their file's after usb disconnect of 1 snd_card.
PA running on CA going through snd_card_file_remove() first would not
have it's file->f_op set NULL.
Now PA is back in __fput() right after the
	file->f_op->release(inode, file);
Some third process happens to be scheduled an CA.
Meanwhile PB running on CB goes through snd_card_file_remove()
and calls snd_card_do_free(card).
snd_card_do_free(card) frees PA's file->f_op.
PA is scheduled again and has a freed, non NULL file->f_op.
> 
> Takashi
> 
> diff -r f38b12373137 sound/core/init.c
> --- a/sound/core/init.c	Wed Oct 04 17:17:32 2006 +0200
> +++ b/sound/core/init.c	Wed Oct 04 17:50:11 2006 +0200
> @@ -721,8 +721,14 @@ int snd_card_file_remove(struct snd_card
>  	spin_unlock(&card->files_lock);
>  	if (last_close) {
>  		wake_up(&card->shutdown_sleep);
> -		if (card->free_on_last_close)
> +		if (card->free_on_last_close) {
> +			/* release and clear f_op here since the dummy f_ops will
> +			 * be freed in snd_card_do_free().
> +			 */
> +			fops_put(file->f_op);
> +			file->f_op = NULL;
>  			snd_card_do_free(card);
> +		}
>  	}
>  	if (!mfile) {
>  		snd_printk(KERN_ERR "ALSA card file remove problem (%p)\n", file);
> 

How about a "disconnecting device" that can emulate a real one
for diconnect reasons?
I've sketched one here:

-------------------------------------------------------------------
/* virtual device
   hides a real device's f_ops,
   exept for release
*/

struct snd_disconnected_file {
	struct file *file;
	int (*release) (struct inode *, struct file *);
	struct snd_disconnected_file *next;
};

static struct snd_disconnected_file *disconnecting_files;
static struct file_operations snd_disconnect_f_ops;

int snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
{
	int err;
// TODO:	zmalloc and initialize struct snd_disconnected_file,
//		rechain
	return err;

	file->f_op = snd_disconnect_f_ops;
	return 0;
}

static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
				   size_t count, loff_t *offset)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
				    size_t count, loff_t *offset)
{
	return -ENODEV;
}

static int snd_disconnect_release(struct inode *inode, struct file *file)
{
	struct snd_disconnected_file * df = disconnecting_files;
	int err = 0;
	while (df)
		if (df->file == file) {
			err = df->release(inode, file);
// TODO: free df, rechain
		}
	return err;
}

static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
{
	return POLLERR | POLLNVAL;
}

static long snd_disconnect_ioctl(struct file *file,
				 unsigned int cmd, unsigned long arg)
{
	return -ENODEV;
}

static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
{
	return -ENODEV;
}

static int snd_disconnect_fasync(int fd, struct file *file, int on)
{
	return -ENODEV;
}

static struct file_operations snd_disconnect_f_ops =
{
	.owner = 	THIS_MODULE,
	.llseek =	snd_disconnect_llseek,
	.read = 	snd_disconnect_read,
	.write =	snd_disconnect_write,
	.release =	snd_disconnect_release,
	.poll =		snd_disconnect_poll,
	.ioctl =	snd_disconnect_ioctl,
	.mmap =		snd_disconnect_mmap,
	.fasync =	snd_disconnect_fasync
};

----------------------------------------

snd_card_disconnect() would call 
  int snd_disconnect_file(file,	file->f_op->release)
instead of allocating/initing the special f_ops by itself.
We'd win memory by the difference
 sizeof(struct snd_shutdown_f_ops) - sizeof(struct snd_disconnected_file)
.
And play safe.
We would need to be sure, a file->f_op's
	int (*release) (struct inode *, struct file *);
id called exactly one time during a file's life though.

      Karsten

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Mittwoch, 4. Oktober 2006 17:57 schrieb Takashi Iwai:
> > > 
> > > The problem is that we use kmalloc for allocating a dummy f_op.
> > > IMO, the simlest solution is to use a static dummy f_op.
> > > 
> > That'd take 1 static dummy f_op per snd_*_release().
> 
> Yes, but it'll remove extra codes at the same time, too.
> Well, OTOH, it requires more additions for assignment of dummy ops...
> 
> > I prefer the patch at the start of this thread :-)
> 
> I think it's not good to set NULL always there.  The NULL is necessary

I disagree. In snd_card_file_remove() the file has already been
released. Is file->f_op still used anywhere later on
except from __fput()'s call to fops_put(file->f_op); ?
First glance didn't show.... 

> only when the card is freed.  So, I prefer the patch like below.  Is
> it OK?
> 
IMO not:
Lets assume 2 cpus CA, CB and two processes PA, PB,
closing their file's after usb disconnect of 1 snd_card.
PA running on CA going through snd_card_file_remove() first would not
have it's file->f_op set NULL.
Now PA is back in __fput() right after the
	file->f_op->release(inode, file);
Some third process happens to be scheduled an CA.
Meanwhile PB running on CB goes through snd_card_file_remove()
and calls snd_card_do_free(card).
snd_card_do_free(card) frees PA's file->f_op.
PA is scheduled again and has a freed, non NULL file->f_op.
> 
> Takashi
> 
> diff -r f38b12373137 sound/core/init.c
> --- a/sound/core/init.c	Wed Oct 04 17:17:32 2006 +0200
> +++ b/sound/core/init.c	Wed Oct 04 17:50:11 2006 +0200
> @@ -721,8 +721,14 @@ int snd_card_file_remove(struct snd_card
>  	spin_unlock(&card->files_lock);
>  	if (last_close) {
>  		wake_up(&card->shutdown_sleep);
> -		if (card->free_on_last_close)
> +		if (card->free_on_last_close) {
> +			/* release and clear f_op here since the dummy f_ops will
> +			 * be freed in snd_card_do_free().
> +			 */
> +			fops_put(file->f_op);
> +			file->f_op = NULL;
>  			snd_card_do_free(card);
> +		}
>  	}
>  	if (!mfile) {
>  		snd_printk(KERN_ERR "ALSA card file remove problem (%p)\n", file);
> 

How about a "disconnecting device" that can emulate a real one
for diconnect reasons?
I've sketched one here:

-------------------------------------------------------------------
/* virtual device
   hides a real device's f_ops,
   exept for release
*/

struct snd_disconnected_file {
	struct file *file;
	int (*release) (struct inode *, struct file *);
	struct snd_disconnected_file *next;
};

static struct snd_disconnected_file *disconnecting_files;
static struct file_operations snd_disconnect_f_ops;

int snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
{
	int err;
// TODO:	zmalloc and initialize struct snd_disconnected_file,
//		rechain
	return err;

	file->f_op = snd_disconnect_f_ops;
	return 0;
}

static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
				   size_t count, loff_t *offset)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
				    size_t count, loff_t *offset)
{
	return -ENODEV;
}

static int snd_disconnect_release(struct inode *inode, struct file *file)
{
	struct snd_disconnected_file * df = disconnecting_files;
	int err = 0;
	while (df)
		if (df->file == file) {
			err = df->release(inode, file);
// TODO: free df, rechain
		}
	return err;
}

static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
{
	return POLLERR | POLLNVAL;
}

static long snd_disconnect_ioctl(struct file *file,
				 unsigned int cmd, unsigned long arg)
{
	return -ENODEV;
}

static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
{
	return -ENODEV;
}

static int snd_disconnect_fasync(int fd, struct file *file, int on)
{
	return -ENODEV;
}

static struct file_operations snd_disconnect_f_ops =
{
	.owner = 	THIS_MODULE,
	.llseek =	snd_disconnect_llseek,
	.read = 	snd_disconnect_read,
	.write =	snd_disconnect_write,
	.release =	snd_disconnect_release,
	.poll =		snd_disconnect_poll,
	.ioctl =	snd_disconnect_ioctl,
	.mmap =		snd_disconnect_mmap,
	.fasync =	snd_disconnect_fasync
};

----------------------------------------

snd_card_disconnect() would call 
  int snd_disconnect_file(file,	file->f_op->release)
instead of allocating/initing the special f_ops by itself.
We'd win memory by the difference
 sizeof(struct snd_shutdown_f_ops) - sizeof(struct snd_disconnected_file)
.
And play safe.
We would need to be sure, a file->f_op's
	int (*release) (struct inode *, struct file *);
id called exactly one time during a file's life though.

      Karsten

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Wed, 4 Oct 2006 22:01:53 +0200,
Karsten Wiese wrote:
> 
> Am Mittwoch, 4. Oktober 2006 17:57 schrieb Takashi Iwai:
> > > > 
> > > > The problem is that we use kmalloc for allocating a dummy f_op.
> > > > IMO, the simlest solution is to use a static dummy f_op.
> > > > 
> > > That'd take 1 static dummy f_op per snd_*_release().
> > 
> > Yes, but it'll remove extra codes at the same time, too.
> > Well, OTOH, it requires more additions for assignment of dummy ops...
> > 
> > > I prefer the patch at the start of this thread :-)
> > 
> > I think it's not good to set NULL always there.  The NULL is necessary
> 
> I disagree. In snd_card_file_remove() the file has already been
> released. Is file->f_op still used anywhere later on
> except from __fput()'s call to fops_put(file->f_op); ?
> First glance didn't show.... 

But who knows that is so in near future, too?

> > only when the card is freed.  So, I prefer the patch like below.  Is
> > it OK?
> > 
> IMO not:
> Lets assume 2 cpus CA, CB and two processes PA, PB,
> closing their file's after usb disconnect of 1 snd_card.
> PA running on CA going through snd_card_file_remove() first would not
> have it's file->f_op set NULL.
> Now PA is back in __fput() right after the
> 	file->f_op->release(inode, file);
> Some third process happens to be scheduled an CA.
> Meanwhile PB running on CB goes through snd_card_file_remove()
> and calls snd_card_do_free(card).
> snd_card_do_free(card) frees PA's file->f_op.
> PA is scheduled again and has a freed, non NULL file->f_op.

Hmmm, right, this is racy.  Let's forget.

> How about a "disconnecting device" that can emulate a real one
> for diconnect reasons?
> I've sketched one here:
> 
> -------------------------------------------------------------------
> /* virtual device
>    hides a real device's f_ops,
>    exept for release
> */
> 
> struct snd_disconnected_file {
> 	struct file *file;
> 	int (*release) (struct inode *, struct file *);
> 	struct snd_disconnected_file *next;
> };
> 
> static struct snd_disconnected_file *disconnecting_files;
> static struct file_operations snd_disconnect_f_ops;
> 
> int snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
> {
> 	int err;
> // TODO:	zmalloc and initialize struct snd_disconnected_file,
> //		rechain
> 	return err;
> 
> 	file->f_op = snd_disconnect_f_ops;
> 	return 0;
> }
> 
> static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
> {
> 	return -ENODEV;
> }
> 
> static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
> 				   size_t count, loff_t *offset)
> {
> 	return -ENODEV;
> }
> 
> static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
> 				    size_t count, loff_t *offset)
> {
> 	return -ENODEV;
> }
> 
> static int snd_disconnect_release(struct inode *inode, struct file *file)
> {
> 	struct snd_disconnected_file * df = disconnecting_files;
> 	int err = 0;
> 	while (df)
> 		if (df->file == file) {
> 			err = df->release(inode, file);
> // TODO: free df, rechain
> 		}
> 	return err;
> }
> 
> static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
> {
> 	return POLLERR | POLLNVAL;
> }
> 
> static long snd_disconnect_ioctl(struct file *file,
> 				 unsigned int cmd, unsigned long arg)
> {
> 	return -ENODEV;
> }
> 
> static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
> {
> 	return -ENODEV;
> }
> 
> static int snd_disconnect_fasync(int fd, struct file *file, int on)
> {
> 	return -ENODEV;
> }
> 
> static struct file_operations snd_disconnect_f_ops =
> {
> 	.owner = 	THIS_MODULE,
> 	.llseek =	snd_disconnect_llseek,
> 	.read = 	snd_disconnect_read,
> 	.write =	snd_disconnect_write,
> 	.release =	snd_disconnect_release,
> 	.poll =		snd_disconnect_poll,
> 	.ioctl =	snd_disconnect_ioctl,
> 	.mmap =		snd_disconnect_mmap,
> 	.fasync =	snd_disconnect_fasync
> };
> 
> ----------------------------------------
> 
> snd_card_disconnect() would call 
>   int snd_disconnect_file(file,	file->f_op->release)
> instead of allocating/initing the special f_ops by itself.
> We'd win memory by the difference
>  sizeof(struct snd_shutdown_f_ops) - sizeof(struct snd_disconnected_file)
> .
> And play safe.

This looks like a good optoin.  But one thing we have to be careful
about is the module counter since the owner is different between the
old f_op and disconnect_f_op...


Takashi

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Wed, 4 Oct 2006 22:01:53 +0200,
Karsten Wiese wrote:
> 
> Am Mittwoch, 4. Oktober 2006 17:57 schrieb Takashi Iwai:
> > > > 
> > > > The problem is that we use kmalloc for allocating a dummy f_op.
> > > > IMO, the simlest solution is to use a static dummy f_op.
> > > > 
> > > That'd take 1 static dummy f_op per snd_*_release().
> > 
> > Yes, but it'll remove extra codes at the same time, too.
> > Well, OTOH, it requires more additions for assignment of dummy ops...
> > 
> > > I prefer the patch at the start of this thread :-)
> > 
> > I think it's not good to set NULL always there.  The NULL is necessary
> 
> I disagree. In snd_card_file_remove() the file has already been
> released. Is file->f_op still used anywhere later on
> except from __fput()'s call to fops_put(file->f_op); ?
> First glance didn't show.... 

But who knows that is so in near future, too?

> > only when the card is freed.  So, I prefer the patch like below.  Is
> > it OK?
> > 
> IMO not:
> Lets assume 2 cpus CA, CB and two processes PA, PB,
> closing their file's after usb disconnect of 1 snd_card.
> PA running on CA going through snd_card_file_remove() first would not
> have it's file->f_op set NULL.
> Now PA is back in __fput() right after the
> 	file->f_op->release(inode, file);
> Some third process happens to be scheduled an CA.
> Meanwhile PB running on CB goes through snd_card_file_remove()
> and calls snd_card_do_free(card).
> snd_card_do_free(card) frees PA's file->f_op.
> PA is scheduled again and has a freed, non NULL file->f_op.

Hmmm, right, this is racy.  Let's forget.

> How about a "disconnecting device" that can emulate a real one
> for diconnect reasons?
> I've sketched one here:
> 
> -------------------------------------------------------------------
> /* virtual device
>    hides a real device's f_ops,
>    exept for release
> */
> 
> struct snd_disconnected_file {
> 	struct file *file;
> 	int (*release) (struct inode *, struct file *);
> 	struct snd_disconnected_file *next;
> };
> 
> static struct snd_disconnected_file *disconnecting_files;
> static struct file_operations snd_disconnect_f_ops;
> 
> int snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
> {
> 	int err;
> // TODO:	zmalloc and initialize struct snd_disconnected_file,
> //		rechain
> 	return err;
> 
> 	file->f_op = snd_disconnect_f_ops;
> 	return 0;
> }
> 
> static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
> {
> 	return -ENODEV;
> }
> 
> static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
> 				   size_t count, loff_t *offset)
> {
> 	return -ENODEV;
> }
> 
> static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
> 				    size_t count, loff_t *offset)
> {
> 	return -ENODEV;
> }
> 
> static int snd_disconnect_release(struct inode *inode, struct file *file)
> {
> 	struct snd_disconnected_file * df = disconnecting_files;
> 	int err = 0;
> 	while (df)
> 		if (df->file == file) {
> 			err = df->release(inode, file);
> // TODO: free df, rechain
> 		}
> 	return err;
> }
> 
> static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
> {
> 	return POLLERR | POLLNVAL;
> }
> 
> static long snd_disconnect_ioctl(struct file *file,
> 				 unsigned int cmd, unsigned long arg)
> {
> 	return -ENODEV;
> }
> 
> static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
> {
> 	return -ENODEV;
> }
> 
> static int snd_disconnect_fasync(int fd, struct file *file, int on)
> {
> 	return -ENODEV;
> }
> 
> static struct file_operations snd_disconnect_f_ops =
> {
> 	.owner = 	THIS_MODULE,
> 	.llseek =	snd_disconnect_llseek,
> 	.read = 	snd_disconnect_read,
> 	.write =	snd_disconnect_write,
> 	.release =	snd_disconnect_release,
> 	.poll =		snd_disconnect_poll,
> 	.ioctl =	snd_disconnect_ioctl,
> 	.mmap =		snd_disconnect_mmap,
> 	.fasync =	snd_disconnect_fasync
> };
> 
> ----------------------------------------
> 
> snd_card_disconnect() would call 
>   int snd_disconnect_file(file,	file->f_op->release)
> instead of allocating/initing the special f_ops by itself.
> We'd win memory by the difference
>  sizeof(struct snd_shutdown_f_ops) - sizeof(struct snd_disconnected_file)
> .
> And play safe.

This looks like a good optoin.  But one thing we have to be careful
about is the module counter since the owner is different between the
old f_op and disconnect_f_op...


Takashi

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Mittwoch, 4. Oktober 2006 22:15 schrieb Takashi Iwai:
> 
> This looks like a good optoin.  But one thing we have to be careful
> about is the module counter since the owner is different between the
> old f_op and disconnect_f_op...
> 
here is rc1, will test later.
Feel free to pick it apart ;-)

-----------------------------------------------
/* virtual device
   hides a real device's f_ops,
   except for release

 *  Copyright (c) by Karsten Wiese <fzu@wemgehoertderstaat.de>
 *
 *
 *   This program is free software; you can redistribute it and/or modify
 *   it under the terms of the GNU General Public License as published by
 *   the Free Software Foundation; either version 2 of the License, or
 *   (at your option) any later version.
 *
 *   This program is distributed in the hope that it will be useful,
 *   but WITHOUT ANY WARRANTY; without even the implied warranty of
 *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *   GNU General Public License for more details.
 *
 *   You should have received a copy of the GNU General Public License
 *   along with this program; if not, write to the Free Software
 *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
 *
 */


#include <linux/fs.h>
#include <linux/poll.h>
#include <linux/module.h>

struct snd_disconnected_file {
	struct file *file;
	int (*release) (struct inode *, struct file *);
	struct snd_disconnected_file *next;
};

static struct snd_disconnected_file *disconnecting_files;
static struct file_operations snd_disconnect_f_ops;
static DEFINE_MUTEX(mutex);

void snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
{
	struct snd_disconnected_file *df, **_dfs;
	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
	if (df == NULL)
		panic("Atomic allocation failed for snd_disconnected_file!");

	df->file = file;
	df->release = release;
	df->next = NULL;

	mutex_lock(&mutex);
	_dfs = &disconnecting_files;
	while (*_dfs != NULL)
		_dfs = &(*_dfs)->next;
	*_dfs = df;
	mutex_unlock(&mutex);

	{
		const struct file_operations *old_f_op = file->f_op;
		fops_get(&snd_disconnect_f_ops);
		file->f_op = &snd_disconnect_f_ops;
		fops_put(old_f_op);
	}
}
EXPORT_SYMBOL(snd_disconnect_file);

static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
				   size_t count, loff_t *offset)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
				    size_t count, loff_t *offset)
{
	return -ENODEV;
}

static int snd_disconnect_release(struct inode *inode, struct file *file)
{
	struct snd_disconnected_file *df, **_dfs, **__dfs;
	int err = 0;
	__dfs = _dfs = &disconnecting_files;

	mutex_lock(&mutex);
	while ((df = *_dfs))
		if (df->file == file) {
			*__dfs = df->next;
			break;
		} else {
			__dfs = _dfs;
			_dfs = &df->next;
		}
	mutex_unlock(&mutex);

	if (df != NULL)	{
		err = df->release(inode, file);
		kfree(df);
		return err;
	}

	panic("%s(%p, %p) failed!", __FUNCTION__, inode, file);
}

static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
{
	return POLLERR | POLLNVAL;
}

static long snd_disconnect_ioctl(struct file *file,
				 unsigned int cmd, unsigned long arg)
{
	return -ENODEV;
}

static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
{
	return -ENODEV;
}

static int snd_disconnect_fasync(int fd, struct file *file, int on)
{
	return -ENODEV;
}

static struct file_operations snd_disconnect_f_ops =
{
	.owner = 	THIS_MODULE,
	.llseek =	snd_disconnect_llseek,
	.read = 	snd_disconnect_read,
	.write =	snd_disconnect_write,
	.release =	snd_disconnect_release,
	.poll =		snd_disconnect_poll,
	.unlocked_ioctl = snd_disconnect_ioctl,
	.compat_ioctl = snd_disconnect_ioctl,
	.mmap =		snd_disconnect_mmap,
	.fasync =	snd_disconnect_fasync
};
--------------------------------------------------------

      Karsten

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Mittwoch, 4. Oktober 2006 22:15 schrieb Takashi Iwai:
> 
> This looks like a good optoin.  But one thing we have to be careful
> about is the module counter since the owner is different between the
> old f_op and disconnect_f_op...
> 
here is rc1, will test later.
Feel free to pick it apart ;-)

-----------------------------------------------
/* virtual device
   hides a real device's f_ops,
   except for release

 *  Copyright (c) by Karsten Wiese <fzu@wemgehoertderstaat.de>
 *
 *
 *   This program is free software; you can redistribute it and/or modify
 *   it under the terms of the GNU General Public License as published by
 *   the Free Software Foundation; either version 2 of the License, or
 *   (at your option) any later version.
 *
 *   This program is distributed in the hope that it will be useful,
 *   but WITHOUT ANY WARRANTY; without even the implied warranty of
 *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *   GNU General Public License for more details.
 *
 *   You should have received a copy of the GNU General Public License
 *   along with this program; if not, write to the Free Software
 *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
 *
 */


#include <linux/fs.h>
#include <linux/poll.h>
#include <linux/module.h>

struct snd_disconnected_file {
	struct file *file;
	int (*release) (struct inode *, struct file *);
	struct snd_disconnected_file *next;
};

static struct snd_disconnected_file *disconnecting_files;
static struct file_operations snd_disconnect_f_ops;
static DEFINE_MUTEX(mutex);

void snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
{
	struct snd_disconnected_file *df, **_dfs;
	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
	if (df == NULL)
		panic("Atomic allocation failed for snd_disconnected_file!");

	df->file = file;
	df->release = release;
	df->next = NULL;

	mutex_lock(&mutex);
	_dfs = &disconnecting_files;
	while (*_dfs != NULL)
		_dfs = &(*_dfs)->next;
	*_dfs = df;
	mutex_unlock(&mutex);

	{
		const struct file_operations *old_f_op = file->f_op;
		fops_get(&snd_disconnect_f_ops);
		file->f_op = &snd_disconnect_f_ops;
		fops_put(old_f_op);
	}
}
EXPORT_SYMBOL(snd_disconnect_file);

static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
				   size_t count, loff_t *offset)
{
	return -ENODEV;
}

static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
				    size_t count, loff_t *offset)
{
	return -ENODEV;
}

static int snd_disconnect_release(struct inode *inode, struct file *file)
{
	struct snd_disconnected_file *df, **_dfs, **__dfs;
	int err = 0;
	__dfs = _dfs = &disconnecting_files;

	mutex_lock(&mutex);
	while ((df = *_dfs))
		if (df->file == file) {
			*__dfs = df->next;
			break;
		} else {
			__dfs = _dfs;
			_dfs = &df->next;
		}
	mutex_unlock(&mutex);

	if (df != NULL)	{
		err = df->release(inode, file);
		kfree(df);
		return err;
	}

	panic("%s(%p, %p) failed!", __FUNCTION__, inode, file);
}

static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
{
	return POLLERR | POLLNVAL;
}

static long snd_disconnect_ioctl(struct file *file,
				 unsigned int cmd, unsigned long arg)
{
	return -ENODEV;
}

static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
{
	return -ENODEV;
}

static int snd_disconnect_fasync(int fd, struct file *file, int on)
{
	return -ENODEV;
}

static struct file_operations snd_disconnect_f_ops =
{
	.owner = 	THIS_MODULE,
	.llseek =	snd_disconnect_llseek,
	.read = 	snd_disconnect_read,
	.write =	snd_disconnect_write,
	.release =	snd_disconnect_release,
	.poll =		snd_disconnect_poll,
	.unlocked_ioctl = snd_disconnect_ioctl,
	.compat_ioctl = snd_disconnect_ioctl,
	.mmap =		snd_disconnect_mmap,
	.fasync =	snd_disconnect_fasync
};
--------------------------------------------------------

      Karsten

Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Thu, 5 Oct 2006 01:41:47 +0200,
Karsten Wiese wrote:
> 
> Am Mittwoch, 4. Oktober 2006 22:15 schrieb Takashi Iwai:
> > 
> > This looks like a good optoin.  But one thing we have to be careful
> > about is the module counter since the owner is different between the
> > old f_op and disconnect_f_op...
> > 
> here is rc1, will test later.
> Feel free to pick it apart ;-)

Any special reason to make it separate instead of patching init.c?
Most of codes (e.g. dummy callbacks) are already in init.c.

> struct snd_disconnected_file {
> 	struct file *file;
> 	int (*release) (struct inode *, struct file *);
> 	struct snd_disconnected_file *next;

We can use a standard list here.

> };
> 
> static struct snd_disconnected_file *disconnecting_files;
> static struct file_operations snd_disconnect_f_ops;
> static DEFINE_MUTEX(mutex);
> 
> void snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
> {
> 	struct snd_disconnected_file *df, **_dfs;
> 	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
> 	if (df == NULL)
> 		panic("Atomic allocation failed for snd_disconnected_file!");

IIRC, the reason that snd_card_disconnect() uses GFP_ATOMIC is that
(usb-)disconnection was atomic in the earlier time.
You're using mutex here, hence no reason to allocate with GFP_ATOMIC.

> 	df->file = file;
> 	df->release = release;
> 	df->next = NULL;
> 
> 	mutex_lock(&mutex);
> 	_dfs = &disconnecting_files;
> 	while (*_dfs != NULL)
> 		_dfs = &(*_dfs)->next;
> 	*_dfs = df;

You can add to the item to head :)  The order doesn't matter.

> 	mutex_unlock(&mutex);
> 
> 	{
> 		const struct file_operations *old_f_op = file->f_op;
> 		fops_get(&snd_disconnect_f_ops);
> 		file->f_op = &snd_disconnect_f_ops;
> 		fops_put(old_f_op);

I wonder whether the old release might be called during this
operation.  Then df won't be freed.


> static int snd_disconnect_release(struct inode *inode, struct file *file)
> {
> 	struct snd_disconnected_file *df, **_dfs, **__dfs;
> 	int err = 0;
> 	__dfs = _dfs = &disconnecting_files;
> 
> 	mutex_lock(&mutex);
> 	while ((df = *_dfs))
> 		if (df->file == file) {
> 			*__dfs = df->next;
> 			break;
> 		} else {
> 			__dfs = _dfs;
> 			_dfs = &df->next;
> 		}
> 	mutex_unlock(&mutex);

A standard list would make the code more readable (unless you use too
many underscores ;)


Thanks,

Takashi

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [Alsa-devel] [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Takashi Iwai]

At Thu, 5 Oct 2006 01:41:47 +0200,
Karsten Wiese wrote:
> 
> Am Mittwoch, 4. Oktober 2006 22:15 schrieb Takashi Iwai:
> > 
> > This looks like a good optoin.  But one thing we have to be careful
> > about is the module counter since the owner is different between the
> > old f_op and disconnect_f_op...
> > 
> here is rc1, will test later.
> Feel free to pick it apart ;-)

Any special reason to make it separate instead of patching init.c?
Most of codes (e.g. dummy callbacks) are already in init.c.

> struct snd_disconnected_file {
> 	struct file *file;
> 	int (*release) (struct inode *, struct file *);
> 	struct snd_disconnected_file *next;

We can use a standard list here.

> };
> 
> static struct snd_disconnected_file *disconnecting_files;
> static struct file_operations snd_disconnect_f_ops;
> static DEFINE_MUTEX(mutex);
> 
> void snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
> {
> 	struct snd_disconnected_file *df, **_dfs;
> 	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
> 	if (df == NULL)
> 		panic("Atomic allocation failed for snd_disconnected_file!");

IIRC, the reason that snd_card_disconnect() uses GFP_ATOMIC is that
(usb-)disconnection was atomic in the earlier time.
You're using mutex here, hence no reason to allocate with GFP_ATOMIC.

> 	df->file = file;
> 	df->release = release;
> 	df->next = NULL;
> 
> 	mutex_lock(&mutex);
> 	_dfs = &disconnecting_files;
> 	while (*_dfs != NULL)
> 		_dfs = &(*_dfs)->next;
> 	*_dfs = df;

You can add to the item to head :)  The order doesn't matter.

> 	mutex_unlock(&mutex);
> 
> 	{
> 		const struct file_operations *old_f_op = file->f_op;
> 		fops_get(&snd_disconnect_f_ops);
> 		file->f_op = &snd_disconnect_f_ops;
> 		fops_put(old_f_op);

I wonder whether the old release might be called during this
operation.  Then df won't be freed.


> static int snd_disconnect_release(struct inode *inode, struct file *file)
> {
> 	struct snd_disconnected_file *df, **_dfs, **__dfs;
> 	int err = 0;
> 	__dfs = _dfs = &disconnecting_files;
> 
> 	mutex_lock(&mutex);
> 	while ((df = *_dfs))
> 		if (df->file == file) {
> 			*__dfs = df->next;
> 			break;
> 		} else {
> 			__dfs = _dfs;
> 			_dfs = &df->next;
> 		}
> 	mutex_unlock(&mutex);

A standard list would make the code more readable (unless you use too
many underscores ;)


Thanks,

Takashi

Re: [PATCH] Reset file->f_op in snd_card_file_remove (). Take 2

[Karsten Wiese]

Am Donnerstag, 5. Oktober 2006 12:43 schrieb Takashi Iwai:
> > here is rc1, will test later.
Just posted rc2 to alsa-devel, see also at the end of mail.

> > Feel free to pick it apart ;-)
> 
> Any special reason to make it separate instead of patching init.c?
> Most of codes (e.g. dummy callbacks) are already in init.c.
> 
Maybe there are other usecases for this. Then it'd be easier to move.

> > struct snd_disconnected_file {
> > 	struct file *file;
> > 	int (*release) (struct inode *, struct file *);
> > 	struct snd_disconnected_file *next;
> 
> We can use a standard list here.
struct list_head?
I thought we'd use one pointer less this way.
If there are benefits that outweight one pointer more per instance,
I'll use standard list.

> 
> > };
> > 
> > static struct snd_disconnected_file *disconnecting_files;
> > static struct file_operations snd_disconnect_f_ops;
> > static DEFINE_MUTEX(mutex);
> > 
> > void snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
> > {
> > 	struct snd_disconnected_file *df, **_dfs;
> > 	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
> > 	if (df == NULL)
> > 		panic("Atomic allocation failed for snd_disconnected_file!");
> 
> IIRC, the reason that snd_card_disconnect() uses GFP_ATOMIC is that
> (usb-)disconnection was atomic in the earlier time.
> You're using mutex here, hence no reason to allocate with GFP_ATOMIC.
Ah ok.

> 
> > 	df->file = file;
> > 	df->release = release;
> > 	df->next = NULL;
> > 
> > 	mutex_lock(&mutex);
> > 	_dfs = &disconnecting_files;
> > 	while (*_dfs != NULL)
> > 		_dfs = &(*_dfs)->next;
> > 	*_dfs = df;
> 
> You can add to the item to head :)  The order doesn't matter.
benefit here

> 
> > 	mutex_unlock(&mutex);
> > 
> > 	{
> > 		const struct file_operations *old_f_op = file->f_op;
> > 		fops_get(&snd_disconnect_f_ops);
> > 		file->f_op = &snd_disconnect_f_ops;
> > 		fops_put(old_f_op);
> 
> I wonder whether the old release might be called during this
> operation.  Then df won't be freed.
this looks better in rc2, I hope.

> 
> 
> > static int snd_disconnect_release(struct inode *inode, struct file *file)
> > {
> > 	struct snd_disconnected_file *df, **_dfs, **__dfs;
> > 	int err = 0;
> > 	__dfs = _dfs = &disconnecting_files;
> > 
> > 	mutex_lock(&mutex);
> > 	while ((df = *_dfs))
> > 		if (df->file == file) {
> > 			*__dfs = df->next;
> > 			break;
> > 		} else {
> > 			__dfs = _dfs;
> > 			_dfs = &df->next;
> > 		}
> > 	mutex_unlock(&mutex);
> 
> A standard list would make the code more readable (unless you use too
> many underscores ;)
Will post rc3 :-)

      Karsten


rc2:
-------------------------------------------------------------
snd_disconnected_file, virtual device

On response to "usb disconnect" an usb-soundcard manipulates
that snd_card's file's f_ops to only allow release
by calling this virtual device's 
   void snd_disconnect_file(struct file *file).
After release is actually called, the virtual device instance is freeed.

Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>


--- alsa-kernel/core/_disconnected.c	2006-10-05 12:16:34.000000000 +0200
+++ alsa-kernel/core/disconnected.c	2006-10-05 11:47:43.000000000 +0200
@@ -0,0 +1,147 @@
+/* virtual device
+   hides a real device's f_ops,
+   exept for release
+
+ *  Copyright (c) by Karsten Wiese <fzu@wemgehoertderstaat.de>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+
+#include <linux/fs.h>
+#include <linux/poll.h>
+#include <linux/module.h>
+
+struct snd_disconnected_file {
+	struct file *file;
+	const struct file_operations *f_op;
+	struct snd_disconnected_file *next;
+};
+
+static struct snd_disconnected_file *disconnecting_files;
+static struct file_operations snd_disconnect_f_ops;
+static DEFINE_MUTEX(mutex);
+
+void snd_disconnect_file(struct file *file)
+{
+	struct snd_disconnected_file *df, **_dfs;
+	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
+	if (df == NULL)
+		panic("Atomic allocation failed for snd_disconnected_file!");
+
+	df->file = file;
+	df->f_op = file->f_op;
+	df->next = NULL;
+
+	mutex_lock(&mutex);
+	_dfs = &disconnecting_files;
+	while (*_dfs != NULL)
+		_dfs = &(*_dfs)->next;
+	*_dfs = df;
+	mutex_unlock(&mutex);
+
+	fops_get(&snd_disconnect_f_ops);
+	file->f_op = &snd_disconnect_f_ops;
+	printk(KERN_INFO "%s\n", __FUNCTION__);
+}
+
+
+static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
+				   size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
+				    size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_release(struct inode *inode, struct file *file)
+{
+	struct snd_disconnected_file *df, **_dfs, **__dfs;
+	int err = 0;
+	__dfs = _dfs = &disconnecting_files;
+
+	mutex_lock(&mutex);
+	while ((df = *_dfs))
+		if (df->file == file) {
+			*__dfs = df->next;
+			break;
+		} else {
+			__dfs = _dfs;
+			_dfs = &df->next;
+		}
+	mutex_unlock(&mutex);
+
+	if (df)	{
+		err = df->f_op->release(inode, file);
+		fops_put(df->f_op);
+		kfree(df);
+		printk(KERN_INFO "%s\n", __FUNCTION__);
+		return err;
+	}
+
+	panic("%s(%p, %p) failed!", __FUNCTION__, inode, file);
+}
+
+static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
+{
+	return POLLERR | POLLNVAL;
+}
+
+static long snd_disconnect_ioctl(struct file *file,
+				 unsigned int cmd, unsigned long arg)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_fasync(int fd, struct file *file, int on)
+{
+	return -ENODEV;
+}
+
+/*
+
+ */
+
+static struct file_operations snd_disconnect_f_ops =
+{
+	.owner = 	THIS_MODULE,
+	.llseek =	snd_disconnect_llseek,
+	.read = 	snd_disconnect_read,
+	.write =	snd_disconnect_write,
+	.release =	snd_disconnect_release,
+	.poll =		snd_disconnect_poll,
+	.unlocked_ioctl = snd_disconnect_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = snd_disconnect_ioctl,
+#endif
+	.mmap =		snd_disconnect_mmap,
+	.fasync =	snd_disconnect_fasync
+};

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [Alsa-devel] [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

Am Donnerstag, 5. Oktober 2006 12:43 schrieb Takashi Iwai:
> > here is rc1, will test later.
Just posted rc2 to alsa-devel, see also at the end of mail.

> > Feel free to pick it apart ;-)
> 
> Any special reason to make it separate instead of patching init.c?
> Most of codes (e.g. dummy callbacks) are already in init.c.
> 
Maybe there are other usecases for this. Then it'd be easier to move.

> > struct snd_disconnected_file {
> > 	struct file *file;
> > 	int (*release) (struct inode *, struct file *);
> > 	struct snd_disconnected_file *next;
> 
> We can use a standard list here.
struct list_head?
I thought we'd use one pointer less this way.
If there are benefits that outweight one pointer more per instance,
I'll use standard list.

> 
> > };
> > 
> > static struct snd_disconnected_file *disconnecting_files;
> > static struct file_operations snd_disconnect_f_ops;
> > static DEFINE_MUTEX(mutex);
> > 
> > void snd_disconnect_file(struct file *file, int (*release) (struct inode *, struct file *))
> > {
> > 	struct snd_disconnected_file *df, **_dfs;
> > 	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
> > 	if (df == NULL)
> > 		panic("Atomic allocation failed for snd_disconnected_file!");
> 
> IIRC, the reason that snd_card_disconnect() uses GFP_ATOMIC is that
> (usb-)disconnection was atomic in the earlier time.
> You're using mutex here, hence no reason to allocate with GFP_ATOMIC.
Ah ok.

> 
> > 	df->file = file;
> > 	df->release = release;
> > 	df->next = NULL;
> > 
> > 	mutex_lock(&mutex);
> > 	_dfs = &disconnecting_files;
> > 	while (*_dfs != NULL)
> > 		_dfs = &(*_dfs)->next;
> > 	*_dfs = df;
> 
> You can add to the item to head :)  The order doesn't matter.
benefit here

> 
> > 	mutex_unlock(&mutex);
> > 
> > 	{
> > 		const struct file_operations *old_f_op = file->f_op;
> > 		fops_get(&snd_disconnect_f_ops);
> > 		file->f_op = &snd_disconnect_f_ops;
> > 		fops_put(old_f_op);
> 
> I wonder whether the old release might be called during this
> operation.  Then df won't be freed.
this looks better in rc2, I hope.

> 
> 
> > static int snd_disconnect_release(struct inode *inode, struct file *file)
> > {
> > 	struct snd_disconnected_file *df, **_dfs, **__dfs;
> > 	int err = 0;
> > 	__dfs = _dfs = &disconnecting_files;
> > 
> > 	mutex_lock(&mutex);
> > 	while ((df = *_dfs))
> > 		if (df->file == file) {
> > 			*__dfs = df->next;
> > 			break;
> > 		} else {
> > 			__dfs = _dfs;
> > 			_dfs = &df->next;
> > 		}
> > 	mutex_unlock(&mutex);
> 
> A standard list would make the code more readable (unless you use too
> many underscores ;)
Will post rc3 :-)

      Karsten


rc2:
-------------------------------------------------------------
snd_disconnected_file, virtual device

On response to "usb disconnect" an usb-soundcard manipulates
that snd_card's file's f_ops to only allow release
by calling this virtual device's 
   void snd_disconnect_file(struct file *file).
After release is actually called, the virtual device instance is freeed.

Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>


--- alsa-kernel/core/_disconnected.c	2006-10-05 12:16:34.000000000 +0200
+++ alsa-kernel/core/disconnected.c	2006-10-05 11:47:43.000000000 +0200
@@ -0,0 +1,147 @@
+/* virtual device
+   hides a real device's f_ops,
+   exept for release
+
+ *  Copyright (c) by Karsten Wiese <fzu@wemgehoertderstaat.de>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+
+#include <linux/fs.h>
+#include <linux/poll.h>
+#include <linux/module.h>
+
+struct snd_disconnected_file {
+	struct file *file;
+	const struct file_operations *f_op;
+	struct snd_disconnected_file *next;
+};
+
+static struct snd_disconnected_file *disconnecting_files;
+static struct file_operations snd_disconnect_f_ops;
+static DEFINE_MUTEX(mutex);
+
+void snd_disconnect_file(struct file *file)
+{
+	struct snd_disconnected_file *df, **_dfs;
+	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_ATOMIC);
+	if (df == NULL)
+		panic("Atomic allocation failed for snd_disconnected_file!");
+
+	df->file = file;
+	df->f_op = file->f_op;
+	df->next = NULL;
+
+	mutex_lock(&mutex);
+	_dfs = &disconnecting_files;
+	while (*_dfs != NULL)
+		_dfs = &(*_dfs)->next;
+	*_dfs = df;
+	mutex_unlock(&mutex);
+
+	fops_get(&snd_disconnect_f_ops);
+	file->f_op = &snd_disconnect_f_ops;
+	printk(KERN_INFO "%s\n", __FUNCTION__);
+}
+
+
+static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
+				   size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
+				    size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_release(struct inode *inode, struct file *file)
+{
+	struct snd_disconnected_file *df, **_dfs, **__dfs;
+	int err = 0;
+	__dfs = _dfs = &disconnecting_files;
+
+	mutex_lock(&mutex);
+	while ((df = *_dfs))
+		if (df->file == file) {
+			*__dfs = df->next;
+			break;
+		} else {
+			__dfs = _dfs;
+			_dfs = &df->next;
+		}
+	mutex_unlock(&mutex);
+
+	if (df)	{
+		err = df->f_op->release(inode, file);
+		fops_put(df->f_op);
+		kfree(df);
+		printk(KERN_INFO "%s\n", __FUNCTION__);
+		return err;
+	}
+
+	panic("%s(%p, %p) failed!", __FUNCTION__, inode, file);
+}
+
+static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
+{
+	return POLLERR | POLLNVAL;
+}
+
+static long snd_disconnect_ioctl(struct file *file,
+				 unsigned int cmd, unsigned long arg)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_fasync(int fd, struct file *file, int on)
+{
+	return -ENODEV;
+}
+
+/*
+
+ */
+
+static struct file_operations snd_disconnect_f_ops =
+{
+	.owner = 	THIS_MODULE,
+	.llseek =	snd_disconnect_llseek,
+	.read = 	snd_disconnect_read,
+	.write =	snd_disconnect_write,
+	.release =	snd_disconnect_release,
+	.poll =		snd_disconnect_poll,
+	.unlocked_ioctl = snd_disconnect_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = snd_disconnect_ioctl,
+#endif
+	.mmap =		snd_disconnect_mmap,
+	.fasync =	snd_disconnect_fasync
+};

Re: [PATCH] Reset file->f_op in snd_card_file_remove (). Take 2

[Karsten Wiese]

here is rc3.

Anything to fix?

We've lots of things disconnectable...
Is this useful in the kernel global?

      Karsten

rc3:
---------------------------------------------------------------------------
snd_disconnected_file, virtual device

On response to "usb disconnect" an usb-soundcard manipulates
that snd_card's file's f_ops to only allow release
by calling this virtual device's 
   void snd_disconnect_file(struct file *file).
After release is actually called, the virtual device instance is freeed.

Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>


--- alsa-kernel/core/_disconnected.c	2006-10-05 12:16:34.000000000 +0200
+++ alsa-kernel/core/disconnected.c	2006-10-05 15:15:06.000000000 +0200
@@ -0,0 +1,141 @@
+/* virtual device
+   hides a real device's f_ops,
+   except for release
+
+ *  Copyright (c) by Karsten Wiese <fzu@wemgehoertderstaat.de>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/poll.h>
+#include <linux/module.h>
+
+static DEFINE_MUTEX(mutex);
+static LIST_HEAD(disconnected_files);
+
+struct snd_disconnected_file {
+	struct file *file;
+	const struct file_operations *f_op;
+	struct list_head list;
+};
+
+static struct file_operations snd_disconnect_f_ops;
+
+void snd_disconnect_file(struct file *file)
+{
+	struct snd_disconnected_file *df;
+	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_KERNEL);
+	if (df == NULL)
+		panic("Atomic allocation failed for snd_disconnected_file!");
+
+	df->file = file;
+	df->f_op = file->f_op;
+
+	mutex_lock(&mutex);
+	list_add(&df->list, &disconnected_files);
+	mutex_unlock(&mutex);
+
+	fops_get(&snd_disconnect_f_ops);
+	file->f_op = &snd_disconnect_f_ops;
+	printk(KERN_INFO "%s rc3\n", __FUNCTION__);
+}
+
+
+static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
+				   size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
+				    size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_release(struct inode *inode, struct file *file)
+{
+	struct snd_disconnected_file *df = NULL;
+	struct list_head *entry;
+
+	mutex_lock(&mutex);
+	list_for_each(entry, &disconnected_files) {
+		struct snd_disconnected_file *_df;
+		_df = list_entry(entry, struct snd_disconnected_file, list);
+		if (_df->file == file) {
+			list_del(entry);
+			df = _df;
+			break;
+		}
+	}
+	mutex_unlock(&mutex);
+
+	if (likely(df != NULL))	{
+		int err = df->f_op->release(inode, file);
+		fops_put(df->f_op);
+		kfree(df);
+		printk(KERN_INFO "%s rc3\n", __FUNCTION__);
+		return err;
+	}
+
+	panic("%s(%p, %p) failed!", __FUNCTION__, inode, file);
+}
+
+static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
+{
+	return POLLERR | POLLNVAL;
+}
+
+static long snd_disconnect_ioctl(struct file *file,
+				 unsigned int cmd, unsigned long arg)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_fasync(int fd, struct file *file, int on)
+{
+	return -ENODEV;
+}
+
+static struct file_operations snd_disconnect_f_ops =
+{
+	.owner = 	THIS_MODULE,
+	.llseek =	snd_disconnect_llseek,
+	.read = 	snd_disconnect_read,
+	.write =	snd_disconnect_write,
+	.release =	snd_disconnect_release,
+	.poll =		snd_disconnect_poll,
+	.unlocked_ioctl = snd_disconnect_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = snd_disconnect_ioctl,
+#endif
+	.mmap =		snd_disconnect_mmap,
+	.fasync =	snd_disconnect_fasync
+};

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [Alsa-devel] [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2

[Karsten Wiese]

here is rc3.

Anything to fix?

We've lots of things disconnectable...
Is this useful in the kernel global?

      Karsten

rc3:
---------------------------------------------------------------------------
snd_disconnected_file, virtual device

On response to "usb disconnect" an usb-soundcard manipulates
that snd_card's file's f_ops to only allow release
by calling this virtual device's 
   void snd_disconnect_file(struct file *file).
After release is actually called, the virtual device instance is freeed.

Signed-off-by: Karsten Wiese <fzu@wemgehoertderstaat.de>


--- alsa-kernel/core/_disconnected.c	2006-10-05 12:16:34.000000000 +0200
+++ alsa-kernel/core/disconnected.c	2006-10-05 15:15:06.000000000 +0200
@@ -0,0 +1,141 @@
+/* virtual device
+   hides a real device's f_ops,
+   except for release
+
+ *  Copyright (c) by Karsten Wiese <fzu@wemgehoertderstaat.de>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/poll.h>
+#include <linux/module.h>
+
+static DEFINE_MUTEX(mutex);
+static LIST_HEAD(disconnected_files);
+
+struct snd_disconnected_file {
+	struct file *file;
+	const struct file_operations *f_op;
+	struct list_head list;
+};
+
+static struct file_operations snd_disconnect_f_ops;
+
+void snd_disconnect_file(struct file *file)
+{
+	struct snd_disconnected_file *df;
+	df = kmalloc(sizeof(struct snd_disconnected_file), GFP_KERNEL);
+	if (df == NULL)
+		panic("Atomic allocation failed for snd_disconnected_file!");
+
+	df->file = file;
+	df->f_op = file->f_op;
+
+	mutex_lock(&mutex);
+	list_add(&df->list, &disconnected_files);
+	mutex_unlock(&mutex);
+
+	fops_get(&snd_disconnect_f_ops);
+	file->f_op = &snd_disconnect_f_ops;
+	printk(KERN_INFO "%s rc3\n", __FUNCTION__);
+}
+
+
+static loff_t snd_disconnect_llseek(struct file *file, loff_t offset, int orig)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_read(struct file *file, char __user *buf,
+				   size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static ssize_t snd_disconnect_write(struct file *file, const char __user *buf,
+				    size_t count, loff_t *offset)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_release(struct inode *inode, struct file *file)
+{
+	struct snd_disconnected_file *df = NULL;
+	struct list_head *entry;
+
+	mutex_lock(&mutex);
+	list_for_each(entry, &disconnected_files) {
+		struct snd_disconnected_file *_df;
+		_df = list_entry(entry, struct snd_disconnected_file, list);
+		if (_df->file == file) {
+			list_del(entry);
+			df = _df;
+			break;
+		}
+	}
+	mutex_unlock(&mutex);
+
+	if (likely(df != NULL))	{
+		int err = df->f_op->release(inode, file);
+		fops_put(df->f_op);
+		kfree(df);
+		printk(KERN_INFO "%s rc3\n", __FUNCTION__);
+		return err;
+	}
+
+	panic("%s(%p, %p) failed!", __FUNCTION__, inode, file);
+}
+
+static unsigned int snd_disconnect_poll(struct file * file, poll_table * wait)
+{
+	return POLLERR | POLLNVAL;
+}
+
+static long snd_disconnect_ioctl(struct file *file,
+				 unsigned int cmd, unsigned long arg)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	return -ENODEV;
+}
+
+static int snd_disconnect_fasync(int fd, struct file *file, int on)
+{
+	return -ENODEV;
+}
+
+static struct file_operations snd_disconnect_f_ops =
+{
+	.owner = 	THIS_MODULE,
+	.llseek =	snd_disconnect_llseek,
+	.read = 	snd_disconnect_read,
+	.write =	snd_disconnect_write,
+	.release =	snd_disconnect_release,
+	.poll =		snd_disconnect_poll,
+	.unlocked_ioctl = snd_disconnect_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = snd_disconnect_ioctl,
+#endif
+	.mmap =		snd_disconnect_mmap,
+	.fasync =	snd_disconnect_fasync
+};