[PATCH v3 1/1] vfs: iversion truncate bug fix

[PATCH v3 1/1] vfs: iversion truncate bug fix

[Dmitry Kasatkin]

When a file is truncated with truncate()/ftruncate() and then closed,
iversion is not updated. This patch uses ATTR_SIZE flag as an indication
to increment iversion.

Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
---
 fs/attr.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 7ee7ba4..b8f55c4 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -176,6 +176,11 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
 			return -EPERM;
 	}
 
+	if ((ia_valid & ATTR_SIZE) && IS_I_VERSION(inode)) {
+		if (attr->ia_size != inode->i_size)
+			inode_inc_iversion(inode);
+	}
+
 	if ((ia_valid & ATTR_MODE)) {
 		mode_t amode = attr->ia_mode;
 		/* Flag setting protected by i_mutex */
-- 
1.7.4.1

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Mimi Zohar]

On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> When a file is truncated with truncate()/ftruncate() and then closed,
> iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> to increment iversion.
> 
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>

Acked-by: Mimi Zohar <zohar@us.ibm.com> 
(Stable should be cc'ed on this patch.)

thanks,

Mimi

> ---
>  fs/attr.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 7ee7ba4..b8f55c4 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -176,6 +176,11 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
>  			return -EPERM;
>  	}
> 
> +	if ((ia_valid & ATTR_SIZE) && IS_I_VERSION(inode)) {
> +		if (attr->ia_size != inode->i_size)
> +			inode_inc_iversion(inode);
> +	}
> +
>  	if ((ia_valid & ATTR_MODE)) {
>  		mode_t amode = attr->ia_mode;
>  		/* Flag setting protected by i_mutex */

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Andrew Morton]

On Thu, 22 Dec 2011 08:26:30 -0500
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > When a file is truncated with truncate()/ftruncate() and then closed,
> > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > to increment iversion.
> > 
> > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> 
> Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> (Stable should be cc'ed on this patch.)

why?

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Mimi Zohar]

On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> On Thu, 22 Dec 2011 08:26:30 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > to increment iversion.
> > > 
> > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > 
> > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > (Stable should be cc'ed on this patch.)
> 
> why?

Why backported?  The IMA measurement list could be incomplete.

Mimi

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Andrew Morton]

On Wed, 04 Jan 2012 19:33:49 -0500
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> > On Thu, 22 Dec 2011 08:26:30 -0500
> > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 
> > > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > > to increment iversion.
> > > > 
> > > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > > 
> > > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > > (Stable should be cc'ed on this patch.)
> > 
> > why?
> 
> Why backported?

Yes.  If you want to submit the patch to the -stable maintainer then
you should explain to him why the fix is important enough to warrant
doing that.  That involves explaining the user-visible effects of
the bug.

>  The IMA measurement list could be incomplete.

In more detail than this.  Maybe he knows what the above sentence
means, but I sure don't.

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Greg KH]

On Wed, Jan 04, 2012 at 04:46:38PM -0800, Andrew Morton wrote:
> On Wed, 04 Jan 2012 19:33:49 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> > > On Thu, 22 Dec 2011 08:26:30 -0500
> > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > > > to increment iversion.
> > > > > 
> > > > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > > > 
> > > > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > > > (Stable should be cc'ed on this patch.)
> > > 
> > > why?
> > 
> > Why backported?
> 
> Yes.  If you want to submit the patch to the -stable maintainer then
> you should explain to him why the fix is important enough to warrant
> doing that.  That involves explaining the user-visible effects of
> the bug.
> 
> >  The IMA measurement list could be incomplete.
> 
> In more detail than this.  Maybe he knows what the above sentence
> means, but I sure don't.

Nope, I don't either :)

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Mimi Zohar]

