linux-ext4 - Re: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 Dec 2013 12:33:43 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	Carlos Maiolino <cmaiolino@...hat.com>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size

If actually prefer if we changed this function from checking the blocks count to checking the symlink length (i_size) to determine if this is a fast or slow symlink.

I don't think there has ever been a kernel that stores symlinks < 60 chars in an external block, and it would definitely avoid the many, many times this function has been wrong because of xattrs and other things that change the number of blocks allocated to an inode. Using the block count instead of i_size makes it impossible to change the number of blocks associated with a symlink without breaking older kernels (and I'm sure this will break again in the future, just as it did when xattrs started appearing on symlinks in the first place. I'd really prefer that we move away from that. 

Also, it doesn't seem obvious to me that a symlink in a bigalloc filesystem should account for ALL of the blocks in the cluster to the inode vs. just the blocks actually needed to store the symlink name. 

Cheers, Andreas

> On Dec 23, 2013, at 5:17, Carlos Maiolino <cmaiolino@...hat.com> wrote:
> 
>> On Fri, Dec 20, 2013 at 01:13:33PM +0800, Yongqiang Yang wrote:
>> From: Yongqiang Yang <xiaoqiangnk@...il.com>
>> 
>> can be reproduced by xfstests 62 with  bigalloc and 128bit size inode.
>> 
>> Signed-off-by: Yongqiang Yang <yangyongqiang01@...du.com>
>> ---
>> fs/ext4/inode.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 9115f28..1869fcf 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -145,7 +145,7 @@ static int ext4_meta_trans_blocks(struct inode
>> *inode, int lblocks,
>> static int ext4_inode_is_fast_symlink(struct inode *inode)
>> {
>>        int ea_blocks = EXT4_I(inode)->i_file_acl ?
>> -               (inode->i_sb->s_blocksize >> 9) : 0;
>> +                       EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;
> 
> Code looks good, but looks like it has an extra TAB here. Just a cosmetic thing;
> despite that, consider it
> 
> Reviewed-by: Carlos Maiolino <cmaiolino@...hat.com>
> 
> 
> -- 
> Carlos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html