[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tiger Yang]

Mark and Joel,

I found two serious bugs about xattr and inline-data.

the first bug:
in ocfs2_mknod(), we check and found the ACL or security xattr entry 
could be set into inode in ocfs2_calc_xattr_init(), then don't reserve 
block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support 
inline-data, then set id_count with the max_inline_data. After that, we 
set acl/security xattr entry in ocfs2_init_acl() or 
ocfs2_init_security_set(), but in there we found inode is full, then 
panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.

the second bug:
we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the 
inline data may overwrite the xattr entries which have already in inode.


thanks,
tiger

[Ocfs2-devel] [PATCH 1/2] ocfs2: reserve xattr block for directory inode in mknod

[Tiger Yang]

As we set directory inode starting with inline data,
there is no place in inode for ACLs or security extended
attributes entries. In this case, we have to reserve
xattr block at the begining of mknod.

Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
---
 fs/ocfs2/xattr.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 4ddd788..053ae3d 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -549,6 +549,7 @@ int ocfs2_calc_xattr_init(struct inode *dir,
 	 * for them is ok.
 	 */
 	if (dir->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE ||
+	    (S_ISDIR(mode) && ocfs2_supports_inline_data(osb)) ||
 	    (s_size + a_size) > OCFS2_XATTR_FREE_IN_IBODY) {
 		ret = ocfs2_reserve_new_metadata_blocks(osb, 1, xattr_ac);
 		if (ret) {
-- 
1.5.4.1

[Ocfs2-devel] [PATCH 1/2] ocfs2: reserve xattr block for directory inode in mknod

[Joel Becker]

On Wed, Mar 04, 2009 at 11:21:13AM +0800, Tiger Yang wrote:
> As we set directory inode starting with inline data,
> there is no place in inode for ACLs or security extended
> attributes entries. In this case, we have to reserve
> xattr block at the begining of mknod.
> 
> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>

	This is fine, but can you update the comment above?  Something
like "If this is a new directory with inline data, we choose to reserve
the entire inline area for directory contents and force an external
xattr block."

Joel

-- 

"War doesn't determine who's right; war determines who's left."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 2/2] ocfs2: fix check condition of max inline data

[Tiger Yang]

We should consider the inline xattr when
we try to write inline data.

Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
---
 fs/ocfs2/aops.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index a067a6c..dc6d734 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -1555,6 +1555,7 @@ static int ocfs2_try_to_write_inline_data(struct address_space *mapping,
 	int ret, written = 0;
 	loff_t end = pos + len;
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
+	struct ocfs2_dinode *di = NULL;
 
 	mlog(0, "Inode %llu, write of %u bytes at off %llu. features: 0x%x\n",
 	     (unsigned long long)oi->ip_blkno, len, (unsigned long long)pos,
@@ -1587,7 +1588,9 @@ static int ocfs2_try_to_write_inline_data(struct address_space *mapping,
 	/*
 	 * Check whether the write can fit.
 	 */
-	if (mmap_page || end > ocfs2_max_inline_data(inode->i_sb))
+	di = (struct ocfs2_dinode *)wc->w_di_bh->b_data;
+	if (mmap_page ||
+	    end > ocfs2_max_inline_data_with_xattr(inode->i_sb, di))
 		return 0;
 
 do_inline_write:
-- 
1.5.4.1

[Ocfs2-devel] [PATCH 2/2] ocfs2: fix check condition of max inline data

[Tao Ma]

Hi tiger,
	I just searched the code in ocfs2. There are other cases(in 
ocfs2_read_inline_data) which may have the problem also.

So I just think that since you have already added inlin-data-xattr 
support in ocfs2, why not remove the function ocfs2_max_inline_data and 
use ocfs2_max_inline_data_with_xattr instead in all the cases?  Make it 
more intelligent?

I am just worried that this function may be used wrongly afterwards and 
cause future bugs like this.

Regards,
Tao

Tiger Yang wrote:
> We should consider the inline xattr when
> we try to write inline data.
> 
> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
> ---
>  fs/ocfs2/aops.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index a067a6c..dc6d734 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -1555,6 +1555,7 @@ static int ocfs2_try_to_write_inline_data(struct address_space *mapping,
>  	int ret, written = 0;
>  	loff_t end = pos + len;
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> +	struct ocfs2_dinode *di = NULL;
>  
>  	mlog(0, "Inode %llu, write of %u bytes at off %llu. features: 0x%x\n",
>  	     (unsigned long long)oi->ip_blkno, len, (unsigned long long)pos,
> @@ -1587,7 +1588,9 @@ static int ocfs2_try_to_write_inline_data(struct address_space *mapping,
>  	/*
>  	 * Check whether the write can fit.
>  	 */
> -	if (mmap_page || end > ocfs2_max_inline_data(inode->i_sb))
> +	di = (struct ocfs2_dinode *)wc->w_di_bh->b_data;
> +	if (mmap_page ||
> +	    end > ocfs2_max_inline_data_with_xattr(inode->i_sb, di))
>  		return 0;
>  
>  do_inline_write:

[Ocfs2-devel] [PATCH 2/2] ocfs2: fix check condition of max inline data

[Tiger Yang]

Hi, Tao,

yes. ocfs2_max_inline_data_with_xattr() in read_inline_data() is more 
critical, I think ocfs2_max_inline_data() is also safe before.

I deliberately left ocfs2_max_inline_data() because in some case, like 
in mknod, di->i_dyn_features have not been set with 
OCFS2_INLINE_XATTR_FL or we couldn't get correct di in somewhere.

thanks,
tiger

Tao Ma wrote:
> Hi tiger,
>     I just searched the code in ocfs2. There are other cases(in 
> ocfs2_read_inline_data) which may have the problem also.
> 
> So I just think that since you have already added inlin-data-xattr 
> support in ocfs2, why not remove the function ocfs2_max_inline_data and 
> use ocfs2_max_inline_data_with_xattr instead in all the cases?  Make it 
> more intelligent?
> 
> I am just worried that this function may be used wrongly afterwards and 
> cause future bugs like this.
> 
> Regards,
> Tao
> 
> Tiger Yang wrote:
>> We should consider the inline xattr when
>> we try to write inline data.
>>
>> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
>> ---
>>  fs/ocfs2/aops.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
>> index a067a6c..dc6d734 100644
>> --- a/fs/ocfs2/aops.c
>> +++ b/fs/ocfs2/aops.c
>> @@ -1555,6 +1555,7 @@ static int ocfs2_try_to_write_inline_data(struct 
>> address_space *mapping,
>>      int ret, written = 0;
>>      loff_t end = pos + len;
>>      struct ocfs2_inode_info *oi = OCFS2_I(inode);
>> +    struct ocfs2_dinode *di = NULL;
>>  
>>      mlog(0, "Inode %llu, write of %u bytes at off %llu. features: 
>> 0x%x\n",
>>           (unsigned long long)oi->ip_blkno, len, (unsigned long long)pos,
>> @@ -1587,7 +1588,9 @@ static int ocfs2_try_to_write_inline_data(struct 
>> address_space *mapping,
>>      /*
>>       * Check whether the write can fit.
>>       */
>> -    if (mmap_page || end > ocfs2_max_inline_data(inode->i_sb))
>> +    di = (struct ocfs2_dinode *)wc->w_di_bh->b_data;
>> +    if (mmap_page ||
>> +        end > ocfs2_max_inline_data_with_xattr(inode->i_sb, di))
>>          return 0;
>>  
>>  do_inline_write:

[Ocfs2-devel] [PATCH 2/2] ocfs2: fix check condition of max inline data

[Tao Ma]

Tiger Yang wrote:
> Hi, Tao,
> 
> yes. ocfs2_max_inline_data_with_xattr() in read_inline_data() is more 
> critical, I think ocfs2_max_inline_data() is also safe before.
> 
> I deliberately left ocfs2_max_inline_data() because in some case, like 
> in mknod, di->i_dyn_features have not been set with 
> OCFS2_INLINE_XATTR_FL or we couldn't get correct di in somewhere.
yes, I already noticed it. But as I have said in the previous mail, 
could you please make it more intelligent? in mknod, we know all the 
cases so we can do it.

Regards,
Tao
> 
> thanks,
> tiger
> 
> Tao Ma wrote:
>> Hi tiger,
>>     I just searched the code in ocfs2. There are other cases(in 
>> ocfs2_read_inline_data) which may have the problem also.
>>
>> So I just think that since you have already added inlin-data-xattr 
>> support in ocfs2, why not remove the function ocfs2_max_inline_data 
>> and use ocfs2_max_inline_data_with_xattr instead in all the cases?  
>> Make it more intelligent?
>>
>> I am just worried that this function may be used wrongly afterwards 
>> and cause future bugs like this.
>>
>> Regards,
>> Tao
>>
>> Tiger Yang wrote:
>>> We should consider the inline xattr when
>>> we try to write inline data.
>>>
>>> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
>>> ---
>>>  fs/ocfs2/aops.c |    5 ++++-
>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
>>> index a067a6c..dc6d734 100644
>>> --- a/fs/ocfs2/aops.c
>>> +++ b/fs/ocfs2/aops.c
>>> @@ -1555,6 +1555,7 @@ static int 
>>> ocfs2_try_to_write_inline_data(struct address_space *mapping,
>>>      int ret, written = 0;
>>>      loff_t end = pos + len;
>>>      struct ocfs2_inode_info *oi = OCFS2_I(inode);
>>> +    struct ocfs2_dinode *di = NULL;
>>>  
>>>      mlog(0, "Inode %llu, write of %u bytes at off %llu. features: 
>>> 0x%x\n",
>>>           (unsigned long long)oi->ip_blkno, len, (unsigned long 
>>> long)pos,
>>> @@ -1587,7 +1588,9 @@ static int 
>>> ocfs2_try_to_write_inline_data(struct address_space *mapping,
>>>      /*
>>>       * Check whether the write can fit.
>>>       */
>>> -    if (mmap_page || end > ocfs2_max_inline_data(inode->i_sb))
>>> +    di = (struct ocfs2_dinode *)wc->w_di_bh->b_data;
>>> +    if (mmap_page ||
>>> +        end > ocfs2_max_inline_data_with_xattr(inode->i_sb, di))
>>>          return 0;
>>>  
>>>  do_inline_write:

[Ocfs2-devel] [PATCH 2/2] ocfs2: fix check condition of max inline data

[Joel Becker]

On Wed, Mar 04, 2009 at 01:49:13PM +0800, Tao Ma wrote:
> Tiger Yang wrote:
> > Hi, Tao,
> > 
> > yes. ocfs2_max_inline_data_with_xattr() in read_inline_data() is more 
> > critical, I think ocfs2_max_inline_data() is also safe before.
> > 
> > I deliberately left ocfs2_max_inline_data() because in some case, like 
> > in mknod, di->i_dyn_features have not been set with 
> > OCFS2_INLINE_XATTR_FL or we couldn't get correct di in somewhere.
> yes, I already noticed it. But as I have said in the previous mail, 
> could you please make it more intelligent? in mknod, we know all the 
> cases so we can do it.

	I just looked, and they all have the di (well, it's called fe in
mknod_locked, but it is still there.  the aops.c functions have it on
the write_ctxt).  I agree with Tao, max_inline_data_with_xattr() is
always the correct function.
	Btw, in mknod, it's safe to call.  The INLINE_XATTR_FL will
*not* be set, which is correct with regards to your previous patch (that
makes sure a block is reserved for the xattrs on an inline directory).
	Good catch!

Joel

-- 

"Win95 file and print sharing are for relatively friendly nets."
	- Paul Leach, Microsoft

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Joel Becker]

On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
> I found two serious bugs about xattr and inline-data.

Tristan,
	Can you add tests for this in the xattr suite?  Thanks!

Joel

>
> the first bug:
> in ocfs2_mknod(), we check and found the ACL or security xattr entry  
> could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
> block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
> inline-data, then set id_count with the max_inline_data. After that, we  
> set acl/security xattr entry in ocfs2_init_acl() or  
> ocfs2_init_security_set(), but in there we found inode is full, then  
> panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.
>
> the second bug:
> we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
> inline data may overwrite the xattr entries which have already in inode.
>
>
> thanks,
> tiger

-- 

"Here's something to think about:  How come you never see a headline
 like ``Psychic Wins Lottery''?"
	- Jay Leno

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[tristan.ye]

On Wed, 2009-03-04 at 18:36 -0800, Joel Becker wrote:
> On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
> > I found two serious bugs about xattr and inline-data.
> 
> Tristan,
> 	Can you add tests for this in the xattr suite?  Thanks!

Of course, I'll be looking back to my testcases, and add these if
missed.

Actually, I've already had inline-data&inline-xattr combination tests in
xattr testing suite, how can I misse such boundary cases?

Tiger,

Thanks for posting the cases:)

Regards,
Tristan



> 
> Joel
> 
> >
> > the first bug:
> > in ocfs2_mknod(), we check and found the ACL or security xattr entry  
> > could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
> > block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
> > inline-data, then set id_count with the max_inline_data. After that, we  
> > set acl/security xattr entry in ocfs2_init_acl() or  
> > ocfs2_init_security_set(), but in there we found inode is full, then  
> > panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.
> >
> > the second bug:
> > we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
> > inline data may overwrite the xattr entries which have already in inode.
> >
> >
> > thanks,
> > tiger
> 

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tao Ma]

Hi tristan,
	the first bug should happen when you create a directory with acl. So do 
you have the test case for that?
	The second one should happen when you try to try to the inline file and 
then you will lose the xattr entry you set.

Regards,
Tao

tristan.ye wrote:
> On Wed, 2009-03-04 at 18:36 -0800, Joel Becker wrote:
>> On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
>>> I found two serious bugs about xattr and inline-data.
>> Tristan,
>> 	Can you add tests for this in the xattr suite?  Thanks!
> 
> Of course, I'll be looking back to my testcases, and add these if
> missed.
> 
> Actually, I've already had inline-data&inline-xattr combination tests in
> xattr testing suite, how can I misse such boundary cases?
> 
> Tiger,
> 
> Thanks for posting the cases:)
> 
> Regards,
> Tristan
> 
> 
> 
>> Joel
>>
>>> the first bug:
>>> in ocfs2_mknod(), we check and found the ACL or security xattr entry  
>>> could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
>>> block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
>>> inline-data, then set id_count with the max_inline_data. After that, we  
>>> set acl/security xattr entry in ocfs2_init_acl() or  
>>> ocfs2_init_security_set(), but in there we found inode is full, then  
>>> panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.
>>>
>>> the second bug:
>>> we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
>>> inline data may overwrite the xattr entries which have already in inode.
>>>
>>>
>>> thanks,
>>> tiger
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[tristan.ye]

On Mon, 2009-03-09 at 12:57 +0800, Tao Ma wrote:
> Hi tristan,
> 	the first bug should happen when you create a directory with acl. So do 
> you have the test case for that?

Yes, I can add this.
> 	The second one should happen when you try to try to the inline file and 
> then you will lose the xattr entry you set.

Actually, I've had similar testcase, and it never failed to me, that
means a previously set xattr entry which has reserved 256 bytes in inode
never be affected or overwriten by the following inline-data writing
attempt when i_size > MAX_INLINE_SIZE - 256.


Regards,
Tristan




> 
> Regards,
> Tao
> 
> tristan.ye wrote:
> > On Wed, 2009-03-04 at 18:36 -0800, Joel Becker wrote:
> >> On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
> >>> I found two serious bugs about xattr and inline-data.
> >> Tristan,
> >> 	Can you add tests for this in the xattr suite?  Thanks!
> > 
> > Of course, I'll be looking back to my testcases, and add these if
> > missed.
> > 
> > Actually, I've already had inline-data&inline-xattr combination tests in
> > xattr testing suite, how can I misse such boundary cases?
> > 
> > Tiger,
> > 
> > Thanks for posting the cases:)
> > 
> > Regards,
> > Tristan
> > 
> > 
> > 
> >> Joel
> >>
> >>> the first bug:
> >>> in ocfs2_mknod(), we check and found the ACL or security xattr entry  
> >>> could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
> >>> block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
> >>> inline-data, then set id_count with the max_inline_data. After that, we  
> >>> set acl/security xattr entry in ocfs2_init_acl() or  
> >>> ocfs2_init_security_set(), but in there we found inode is full, then  
> >>> panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.
> >>>
> >>> the second bug:
> >>> we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
> >>> inline data may overwrite the xattr entries which have already in inode.
> >>>
> >>>
> >>> thanks,
> >>> tiger
> > 
> > 
> > _______________________________________________
> > Ocfs2-devel mailing list
> > Ocfs2-devel at oss.oracle.com
> > http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tao Ma]

tristan.ye wrote:
> On Mon, 2009-03-09 at 12:57 +0800, Tao Ma wrote:
>> Hi tristan,
>> 	the first bug should happen when you create a directory with acl. So do 
>> you have the test case for that?
> 
> Yes, I can add this.
>> 	The second one should happen when you try to try to the inline file and 
>> then you will lose the xattr entry you set.
> 
> Actually, I've had similar testcase, and it never failed to me, that
> means a previously set xattr entry which has reserved 256 bytes in inode
> never be affected or overwriten by the following inline-data writing
> attempt when i_size > MAX_INLINE_SIZE - 256.
Just go through the code and guess  ;) the process should be:
1. fill in some inline data.
2. set entry
3. write i_size > MAX_INLINE_SIZE - 256.

I guess your case miss step 1.

Tiger,
	Can bug 2 reproduced by my guess steps?

Regards,
Tao
> 
> 
> Regards,
> Tristan
> 
> 
> 
> 
>> Regards,
>> Tao
>>
>> tristan.ye wrote:
>>> On Wed, 2009-03-04 at 18:36 -0800, Joel Becker wrote:
>>>> On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
>>>>> I found two serious bugs about xattr and inline-data.
>>>> Tristan,
>>>> 	Can you add tests for this in the xattr suite?  Thanks!
>>> Of course, I'll be looking back to my testcases, and add these if
>>> missed.
>>>
>>> Actually, I've already had inline-data&inline-xattr combination tests in
>>> xattr testing suite, how can I misse such boundary cases?
>>>
>>> Tiger,
>>>
>>> Thanks for posting the cases:)
>>>
>>> Regards,
>>> Tristan
>>>
>>>
>>>
>>>> Joel
>>>>
>>>>> the first bug:
>>>>> in ocfs2_mknod(), we check and found the ACL or security xattr entry  
>>>>> could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
>>>>> block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
>>>>> inline-data, then set id_count with the max_inline_data. After that, we  
>>>>> set acl/security xattr entry in ocfs2_init_acl() or  
>>>>> ocfs2_init_security_set(), but in there we found inode is full, then  
>>>>> panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.
>>>>>
>>>>> the second bug:
>>>>> we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
>>>>> inline data may overwrite the xattr entries which have already in inode.
>>>>>
>>>>>
>>>>> thanks,
>>>>> tiger
>>>
>>> _______________________________________________
>>> Ocfs2-devel mailing list
>>> Ocfs2-devel at oss.oracle.com
>>> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[tristan.ye]

On Mon, 2009-03-09 at 13:42 +0800, Tao Ma wrote:
> 
> tristan.ye wrote:
> > On Mon, 2009-03-09 at 12:57 +0800, Tao Ma wrote:
> >> Hi tristan,
> >> 	the first bug should happen when you create a directory with acl. So do 
> >> you have the test case for that?
> > 
> > Yes, I can add this.
> >> 	The second one should happen when you try to try to the inline file and 
> >> then you will lose the xattr entry you set.
> > 
> > Actually, I've had similar testcase, and it never failed to me, that
> > means a previously set xattr entry which has reserved 256 bytes in inode
> > never be affected or overwriten by the following inline-data writing
> > attempt when i_size > MAX_INLINE_SIZE - 256.
> Just go through the code and guess  ;) the process should be:
> 1. fill in some inline data.
> 2. set entry
> 3. write i_size > MAX_INLINE_SIZE - 256.

I still can not reproduce this bug with your testing step on joel's
cacheme branch, maybe it is a regression bug on mainline kernel, tiger,
how did you think about it?


> 
> I guess your case miss step 1.
> 
> Tiger,
> 	Can bug 2 reproduced by my guess steps?
> 
> Regards,
> Tao
> > 
> > 
> > Regards,
> > Tristan
> > 
> > 
> > 
> > 
> >> Regards,
> >> Tao
> >>
> >> tristan.ye wrote:
> >>> On Wed, 2009-03-04 at 18:36 -0800, Joel Becker wrote:
> >>>> On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
> >>>>> I found two serious bugs about xattr and inline-data.
> >>>> Tristan,
> >>>> 	Can you add tests for this in the xattr suite?  Thanks!
> >>> Of course, I'll be looking back to my testcases, and add these if
> >>> missed.
> >>>
> >>> Actually, I've already had inline-data&inline-xattr combination tests in
> >>> xattr testing suite, how can I misse such boundary cases?
> >>>
> >>> Tiger,
> >>>
> >>> Thanks for posting the cases:)
> >>>
> >>> Regards,
> >>> Tristan
> >>>
> >>>
> >>>
> >>>> Joel
> >>>>
> >>>>> the first bug:
> >>>>> in ocfs2_mknod(), we check and found the ACL or security xattr entry  
> >>>>> could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
> >>>>> block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
> >>>>> inline-data, then set id_count with the max_inline_data. After that, we  
> >>>>> set acl/security xattr entry in ocfs2_init_acl() or  
> >>>>> ocfs2_init_security_set(), but in there we found inode is full, then  
> >>>>> panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.
> >>>>>
> >>>>> the second bug:
> >>>>> we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
> >>>>> inline data may overwrite the xattr entries which have already in inode.
> >>>>>
> >>>>>
> >>>>> thanks,
> >>>>> tiger
> >>>
> >>> _______________________________________________
> >>> Ocfs2-devel mailing list
> >>> Ocfs2-devel at oss.oracle.com
> >>> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
> > 

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tiger Yang]

Hi, all,

I found these two bugs in latest mainline kernel not other branch.

thanks,
tiger

tristan.ye wrote:
> On Mon, 2009-03-09 at 13:42 +0800, Tao Ma wrote:
>> tristan.ye wrote:
>>> On Mon, 2009-03-09 at 12:57 +0800, Tao Ma wrote:
>>>> Hi tristan,
>>>> 	the first bug should happen when you create a directory with acl. So do 
>>>> you have the test case for that?
>>> Yes, I can add this.
>>>> 	The second one should happen when you try to try to the inline file and 
>>>> then you will lose the xattr entry you set.
>>> Actually, I've had similar testcase, and it never failed to me, that
>>> means a previously set xattr entry which has reserved 256 bytes in inode
>>> never be affected or overwriten by the following inline-data writing
>>> attempt when i_size > MAX_INLINE_SIZE - 256.
>> Just go through the code and guess  ;) the process should be:
>> 1. fill in some inline data.
>> 2. set entry
>> 3. write i_size > MAX_INLINE_SIZE - 256.
> 
> I still can not reproduce this bug with your testing step on joel's
> cacheme branch, maybe it is a regression bug on mainline kernel, tiger,
> how did you think about it?
> 
> 
>> I guess your case miss step 1.
>>
>> Tiger,
>> 	Can bug 2 reproduced by my guess steps?
>>
>> Regards,
>> Tao
>>>
>>> Regards,
>>> Tristan
>>>
>>>
>>>
>>>
>>>> Regards,
>>>> Tao
>>>>
>>>> tristan.ye wrote:
>>>>> On Wed, 2009-03-04 at 18:36 -0800, Joel Becker wrote:
>>>>>> On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
>>>>>>> I found two serious bugs about xattr and inline-data.
>>>>>> Tristan,
>>>>>> 	Can you add tests for this in the xattr suite?  Thanks!
>>>>> Of course, I'll be looking back to my testcases, and add these if
>>>>> missed.
>>>>>
>>>>> Actually, I've already had inline-data&inline-xattr combination tests in
>>>>> xattr testing suite, how can I misse such boundary cases?
>>>>>
>>>>> Tiger,
>>>>>
>>>>> Thanks for posting the cases:)
>>>>>
>>>>> Regards,
>>>>> Tristan
>>>>>
>>>>>
>>>>>
>>>>>> Joel
>>>>>>
>>>>>>> the first bug:
>>>>>>> in ocfs2_mknod(), we check and found the ACL or security xattr entry  
>>>>>>> could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
>>>>>>> block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
>>>>>>> inline-data, then set id_count with the max_inline_data. After that, we  
>>>>>>> set acl/security xattr entry in ocfs2_init_acl() or  
>>>>>>> ocfs2_init_security_set(), but in there we found inode is full, then  
>>>>>>> panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.
>>>>>>>
>>>>>>> the second bug:
>>>>>>> we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
>>>>>>> inline data may overwrite the xattr entries which have already in inode.
>>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>> tiger
>>>>> _______________________________________________
>>>>> Ocfs2-devel mailing list
>>>>> Ocfs2-devel at oss.oracle.com
>>>>> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Joel Becker]

