From: Nicolai Stange <nicstange@gmail.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Nicolai Stange <nicstange@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/8] debugfs: prevent access to possibly dead file_operations at file open
References: <1457267103-28910-1-git-send-email-nicstange@gmail.com>
	<1457267103-28910-2-git-send-email-nicstange@gmail.com>
	<87d1r2uh95.fsf@rasmusvillemoes.dk>
Date: Fri, 11 Mar 2016 20:28:59 +0100
In-Reply-To: <87d1r2uh95.fsf@rasmusvillemoes.dk> (Rasmus Villemoes's message
	of "Thu, 10 Mar 2016 22:59:02 +0100")
Message-ID: <87ziu4ak5g.fsf@gmail.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.92 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1366
Lines: 48

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> On Sun, Mar 06 2016, Nicolai Stange <nicstange@gmail.com> wrote:
>
>> + *
>> + * Allow any ongoing concurrent call into debugfs_remove() or
>> + * debugfs_remove_recursive() blocked by a former call to
>> + * debugfs_use_file_start() to proceed and return to its caller.
>> + */
>> +static void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
>> +{
>> +	srcu_read_unlock(&debugfs_srcu, srcu_idx);
>> +}
>> +
>> +#define F_DENTRY(filp) ((filp)->f_path.dentry)
>> +
>> +#define REAL_FOPS_DEREF(dentry)					\
>> +	((const struct file_operations *)(dentry)->d_fsdata)
>> +
>> +static int open_proxy_open(struct inode *inode, struct file *filp)
>> +{
>> +	const struct dentry *dentry = F_DENTRY(filp);
>> +	const struct file_operations *real_fops = NULL;
>> +	int srcu_idx, r;
>> +
>> +	r = debugfs_use_file_start(dentry, &srcu_idx);
>> +	if (r) {
>> +		debugfs_use_file_finish(srcu_idx);
>> +		r = -ENOENT;
>> +		goto out;
>
> this...
>
>> +out:
>> +	fops_put(real_fops);
>> +	debugfs_use_file_finish(srcu_idx);
>
> ... and that seems like an obvious double unlock?

Yes, it is. Thank you so much for reviewing and finding this!

Since v4 of this series has been applied to the driver-core tree's
driver-core-testing branch already, I'll send a patch addressing this
issue tomorrow.

Best,

Nicolai