Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1d174049-0eaf-01d5-2f85-c3cd3f4a40a0@opensource.cirrus.com>
Date:   Tue, 16 May 2023 18:50:16 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH 3/5] debugfs: Update debugfs_create_str() kerneldoc to
 warn about pointer race
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     <rafael@kernel.org>, <linux-kernel@vger.kernel.org>,
        <patches@opensource.cirrus.com>
References: <20230516160753.32317-1-rf@opensource.cirrus.com>
 <20230516160753.32317-4-rf@opensource.cirrus.com>
 <2023051607-trial-worrisome-87df@gregkh>
From:   Richard Fitzgerald <rf@opensource.cirrus.com>
In-Reply-To: <2023051607-trial-worrisome-87df@gregkh>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 16/5/23 17:35, Greg KH wrote:
> On Tue, May 16, 2023 at 05:07:51PM +0100, Richard Fitzgerald wrote:
>> Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
>> value must not change after the function returns, because of a race with
>> debugfs_read_file_str() accessing the pointer.
>>
>> The only safe case is a change from NULL to non-NULL because in that case
>> debugfs_read_file_str() will see either the NULL or the valid pointer.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   fs/debugfs/file.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index 0c039a3d9a42..77794871f26d 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
>>    *          directory dentry if set.  If this parameter is %NULL, then the
>>    *          file will be created in the root of the debugfs filesystem.
>>    * @value: a pointer to the variable that the file should read to and write
>> - *         from.
>> + *         from. The char* pointer must not change, except from NULL to
>> + *         non-NULL.
> 
> This feels odd.  Why wouldn't you want to change the string value?  Or
> why would you?

Well, if you _would_ want to change the string value, then the
implementation of debugfs_create_str() is certainly broken and could
only be fixed by involving a shared mutex to protect use of the pointer.

> 
> And why is this one-way transition ok?
> 

This one case happens to be safe because it either sees NULL (which it
handles) or a valid pointer (which is ok). It will not result in using a
stale pointer. This wasn't a deliberate design intent but happens to be
safe, and easily maintainable behavior.

A transition from valid->NULL or old->new isn't safe because the
read function could get the old pointer but racing with that is the
change to the pointer, and so the debugfs code could try to use a
stale pointer.

> Given that this is only used internally, why is it exported?
> 

It isn't only used internally. I found 3 drivers that use it.
But there are no uses internal to debugfs.

I didn't write debugfs_create_str(), I only tried to use it and made
an attempt to fix some problems.

Given the limitations of the basic implementation of
debugfs_create_str() and its file reading function (the lack of
protection against the pointer changing) perhaps drop this chain? Don't
bother fixing it, instead deprecate it for being unsafe?

> thanks,
> 
> greg k-h