On Mon, Mar 09, 2009 at 02:35:36PM +0800, Tiger Yang wrote:
> I found these two bugs in latest mainline kernel not other branch.

	The bugs should still be in cacheme - cacheme doesn't change
the inline data code.

Joel

-- 

"One of the symptoms of an approaching nervous breakdown is the
 belief that one's work is terribly important."
         - Bertrand Russell 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[tristan.ye]

On Sun, 2009-03-08 at 23:47 -0700, Joel Becker wrote:
> On Mon, Mar 09, 2009 at 02:35:36PM +0800, Tiger Yang wrote:
> > I found these two bugs in latest mainline kernel not other branch.
> 
> 	The bugs should still be in cacheme - cacheme doesn't change
> the inline data code.

Fairly strange, I've already done the tests on cacheme branch, no such
bugs happened to me...

Tristan

> 
> Joel
> 

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tao Ma]

Hi Tiger,
	so could you please describe how to reproduce these 2 bugs step by 
step? It would be easy for tristan to code them down and add them to the 
test case. And also we all can see whether we can reproduce them in our 
own env.

Regards,
Tao

tristan.ye wrote:
> On Sun, 2009-03-08 at 23:47 -0700, Joel Becker wrote:
>> On Mon, Mar 09, 2009 at 02:35:36PM +0800, Tiger Yang wrote:
>>> I found these two bugs in latest mainline kernel not other branch.
>> 	The bugs should still be in cacheme - cacheme doesn't change
>> the inline data code.
> 
> Fairly strange, I've already done the tests on cacheme branch, no such
> bugs happened to me...
> 
> Tristan
> 
>> Joel
>>
> 

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tiger Yang]

