2006-09-28 20:27:44

by Karsten Wiese

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

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 <[email protected]>


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


2006-09-29 10:48:08

by Takashi Iwai

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

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 <[email protected]>

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 <[email protected]>
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 <[email protected]>
---

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

2006-09-29 12:28:39

by Karsten Wiese

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

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 <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>
> ---
>
> 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;
> }
>
>
>

2006-09-29 12:34:26

by Ingo Molnar

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


* Karsten Wiese <[email protected]> wrote:

> Florin's patch fixes it.

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

Ingo

2006-09-29 12:45:49

by Takashi Iwai

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

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 <[email protected]>
> >
> > 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

2006-10-01 18:28:50

by Karsten Wiese

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

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 <[email protected]>
> > >
> > > 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






Attachments:
(No filename) (3.04 kB)
crash9.bz2 (8.20 kB)
Download all attachments

2006-10-04 09:22:41

by Takashi Iwai

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

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 <[email protected]>
> > > >
> > > > 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

2006-10-04 14:18:11

by Takashi Iwai

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

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

2006-10-04 15:35:36

by Karsten Wiese

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

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

2006-10-04 15:57:48

by Takashi Iwai

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

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

2006-10-04 20:00:47

by Karsten Wiese

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

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

2006-10-04 20:15:43

by Takashi Iwai

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

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

2006-10-04 23:40:40

by Karsten Wiese

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

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 <[email protected]>
*
*
* 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

2006-10-05 10:43:54

by Takashi Iwai

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

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

2006-10-05 11:32:24

by Karsten Wiese

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

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 <[email protected]>


--- 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 <[email protected]>
+ *
+ *
+ * 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
+};

2006-10-05 14:11:00

by Karsten Wiese

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

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 <[email protected]>


--- 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 <[email protected]>
+ *
+ *
+ * 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
+};