From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f174.google.com (mail-wi0-f174.google.com [209.85.212.174]) by dpdk.org (Postfix) with ESMTP id EB12A569C for ; Fri, 20 Mar 2015 16:57:30 +0100 (CET) Received: by wibg7 with SMTP id g7so28894911wib.1 for ; Fri, 20 Mar 2015 08:57:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mirantis.com; s=google; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=ijBfzzXsvVvbBgCVs4Ey9Byudya6md5B4PRyMXAFb/I=; b=XvUEwbUmYPZUkOk2U8mG8fnuSdV48aEejC4ip2jGCE75OBNUkGAxA06supqcQQkKIA bHL0bilJz9i2R4v2NXYSkQ1ImlkkXbIqMl1N6M0wKMjHUBvdTYDej6q6XJL6gM5g9DkA IcleX5yKNQc2aYHFIjccdWI4MUauE9lbEuQsk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=ijBfzzXsvVvbBgCVs4Ey9Byudya6md5B4PRyMXAFb/I=; b=F8nuKDoWBhK1XgL6hkITkyRWLp2IpX0NTC3CNzFf00r85fWzd/NC6CVdwY5FBZRcAb YGp6SeJy5EbCbNC+EZWNrPuO3vpYb2Iam+Ug1kCjexWzhxTVgQ/DJkU5F5H69JzwzS17 0sQMmZPcTE9mgOmJkC817OJThkVSssOeJAcze6iMGmiOOdsqgWP6qiJJcdxJ9ELL2+t9 86EJ136hGEtncyKb2hn+jY+lUgGfXAQBJiZZ6alWk8ESLVdVNkQNOVAPBjd5h5Gsxpy2 J//r0BhOAvk4k8sPnlGxWajrExbcQf3/2EWXMbbssicdvZ4mcSoZZwJe3qgNdtUyROLo joHg== X-Gm-Message-State: ALoCoQkaj6y1rYejhkJ7T78vMf1amG/ofG4TEMG5pbK6TVgy8kZEGu2FoqnIGdTe7lUtDiA0mNU6 MIME-Version: 1.0 X-Received: by 10.180.108.81 with SMTP id hi17mr6138900wib.91.1426867050732; Fri, 20 Mar 2015 08:57:30 -0700 (PDT) Received: by 10.194.76.7 with HTTP; Fri, 20 Mar 2015 08:57:30 -0700 (PDT) In-Reply-To: References: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> Date: Fri, 20 Mar 2015 17:57:30 +0200 Message-ID: From: Pavel Boldin To: "Ouyang, Changchun" Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" 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: Fri, 20 Mar 2015 15:57:31 -0000 Changchun, Please review the updated patch. Pavel On Tue, Mar 17, 2015 at 3:29 AM, Ouyang, Changchun < changchun.ouyang@intel.com> wrote: > > > > -----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 > > > > From: Pavel Boldin > > > > The `eventfd_link' module provides an API to "steal" fd from another > process > > had been written with a bug that leaks `struct file' because of the extra > > reference counter increment and missing `fput' call. > > > > The other bug is using another process' `task_struct' without > incrementing a > > reference counter. > > > > Fix these bugs and refactor the module. > > --- > > examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++----- > > -------- > > 1 file changed, 130 insertions(+), 106 deletions(-) > > > > 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; > > > > - task_lock (task); > > + task_lock(task); > > files = 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. > 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 > > > > > > 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 = fcheck_files(files, fd); > > + if (file) > > + { > > + if (file->f_mode & FMODE_PATH > > + || !atomic_long_inc_not_zero(&file->f_count)) > > + file = NULL; > > } > > + rcu_read_unlock(); > > + > > + return file; > > +} > > + > > +static int > > +close_fd(unsigned fd) > > +{ > > + struct file *file; > > + struct files_struct *files = current->files; > > + struct fdtable *fdt; > > + > > + spin_lock(&files->file_lock); > > + fdt = files_fdtable(files); > > + if (fd >= fdt->max_fds) > > + goto out_unlock; > > + file = 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; > > } > > > > > > static long > > -eventfd_link_ioctl (struct file *f, unsigned int ioctl, unsigned long > arg) > > +eventfd_link_ioctl_copy(unsigned long arg) > > { > > - void __user *argp = (void __user *) arg; > > + long ret = -EFAULT; > > struct task_struct *task_target = NULL; > > - struct file *file; > > - struct files_struct *files; > > - struct fdtable *fdt; > > + struct file *target_file = NULL; > > + struct files_struct *target_files = 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 = find_vpid(eventfd_copy.target_pid); > > + if (pid == NULL) { > > + printk(KERN_INFO "Unable to find pid %d\n", > > + eventfd_copy.target_pid); > > + goto out; > > + } > > + > > + task_target = get_pid_task(pid, PIDTYPE_PID); > > + if (task_target == NULL) { > > + printk(KERN_INFO "Failed to get task for pid %d\n", > > + eventfd_copy.target_pid); > > + goto out; > > + } > > + > > + ret = close_fd(eventfd_copy.source_fd); > > + if (ret) > > + goto out_task; > > + ret = -EFAULT; > > + > > + /* > > + * Find the file struct associated with the target fd. > > + */ > > + > > + target_files = get_files_struct(task_target); > > + if (target_files == NULL) { > > + printk (KERN_INFO "Failed to get target files struct\n"); > > + goto out_task; > > + } > > + > > + target_file = fget_from_files(target_files, > eventfd_copy.target_fd); > > + > > + if (target_file == 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 = 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; > > > > 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 = > > - pid_task (find_vpid > > (eventfd_copy.target_pid), PIDTYPE_PID); > > - if (task_target == NULL) > > - { > > - printk (KERN_DEBUG "Failed to get mem ctx > > for target pid\n"); > > - return -EFAULT; > > - } > > - > > - files = get_files_struct (current); > > - if (files == NULL) > > - { > > - printk (KERN_DEBUG "Failed to get files > > struct\n"); > > - return -EFAULT; > > - } > > - > > - rcu_read_lock (); > > - file = fcheck_files (files, > eventfd_copy.source_fd); > > - if (file) > > - { > > - if (file->f_mode & FMODE_PATH > > - > > || !atomic_long_inc_not_zero (&file->f_count)) > > - file = NULL; > > - } > > - rcu_read_unlock (); > > - put_files_struct (files); > > - > > - if (file == 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 = files_fdtable (files); > > - fdt->fd[eventfd_copy.source_fd] = NULL; > > - spin_unlock (&files->file_lock); > > - > > - /* > > - * Find the file struct associated with the target > fd. > > - */ > > - > > - files = get_files_struct (task_target); > > - if (files == NULL) > > - { > > - printk (KERN_DEBUG "Failed to get files > > struct\n"); > > - return -EFAULT; > > - } > > - > > - rcu_read_lock (); > > - file = fcheck_files (files, > eventfd_copy.target_fd); > > - if (file) > > - { > > - if (file->f_mode & FMODE_PATH > > - > > || !atomic_long_inc_not_zero (&file->f_count)) > > - file = NULL; > > - } > > - rcu_read_unlock (); > > - put_files_struct (files); > > - > > - if (file == 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 = eventfd_link_ioctl_copy(arg); > > + break; > > default: > > - return -ENOIOCTLCMD; > > + ret = -ENOIOCTLCMD; > > + break; > > } > > + > > + return ret; > > } > > > > static const struct file_operations eventfd_link_fops = { @@ -184,21 > +209,20 > > @@ static struct miscdevice eventfd_link_misc = { }; > > > > 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); > > } > > > > -module_init (eventfd_link_init); > > +module_init(eventfd_link_init); > > > > static void __exit > > -eventfd_link_exit (void) > > +eventfd_link_exit(void) > > { > > - misc_deregister (&eventfd_link_misc); > > + misc_deregister(&eventfd_link_misc); > > } > > > > -module_exit (eventfd_link_exit); > > +module_exit(eventfd_link_exit); > > > > MODULE_VERSION ("0.0.1"); > > MODULE_LICENSE ("GPL v2"); > > -- > > 1.9.1 > >