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

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

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 at 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

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

Signed-off-by: Tiger Yang <tiger.yang at 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

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 at 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:

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

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