On Wed, 2012-01-04 at 18:06 -0800, Greg KH wrote:
> On Wed, Jan 04, 2012 at 04:46:38PM -0800, Andrew Morton wrote:
> > On Wed, 04 Jan 2012 19:33:49 -0500
> > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 
> > > On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> > > > On Thu, 22 Dec 2011 08:26:30 -0500
> > > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > > > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > > > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > > > > to increment iversion.
> > > > > > 
> > > > > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > > > > 
> > > > > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > > > > (Stable should be cc'ed on this patch.)
> > > > 
> > > > why?
> > > 
> > > Why backported?
> > 
> > Yes.  If you want to submit the patch to the -stable maintainer then
> > you should explain to him why the fix is important enough to warrant
> > doing that.  That involves explaining the user-visible effects of
> > the bug.
> > 
> > >  The IMA measurement list could be incomplete.
> > 
> > In more detail than this.  Maybe he knows what the above sentence
> > means, but I sure don't.
> 
> Nope, I don't either :)

On fput(), i_version is used to detect and flag files that have changed
and need to be re-measured in the IMA measurement policy.  When a file
is truncated with truncate()/ftruncate() and then closed, i_version is
not updated.  As a result, although the file has changed, it will not be
re-measured and added to the IMA measurement list on subsequent access.

Mimi

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Dave Chinner]

On Wed, Jan 04, 2012 at 11:17:12PM -0500, Mimi Zohar wrote:
> On Wed, 2012-01-04 at 18:06 -0800, Greg KH wrote:
> > On Wed, Jan 04, 2012 at 04:46:38PM -0800, Andrew Morton wrote:
> > > On Wed, 04 Jan 2012 19:33:49 -0500
> > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> > > > > On Thu, 22 Dec 2011 08:26:30 -0500
> > > > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > > > > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > > > > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > > > > > to increment iversion.
> > > > > > > 
> > > > > > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > > > > > 
> > > > > > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > > > > > (Stable should be cc'ed on this patch.)
> > > > > 
> > > > > why?
> > > > 
> > > > Why backported?
> > > 
> > > Yes.  If you want to submit the patch to the -stable maintainer then
> > > you should explain to him why the fix is important enough to warrant
> > > doing that.  That involves explaining the user-visible effects of
> > > the bug.
> > > 
> > > >  The IMA measurement list could be incomplete.
> > > 
> > > In more detail than this.  Maybe he knows what the above sentence
> > > means, but I sure don't.
> > 
> > Nope, I don't either :)
> 
> On fput(), i_version is used to detect and flag files that have changed
> and need to be re-measured in the IMA measurement policy.  When a file
> is truncated with truncate()/ftruncate() and then closed, i_version is
> not updated.  As a result, although the file has changed, it will not be
> re-measured and added to the IMA measurement list on subsequent access.

That's seems like a rather unreliable way of detecting that a file
has changed to me.  I mean, only ext4 uses inode_inc_version() when
it internally dirties an inode, and only ext4 sets the MS_I_VERSION
so that inode_inc_version is only called for ext4 inodes when
timestamps change.

Other filesystems randomly open code i_version inrements, and many
don't touch it at all when inodes are changed, so it this really
doesn't seem like anything anyone can rely on for detecting changes
except maybe for ext4 users.

Hence just adding an increment to the truncate case doesn't seem to
be sufficient to me. e.g. what about the equivalent case of having a
hole punched in the file via fallocate? The file has definitely
changed, but i_version won't change....

Perhaps bumping i_version in __mark_inode_dirty() might be the best
way to capture all changes (other than timestamp updates) to any
inode regardless of the filesystem type?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Greg KH]

