Re: [Linux-ima-devel] [PATCH] fs: ubifs: Add i_version support

[Sascha H. <s....@pe...>]

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....@pe...>
> > > > ---
> > > > 
> > > >  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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |