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

[Michael Halcrow <mhalcrow@us.ibm.com>]

On Wed, May 21, 2008 at 01:16:15AM +0200, Andrea Righi wrote:
> Michael Halcrow wrote:
>> +	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?

Nobody else actually has a reference to req at this point, so the
unlock is not really necessary. It might not be a bad idea to jump to
out_unlock anyway just to avoid future bugs.

>> +		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;
>> +}