On Wed, Jan 04, 2012 at 11:17:12PM -0500, Mimi Zohar wrote:
> On Wed, 2012-01-04 at 18:06 -0800, Greg KH wrote:
> > On Wed, Jan 04, 2012 at 04:46:38PM -0800, Andrew Morton wrote:
> > > On Wed, 04 Jan 2012 19:33:49 -0500
> > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> > > > > On Thu, 22 Dec 2011 08:26:30 -0500
> > > > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > > > > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > > > > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > > > > > to increment iversion.
> > > > > > > 
> > > > > > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > > > > > 
> > > > > > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > > > > > (Stable should be cc'ed on this patch.)
> > > > > 
> > > > > why?
> > > > 
> > > > Why backported?
> > > 
> > > Yes.  If you want to submit the patch to the -stable maintainer then
> > > you should explain to him why the fix is important enough to warrant
> > > doing that.  That involves explaining the user-visible effects of
> > > the bug.
> > > 
> > > >  The IMA measurement list could be incomplete.
> > > 
> > > In more detail than this.  Maybe he knows what the above sentence
> > > means, but I sure don't.
> > 
> > Nope, I don't either :)
> 
> On fput(), i_version is used to detect and flag files that have changed
> and need to be re-measured in the IMA measurement policy.  When a file
> is truncated with truncate()/ftruncate() and then closed, i_version is
> not updated.  As a result, although the file has changed, it will not be
> re-measured and added to the IMA measurement list on subsequent access.

And what am I supposed to do with this?

Please, go read Documentation/stable_kernel_rules.txt for how to
properly submit patches to the stable kernel tree.  The information here
needs to be in the patch changelog itself, not in some random email
thread that will get lost instantly into my email-archive-from-hell
after I am done reading this.

greg k-h

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Mimi Zohar]

On Thu, 2012-01-05 at 08:54 -0800, Greg KH wrote:
> On Wed, Jan 04, 2012 at 11:17:12PM -0500, Mimi Zohar wrote:
> > On Wed, 2012-01-04 at 18:06 -0800, Greg KH wrote:
> > > On Wed, Jan 04, 2012 at 04:46:38PM -0800, Andrew Morton wrote:
> > > > On Wed, 04 Jan 2012 19:33:49 -0500
> > > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> > > > > > On Thu, 22 Dec 2011 08:26:30 -0500
> > > > > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > > > 
> > > > > > > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > > > > > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > > > > > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > > > > > > to increment iversion.
> > > > > > > > 
> > > > > > > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > > > > > > 
> > > > > > > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > > > > > > (Stable should be cc'ed on this patch.)
> > > > > > 
> > > > > > why?
> > > > > 
> > > > > Why backported?
> > > > 
> > > > Yes.  If you want to submit the patch to the -stable maintainer then
> > > > you should explain to him why the fix is important enough to warrant
> > > > doing that.  That involves explaining the user-visible effects of
> > > > the bug.
> > > > 
> > > > >  The IMA measurement list could be incomplete.
> > > > 
> > > > In more detail than this.  Maybe he knows what the above sentence
> > > > means, but I sure don't.
> > > 
> > > Nope, I don't either :)
> > 
> > On fput(), i_version is used to detect and flag files that have changed
> > and need to be re-measured in the IMA measurement policy.  When a file
> > is truncated with truncate()/ftruncate() and then closed, i_version is
> > not updated.  As a result, although the file has changed, it will not be
> > re-measured and added to the IMA measurement list on subsequent access.
> 
> And what am I supposed to do with this?
> 
> Please, go read Documentation/stable_kernel_rules.txt for how to
> properly submit patches to the stable kernel tree.  The information here
> needs to be in the patch changelog itself, not in some random email
> thread that will get lost instantly into my email-archive-from-hell
> after I am done reading this.
> 
> greg k-h

Yes, I've read Documentation/stable_kernel_rules.txt and think this
patch meets the criteria for being backported.

As far as I'm aware, this patch hasn't been upstreamed yet and is
waiting for someone, besides myself, to Ack it.  Once Acked, either
Dmitry or I can send a pull request with an updated patch description.
Should this patch go in via the security tree?

thanks,

Mimi

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Christoph Hellwig]

On Thu, Jan 05, 2012 at 03:53:54PM +1100, Dave Chinner wrote:
> That's seems like a rather unreliable way of detecting that a file
> has changed to me.  I mean, only ext4 uses inode_inc_version() when
> it internally dirties an inode, and only ext4 sets the MS_I_VERSION
> so that inode_inc_version is only called for ext4 inodes when
> timestamps change.

And even ext4 only does it when using the non-default "i_version"
mount option.

