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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