Hi,

I think Tristan's test script could catch these two bugs against 
mainline. He didn't run his test script in mainline yet.

bug1:
Just create a file in a directory which has default ACLs.

bug2:
1. touch a file in a file system which blocksize large than 512.
2. set xattr in it.
3. write i_size > (MAX_INLINE_SIZE - 256).
then you can find the inline data overwrite the xattr in inode.

thanks,
tiger

Tao Ma wrote:
> Hi Tiger,
>     so could you please describe how to reproduce these 2 bugs step by 
> step? It would be easy for tristan to code them down and add them to the 
> test case. And also we all can see whether we can reproduce them in our 
> own env.
> 
> Regards,
> Tao
> 
> tristan.ye wrote:
>> On Sun, 2009-03-08 at 23:47 -0700, Joel Becker wrote:
>>> On Mon, Mar 09, 2009 at 02:35:36PM +0800, Tiger Yang wrote:
>>>> I found these two bugs in latest mainline kernel not other branch.
>>>     The bugs should still be in cacheme - cacheme doesn't change
>>> the inline data code.
>>
>> Fairly strange, I've already done the tests on cacheme branch, no such
>> bugs happened to me...
>>
>> Tristan
>>
>>> Joel
>>>
>>

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tiger Yang]

Hi, all,

