Messages in this thread Patch in this message |  | | | Date | Wed, 04 Oct 2006 17:57:41 +0200 | | From | Takashi Iwai <> | | 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); - 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/
|  |