| Message ID | 10CFECF7-B9FC-4562-A445-4811F4C27655@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | tracefs: avoid setting i_mode to a temp value | expand |
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))
> 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))
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
> 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 >
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))
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(-)