> Hence just adding an increment to the truncate case doesn't seem to
> be sufficient to me. e.g. what about the equivalent case of having a
> hole punched in the file via fallocate? The file has definitely
> changed, but i_version won't change....
> 
> Perhaps bumping i_version in __mark_inode_dirty() might be the best
> way to capture all changes (other than timestamp updates) to any
> inode regardless of the filesystem type?

It has the same problem as the timestamp updates doing that right now -
the fs can't do locking around it, and it can't return errors.  That's
something affecting at least btrfs, xfs and IIRC ubifs, and probably
the cluster filesystems as well.  The right answer is to replace the
timestmap updates which are the only places doing that with a method
as Josef had planned to do, and then we can include the i_version
updates in there.

That assumes we figure out a coherent way to do it - except for the
conditional abuse in file_updates_times it's currently entirely under
fs control.  So the best way to fix it would be to:

 - move the fs-private use into those filesystems actually using it.
   Note that a lot less actually check for it rather than just updating
   it based on some cargo cult, and most only do so for directories.
 - figure a why what exact change count semantics NFS (and IMA) want
   and find a way to implement them so that the fs can tell the callers
   that they don't exist.

Btw, does IMA care about these beeing persistent?

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[James Bottomley]

On Thu, 2012-01-05 at 12:19 -0500, Mimi Zohar wrote:
> On Thu, 2012-01-05 at 08:54 -0800, Greg KH wrote:
> > On Wed, Jan 04, 2012 at 11:17:12PM -0500, Mimi Zohar wrote:
> > > On Wed, 2012-01-04 at 18:06 -0800, Greg KH wrote:
> > > > On Wed, Jan 04, 2012 at 04:46:38PM -0800, Andrew Morton wrote:
> > > > > On Wed, 04 Jan 2012 19:33:49 -0500
> > > > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > On Wed, 2012-01-04 at 15:28 -0800, Andrew Morton wrote:
> > > > > > > On Thu, 22 Dec 2011 08:26:30 -0500
> > > > > > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > > > > 
> > > > > > > > On Thu, 2011-12-22 at 12:54 +0200, Dmitry Kasatkin wrote:
> > > > > > > > > When a file is truncated with truncate()/ftruncate() and then closed,
> > > > > > > > > iversion is not updated. This patch uses ATTR_SIZE flag as an indication
> > > > > > > > > to increment iversion.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> > > > > > > > 
> > > > > > > > Acked-by: Mimi Zohar <zohar@us.ibm.com> 
> > > > > > > > (Stable should be cc'ed on this patch.)
> > > > > > > 
> > > > > > > why?
> > > > > > 
> > > > > > Why backported?
> > > > > 
> > > > > Yes.  If you want to submit the patch to the -stable maintainer then
> > > > > you should explain to him why the fix is important enough to warrant
> > > > > doing that.  That involves explaining the user-visible effects of
> > > > > the bug.
> > > > > 
> > > > > >  The IMA measurement list could be incomplete.
> > > > > 
> > > > > In more detail than this.  Maybe he knows what the above sentence
> > > > > means, but I sure don't.
> > > > 
> > > > Nope, I don't either :)
> > > 
> > > On fput(), i_version is used to detect and flag files that have changed
> > > and need to be re-measured in the IMA measurement policy.  When a file
> > > is truncated with truncate()/ftruncate() and then closed, i_version is
> > > not updated.  As a result, although the file has changed, it will not be
> > > re-measured and added to the IMA measurement list on subsequent access.
> > 
> > And what am I supposed to do with this?
> > 
> > Please, go read Documentation/stable_kernel_rules.txt for how to
> > properly submit patches to the stable kernel tree.  The information here
> > needs to be in the patch changelog itself, not in some random email
> > thread that will get lost instantly into my email-archive-from-hell
> > after I am done reading this.
> > 
> > greg k-h
> 
> Yes, I've read Documentation/stable_kernel_rules.txt and think this
> patch meets the criteria for being backported.
> 
> As far as I'm aware, this patch hasn't been upstreamed yet and is
> waiting for someone, besides myself, to Ack it.  Once Acked, either
> Dmitry or I can send a pull request with an updated patch description.
> Should this patch go in via the security tree?

If it hasn't been upstreamed yet, just make sure you put

cc: stable@kernel.org

In the signoffs of the patch you're sending upstream and the backport
will occur automatically when the patch is finally upstreamed.

James

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Mimi Zohar]

