From: Paolo Bonzini <pbonzini@redhat.com> Date: Tue, 18 Aug 2009 16:59:32 -0400 Subject: [misc] saner FASYNC handling on file close Message-id: <1250614772-3268-1-git-send-email-pbonzini@redhat.com> Patchwork-id: 20751 O-Subject: [RHEL5.5 PATCH v2] BZ510746: saner FASYNC handling on file close Bugzilla: 510746 RH-Acked-by: Christopher Lalancette <clalance@redhat.com> RH-Acked-by: Anton Arapov <Anton@redhat.com> RH-Acked-by: Oleg Nesterov <oleg@redhat.com> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=510746 Brew build: http://brewweb.devel.redhat.com/brew/taskinfo?taskID=1932337 Upstream: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=233e70f http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e5bc49b This bug is related to the SCM_RIGHTS DoS of bug 470201, in that it is triggered by the same testcase. It is not as serious, however, because this is just a WARN_ON_ONCE rather than a kernel panic. This is a backport of an upstream cleanup patch whose comment was as follows: As it is, all instances of ->release() for files that have ->fasync() need to remember to evict file from fasync lists; forgetting that creates a hole and we actually have a bunch that *does* forget. So let's keep our lives simple - let __fput() check FASYNC in file->f_flags and call ->fasync() there if it's been set. And lose that crap in ->release() instances - leaving it there is still valid, but we don't have to bother anymore. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> It fixes the bug just by virtue of removing the code path leading to the "BUG: warning at kernel/softirq.c:138/local_bh_enable()" in the current RHEL5 kernel. I cannot exclude in principle that a more complicated testcase involving FASYNC still exhibits the bug. Compared to v1, I squashed in Oleg's suggested patch (the second listed above), fixing a preexisting memory leak that this patch alone would turn into a crash. v1 was tested by Jan Tluka on RHTS for the Bugzilla testcase. The difference between v1 and v2 is only related to pipes, so I tested it using some test code using FASYNC on pipes. I couldn't reproduce any kind of crash though. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Cc: Prarit Bhargava <prarit@redhat.com> Cc: Oleg Nesterov <oleg@redhat.com> drivers/char/hpet.c | 3 --- drivers/char/ipmi/ipmi_devintf.c | 2 -- drivers/char/ipmi/ipmi_watchdog.c | 1 - drivers/char/rtc.c | 3 --- drivers/char/sonypi.c | 1 - drivers/ieee1394/dv1394.c | 3 --- drivers/infiniband/core/uverbs_main.c | 2 -- drivers/input/evdev.c | 1 - drivers/input/joydev.c | 2 -- drivers/input/misc/hp_sdc_rtc.c | 13 ------------- drivers/input/mousedev.c | 2 -- drivers/input/serio/serio_raw.c | 1 - drivers/message/i2o/i2o_config.c | 21 +++++---------------- drivers/net/tun.c | 2 -- drivers/rtc/rtc-dev.c | 3 --- drivers/scsi/megaraid/megaraid_sas.c | 12 ------------ drivers/scsi/sg.c | 1 - drivers/telephony/ixj.c | 1 - drivers/uio/uio.c | 3 --- drivers/usb/gadget/inode.c | 1 - fs/file_table.c | 4 ++++ fs/fuse/dev.c | 1 - fs/pipe.c | 11 ++++------- net/socket.c | 1 - sound/core/control.c | 1 - sound/core/init.c | 5 ++++- sound/core/pcm_native.c | 1 - sound/core/timer.c | 1 - 28 files changed, 17 insertions(+), 86 deletions(-) diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c index 8afba33..aff9be4 100644 --- a/drivers/char/hpet.c +++ b/drivers/char/hpet.c @@ -340,9 +340,6 @@ static int hpet_release(struct inode *inode, struct file *file) if (irq) free_irq(irq, devp); - if (file->f_flags & FASYNC) - hpet_fasync(-1, file, 0); - file->private_data = NULL; return 0; } diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c index 2e70748..f67b789 100644 --- a/drivers/char/ipmi/ipmi_devintf.c +++ b/drivers/char/ipmi/ipmi_devintf.c @@ -157,8 +157,6 @@ static int ipmi_release(struct inode *inode, struct file *file) if (rv) return rv; - ipmi_fasync (-1, file, 0); - /* FIXME - free the messages in the list. */ kfree(priv); diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c index 23ce321..23b14a4 100644 --- a/drivers/char/ipmi/ipmi_watchdog.c +++ b/drivers/char/ipmi/ipmi_watchdog.c @@ -820,7 +820,6 @@ static int ipmi_close(struct inode *ino, struct file *filep) clear_bit(0, &ipmi_wdog_open); } - ipmi_fasync (-1, filep, 0); expect_close = 0; return 0; diff --git a/drivers/char/rtc.c b/drivers/char/rtc.c index bb0e5a3..85b0074 100644 --- a/drivers/char/rtc.c +++ b/drivers/char/rtc.c @@ -760,9 +760,6 @@ static int rtc_release(struct inode *inode, struct file *file) } spin_unlock_irq(&rtc_lock); - if (file->f_flags & FASYNC) { - rtc_fasync (-1, file, 0); - } no_irq: #endif diff --git a/drivers/char/sonypi.c b/drivers/char/sonypi.c index d4e434d..e7de47c 100644 --- a/drivers/char/sonypi.c +++ b/drivers/char/sonypi.c @@ -938,7 +938,6 @@ static int sonypi_misc_fasync(int fd, struct file *filp, int on) static int sonypi_misc_release(struct inode *inode, struct file *file) { - sonypi_misc_fasync(-1, file, 0); down(&sonypi_device.lock); sonypi_device.open_count--; up(&sonypi_device.lock); diff --git a/drivers/ieee1394/dv1394.c b/drivers/ieee1394/dv1394.c index 87532dd..a774525 100644 --- a/drivers/ieee1394/dv1394.c +++ b/drivers/ieee1394/dv1394.c @@ -1836,9 +1836,6 @@ static int dv1394_release(struct inode *inode, struct file *file) /* OK to free the DMA buffer, no more mappings can exist */ do_dv1394_shutdown(video, 1); - /* clean up async I/O users */ - dv1394_fasync(-1, file, 0); - /* give someone else a turn */ clear_bit(0, &video->open); diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 68b75e7..151a411 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -358,8 +358,6 @@ static int ib_uverbs_event_close(struct inode *inode, struct file *filp) } spin_unlock_irq(&file->lock); - ib_uverbs_event_fasync(-1, filp, 0); - if (file->is_async) { ib_unregister_event_handler(&file->uverbs_file->event_handler); kref_put(&file->uverbs_file->ref, ib_uverbs_release_file); diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index 4bf4818..df834a5 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -109,7 +109,6 @@ static int evdev_release(struct inode * inode, struct file * file) list->evdev->grab = NULL; } - evdev_fasync(-1, file, 0); list_del(&list->node); if (!--list->evdev->open) { diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c index d671575..87e0a46 100644 --- a/drivers/input/joydev.c +++ b/drivers/input/joydev.c @@ -149,8 +149,6 @@ static int joydev_release(struct inode * inode, struct file * file) { struct joydev_list *list = file->private_data; - joydev_fasync(-1, file, 0); - list_del(&list->node); if (!--list->joydev->open) { diff --git a/drivers/input/misc/hp_sdc_rtc.c b/drivers/input/misc/hp_sdc_rtc.c index 1be9639..1f2ce29 100644 --- a/drivers/input/misc/hp_sdc_rtc.c +++ b/drivers/input/misc/hp_sdc_rtc.c @@ -69,7 +69,6 @@ static int hp_sdc_rtc_ioctl(struct inode *inode, struct file *file, static unsigned int hp_sdc_rtc_poll(struct file *file, poll_table *wait); static int hp_sdc_rtc_open(struct inode *inode, struct file *file); -static int hp_sdc_rtc_release(struct inode *inode, struct file *file); static int hp_sdc_rtc_fasync (int fd, struct file *filp, int on); static int hp_sdc_rtc_read_proc(char *page, char **start, off_t off, @@ -411,17 +410,6 @@ static int hp_sdc_rtc_open(struct inode *inode, struct file *file) return 0; } -static int hp_sdc_rtc_release(struct inode *inode, struct file *file) -{ - /* Turn off interrupts? */ - - if (file->f_flags & FASYNC) { - hp_sdc_rtc_fasync (-1, file, 0); - } - - return 0; -} - static int hp_sdc_rtc_fasync (int fd, struct file *filp, int on) { return fasync_helper (fd, filp, on, &hp_sdc_rtc_async_queue); @@ -677,7 +665,6 @@ static struct file_operations hp_sdc_rtc_fops = { .poll = hp_sdc_rtc_poll, .ioctl = hp_sdc_rtc_ioctl, .open = hp_sdc_rtc_open, - .release = hp_sdc_rtc_release, .fasync = hp_sdc_rtc_fasync, }; diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c index ea8a5e4..7604379 100644 --- a/drivers/input/mousedev.c +++ b/drivers/input/mousedev.c @@ -384,8 +384,6 @@ static int mousedev_release(struct inode * inode, struct file * file) { struct mousedev_list *list = file->private_data; - mousedev_fasync(-1, file, 0); - list_del(&list->node); if (!--list->mousedev->open) { diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c index 71a8eea..e242ad8 100644 --- a/drivers/input/serio/serio_raw.c +++ b/drivers/input/serio/serio_raw.c @@ -131,7 +131,6 @@ static int serio_raw_release(struct inode *inode, struct file *file) mutex_lock(&serio_raw_mutex); - serio_raw_fasync(-1, file, 0); serio_raw_cleanup(serio_raw); mutex_unlock(&serio_raw_mutex); diff --git a/drivers/message/i2o/i2o_config.c b/drivers/message/i2o/i2o_config.c index 7d23e08..51c348b 100644 --- a/drivers/message/i2o/i2o_config.c +++ b/drivers/message/i2o/i2o_config.c @@ -1078,28 +1078,17 @@ static int cfg_fasync(int fd, struct file *fp, int on) static int cfg_release(struct inode *inode, struct file *file) { ulong id = (ulong) file->private_data; - struct i2o_cfg_info *p1, *p2; + struct i2o_cfg_info *p, **q; unsigned long flags; lock_kernel(); - p1 = p2 = NULL; - spin_lock_irqsave(&i2o_config_lock, flags); - for (p1 = open_files; p1;) { - if (p1->q_id == id) { - - if (p1->fasync) - cfg_fasync(-1, file, 0); - if (p2) - p2->next = p1->next; - else - open_files = p1->next; - - kfree(p1); + for (q = &open_files; (p = *q) != NULL; q = &p->next) { + if (p->q_id == id) { + *q = p->next; + kfree(p); break; } - p2 = p1; - p1 = p1->next; } spin_unlock_irqrestore(&i2o_config_lock, flags); unlock_kernel(); diff --git a/drivers/net/tun.c b/drivers/net/tun.c index c73f5cf..8dd6048 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1072,8 +1072,6 @@ static int tun_chr_close(struct inode *inode, struct file *file) DBG(KERN_INFO "%s: tun_chr_close\n", tun->dev->name); - tun_chr_fasync(-1, file, 0); - rtnl_lock(); /* Detach from net device */ diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 9a78ed7..e34165f 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -366,9 +366,6 @@ static int rtc_dev_release(struct inode *inode, struct file *file) if (rtc->ops->release) rtc->ops->release(rtc->class_dev.dev); - if (file->f_flags & FASYNC) - rtc_dev_fasync(-1, file, 0); - mutex_unlock(&rtc->char_lock); return 0; } diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c index 728cc63..7954f55 100644 --- a/drivers/scsi/megaraid/megaraid_sas.c +++ b/drivers/scsi/megaraid/megaraid_sas.c @@ -3522,17 +3522,6 @@ static int megasas_mgmt_open(struct inode *inode, struct file *filep) } /** - * megasas_mgmt_release - char node "release" entry point - */ -static int megasas_mgmt_release(struct inode *inode, struct file *filep) -{ - filep->private_data = NULL; - fasync_helper(-1, filep, 0, &megasas_async_queue); - - return 0; -} - -/** * megasas_mgmt_fasync - Async notifier registration from applications * * This function adds the calling process to a driver global queue. When an @@ -3897,7 +3886,6 @@ megasas_mgmt_compat_ioctl(struct file *file, unsigned int cmd, static const struct file_operations megasas_mgmt_fops = { .owner = THIS_MODULE, .open = megasas_mgmt_open, - .release = megasas_mgmt_release, .fasync = megasas_mgmt_fasync, .unlocked_ioctl = megasas_mgmt_ioctl, .poll = megasas_mgmt_poll, diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index 7fe5bd6..79ecf4e 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -316,7 +316,6 @@ sg_release(struct inode *inode, struct file *filp) if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp))) return -ENXIO; SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name)); - sg_fasync(-1, filp, 0); /* remove filp from async notification list */ if (0 == sg_remove_sfp(sdp, sfp)) { /* Returns 1 when sdp gone */ if (!sdp->detached) { scsi_device_put(sdp->device); diff --git a/drivers/telephony/ixj.c b/drivers/telephony/ixj.c index f6b2948..dc9f0e6 100644 --- a/drivers/telephony/ixj.c +++ b/drivers/telephony/ixj.c @@ -2322,7 +2322,6 @@ static int ixj_release(struct inode *inode, struct file *file_p) j->rec_codec = j->play_codec = 0; j->rec_frame_size = j->play_frame_size = 0; j->flags.cidsent = j->flags.cidring = 0; - ixj_fasync(-1, file_p, 0); /* remove from list of async notification */ if(j->cardtype == QTI_LINEJACK && !j->readers && !j->writers) { ixj_set_port(j, PORT_PSTN); diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index cb24203..cf2cb47 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -331,9 +331,6 @@ static int uio_release(struct inode *inode, struct file *filep) ret = idev->info->release(idev->info, inode); module_put(idev->owner); - - if (filep->f_flags & FASYNC) - ret = uio_fasync(-1, filep, 0); kfree(listener); return ret; } diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c index 084fe62..2dd7808 100644 --- a/drivers/usb/gadget/inode.c +++ b/drivers/usb/gadget/inode.c @@ -1219,7 +1219,6 @@ dev_release (struct inode *inode, struct file *fd) * alternatively, all host requests will time out. */ - fasync_helper (-1, fd, 0, &dev->fasync); kfree (dev->buf); dev->buf = NULL; put_dev (dev); diff --git a/fs/file_table.c b/fs/file_table.c index d363afd..a85718c 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -166,6 +166,10 @@ void fastcall __fput(struct file *file) eventpoll_release(file); locks_remove_flock(file); + if (unlikely(file->f_flags & FASYNC)) { + if (file->f_op && file->f_op->fasync) + file->f_op->fasync(-1, file, 0); + } if (file->f_op && file->f_op->release) file->f_op->release(inode, file); security_file_free(file); diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 671d3df..5f84970 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1072,7 +1072,6 @@ static int fuse_dev_release(struct inode *inode, struct file *file) end_requests(fc, &fc->pending); end_requests(fc, &fc->processing); spin_unlock(&fc->lock); - fasync_helper(-1, file, 0, &fc->fasync); fuse_conn_put(fc); } diff --git a/fs/pipe.c b/fs/pipe.c index 77c2356..717a9bb 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -657,12 +657,12 @@ pipe_rdwr_fasync(int fd, struct file *filp, int on) int retval; mutex_lock(&inode->i_mutex); - retval = fasync_helper(fd, filp, on, &pipe->fasync_readers); - - if (retval >= 0) + if (retval >= 0) { retval = fasync_helper(fd, filp, on, &pipe->fasync_writers); - + if (retval < 0) /* this can happen only if on == T */ + fasync_helper(-1, filp, 0, &pipe->fasync_readers); + } mutex_unlock(&inode->i_mutex); if (retval < 0) @@ -675,14 +675,12 @@ pipe_rdwr_fasync(int fd, struct file *filp, int on) static int pipe_read_release(struct inode *inode, struct file *filp) { - pipe_read_fasync(-1, filp, 0); return pipe_release(inode, 1, 0); } static int pipe_write_release(struct inode *inode, struct file *filp) { - pipe_write_fasync(-1, filp, 0); return pipe_release(inode, 0, 1); } @@ -691,7 +689,6 @@ pipe_rdwr_release(struct inode *inode, struct file *filp) { int decr, decw; - pipe_rdwr_fasync(-1, filp, 0); decr = (filp->f_mode & FMODE_READ) != 0; decw = (filp->f_mode & FMODE_WRITE) != 0; return pipe_release(inode, decr, decw); diff --git a/net/socket.c b/net/socket.c index f82cd96..5589e76 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1028,7 +1028,6 @@ static int sock_close(struct inode *inode, struct file *filp) printk(KERN_DEBUG "sock_close: NULL inode\n"); return 0; } - sock_fasync(-1, filp, 0); sock_release(SOCKET_I(inode)); return 0; } diff --git a/sound/core/control.c b/sound/core/control.c index 5e7525c..2448150 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -114,7 +114,6 @@ static int snd_ctl_release(struct inode *inode, struct file *file) unsigned int idx; ctl = file->private_data; - fasync_helper(-1, file, 0, &ctl->fasync); file->private_data = NULL; card = ctl->card; write_lock_irqsave(&card->ctl_files_rwlock, flags); diff --git a/sound/core/init.c b/sound/core/init.c index cba0e25..8c7615d 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -217,8 +217,11 @@ static int snd_disconnect_release(struct inode *inode, struct file *file) } spin_unlock(&shutdown_lock); - if (likely(df)) + if (likely(df)) { + if ((file->f_flags & FASYNC) && df->disconnected_f_op->fasync) + df->disconnected_f_op->fasync(-1, file, 0); return df->disconnected_f_op->release(inode, file); + } panic("%s(%p, %p) failed!", __FUNCTION__, inode, file); } diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index a0ead36..265f764 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2179,7 +2179,6 @@ static int snd_pcm_release(struct inode *inode, struct file *file) substream = pcm_file->substream; snd_assert(substream != NULL, return -ENXIO); pcm = substream->pcm; - fasync_helper(-1, file, 0, &substream->runtime->fasync); mutex_lock(&pcm->open_mutex); snd_pcm_release_substream(substream); kfree(pcm_file); diff --git a/sound/core/timer.c b/sound/core/timer.c index 160e40e..14a51d6 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1250,7 +1250,6 @@ static int snd_timer_user_release(struct inode *inode, struct file *file) if (file->private_data) { tu = file->private_data; file->private_data = NULL; - fasync_helper(-1, file, 0, &tu->fasync); if (tu->timeri) snd_timer_close(tu->timeri); kfree(tu->queue);