| Message ID | 20170912103900.8629-1-s.hauer@pengutronix.de |
|---|---|
| State | RFC |
| Headers | show |
| Series | fs: ubifs: Add i_version support | expand |
Sascha, Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer: > This adds i_version support to UBIFS. The inodes i_version is used by > IMA to detect changes to an inode and thus necessary to support IMA on > UBIFS. The i_version is stored in the previously unused space in the > UBIFS inode struct. Unlike in ext4 i_version support is unconditionally > enabled in UBIFS as I saw no reason to make it optional. But we need a new UBIFS feature flag to indicate that this filesystem has valid i_version fields. > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > fs/ubifs/dir.c | 30 +++++++++++++++++++----------- > fs/ubifs/file.c | 5 +++++ > fs/ubifs/journal.c | 3 ++- > fs/ubifs/super.c | 2 ++ > fs/ubifs/ubifs-media.h | 3 ++- > 5 files changed, 30 insertions(+), 13 deletions(-) > > I did this patch exclusively to support IMA on UBIFS. IMA uses the inode's > i_version field to detect changes on inodes. A proper i_version support > needs to make the i_version persistent on disk, although IMA itself doesn't > need a persistent i_version. Last time an earlier version of this patch > > was sent by Oleksij Rempel Richard said: > > What about making i_version persistent? > > We still have some empty fields in UBIFS' inode data structure. > > But first we have to be very sure that we need it. > > This patch exactly implements this suggestion, leaving the question if we > really need it. I added the IMA maintainers to Cc in the hope that Mimi or > Dmitry can give a good reason why there's no alternative to i_version for > IMA. Yes, it would be good to know more about the user, IMA. Does IMA store the version somewhere? Are there requirements on ordering? i.e. What if UBIFS faces a power-cut and the UBIFS i_version is behind IMA's version. Maybe we have to teach UBIFS to update an inode less lazy that it currently does... Thanks, //richard
On Tue, Sep 12, 2017 at 02:38:02PM +0200, Richard Weinberger wrote: > Sascha, > > Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer: > > This adds i_version support to UBIFS. The inodes i_version is used by > > IMA to detect changes to an inode and thus necessary to support IMA on > > UBIFS. The i_version is stored in the previously unused space in the > > UBIFS inode struct. Unlike in ext4 i_version support is unconditionally > > enabled in UBIFS as I saw no reason to make it optional. > > But we need a new UBIFS feature flag to indicate that this filesystem has > valid i_version fields. I assume you mean a new UBIFS_FLG_*, right? Who should set this flag? The Kernel once the filesystem has been mounted with iversion support enabled? This would mean we indeed need a iversion mount flag to give the user a chance to continue without iversion support and keep the filesystem compatible with older kernels. > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > fs/ubifs/dir.c | 30 +++++++++++++++++++----------- > > fs/ubifs/file.c | 5 +++++ > > fs/ubifs/journal.c | 3 ++- > > fs/ubifs/super.c | 2 ++ > > fs/ubifs/ubifs-media.h | 3 ++- > > 5 files changed, 30 insertions(+), 13 deletions(-) > > > > I did this patch exclusively to support IMA on UBIFS. IMA uses the inode's > > i_version field to detect changes on inodes. A proper i_version support > > needs to make the i_version persistent on disk, although IMA itself doesn't > > need a persistent i_version. Last time an earlier version of this patch > > > > was sent by Oleksij Rempel Richard said: > > > What about making i_version persistent? > > > We still have some empty fields in UBIFS' inode data structure. > > > But first we have to be very sure that we need it. > > > > This patch exactly implements this suggestion, leaving the question if we > > really need it. I added the IMA maintainers to Cc in the hope that Mimi or > > Dmitry can give a good reason why there's no alternative to i_version for > > IMA. > > Yes, it would be good to know more about the user, IMA. Does IMA store the > version somewhere? No. IMA solely uses i_version to detect if an inode has been changed since the last time it has seen this inode. IMA measures all files it hasn't seen before initially and stores the i_version in a struct integrity_iint_cache *. When an inode is written to next time IMA checks if the cached i_version still matches the inode's i_version and if it doesn't, it re-measures the inode. All this is purely runtime. > Are there requirements on ordering? i.e. What if UBIFS faces a power-cut > and the UBIFS i_version is behind IMA's version. Since IMA doesn't store the i_version anywhere this won't happen. > Maybe we have to teach UBIFS to update an inode less lazy that it currently > does... No, I don't think so. Sascha
Sascha, Am Dienstag, 12. September 2017, 15:46:16 CEST schrieb Sascha Hauer: > On Tue, Sep 12, 2017 at 02:38:02PM +0200, Richard Weinberger wrote: > > Sascha, > > > > Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer: > > > This adds i_version support to UBIFS. The inodes i_version is used by > > > IMA to detect changes to an inode and thus necessary to support IMA on > > > UBIFS. The i_version is stored in the previously unused space in the > > > UBIFS inode struct. Unlike in ext4 i_version support is unconditionally > > > enabled in UBIFS as I saw no reason to make it optional. > > > > But we need a new UBIFS feature flag to indicate that this filesystem has > > valid i_version fields. > > I assume you mean a new UBIFS_FLG_*, right? Yes. > Who should set this flag? The Kernel once the filesystem has been > mounted with iversion support enabled? This would mean we indeed need a > iversion mount flag to give the user a chance to continue without > iversion support and keep the filesystem compatible with older kernels. mkfs.ubifs or the kernel for a new default filesystem. Isn't mounting a i_version enabled filesystem without i_version support a bad idea since the version counters will be out of sync? > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > > > > fs/ubifs/dir.c | 30 +++++++++++++++++++----------- > > > fs/ubifs/file.c | 5 +++++ > > > fs/ubifs/journal.c | 3 ++- > > > fs/ubifs/super.c | 2 ++ > > > fs/ubifs/ubifs-media.h | 3 ++- > > > 5 files changed, 30 insertions(+), 13 deletions(-) > > > > > > I did this patch exclusively to support IMA on UBIFS. IMA uses the > > > inode's > > > i_version field to detect changes on inodes. A proper i_version support > > > needs to make the i_version persistent on disk, although IMA itself > > > doesn't > > > need a persistent i_version. Last time an earlier version of this patch > > > > > > was sent by Oleksij Rempel Richard said: > > > > What about making i_version persistent? > > > > We still have some empty fields in UBIFS' inode data structure. > > > > But first we have to be very sure that we need it. > > > > > > This patch exactly implements this suggestion, leaving the question if > > > we > > > really need it. I added the IMA maintainers to Cc in the hope that Mimi > > > or > > > Dmitry can give a good reason why there's no alternative to i_version > > > for > > > IMA. > > > > Yes, it would be good to know more about the user, IMA. Does IMA store the > > version somewhere? > > No. IMA solely uses i_version to detect if an inode has been changed since > the last time it has seen this inode. > IMA measures all files it hasn't seen before initially and stores the > i_version in a struct integrity_iint_cache *. When an inode is written to > next time IMA checks if the cached i_version still matches the inode's > i_version and if it doesn't, it re-measures the inode. All this is purely > runtime. > > > Are there requirements on ordering? i.e. What if UBIFS faces a power-cut > > and the UBIFS i_version is behind IMA's version. > > Since IMA doesn't store the i_version anywhere this won't happen. > > > Maybe we have to teach UBIFS to update an inode less lazy that it > > currently > > does... > > No, I don't think so. So, for the IMA use-case we don't even have to persist i_version. That would be cool. I need to read what other filesystems do, it is still not completely clear to me what the expected i_version semantics are. Satisfying IMA seems to be easy but we need to be very sure to not break other futuer i_version users... Thanks, //richard
On Tue, Sep 12, 2017 at 03:57:57PM +0200, Richard Weinberger wrote: > Sascha, > > Am Dienstag, 12. September 2017, 15:46:16 CEST schrieb Sascha Hauer: > > On Tue, Sep 12, 2017 at 02:38:02PM +0200, Richard Weinberger wrote: > > > Sascha, > > > > > > Am Dienstag, 12. September 2017, 12:39:00 CEST schrieb Sascha Hauer: > > > > This adds i_version support to UBIFS. The inodes i_version is used by > > > > IMA to detect changes to an inode and thus necessary to support IMA on > > > > UBIFS. The i_version is stored in the previously unused space in the > > > > UBIFS inode struct. Unlike in ext4 i_version support is unconditionally > > > > enabled in UBIFS as I saw no reason to make it optional. > > > > > > But we need a new UBIFS feature flag to indicate that this filesystem has > > > valid i_version fields. > > > > I assume you mean a new UBIFS_FLG_*, right? > > Yes. > > > Who should set this flag? The Kernel once the filesystem has been > > mounted with iversion support enabled? This would mean we indeed need a > > iversion mount flag to give the user a chance to continue without > > iversion support and keep the filesystem compatible with older kernels. > > mkfs.ubifs or the kernel for a new default filesystem. > Isn't mounting a i_version enabled filesystem without i_version support > a bad idea since the version counters will be out of sync? We should probably prevent an old kernel from mounting a UBIFS with i_version support enabled since that would reset the counters for files we write to back to 0. When we mount a UBIFS containing i_version without i_version support the kernel simply won't increase the i_version numbers on a file write. I don't think this is a problem. I bet this is the behaviour with ext4 aswell (though I haven't tested it). > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > --- > > > > > > > > fs/ubifs/dir.c | 30 +++++++++++++++++++----------- > > > > fs/ubifs/file.c | 5 +++++ > > > > fs/ubifs/journal.c | 3 ++- > > > > fs/ubifs/super.c | 2 ++ > > > > fs/ubifs/ubifs-media.h | 3 ++- > > > > 5 files changed, 30 insertions(+), 13 deletions(-) > > > > > > > > I did this patch exclusively to support IMA on UBIFS. IMA uses the > > > > inode's > > > > i_version field to detect changes on inodes. A proper i_version support > > > > needs to make the i_version persistent on disk, although IMA itself > > > > doesn't > > > > need a persistent i_version. Last time an earlier version of this patch > > > > > > > > was sent by Oleksij Rempel Richard said: > > > > > What about making i_version persistent? > > > > > We still have some empty fields in UBIFS' inode data structure. > > > > > But first we have to be very sure that we need it. > > > > > > > > This patch exactly implements this suggestion, leaving the question if > > > > we > > > > really need it. I added the IMA maintainers to Cc in the hope that Mimi > > > > or > > > > Dmitry can give a good reason why there's no alternative to i_version > > > > for > > > > IMA. > > > > > > Yes, it would be good to know more about the user, IMA. Does IMA store the > > > version somewhere? > > > > No. IMA solely uses i_version to detect if an inode has been changed since > > the last time it has seen this inode. > > IMA measures all files it hasn't seen before initially and stores the > > i_version in a struct integrity_iint_cache *. When an inode is written to > > next time IMA checks if the cached i_version still matches the inode's > > i_version and if it doesn't, it re-measures the inode. All this is purely > > runtime. > > > > > Are there requirements on ordering? i.e. What if UBIFS faces a power-cut > > > and the UBIFS i_version is behind IMA's version. > > > > Since IMA doesn't store the i_version anywhere this won't happen. > > > > > Maybe we have to teach UBIFS to update an inode less lazy that it > > > currently > > > does... > > > > No, I don't think so. > > So, for the IMA use-case we don't even have to persist i_version. > That would be cool. Yes, that's what earlier versions of this patch did, nacked by Christoph Hellwig with the words: > Maybe IMA doesn't care, but if you set MS_I_VERSION the fs does give > a guarantee. Sp NAK on this patch as-is. (see https://lkml.org/lkml/2017/4/12/61) Reading this sentence again it may be a possibility to just increase the i_version field without setting the MS_I_VERSION flag. > > I need to read what other filesystems do, it is still not completely clear to > me what the expected i_version semantics are. Satisfying IMA seems to be easy > but we need to be very sure to not break other futuer i_version users... Sure. I am also not sure whether I implemented it correctly since it's implementation defined by some filesystem drivers which I am afraid are not even consistent. Sascha
Sascha, Am Dienstag, 12. September 2017, 16:23:18 CEST schrieb Sascha Hauer: > > So, for the IMA use-case we don't even have to persist i_version. > > That would be cool. > > Yes, that's what earlier versions of this patch did, nacked by Christoph > > Hellwig with the words: > > Maybe IMA doesn't care, but if you set MS_I_VERSION the fs does give > > a guarantee. Sp NAK on this patch as-is. > > (see https://lkml.org/lkml/2017/4/12/61) > > Reading this sentence again it may be a possibility to just increase the > i_version field without setting the MS_I_VERSION flag. Yes. > > I need to read what other filesystems do, it is still not completely clear > > to me what the expected i_version semantics are. Satisfying IMA seems to > > be easy but we need to be very sure to not break other futuer i_version > > users... > Sure. I am also not sure whether I implemented it correctly since it's > implementation defined by some filesystem drivers which I am afraid are > not even consistent. As usual, let's try to keep at least UBIFS kind of sane. ;-) Thanks, //richard
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c index 417fe0b29f23..addf161df313 100644 --- a/fs/ubifs/dir.c +++ b/fs/ubifs/dir.c @@ -195,6 +195,13 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir, return inode; } +static void ubifs_dir_update_time(struct inode *dir, struct timespec time) +{ + dir->i_mtime = dir->i_ctime = time; + + inode_inc_iversion(dir); +} + static int dbg_check_name(const struct ubifs_info *c, const struct ubifs_dent_node *dent, const struct fscrypt_name *nm) @@ -356,7 +363,8 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode, mutex_lock(&dir_ui->ui_mutex); dir->i_size += sz_change; dir_ui->ui_size = dir->i_size; - dir->i_mtime = dir->i_ctime = inode->i_ctime; + ubifs_dir_update_time(dir, inode->i_ctime); + err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0); if (err) goto out_cancel; @@ -770,7 +778,7 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir, inode->i_ctime = current_time(inode); dir->i_size += sz_change; dir_ui->ui_size = dir->i_size; - dir->i_mtime = dir->i_ctime = inode->i_ctime; + ubifs_dir_update_time(dir, inode->i_ctime); err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0); if (err) goto out_cancel; @@ -846,7 +854,7 @@ static int ubifs_unlink(struct inode *dir, struct dentry *dentry) drop_nlink(inode); dir->i_size -= sz_change; dir_ui->ui_size = dir->i_size; - dir->i_mtime = dir->i_ctime = inode->i_ctime; + ubifs_dir_update_time(dir, inode->i_ctime); err = ubifs_jnl_update(c, dir, &nm, inode, 1, 0); if (err) goto out_cancel; @@ -951,7 +959,7 @@ static int ubifs_rmdir(struct inode *dir, struct dentry *dentry) drop_nlink(dir); dir->i_size -= sz_change; dir_ui->ui_size = dir->i_size; - dir->i_mtime = dir->i_ctime = inode->i_ctime; + ubifs_dir_update_time(dir, inode->i_ctime); err = ubifs_jnl_update(c, dir, &nm, inode, 1, 0); if (err) goto out_cancel; @@ -1023,7 +1031,7 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) inc_nlink(dir); dir->i_size += sz_change; dir_ui->ui_size = dir->i_size; - dir->i_mtime = dir->i_ctime = inode->i_ctime; + ubifs_dir_update_time(dir, inode->i_ctime); err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0); if (err) { ubifs_err(c, "cannot create directory, error %d", err); @@ -1114,7 +1122,7 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry, mutex_lock(&dir_ui->ui_mutex); dir->i_size += sz_change; dir_ui->ui_size = dir->i_size; - dir->i_mtime = dir->i_ctime = inode->i_ctime; + ubifs_dir_update_time(dir, inode->i_ctime); err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0); if (err) goto out_cancel; @@ -1245,7 +1253,7 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry, mutex_lock(&dir_ui->ui_mutex); dir->i_size += sz_change; dir_ui->ui_size = dir->i_size; - dir->i_mtime = dir->i_ctime = inode->i_ctime; + ubifs_dir_update_time(dir, inode->i_ctime); err = ubifs_jnl_update(c, dir, &nm, inode, 0, 0); if (err) goto out_cancel; @@ -1450,8 +1458,8 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry, old_dir->i_size -= old_sz; ubifs_inode(old_dir)->ui_size = old_dir->i_size; - old_dir->i_mtime = old_dir->i_ctime = time; - new_dir->i_mtime = new_dir->i_ctime = time; + ubifs_dir_update_time(old_dir, time); + ubifs_dir_update_time(new_dir, time); /* * And finally, if we unlinked a direntry which happened to have the @@ -1595,8 +1603,8 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry, time = current_time(old_dir); fst_inode->i_ctime = time; snd_inode->i_ctime = time; - old_dir->i_mtime = old_dir->i_ctime = time; - new_dir->i_mtime = new_dir->i_ctime = time; + ubifs_dir_update_time(old_dir, time); + ubifs_dir_update_time(new_dir, time); if (old_dir != new_dir) { if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) { diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c index 8cad0b19b404..54f58172b6e7 100644 --- a/fs/ubifs/file.c +++ b/fs/ubifs/file.c @@ -1104,6 +1104,7 @@ static void do_attr_changes(struct inode *inode, const struct iattr *attr) mode &= ~S_ISGID; inode->i_mode = mode; } + inode_inc_iversion(inode); } /** @@ -1409,6 +1410,8 @@ int ubifs_update_time(struct inode *inode, struct timespec *time, if (!(inode->i_sb->s_flags & MS_LAZYTIME)) iflags |= I_DIRTY_SYNC; + inode_inc_iversion(inode); + release = ui->dirty; __mark_inode_dirty(inode, iflags); mutex_unlock(&ui->ui_mutex); @@ -1443,6 +1446,7 @@ static int update_mctime(struct inode *inode) mutex_lock(&ui->ui_mutex); inode->i_mtime = inode->i_ctime = current_time(inode); + inode_inc_iversion(inode); release = ui->dirty; mark_inode_dirty_sync(inode); mutex_unlock(&ui->ui_mutex); @@ -1588,6 +1592,7 @@ static int ubifs_vm_page_mkwrite(struct vm_fault *vmf) mutex_lock(&ui->ui_mutex); inode->i_mtime = inode->i_ctime = current_time(inode); + inode_inc_iversion(inode); release = ui->dirty; mark_inode_dirty_sync(inode); mutex_unlock(&ui->ui_mutex); diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 04c4ec6483e5..9a2062d57bc8 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -67,7 +67,7 @@ static inline void zero_ino_node_unused(struct ubifs_ino_node *ino) { memset(ino->padding1, 0, 4); - memset(ino->padding2, 0, 26); + memset(ino->padding2, 0, 18); } /** @@ -459,6 +459,7 @@ static void pack_inode(struct ubifs_info *c, struct ubifs_ino_node *ino, ino->ctime_nsec = cpu_to_le32(inode->i_ctime.tv_nsec); ino->mtime_sec = cpu_to_le64(inode->i_mtime.tv_sec); ino->mtime_nsec = cpu_to_le32(inode->i_mtime.tv_nsec); + ino->iversion = cpu_to_le64(inode->i_version); ino->uid = cpu_to_le32(i_uid_read(inode)); ino->gid = cpu_to_le32(i_gid_read(inode)); ino->mode = cpu_to_le32(inode->i_mode); diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index bffadbb67e47..545c268033cc 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -143,6 +143,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum) inode->i_ctime.tv_nsec = le32_to_cpu(ino->ctime_nsec); inode->i_mode = le32_to_cpu(ino->mode); inode->i_size = le64_to_cpu(ino->size); + inode->i_version = le64_to_cpu(ino->iversion); ui->data_len = le32_to_cpu(ino->data_len); ui->flags = le32_to_cpu(ino->flags); @@ -2056,6 +2057,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent) sb->s_op = &ubifs_super_operations; sb->s_xattr = ubifs_xattr_handlers; sb->s_cop = &ubifs_crypt_operations; + sb->s_flags |= MS_I_VERSION; mutex_lock(&c->umount_mutex); err = mount_ubifs(c); diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h index e8c23c9d4f4a..d4689c7e9df5 100644 --- a/fs/ubifs/ubifs-media.h +++ b/fs/ubifs/ubifs-media.h @@ -525,7 +525,8 @@ struct ubifs_ino_node { __u8 padding1[4]; /* Watch 'zero_ino_node_unused()' if changing! */ __le32 xattr_names; __le16 compr_type; - __u8 padding2[26]; /* Watch 'zero_ino_node_unused()' if changing! */ + __le64 iversion; + __u8 padding2[18]; /* Watch 'zero_ino_node_unused()' if changing! */ __u8 data[]; } __packed;