ubifs: atomically set inode->i_flags in ubifs_set_inode_flags()

PATCH In review Zhang Zhen 2 participants 3 messages Apr 15, 2014 – Apr 16, 2014 1 message not in the archive

Subsystems (2)

Memory technology devices (MTD) UBI file system (UBIFS)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Zhang Zhen

author

2014-04-15 06:16 UTC #1 lore raw

PATCH 1/1 ubifs: atomically set inode->i_flags in ubifs_set_inode_flags()

Use set_mask_bits() to atomically set i_flags instead of clearing out the S_IMMUTABLE, S_APPEND, etc. flags and then setting them from the UBIFS_IMMUTABLE, UBIFS_APPEND_FL, etc. flags, since this opens up a race where an immutable file has the immutable flag cleared for a brief window of time.

Signed-off-by Zhang Zhen <[email protected]>

---
 fs/ubifs/ioctl.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

fs/ubifs/ioctl.c +8 -5

index 648b143..5d6df97 100644
@@ -27,6 +27,7 @@
 #include <linux/compat.h>
 #include <linux/mount.h>
 #include "ubifs.h"
+#include <linux/bitops.h>
 
 /**
  * ubifs_set_inode_flags - set VFS inode flags.

@@ -37,16 +38,18 @@
 void ubifs_set_inode_flags(struct inode *inode)
 {
 	unsigned int flags = ubifs_inode(inode)->flags;
+	unsigned int new_fl = 0;
 
-	inode->i_flags &= ~(S_SYNC | S_APPEND | S_IMMUTABLE | S_DIRSYNC);
 	if (flags & UBIFS_SYNC_FL)
-		inode->i_flags |= S_SYNC;
+		new_fl |= S_SYNC;
 	if (flags & UBIFS_APPEND_FL)
-		inode->i_flags |= S_APPEND;
+		new_fl |= S_APPEND;
 	if (flags & UBIFS_IMMUTABLE_FL)
-		inode->i_flags |= S_IMMUTABLE;
+		new_fl |= S_IMMUTABLE;
 	if (flags & UBIFS_DIRSYNC_FL)
-		inode->i_flags |= S_DIRSYNC;
+		new_fl |= S_DIRSYNC;
+	set_mask_bits(&inode->i_flags,
+			S_SYNC|S_APPEND|S_IMMUTABLE|S_DIRSYNC, new_fl);
 }
 
 /*

Signature · 4 lines

1.8.5.5


.

Artem Bityutskiy

2014-04-15 13:31 UTC #2 lore raw

Quoted 7 lines · Zhang Zhen

Use set_mask_bits() to atomically set i_flags instead of clearing out the S_IMMUTABLE, S_APPEND, etc. flags and then setting them from the UBIFS_IMMUTABLE, UBIFS_APPEND_FL, etc. flags, since this opens up a race where an immutable file has the immutable flag cleared for a brief window of time.
Signed-off-by Zhang Zhen <[email protected]>

Looks correct to me. Does it fix a real-life bug or just a hypothetical issue? Also, did you test it?

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Zhang Zhen

author

2014-04-16 02:10 UTC #3 lore raw

Quoted 7 lines · Artem Bityutskiy

Quoted 6 lines · Zhang Zhen
Use set_mask_bits() to atomically set i_flags instead of clearing out the S_IMMUTABLE, S_APPEND, etc. flags and then setting them from the UBIFS_IMMUTABLE, UBIFS_APPEND_FL, etc. flags, since this opens up a race where an immutable file has the immutable flag cleared for a brief window of time.

Quoted 5 lines

Quoted 1 line
Signed-off-by Zhang Zhen <[email protected]>
Looks correct to me. Does it fix a real-life bug or just a hypothetical issue? Also, did you test it?

Hi,

It's just a hypothetical issue.

I noticed the commit 00a1a053ebe5febcfc2ec498bd894f035ad2aa06 added the function set_mask_bits() to atomically set i_flags, and it is also useful to UBIFS. So i fix it.

Sorry, Because of lacking of test board, i can only guarantee that it has no compile error.

Best regards!