linux-kernel - Re: [PATCH v2] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

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]

Message-ID: <CAKYAXd8Q38vG83Hc2jB3tM9xjkgWH=dNae6qLTuVceBGqRvdsQ@mail.gmail.com>
Date:	Fri, 14 Dec 2012 14:41:20 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	jaegeuk.kim@...sung.com
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-f2fs-devel@...ts.sourceforge.net,
	Namjae Jeon <namjae.jeon@...sung.com>,
	Amit Sahrawat <a.sahrawat@...sung.com>
Subject: Re: [PATCH v2] f2fs: fix up f2fs_get_parent issue to retrieve correct
 parent inode number

2012/12/14, Jaegeuk Kim <jaegeuk.kim@...sung.com>:
> Hi,
>
>> diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c
>> index a60f042..5e48bac 100644
>> --- a/fs/f2fs/hash.c
>> +++ b/fs/f2fs/hash.c
>> @@ -76,6 +76,10 @@ f2fs_hash_t f2fs_dentry_hash(const char *name, int
>> len)
>>  	const char *p;
>>  	__u32 in[8], buf[4];
>>
>> +	if ((len <= 2) && (name[0] == '.') &&
>> +		(name[1] == '.' || name[1] == '\0'))
>> +		return 0;
>
> If len == 1, we should avoid referencing name[1].
> Likewise VFS does, I rewrote that like below.
>
> 	if (name[0] == '.') {
> 		switch (len) {
> 		case 1:
> 			return 0;
> 		case 2:
> 			if (name[1] == '.')
> 				return 0;
> 		}
> 	}
>
> So, how about this patch?

I think that there is no issue on current patch. Since, the strings
are always expected to be NULL terminated.

"." should include '\0', So we can distingsh by checking only name[0], name[1].

When we do:
char *ptr="hello"; -> it will always be NULL terminated -> "hello" in
memory followed by '\0';
when we reserver space
char ptr[5];-> We need to reserver space for '\0' at the end.

Also, even if we donot encounter NULL at the end.
there should be no problem referencing the memory, it is just we can
get garbage value from that refrencing.

May be I am not able to think the code path which will cause the problem.
Is it possible that "." is not null termindated ?

Otherwise, in my opinion - we can retain the suggested changes, there
is no need to introducing switch case, even
though technically there is no issues in the code you suggested also.

So, final decision is yours.

Thanks.

>
> From 391d584afadfb177584a3a3e4a7ec97e1a674457 Mon Sep 17 00:00:00 2001
> From: Namjae Jeon <namjae.jeon@...sung.com>
> Date: Thu, 13 Dec 2012 23:44:11 +0900
> Subject: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct
> parent
>  inode number
>
> Test Case:
> [NFS Client]
> ls -lR .
>
> [NFS Server]
> while [ 1 ]
> do
> echo 3 > /proc/sys/vm/drop_caches
> done
>
> Error on NFS Client: "No such file or directory"
>
> When cache is dropped at the server, it results in lookup failure at the
> NFS client due to non-connection with the parent. The default path is it
> initiates a lookup by calculating the hash value for the name, even
> though
> the hash values stored on the disk for "." and ".." is maintained as
> zero,
> which results in failure from find_in_block due to not matching HASH
> values.
> Fix up, by using the correct hashing values for these entries.
>
> Signed-off-by: Namjae Jeon <namjae.jeon@...sung.com>
> Signed-off-by: Amit Sahrawat <a.sahrawat@...sung.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@...sung.com>
> ---
>  fs/f2fs/dir.c  |  4 ++--
>  fs/f2fs/hash.c | 10 ++++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index b4e24f3..e1f66df 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -540,13 +540,13 @@ int f2fs_make_empty(struct inode *inode, struct
> inode *parent)
>
>  	de = &dentry_blk->dentry[0];
>  	de->name_len = cpu_to_le16(1);
> -	de->hash_code = 0;
> +	de->hash_code = f2fs_dentry_hash(".", 1);
>  	de->ino = cpu_to_le32(inode->i_ino);
>  	memcpy(dentry_blk->filename[0], ".", 1);
>  	set_de_type(de, inode);
>
>  	de = &dentry_blk->dentry[1];
> -	de->hash_code = 0;
> +	de->hash_code = f2fs_dentry_hash("..", 2);
>  	de->name_len = cpu_to_le16(2);
>  	de->ino = cpu_to_le32(parent->i_ino);
>  	memcpy(dentry_blk->filename[1], "..", 2);
> diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c
> index a60f042..9d7a7c6 100644
> --- a/fs/f2fs/hash.c
> +++ b/fs/f2fs/hash.c
> @@ -76,6 +76,16 @@ f2fs_hash_t f2fs_dentry_hash(const char *name, int
> len)
>  	const char *p;
>  	__u32 in[8], buf[4];
>
> +	if (name[0] == '.') {
> +		switch (len) {
> +		case 1:
> +			return 0;
> +		case 2:
> +			if (name[1] == '.')
> +				return 0;
> +		}
> +	}
> +
>  	/* Initialize the default seed for the hash checksum functions */
>  	buf[0] = 0x67452301;
>  	buf[1] = 0xefcdab89;
> --
> 1.8.0.1.250.gb7973fb
>
> --
> Jaegeuk Kim
> Samsung
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/