Finally, Tristan and I found the reason why he could not reproduce bug2.
It's very interesting.

He runs the command "dd if=/dev/zero of=testfile bs=1 count=3641" which 
can not hit the bug2. But I found "dd if=/dev/zero of=testfile bs=3641 
count=1" will hit the bug2.

I trace the source code in ocfs2_try_to_write_inline_data(). Those two 
commands run the different way in kernel. In first situation, the codes 
going into "handle inodes which already have inline data 1st", then 
fault at ocfs2_size_fits_inline_data(), then go 
ocfs2_conver_inline_data_to_extents(). In second situation, It passed 
the wrong check about inline data size: ocfs2_max_inline_data(), then 
cause the fault.

As we discussed, Tristan will add the new command into his test script. 
Meanwhile he will reserve the old one because it cover more codes.

thanks,
tiger

Tiger Yang wrote:
> Hi,
> 
> I think Tristan's test script could catch these two bugs against 
> mainline. He didn't run his test script in mainline yet.
> 
> bug1:
> Just create a file in a directory which has default ACLs.
> 
> bug2:
> 1. touch a file in a file system which blocksize large than 512.
> 2. set xattr in it.
> 3. write i_size > (MAX_INLINE_SIZE - 256).
> then you can find the inline data overwrite the xattr in inode.
> 
> thanks,
> tiger
> 
> Tao Ma wrote:
>> Hi Tiger,
>>     so could you please describe how to reproduce these 2 bugs step by 
>> step? It would be easy for tristan to code them down and add them to the 
>> test case. And also we all can see whether we can reproduce them in our 
>> own env.
>>
>> Regards,
>> Tao
>>
>> tristan.ye wrote:
>>> On Sun, 2009-03-08 at 23:47 -0700, Joel Becker wrote:
>>>> On Mon, Mar 09, 2009 at 02:35:36PM +0800, Tiger Yang wrote:
>>>>> I found these two bugs in latest mainline kernel not other branch.
>>>>     The bugs should still be in cacheme - cacheme doesn't change
>>>> the inline data code.
>>> Fairly strange, I've already done the tests on cacheme branch, no such
>>> bugs happened to me...
>>>
>>> Tristan
>>>
>>>> Joel
>>>>
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Wengang Wang]

