[PATCH] tracefs: avoid setting i_mode to a temp value

[PATCH] tracefs: avoid setting i_mode to a temp value

[Sishuai Gong]

Right now inode->i_mode is updated twice to reach the desired value
in tracefs_apply_options(). Because there is no lock protecting the two
writes, other threads might read the intermediate value of inode->i_mode.

Thread-1			Thread-2
// tracefs_apply_options()	//e.g., acl_permission_check
inode->i_mode &= ~S_IALLUGO;
				unsigned int mode = inode->i_mode;
inode->i_mode |= opts->mode;

I think there is no need to introduce a lock but it is better to
only update inode->i_mode ONCE, so the readers will either see the old
or latest value, rather than an intermediate/temporary value.

Signed-off-by: Sishuai Gong <sishuai.system@gmail.com>
---
 fs/tracefs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 57ac8aa4a724..dca84ebb62fa 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -297,8 +297,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
 	 */
 
 	if (!remount || opts->opts & BIT(Opt_mode)) {
-		inode->i_mode &= ~S_IALLUGO;
-		inode->i_mode |= opts->mode;
+		inode->i_mode = (inode->i_mode & ~S_IALLUGO) | opts->mode;
 	}
 
 	if (!remount || opts->opts & BIT(Opt_uid))
-- 
2.39.2 (Apple Git-143)

Re: [PATCH] tracefs: avoid setting i_mode to a temp value

[Steven Rostedt]

On Thu, 10 Aug 2023 20:59:26 -0400
Sishuai Gong <sishuai.system@gmail.com> wrote:

> Right now inode->i_mode is updated twice to reach the desired value
> in tracefs_apply_options(). Because there is no lock protecting the two
> writes, other threads might read the intermediate value of inode->i_mode.
> 
> Thread-1			Thread-2
> // tracefs_apply_options()	//e.g., acl_permission_check
> inode->i_mode &= ~S_IALLUGO;
> 				unsigned int mode = inode->i_mode;
> inode->i_mode |= opts->mode;
> 
> I think there is no need to introduce a lock but it is better to
> only update inode->i_mode ONCE, so the readers will either see the old
> or latest value, rather than an intermediate/temporary value.
> 
> Signed-off-by: Sishuai Gong <sishuai.system@gmail.com>
> ---
>  fs/tracefs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index 57ac8aa4a724..dca84ebb62fa 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -297,8 +297,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
>  	 */
>  
>  	if (!remount || opts->opts & BIT(Opt_mode)) {
> -		inode->i_mode &= ~S_IALLUGO;
> -		inode->i_mode |= opts->mode;
> +		inode->i_mode = (inode->i_mode & ~S_IALLUGO) | opts->mode;

You do realize that the compiler could decide to keep the original logic
even with this change? If it was crucial that the compiler did not, you
would need to have:

	if (!remount || opts->opts & BIT(Opt_mode)) {
		umode_t tmp = READ_ONCE(inode->i_mode);

		tmp &= ~S_IALLUGO
		tmp |= opts->mode;
		WRITE_ONCE(inode->i_mode, tmp);
	}		

And if you notice the !remount flag, this is only preformed when the file
system is actually mounted. Are the files visible before then?

Can you produce this race?

-- Steve



>  	}
>  
>  	if (!remount || opts->opts & BIT(Opt_uid))

Re: [PATCH] tracefs: avoid setting i_mode to a temp value

[Sishuai Gong]

> On Aug 16, 2023, at 3:52 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 10 Aug 2023 20:59:26 -0400
> Sishuai Gong <sishuai.system@gmail.com> wrote:
> 
>> Right now inode->i_mode is updated twice to reach the desired value
>> in tracefs_apply_options(). Because there is no lock protecting the two
>> writes, other threads might read the intermediate value of inode->i_mode.
>> 
>> Thread-1 Thread-2
>> // tracefs_apply_options() //e.g., acl_permission_check
>> inode->i_mode &= ~S_IALLUGO;
>> unsigned int mode = inode->i_mode;
>> inode->i_mode |= opts->mode;
>> 
>> I think there is no need to introduce a lock but it is better to
>> only update inode->i_mode ONCE, so the readers will either see the old
>> or latest value, rather than an intermediate/temporary value.
>> 
>> Signed-off-by: Sishuai Gong <sishuai.system@gmail.com>
>> ---
>> fs/tracefs/inode.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
>> index 57ac8aa4a724..dca84ebb62fa 100644
>> --- a/fs/tracefs/inode.c
>> +++ b/fs/tracefs/inode.c
>> @@ -297,8 +297,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
>>  */
>> 
>> if (!remount || opts->opts & BIT(Opt_mode)) {
>> - inode->i_mode &= ~S_IALLUGO;
>> - inode->i_mode |= opts->mode;
>> + inode->i_mode = (inode->i_mode & ~S_IALLUGO) | opts->mode;
> 
> You do realize that the compiler could decide to keep the original logic
> even with this change? If it was crucial that the compiler did not, you
> would need to have:
> 
> if (!remount || opts->opts & BIT(Opt_mode)) {
> umode_t tmp = READ_ONCE(inode->i_mode);
> 
> tmp &= ~S_IALLUGO
> tmp |= opts->mode;
> WRITE_ONCE(inode->i_mode, tmp);
> } 
> 
You are right. This will prevent the compiler from emitting two writes.
I will incorporate your suggestion in the new version.

> And if you notice the !remount flag, this is only preformed when the file
> system is actually mounted. Are the files visible before then?
> 
> Can you produce this race?
This data race was detected when I was testing the kernel (e.g., fuzzing)
but I did not make the attempt to reproduce it.

> 
> -- Steve
> 
> 
> 
>> }
>> 
>> if (!remount || opts->opts & BIT(Opt_uid))

Re: [PATCH] tracefs: avoid setting i_mode to a temp value

[Steven Rostedt]

On Thu, 17 Aug 2023 19:47:34 -0400
Sishuai Gong <sishuai.system@gmail.com> wrote:

> > Can you produce this race?  
> This data race was detected when I was testing the kernel (e.g., fuzzing)
> but I did not make the attempt to reproduce it.

Now, I'm curious to what exactly is this fixing? The intermediate value is
the S_IALLUGO bits cleared. Doesn't that mean that nothing has permission?

It's not a big deal if that's the case, as it just means things are locked
down a bit more than normal.

My question is, do we really care, and why should we?

-- Steve
 

Re: [PATCH] tracefs: avoid setting i_mode to a temp value

[Sishuai Gong]

> On Aug 17, 2023, at 8:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 17 Aug 2023 19:47:34 -0400
> Sishuai Gong <sishuai.system@gmail.com> wrote:
> 
>>> Can you produce this race?  
>> This data race was detected when I was testing the kernel (e.g., fuzzing)
>> but I did not make the attempt to reproduce it.
> 
> Now, I'm curious to what exactly is this fixing? The intermediate value is
> the S_IALLUGO bits cleared. Doesn't that mean that nothing has permission?
> 
> It's not a big deal if that's the case, as it just means things are locked
> down a bit more than normal.
You are right. Even if the intermediate value is read, it is unlikely to cause anything
serious. The reader I observed is acl_permission_check(), which will not be affected
by the race.

> 
> My question is, do we really care, and why should we?
This shouldn’t be a serious problem. Maybe we could consider this patch as an
annotation to the race.

> 
> -- Steve
>