On Thu, 2012-01-05 at 13:14 -0500, Christoph Hellwig wrote:
> On Thu, Jan 05, 2012 at 03:53:54PM +1100, Dave Chinner wrote:
> > That's seems like a rather unreliable way of detecting that a file
> > has changed to me.  I mean, only ext4 uses inode_inc_version() when
> > it internally dirties an inode, and only ext4 sets the MS_I_VERSION
> > so that inode_inc_version is only called for ext4 inodes when
> > timestamps change.
> 
> And even ext4 only does it when using the non-default "i_version"
> mount option.
> 
> > Hence just adding an increment to the truncate case doesn't seem to
> > be sufficient to me. e.g. what about the equivalent case of having a
> > hole punched in the file via fallocate? The file has definitely
> > changed, but i_version won't change....
> > 
> > Perhaps bumping i_version in __mark_inode_dirty() might be the best
> > way to capture all changes (other than timestamp updates) to any
> > inode regardless of the filesystem type?
> 
> It has the same problem as the timestamp updates doing that right now -
> the fs can't do locking around it, and it can't return errors.  That's
> something affecting at least btrfs, xfs and IIRC ubifs, and probably
> the cluster filesystems as well.  The right answer is to replace the
> timestmap updates which are the only places doing that with a method
> as Josef had planned to do, and then we can include the i_version
> updates in there.
> 
> That assumes we figure out a coherent way to do it - except for the
> conditional abuse in file_updates_times it's currently entirely under
> fs control.  So the best way to fix it would be to:
> 
>  - move the fs-private use into those filesystems actually using it.
>    Note that a lot less actually check for it rather than just updating
>    it based on some cargo cult, and most only do so for directories.
>  - figure a why what exact change count semantics NFS (and IMA) want
>    and find a way to implement them so that the fs can tell the callers
>    that they don't exist.
> 
> Btw, does IMA care about these beeing persistent?

By 'persistent' I assume you mean across boots.  IMA (and IMA-appraisal)
measure and appraise files the first time they're accessed/executed.  So
no, it does not need to be persistent. IMA/IMA-appraisal just need some
way to detect file change in order to know whether the file needs to be
re-measured/appraised on subsequent access.

Mimi

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[J. Bruce Fields]

On Thu, Jan 05, 2012 at 01:14:01PM -0500, Christoph Hellwig wrote:
> On Thu, Jan 05, 2012 at 03:53:54PM +1100, Dave Chinner wrote:
> > That's seems like a rather unreliable way of detecting that a file
> > has changed to me.  I mean, only ext4 uses inode_inc_version() when
> > it internally dirties an inode, and only ext4 sets the MS_I_VERSION
> > so that inode_inc_version is only called for ext4 inodes when
> > timestamps change.
> 
> And even ext4 only does it when using the non-default "i_version"
> mount option.
> 
> > Hence just adding an increment to the truncate case doesn't seem to
> > be sufficient to me. e.g. what about the equivalent case of having a
> > hole punched in the file via fallocate? The file has definitely
> > changed, but i_version won't change....
> > 
> > Perhaps bumping i_version in __mark_inode_dirty() might be the best
> > way to capture all changes (other than timestamp updates) to any
> > inode regardless of the filesystem type?
> 
> It has the same problem as the timestamp updates doing that right now -
> the fs can't do locking around it, and it can't return errors.  That's
> something affecting at least btrfs, xfs and IIRC ubifs, and probably
> the cluster filesystems as well.  The right answer is to replace the
> timestmap updates which are the only places doing that with a method
> as Josef had planned to do, and then we can include the i_version
> updates in there.
> 
> That assumes we figure out a coherent way to do it - except for the
> conditional abuse in file_updates_times it's currently entirely under
> fs control.  So the best way to fix it would be to:
> 
>  - move the fs-private use into those filesystems actually using it.
>    Note that a lot less actually check for it rather than just updating
>    it based on some cargo cult, and most only do so for directories.
>  - figure a why what exact change count semantics NFS (and IMA) want

