Re: [PATCH] eCryptfs: Privileged kthread for lower file opens

[Andrea Righi <righiandr@users.sourceforge.net>]

Michael Halcrow wrote:
[snip]

Hi,
2 comments on the patch.

> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
> new file mode 100644
> index 0000000..dba7c67
> --- /dev/null
> +++ b/fs/ecryptfs/kthread.c
> @@ -0,0 +1,211 @@
> +/**
> + * eCryptfs: Linux filesystem encryption layer
> + *
> + * Copyright (C) 2008 International Business Machines Corp.
> + *   Author(s): Michael A. Halcrow <mahalcro@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
> + * 02111-1307, USA.
> + */
> +
> +#include <linux/kthread.h>
> +#include <linux/freezer.h>
> +#include <linux/wait.h>
> +#include <linux/mount.h>
> +#include "ecryptfs_kernel.h"
> +
> +struct task_struct *ecryptfs_kthread;
> +struct ecryptfs_kthread_ctl ecryptfs_kthread_ctl;
> +
> +struct kmem_cache *ecryptfs_open_req_cache;
> +
> +/**
> + * ecryptfs_threadfn
> + * @ignored: ignored
> + *
> + * The eCryptfs kernel thread that has the responsibility of getting
> + * the lower persistent file with RW permissions.
> + *
> + * Returns zero on success; non-zero otherwise
> + */
> +static int ecryptfs_threadfn(void *ignored)
> +{
> +	set_freezable();
> +	while (1)  {
> +		struct ecryptfs_open_req *req;
> +
> +		wait_event_freezable(
> +			ecryptfs_kthread_ctl.wait,
> +			!list_empty(&ecryptfs_kthread_ctl.req_list));
> +		mutex_lock(&ecryptfs_kthread_ctl.mux);
> +		if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) {
> +			mutex_unlock(&ecryptfs_kthread_ctl.mux);
> +			goto out;
> +		}
> +		while (!list_empty(&ecryptfs_kthread_ctl.req_list)) {
> +			req = list_first_entry(&ecryptfs_kthread_ctl.req_list,
> +					       struct ecryptfs_open_req,
> +					       kthread_ctl_list);
> +			BUG_ON(!req);
> +			mutex_lock(&req->mux);
> +			list_del(&req->kthread_ctl_list);
> +			if (req->flags & ECRYPTFS_REQ_ZOMBIE)
> +				mutex_unlock(&req->mux);
> +			else {
> +				dget(req->lower_dentry);
> +				mntget(req->lower_mnt);
> +				(*req->lower_file) = dentry_open(
> +					req->lower_dentry, req->lower_mnt,
> +					(O_RDWR | O_LARGEFILE));
> +				req->flags |= ECRYPTFS_REQ_PROCESSED;
> +				wake_up_process(req->requesting_task);
> +				mutex_unlock(&req->mux);
> +			}

Why not:
			if (!(req->flags & ECRYPTFS_REQ_ZOMBIE))
				dget(req->lower_dentry);
				mntget(req->lower_mnt);
				(*req->lower_file) = dentry_open(
					req->lower_dentry, req->lower_mnt,
					(O_RDWR | O_LARGEFILE));
				req->flags |= ECRYPTFS_REQ_PROCESSED;
				wake_up_process(req->requesting_task);
			}
			mutex_unlock(&req->mux);

> +		}
> +		mutex_unlock(&ecryptfs_kthread_ctl.mux);
> +	}
> +out:
> +	do_exit(0);
> +}
> +
> +int ecryptfs_init_kthread(void)
> +{
> +	int rc = 0;
> +
> +	mutex_init(&ecryptfs_kthread_ctl.mux);
> +	init_waitqueue_head(&ecryptfs_kthread_ctl.wait);
> +	INIT_LIST_HEAD(&ecryptfs_kthread_ctl.req_list);
> +	ecryptfs_kthread = kthread_create(&ecryptfs_threadfn, NULL,
> +					  "ecryptfs-kthread");
> +	if (IS_ERR(ecryptfs_kthread)) {
> +		rc = PTR_ERR(ecryptfs_kthread);
> +		printk(KERN_ERR "%s: Failed to create kernel thread; rc = [%d]"
> +		       "\n", __func__, rc);
> +	}
> +	wake_up_process(ecryptfs_kthread);
> +	return rc;
> +}
> +
> +void ecryptfs_destroy_kthread(void)
> +{
> +	struct ecryptfs_open_req tmp_req;
> +	struct ecryptfs_open_req *req;
> +
> +	mutex_lock(&ecryptfs_kthread_ctl.mux);
> +	ecryptfs_kthread_ctl.flags |= ECRYPTFS_KTHREAD_ZOMBIE;
> +	list_for_each_entry(req, &ecryptfs_kthread_ctl.req_list,
> +			    kthread_ctl_list) {
> +		mutex_lock(&req->mux);
> +		req->flags |= ECRYPTFS_REQ_ZOMBIE;
> +		wake_up_process(req->requesting_task);
> +		mutex_unlock(&req->mux);
> +	}
> +	memset(&tmp_req, 0, sizeof(tmp_req));
> +	tmp_req.flags = ECRYPTFS_REQ_ZOMBIE;
> +	list_add_tail(&tmp_req.kthread_ctl_list,
> +		      &ecryptfs_kthread_ctl.req_list);
> +	mutex_unlock(&ecryptfs_kthread_ctl.mux);
> +	wake_up(&ecryptfs_kthread_ctl.wait);
> +}
> +
> +/**
> + * ecryptfs_privileged_open
> + * @lower_file: Result of dentry_open by root on lower dentry
> + * @lower_dentry: Lower dentry for file to open
> + * @lower_mnt: Lower vfsmount for file to open
> + *
> + * This function gets a r/w file opened againt the lower dentry.
> + *
> + * Returns zero on success; non-zero otherwise
> + */
> +int ecryptfs_privileged_open(struct file **lower_file,
> +			     struct dentry *lower_dentry,
> +			     struct vfsmount *lower_mnt)
> +{
> +	struct ecryptfs_open_req *req;
> +	int rc = 0;
> +
> +	/* Corresponding dput() and mntput() are done when the
> +	 * persistent file is fput() when the eCryptfs inode is
> +	 * destroyed. */
> +	dget(lower_dentry);
> +	mntget(lower_mnt);
> +	(*lower_file) = dentry_open(lower_dentry, lower_mnt,
> +				    (O_RDWR | O_LARGEFILE));
> +	if (!IS_ERR(*lower_file))
> +		goto out;
> +	req = kmem_cache_alloc(ecryptfs_open_req_cache, GFP_KERNEL);
> +	if (!req) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +	mutex_init(&req->mux);
> +	req->lower_file = lower_file;
> +	req->lower_dentry = lower_dentry;
> +	req->lower_mnt = lower_mnt;
> +	req->requesting_task = current;
> +	req->flags = 0;
> +	mutex_lock(&ecryptfs_kthread_ctl.mux);
> +	mutex_lock(&req->mux);
> +	if (ecryptfs_kthread_ctl.flags & ECRYPTFS_KTHREAD_ZOMBIE) {
> +		rc = -EIO;

Isn't a mutex_unlock(&req->mux) missing here?

> +		mutex_unlock(&ecryptfs_kthread_ctl.mux);
> +		printk(KERN_ERR "%s: We are in the middle of shutting down; "
> +		       "aborting privileged request to open lower file\n",
> +			__func__);
> +		goto out_free;
> +	}
> +	list_add_tail(&req->kthread_ctl_list, &ecryptfs_kthread_ctl.req_list);
> +	mutex_unlock(&req->mux);
> +	mutex_unlock(&ecryptfs_kthread_ctl.mux);
> +	wake_up(&ecryptfs_kthread_ctl.wait);
> +schedule:
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	schedule();
> +	mutex_lock(&req->mux);
> +	if (req->flags == 0) {
> +		mutex_unlock(&req->mux);
> +		goto schedule;
> +	}
> +	set_current_state(TASK_RUNNING);
> +	if (req->flags & ECRYPTFS_REQ_DROPPED
> +	    || req->flags & ECRYPTFS_REQ_ZOMBIE) {
> +		rc = -EIO;
> +		printk(KERN_WARNING "%s: Privileged open request dropped\n",
> +		       __func__);
> +		goto out_unlock;
> +	}
> +	if (IS_ERR(*req->lower_file)) {
> +		rc = PTR_ERR(*req->lower_file);
> +		dget(lower_dentry);
> +		mntget(lower_mnt);
> +		(*lower_file) = dentry_open(lower_dentry, lower_mnt,
> +					    (O_RDONLY | O_LARGEFILE));
> +		if (IS_ERR(*lower_file)) {
> +			rc = PTR_ERR(*req->lower_file);
> +			(*lower_file) = NULL;
> +			printk(KERN_WARNING "%s: Error attempting privileged "
> +			       "open of lower file with either RW or RO "
> +			       "perms; rc = [%d]. Giving up.\n",
> +			       __func__, rc);
> +		}
> +	}
> +out_unlock:
> +	mutex_unlock(&req->mux);
> +out_free:
> +	kmem_cache_free(ecryptfs_open_req_cache, req);
> +out:
> +	return rc;
> +}