Date: Wed, 04 Oct 2006 22:15:39 +0200
Message-ID: <s5hvemzq1lg.wl%tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Karsten Wiese <annabellesgarden@yahoo.de>
Cc: linux-kernel@vger.kernel.org, alsa-devel@lists.sourceforge.net,
       mingo@elte.hu
Subject: Re: [PATCH] Reset file->f_op in snd_card_file_remove(). Take 2
In-Reply-To: <200610042201.53337.annabellesgarden@yahoo.de>
References: <200609282228.02611.annabellesgarden@yahoo.de>
	<200610041736.37639.annabellesgarden@yahoo.de>
	<s5hmz8cjcp6.wl%tiwai@suse.de>
	<200610042201.53337.annabellesgarden@yahoo.de>
User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 MULE XEmacs/21.5 (beta25)
 (eggplant) (+CVS-20060326) (i386-suse-linux)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4670
Lines: 156

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/