> He runs the command "dd if=/dev/zero of=testfile bs=1 count=3641" which 
> can not hit the bug2. But I found "dd if=/dev/zero of=testfile bs=3641 
> count=1" will hit the bug2.

3641, cool number!

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[Tao Ma]

Wengang Wang wrote:
>> He runs the command "dd if=/dev/zero of=testfile bs=1 count=3641" which 
>> can not hit the bug2. But I found "dd if=/dev/zero of=testfile bs=3641 
>> count=1" will hit the bug2.
> 
> 3641, cool number!
Hey, 3641 is the max_inline_size_with_xattr + 1 when bs = 4K. ;) So you 
see, they create it intentionally.

Regards,
Tao

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[tristan.ye]

On Mon, 2009-03-09 at 18:36 +0800, Tiger Yang wrote:
> Hi, all,
> 
> Finally, Tristan and I found the reason why he could not reproduce bug2.
> It's very interesting.
> 
> He runs the command "dd if=/dev/zero of=testfile bs=1 count=3641" which 
> can not hit the bug2. But I found "dd if=/dev/zero of=testfile bs=3641 
> count=1" will hit the bug2.
> 
> I trace the source code in ocfs2_try_to_write_inline_data(). Those two 
> commands run the different way in kernel. In first situation, the codes 
> going into "handle inodes which already have inline data 1st", then 
> fault at ocfs2_size_fits_inline_data(), then go 
> ocfs2_conver_inline_data_to_extents(). In second situation, It passed 
> the wrong check about inline data size: ocfs2_max_inline_data(), then 
> cause the fault.
> 
> As we discussed, Tristan will add the new command into his test script. 
> Meanwhile he will reserve the old one because it cover more codes.