For NFS:

The NFSv4 protocol has a "change attribute" that a client should be able
to use determine whether its cache is up to date.  It therefore doesn't
have to be a "count" necessarily, as long as it changes every time.
(And is extremely unlikely to repeat.)

It should be on by default for any exportable filesystem capable of
supporting it.

It has to change on both data and metadata changes.  (Like ctime.)

It has to work for both files and directories.

It has to persist on disk--a client should be able to revalidate its
cache by comparing change attributes fetched before and after a server
reboot.  (But: I don't know if there's any easy way to avoid the corner
case where unsync'd updates cause i_version to go backwards after
reboot, and then to repeat a previous i_version value after subsequent
updates.)

--b.

>    and find a way to implement them so that the fs can tell the callers
>    that they don't exist.
> 
> Btw, does IMA care about these beeing persistent?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Mimi Zohar]

On Thu, 2012-01-05 at 14:49 -0500, Mimi Zohar wrote:
> On Thu, 2012-01-05 at 13:14 -0500, Christoph Hellwig wrote:
> > On Thu, Jan 05, 2012 at 03:53:54PM +1100, Dave Chinner wrote:
> > > That's seems like a rather unreliable way of detecting that a file
> > > has changed to me.  I mean, only ext4 uses inode_inc_version() when
> > > it internally dirties an inode, and only ext4 sets the MS_I_VERSION
> > > so that inode_inc_version is only called for ext4 inodes when
> > > timestamps change.
> > 
> > And even ext4 only does it when using the non-default "i_version"
> > mount option.
> > 
> > > Hence just adding an increment to the truncate case doesn't seem to
> > > be sufficient to me. e.g. what about the equivalent case of having a
> > > hole punched in the file via fallocate? The file has definitely
> > > changed, but i_version won't change....
> > > 
> > > Perhaps bumping i_version in __mark_inode_dirty() might be the best
> > > way to capture all changes (other than timestamp updates) to any
> > > inode regardless of the filesystem type?
> > 
> > It has the same problem as the timestamp updates doing that right now -
> > the fs can't do locking around it, and it can't return errors.  That's
> > something affecting at least btrfs, xfs and IIRC ubifs, and probably
> > the cluster filesystems as well.  The right answer is to replace the
> > timestmap updates which are the only places doing that with a method
> > as Josef had planned to do, and then we can include the i_version
> > updates in there.
> > 
> > That assumes we figure out a coherent way to do it - except for the
> > conditional abuse in file_updates_times it's currently entirely under
> > fs control.  So the best way to fix it would be to:
> > 
> >  - move the fs-private use into those filesystems actually using it.
> >    Note that a lot less actually check for it rather than just updating
> >    it based on some cargo cult, and most only do so for directories.
> >  - figure a why what exact change count semantics NFS (and IMA) want
> >    and find a way to implement them so that the fs can tell the callers
> >    that they don't exist.
> > 
> > Btw, does IMA care about these beeing persistent?
> 
> By 'persistent' I assume you mean across boots.  IMA (and IMA-appraisal)
> measure and appraise files the first time they're accessed/executed.  So
> no, it does not need to be persistent. IMA/IMA-appraisal just need some
> way to detect file change in order to know whether the file needs to be
> re-measured/appraised on subsequent access.

One other IMA requirement would be the ability to detect if a file
mmapped executable, after the point that it has been locked from
modification, has changed since the last time measured/appraised.  The
concern here, in particular, is the ability to detect file change while
holding the mmap_sem.

thanks,

Mimi

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Andrew Morton]

