[PATCH 1/1] do_truncate needs to increment iversion

[PATCH 1/1] do_truncate needs to increment iversion

[Dmitry Kasatkin]

IMA/EVM uses iversion to identify if file content has been changed.
It has been found that when file is opened, truncated with ftruncate()
and then closed, iversion is not updated.

This patch adds iversion incrementation to do_truncate().

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

diff --git a/fs/open.c b/fs/open.c
index 22c41b5..d7e8df2 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -56,6 +56,8 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 		newattrs.ia_valid |= ret | ATTR_FORCE;
 
 	mutex_lock(&dentry->d_inode->i_mutex);
+	if (IS_I_VERSION(dentry->d_inode))
+		inode_inc_iversion(dentry->d_inode);
 	ret = notify_change(dentry, &newattrs);
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return ret;
-- 
1.7.4.1

Re: [PATCH 1/1] do_truncate needs to increment iversion

[Andrew Morton]

On Wed, 14 Dec 2011 18:44:57 +0200
Dmitry Kasatkin <dmitry.kasatkin@intel.com> wrote:

> IMA/EVM uses iversion to identify if file content has been changed.
> It has been found that when file is opened, truncated with ftruncate()
> and then closed, iversion is not updated.
> 
> This patch adds iversion incrementation to do_truncate().

I confess that I've never really fully understood what i_version is
supposed to do, so I'm not in any position to comment on where, when
and why the thing should be updated.

At present it gets updated in file_update_time() so I suppose that
we're not calling file_update_time() in the ftruncate case, which
sounds wrong?

It's notable that file_update_time() will not update i_version if the
inode has NOCMTIME.  This might be wrong, but one would need to know
the intent of i_version to be able to say.  Do your files have
S_NOCMTIME set?

> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -56,6 +56,8 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>  		newattrs.ia_valid |= ret | ATTR_FORCE;
>  
>  	mutex_lock(&dentry->d_inode->i_mutex);
> +	if (IS_I_VERSION(dentry->d_inode))
> +		inode_inc_iversion(dentry->d_inode);
>  	ret = notify_change(dentry, &newattrs);
>  	mutex_unlock(&dentry->d_inode->i_mutex);
>  	return ret;

With four evaluations, it is time to cache dentry->d_inode in a local.

Re: [PATCH 1/1] do_truncate needs to increment iversion

[Kasatkin, Dmitry]

On Thu, Dec 15, 2011 at 11:55 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 14 Dec 2011 18:44:57 +0200
> Dmitry Kasatkin <dmitry.kasatkin@intel.com> wrote:
>
>> IMA/EVM uses iversion to identify if file content has been changed.
>> It has been found that when file is opened, truncated with ftruncate()
>> and then closed, iversion is not updated.
>>
>> This patch adds iversion incrementation to do_truncate().
>
> I confess that I've never really fully understood what i_version is
> supposed to do, so I'm not in any position to comment on where, when
> and why the thing should be updated.
>
> At present it gets updated in file_update_time() so I suppose that
> we're not calling file_update_time() in the ftruncate case, which
> sounds wrong?

What I see from notify_change(), it updates the time...
So what is really missing is incrimination of i_version...

>
> It's notable that file_update_time() will not update i_version if the
> inode has NOCMTIME.  This might be wrong, but one would need to know
> the intent of i_version to be able to say.  Do your files have
> S_NOCMTIME set?

Yes. I saw it and even made a test to verify if is the reason for
i_version not to be updated...
But it was not set.
I have the same feeling that i_version should be updated despite NOCMTIME,
because i_version supposed to work if mounted with "iversion" option....
I think it should not have dependency to other mount options...

>
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -56,6 +56,8 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>>               newattrs.ia_valid |= ret | ATTR_FORCE;
>>
>>       mutex_lock(&dentry->d_inode->i_mutex);
>> +     if (IS_I_VERSION(dentry->d_inode))
>> +             inode_inc_iversion(dentry->d_inode);
>>       ret = notify_change(dentry, &newattrs);
>>       mutex_unlock(&dentry->d_inode->i_mutex);
>>       return ret;
>
> With four evaluations, it is time to cache dentry->d_inode in a local.

True...

Thanks..
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] do_truncate needs to increment iversion

[Kasatkin, Dmitry]

On Thu, Dec 15, 2011 at 11:55 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 14 Dec 2011 18:44:57 +0200
> Dmitry Kasatkin <dmitry.kasatkin@intel.com> wrote:
>
>> IMA/EVM uses iversion to identify if file content has been changed.
>> It has been found that when file is opened, truncated with ftruncate()
>> and then closed, iversion is not updated.
>>
>> This patch adds iversion incrementation to do_truncate().
>
> I confess that I've never really fully understood what i_version is
> supposed to do, so I'm not in any position to comment on where, when
> and why the thing should be updated.
>
> At present it gets updated in file_update_time() so I suppose that
> we're not calling file_update_time() in the ftruncate case, which
> sounds wrong?

What I see from notify_change(), it updates the time...
So what is really missing is incrimination of i_version...

>
> It's notable that file_update_time() will not update i_version if the
> inode has NOCMTIME.  This might be wrong, but one would need to know
> the intent of i_version to be able to say.  Do your files have
> S_NOCMTIME set?

Yes. I saw it and even made a test to verify if is the reason for
i_version not to be updated...
But it was not set.
I have the same feeling that i_version should be updated despite NOCMTIME,
because i_version supposed to work if mounted with "iversion" option....
I think it should not have dependency to other mount options...

>
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -56,6 +56,8 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>>               newattrs.ia_valid |= ret | ATTR_FORCE;
>>
>>       mutex_lock(&dentry->d_inode->i_mutex);
>> +     if (IS_I_VERSION(dentry->d_inode))
>> +             inode_inc_iversion(dentry->d_inode);
>>       ret = notify_change(dentry, &newattrs);
>>       mutex_unlock(&dentry->d_inode->i_mutex);
>>       return ret;
>
> With four evaluations, it is time to cache dentry->d_inode in a local.

True...

Thanks..