Cool, tiger.

After testing, these 2 bugs can be reproduced both on mainline and
joel's cacheme kernel tree,for bug1, acl_tests in ocfs2-test is good
enough to expose. for bug2, as you suggested, I may add a extra testcase
later.

Thanks,
Tristan

> 
> thanks,
> tiger
> 
> Tiger Yang wrote:
> > Hi,
> > 
> > I think Tristan's test script could catch these two bugs against 
> > mainline. He didn't run his test script in mainline yet.
> > 
> > bug1:
> > Just create a file in a directory which has default ACLs.
> > 
> > bug2:
> > 1. touch a file in a file system which blocksize large than 512.
> > 2. set xattr in it.
> > 3. write i_size > (MAX_INLINE_SIZE - 256).
> > then you can find the inline data overwrite the xattr in inode.
> > 
> > thanks,
> > tiger
> > 
> > Tao Ma wrote:
> >> Hi Tiger,
> >>     so could you please describe how to reproduce these 2 bugs step by 
> >> step? It would be easy for tristan to code them down and add them to the 
> >> test case. And also we all can see whether we can reproduce them in our 
> >> own env.
> >>
> >> Regards,
> >> Tao
> >>
> >> tristan.ye wrote:
> >>> On Sun, 2009-03-08 at 23:47 -0700, Joel Becker wrote:
> >>>> On Mon, Mar 09, 2009 at 02:35:36PM +0800, Tiger Yang wrote:
> >>>>> I found these two bugs in latest mainline kernel not other branch.
> >>>>     The bugs should still be in cacheme - cacheme doesn't change
> >>>> the inline data code.
> >>> Fairly strange, I've already done the tests on cacheme branch, no such
> >>> bugs happened to me...
> >>>
> >>> Tristan
> >>>
> >>>> Joel
> >>>>
> > 
> > _______________________________________________
> > Ocfs2-devel mailing list
> > Ocfs2-devel at oss.oracle.com
> > http://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH 0/2] ocfs2: two bug fixes about xattr and inline-data