On Thu, 05 Jan 2012 10:39:41 -0800
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> > > Please, go read Documentation/stable_kernel_rules.txt for how to
> > > properly submit patches to the stable kernel tree.  The information here
> > > needs to be in the patch changelog itself, not in some random email
> > > thread that will get lost instantly into my email-archive-from-hell
> > > after I am done reading this.
> > > 
> > > greg k-h
> > 
> > Yes, I've read Documentation/stable_kernel_rules.txt and think this
> > patch meets the criteria for being backported.
> > 
> > As far as I'm aware, this patch hasn't been upstreamed yet and is
> > waiting for someone, besides myself, to Ack it.  Once Acked, either
> > Dmitry or I can send a pull request with an updated patch description.
> > Should this patch go in via the security tree?
> 
> If it hasn't been upstreamed yet, just make sure you put
> 
> cc: stable@kernel.org
> 
> In the signoffs of the patch you're sending upstream and the backport
> will occur automatically when the patch is finally upstreamed.

Mimi didn't write or send the patch.

This happily git-free maintainer simply goes in and edits the
changelog.  I do this very very frequently.


Here's what I currently have.  I plan to send this to Rip Van Viro.




From: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
Subject: vfs: increment iversion when a file is truncated

When a file is truncated with truncate()/ftruncate() and then closed,
iversion is not updated.  This patch uses ATTR_SIZE flag as an indication
to increment iversion.

Mimi said:

On fput(), i_version is used to detect and flag files that have changed
and need to be re-measured in the IMA measurement policy.  When a file
is truncated with truncate()/ftruncate() and then closed, i_version is
not updated.  As a result, although the file has changed, it will not be
re-measured and added to the IMA measurement list on subsequent access.

Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
Acked-by: Mimi Zohar <zohar@us.ibm.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/attr.c |    5 +++++
 1 file changed, 5 insertions(+)

diff -puN fs/attr.c~vfs-increment-iversion-when-a-file-is-truncated fs/attr.c
--- a/fs/attr.c~vfs-increment-iversion-when-a-file-is-truncated
+++ a/fs/attr.c
@@ -176,6 +176,11 @@ int notify_change(struct dentry * dentry
 			return -EPERM;
 	}
 
+	if ((ia_valid & ATTR_SIZE) && IS_I_VERSION(inode)) {
+		if (attr->ia_size != inode->i_size)
+			inode_inc_iversion(inode);
+	}
+
 	if ((ia_valid & ATTR_MODE)) {
 		umode_t amode = attr->ia_mode;
 		/* Flag setting protected by i_mutex */
_

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Mimi Zohar]

On Thu, 2012-01-05 at 14:30 -0800, Andrew Morton wrote:
> On Thu, 05 Jan 2012 10:39:41 -0800
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > > > Please, go read Documentation/stable_kernel_rules.txt for how to
> > > > properly submit patches to the stable kernel tree.  The information here
> > > > needs to be in the patch changelog itself, not in some random email
> > > > thread that will get lost instantly into my email-archive-from-hell
> > > > after I am done reading this.
> > > > 
> > > > greg k-h
> > > 
> > > Yes, I've read Documentation/stable_kernel_rules.txt and think this
> > > patch meets the criteria for being backported.
> > > 
> > > As far as I'm aware, this patch hasn't been upstreamed yet and is
> > > waiting for someone, besides myself, to Ack it.  Once Acked, either
> > > Dmitry or I can send a pull request with an updated patch description.
> > > Should this patch go in via the security tree?
> > 
> > If it hasn't been upstreamed yet, just make sure you put
> > 
> > cc: stable@kernel.org
> > 
> > In the signoffs of the patch you're sending upstream and the backport
> > will occur automatically when the patch is finally upstreamed.
> 
> Mimi didn't write or send the patch.

Thanks for explaining it to everyone and upstreaming the patch. 

Mimi

