linux-kernel - Re: [PATCH v2 08/10] blktrace: add checks for created debugfs files on setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c69b67d1-f887-600b-f3ab-54ab0b7dcb13@acm.org>
Date:   Sun, 19 Apr 2020 16:17:46 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     axboe@...nel.dk, viro@...iv.linux.org.uk,
        gregkh@...uxfoundation.org, rostedt@...dmis.org, mingo@...hat.com,
        jack@...e.cz, ming.lei@...hat.com, nstange@...e.de,
        akpm@...ux-foundation.org, mhocko@...e.com, yukuai3@...wei.com,
        linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 08/10] blktrace: add checks for created debugfs files
 on setup

On 4/19/20 4:05 PM, Luis Chamberlain wrote:
> On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
>> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
>>> Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
>>> select DEBUG_FS, and blktrace exposes an API which userspace uses
>>> relying on certain files created in debugfs. If files are not created
>>> blktrace will not work correctly, so we do want to ensure that a
>>> blktrace setup creates these files properly, and otherwise inform
>>> userspace.
>>>
>>> Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>
>>> ---
>>>    kernel/trace/blktrace.c | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
>>> index 9cc0153849c3..fc32a8665ce8 100644
>>> --- a/kernel/trace/blktrace.c
>>> +++ b/kernel/trace/blktrace.c
>>> @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
>>>    					  struct dentry *dir,
>>>    					  struct blk_trace *bt)
>>>    {
>>> -	int ret = -EIO;
>>> -
>>>    	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
>>>    					       &blk_dropped_fops);
>>> +	if (!bt->dropped_file)
>>> +		return -ENOMEM;
>>>    	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
>>> +	if (!bt->msg_file)
>>> +		return -ENOMEM;
>>>    	bt->rchan = relay_open("trace", dir, buts->buf_size,
>>>    				buts->buf_nr, &blk_relay_callbacks, bt);
>>>    	if (!bt->rchan)
>>> -		return ret;
>>> +		return -EIO;
>>>    	return 0;
>>>    }
>>
>> I should have had a look at this patch before I replied to the previous
>> patch.
>>
>> Do you agree that the following code can be triggered by
>> debugfs_create_file() and also that debugfs_create_file() never returns
>> NULL?
> 
> If debugfs is enabled, and not that we know it is in this blktrace code,
> as we select it, it can return ERR_PTR(-ERROR) if an error occurs.

This is what I found in include/linux/debugfs.h in case debugfs is disabled:

static inline struct dentry *debugfs_create_file(const char *name,
	umode_t mode, struct dentry *parent, void *data,
	const struct file_operations *fops)
{
	return ERR_PTR(-ENODEV);
}

I have not found any code path that can cause debugfs_create_file() to 
return NULL. Did I perhaps overlook something? If not, it's not clear to 
me why the above patch adds checks that check whether 
debugfs_create_file() returns NULL?

Thanks,

Bart.