[tristan.ye]

On Wed, 2009-03-04 at 18:36 -0800, Joel Becker wrote:
> On Wed, Mar 04, 2009 at 11:18:19AM +0800, Tiger Yang wrote:
> > I found two serious bugs about xattr and inline-data.
> 
> Tristan,
> 	Can you add tests for this in the xattr suite?  Thanks!
> 
> Joel
> 
> >
> > the first bug:
> > in ocfs2_mknod(), we check and found the ACL or security xattr entry  
> > could be set into inode in ocfs2_calc_xattr_init(), then don't reserve  
> > block for them. But in ocfs2_mknod_locked(), if we found ocfs2 support  
> > inline-data, then set id_count with the max_inline_data. After that, we  
> > set acl/security xattr entry in ocfs2_init_acl() or  
> > ocfs2_init_security_set(), but in there we found inode is full, then  
> > panic at ocfs2_claim_metadata in ocfs2_xattr_block_set.

Actually, my acl tests have included such case in default acl test by
looking back to my previous testcases.
ocfs2-test/programs/acl_tests/acl_tests.sh

> >
> > the second bug:
> > we don't check inline xattr in ocfs2_try_to_write_inline_data(), so the  
> > inline data may overwrite the xattr entries which have already in inode.

And xattr's combination test has had it already too:-)


Regards,
Tristan

> >
> >
> > thanks,
> > tiger
>