> This happily git-free maintainer simply goes in and edits the
> changelog.  I do this very very frequently.
> 
> Here's what I currently have.  I plan to send this to Rip Van Viro.
> 
> 
> 
> 
> From: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> Subject: vfs: increment iversion when a file is truncated
> 
> When a file is truncated with truncate()/ftruncate() and then closed,
> iversion is not updated.  This patch uses ATTR_SIZE flag as an indication
> to increment iversion.
> 
> Mimi said:
> 
> On fput(), i_version is used to detect and flag files that have changed
> and need to be re-measured in the IMA measurement policy.  When a file
> is truncated with truncate()/ftruncate() and then closed, i_version is
> not updated.  As a result, although the file has changed, it will not be
> re-measured and added to the IMA measurement list on subsequent access.
> 
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> Acked-by: Mimi Zohar <zohar@us.ibm.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/attr.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff -puN fs/attr.c~vfs-increment-iversion-when-a-file-is-truncated fs/attr.c
> --- a/fs/attr.c~vfs-increment-iversion-when-a-file-is-truncated
> +++ a/fs/attr.c
> @@ -176,6 +176,11 @@ int notify_change(struct dentry * dentry
>  			return -EPERM;
>  	}
> 
> +	if ((ia_valid & ATTR_SIZE) && IS_I_VERSION(inode)) {
> +		if (attr->ia_size != inode->i_size)
> +			inode_inc_iversion(inode);
> +	}
> +
>  	if ((ia_valid & ATTR_MODE)) {
>  		umode_t amode = attr->ia_mode;
>  		/* Flag setting protected by i_mutex */
> _
> 

Re: [PATCH v3 1/1] vfs: iversion truncate bug fix

[Kasatkin, Dmitry]

On Fri, Jan 6, 2012 at 12:30 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 05 Jan 2012 10:39:41 -0800
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>
>> > > Please, go read Documentation/stable_kernel_rules.txt for how to
>> > > properly submit patches to the stable kernel tree.  The information here
>> > > needs to be in the patch changelog itself, not in some random email
>> > > thread that will get lost instantly into my email-archive-from-hell
>> > > after I am done reading this.
>> > >
>> > > greg k-h
>> >
>> > Yes, I've read Documentation/stable_kernel_rules.txt and think this
>> > patch meets the criteria for being backported.
>> >
>> > As far as I'm aware, this patch hasn't been upstreamed yet and is
>> > waiting for someone, besides myself, to Ack it.  Once Acked, either
>> > Dmitry or I can send a pull request with an updated patch description.
>> > Should this patch go in via the security tree?
>>
>> If it hasn't been upstreamed yet, just make sure you put
>>
>> cc: stable@kernel.org
>>
>> In the signoffs of the patch you're sending upstream and the backport
>> will occur automatically when the patch is finally upstreamed.
>
> Mimi didn't write or send the patch.
>
> This happily git-free maintainer simply goes in and edits the
> changelog.  I do this very very frequently.
>
>
> Here's what I currently have.  I plan to send this to Rip Van Viro.
>
>

Hello,

Should I do anything about re-submitting this patch now?
Or it went forward already?

Thanks,
Dmitry

>
>
> From: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> Subject: vfs: increment iversion when a file is truncated
>
> When a file is truncated with truncate()/ftruncate() and then closed,
> iversion is not updated.  This patch uses ATTR_SIZE flag as an indication
> to increment iversion.
>
> Mimi said:
>
> On fput(), i_version is used to detect and flag files that have changed
> and need to be re-measured in the IMA measurement policy.  When a file
> is truncated with truncate()/ftruncate() and then closed, i_version is
> not updated.  As a result, although the file has changed, it will not be
> re-measured and added to the IMA measurement list on subsequent access.
>
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>
> Acked-by: Mimi Zohar <zohar@us.ibm.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  fs/attr.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff -puN fs/attr.c~vfs-increment-iversion-when-a-file-is-truncated fs/attr.c
> --- a/fs/attr.c~vfs-increment-iversion-when-a-file-is-truncated
> +++ a/fs/attr.c
> @@ -176,6 +176,11 @@ int notify_change(struct dentry * dentry
>                        return -EPERM;
>        }
>
> +       if ((ia_valid & ATTR_SIZE) && IS_I_VERSION(inode)) {
> +               if (attr->ia_size != inode->i_size)
> +                       inode_inc_iversion(inode);
> +       }
> +
>        if ((ia_valid & ATTR_MODE)) {
>                umode_t amode = attr->ia_mode;
>                /* Flag setting protected by i_mutex */
> _