From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id A91175A84 for ; Tue, 17 Mar 2015 02:29:59 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 16 Mar 2015 18:27:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,412,1422950400"; d="scan'208";a="699643442" Received: from kmsmsx151.gar.corp.intel.com ([172.21.73.86]) by orsmga002.jf.intel.com with ESMTP; 16 Mar 2015 18:29:58 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by KMSMSX151.gar.corp.intel.com (172.21.73.86) with Microsoft SMTP Server (TLS) id 14.3.224.2; Tue, 17 Mar 2015 09:29:56 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.198]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.108]) with mapi id 14.03.0224.002; Tue, 17 Mar 2015 09:29:55 +0800 From: "Ouyang, Changchun" To: Pavel Boldin , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races Thread-Index: AQHQYAwOSn2b6/Q75UK5DV0U+x5hn50f4t+A Date: Tue, 17 Mar 2015 01:29:54 +0000 Message-ID: References: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> In-Reply-To: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Mar 2015 01:30:00 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pavel Boldin > Sent: Tuesday, March 17, 2015 12:41 AM > To: dev@dpdk.org > Cc: Pavel Boldin > Subject: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races >=20 > From: Pavel Boldin >=20 > The `eventfd_link' module provides an API to "steal" fd from another proc= ess > had been written with a bug that leaks `struct file' because of the extra > reference counter increment and missing `fput' call. >=20 > The other bug is using another process' `task_struct' without incrementin= g a > reference counter. >=20 > Fix these bugs and refactor the module. > --- > examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++----- > -------- > 1 file changed, 130 insertions(+), 106 deletions(-) >=20 > diff --git a/examples/vhost/eventfd_link/eventfd_link.c > b/examples/vhost/eventfd_link/eventfd_link.c > index 69470ba..9f1f8fb 100644 > --- a/examples/vhost/eventfd_link/eventfd_link.c > +++ b/examples/vhost/eventfd_link/eventfd_link.c > @@ -42,15 +42,15 @@ > * get_files_struct is copied from fs/file.c > */ > struct files_struct * > -get_files_struct (struct task_struct *task) > +get_files_struct(struct task_struct *task) > { > struct files_struct *files; >=20 > - task_lock (task); > + task_lock(task); > files =3D task->files; > if (files) > - atomic_inc (&files->count); > - task_unlock (task); > + atomic_inc(&files->count); > + task_unlock(task); I don't find any actual code change for above.=20 If they are tab, space, indent refine, I suggest it needs a separate patch named code cleanup, Then it is easy to review. Same for other places. Done. Changchun >=20 > return files; > } > @@ -59,117 +59,142 @@ get_files_struct (struct task_struct *task) > * put_files_struct is extracted from fs/file.c > */ > void > -put_files_struct (struct files_struct *files) > +put_files_struct(struct files_struct *files) > { > - if (atomic_dec_and_test (&files->count)) > - { > + if (atomic_dec_and_test(&files->count)) > BUG (); > +} > + > +static struct file * > +fget_from_files(struct files_struct *files, unsigned fd) { > + struct file *file; > + > + rcu_read_lock(); > + file =3D fcheck_files(files, fd); > + if (file) > + { > + if (file->f_mode & FMODE_PATH > + || !atomic_long_inc_not_zero(&file->f_count)) > + file =3D NULL; > } > + rcu_read_unlock(); > + > + return file; > +} > + > +static int > +close_fd(unsigned fd) > +{ > + struct file *file; > + struct files_struct *files =3D current->files; > + struct fdtable *fdt; > + > + spin_lock(&files->file_lock); > + fdt =3D files_fdtable(files); > + if (fd >=3D fdt->max_fds) > + goto out_unlock; > + file =3D fdt->fd[fd]; > + if (!file) > + goto out_unlock; > + rcu_assign_pointer(fdt->fd[fd], NULL); > + __clear_bit(fd, fdt->close_on_exec); > + spin_unlock(&files->file_lock); > + return filp_close(file, files); > + > +out_unlock: > + spin_unlock(&files->file_lock); > + return -EBADF; > } >=20 >=20 > static long > -eventfd_link_ioctl (struct file *f, unsigned int ioctl, unsigned long ar= g) > +eventfd_link_ioctl_copy(unsigned long arg) > { > - void __user *argp =3D (void __user *) arg; > + long ret =3D -EFAULT; > struct task_struct *task_target =3D NULL; > - struct file *file; > - struct files_struct *files; > - struct fdtable *fdt; > + struct file *target_file =3D NULL; > + struct files_struct *target_files =3D NULL; > struct eventfd_copy eventfd_copy; > + struct pid *pid; > + > + if (copy_from_user(&eventfd_copy, (void __user*)arg, > + sizeof(struct eventfd_copy))) > + goto out; > + > + /* > + * Find the task struct for the target pid > + */ > + pid =3D find_vpid(eventfd_copy.target_pid); > + if (pid =3D=3D NULL) { > + printk(KERN_INFO "Unable to find pid %d\n", > + eventfd_copy.target_pid); > + goto out; > + } > + > + task_target =3D get_pid_task(pid, PIDTYPE_PID); > + if (task_target =3D=3D NULL) { > + printk(KERN_INFO "Failed to get task for pid %d\n", > + eventfd_copy.target_pid); > + goto out; > + } > + > + ret =3D close_fd(eventfd_copy.source_fd); > + if (ret) > + goto out_task; > + ret =3D -EFAULT; > + > + /* > + * Find the file struct associated with the target fd. > + */ > + > + target_files =3D get_files_struct(task_target); > + if (target_files =3D=3D NULL) { > + printk (KERN_INFO "Failed to get target files struct\n"); > + goto out_task; > + } > + > + target_file =3D fget_from_files(target_files, eventfd_copy.target_fd); > + > + if (target_file =3D=3D NULL) { > + printk (KERN_INFO "Failed to get file from target pid\n"); > + goto out_target_files; > + } > + > + > + /* > + * Install the file struct from the target process into the > + * file desciptor of the source process, > + */ > + > + fd_install(eventfd_copy.source_fd, target_file); > + > + ret =3D 0; > + > +out_target_files: > + put_files_struct(target_files); > +out_task: > + put_task_struct(task_target); > +out: > + return ret; > +} > + > +static long > +eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long > +arg) { > + long ret; >=20 > switch (ioctl) > { > case EVENTFD_COPY: > - if (copy_from_user (&eventfd_copy, argp, sizeof > (struct eventfd_copy))) > - return -EFAULT; > - > - /* > - * Find the task struct for the target pid > - */ > - task_target =3D > - pid_task (find_vpid > (eventfd_copy.target_pid), PIDTYPE_PID); > - if (task_target =3D=3D NULL) > - { > - printk (KERN_DEBUG "Failed to get mem ctx > for target pid\n"); > - return -EFAULT; > - } > - > - files =3D get_files_struct (current); > - if (files =3D=3D NULL) > - { > - printk (KERN_DEBUG "Failed to get files > struct\n"); > - return -EFAULT; > - } > - > - rcu_read_lock (); > - file =3D fcheck_files (files, eventfd_copy.source_fd); > - if (file) > - { > - if (file->f_mode & FMODE_PATH > - > || !atomic_long_inc_not_zero (&file->f_count)) > - file =3D NULL; > - } > - rcu_read_unlock (); > - put_files_struct (files); > - > - if (file =3D=3D NULL) > - { > - printk (KERN_DEBUG "Failed to get file from > source pid\n"); > - return 0; > - } > - > - /* > - * Release the existing eventfd in the source process > - */ > - spin_lock (&files->file_lock); > - fput(file); > - filp_close (file, files); > - fdt =3D files_fdtable (files); > - fdt->fd[eventfd_copy.source_fd] =3D NULL; > - spin_unlock (&files->file_lock); > - > - /* > - * Find the file struct associated with the target fd. > - */ > - > - files =3D get_files_struct (task_target); > - if (files =3D=3D NULL) > - { > - printk (KERN_DEBUG "Failed to get files > struct\n"); > - return -EFAULT; > - } > - > - rcu_read_lock (); > - file =3D fcheck_files (files, eventfd_copy.target_fd); > - if (file) > - { > - if (file->f_mode & FMODE_PATH > - > || !atomic_long_inc_not_zero (&file->f_count)) > - file =3D NULL; > - } > - rcu_read_unlock (); > - put_files_struct (files); > - > - if (file =3D=3D NULL) > - { > - printk (KERN_DEBUG "Failed to get file from > target pid\n"); > - return 0; > - } > - > - > - /* > - * Install the file struct from the target process into > the > - * file desciptor of the source process, > - */ > - > - fd_install (eventfd_copy.source_fd, file); > - > - return 0; > - > + ret =3D eventfd_link_ioctl_copy(arg); > + break; > default: > - return -ENOIOCTLCMD; > + ret =3D -ENOIOCTLCMD; > + break; > } > + > + return ret; > } >=20 > static const struct file_operations eventfd_link_fops =3D { @@ -184,21 += 209,20 > @@ static struct miscdevice eventfd_link_misc =3D { }; >=20 > static int __init > -eventfd_link_init (void) > +eventfd_link_init(void) > { > - printk(KERN_INFO "eventfd_link is broken, use it at your risk\n"); > - return misc_register (&eventfd_link_misc); > + return misc_register(&eventfd_link_misc); > } >=20 > -module_init (eventfd_link_init); > +module_init(eventfd_link_init); >=20 > static void __exit > -eventfd_link_exit (void) > +eventfd_link_exit(void) > { > - misc_deregister (&eventfd_link_misc); > + misc_deregister(&eventfd_link_misc); > } >=20 > -module_exit (eventfd_link_exit); > +module_exit(eventfd_link_exit); >=20 > MODULE_VERSION ("0.0.1"); > MODULE_LICENSE ("GPL v2"); > -- > 1.9.1