| Message ID | 534CCE74.9090306@huawei.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, 2014-04-15 at 14:15 +0800, Zhang Zhen wrote: > 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 <zhenzhang.zhang@huawei.com> Looks correct to me. Does it fix a real-life bug or just a hypothetical issue? Also, did you test it?
On 2014/4/15 21:31, Artem Bityutskiy wrote: > On Tue, 2014-04-15 at 14:15 +0800, Zhang Zhen wrote: >> 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 <zhenzhang.zhang@huawei.com> > > 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!
diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c index 648b143..5d6df97 100644 --- a/fs/ubifs/ioctl.c +++ b/fs/ubifs/ioctl.c @@ -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); } /*
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 <zhenzhang.zhang@huawei.com> --- fs/ubifs/ioctl.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)