[patch] f2fs: dereferencing an ERR_PTR

[patch] f2fs: dereferencing an ERR_PTR

[Dan Carpenter]

There is an error path where "dir" is an ERR_PTR.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 5148d90..921aede 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -71,7 +71,8 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 out:
 	f2fs_msg(inode->i_sb, KERN_NOTICE, "recover_inode and its dentry: "
 			"ino = %x, name = %s, dir = %lx, err = %d",
-			ino_of_node(ipage), raw_inode->i_name, dir->i_ino, err);
+			ino_of_node(ipage), raw_inode->i_name,
+			IS_ERR(dir) ? 0 : dir->i_ino, err);
 	return err;
 }
 

Re: [patch] f2fs: dereferencing an ERR_PTR

[Dan Carpenter]

Btw, Linus's new Sparse changes don't like the f2fs's NEW_ADDR
macro.

#define NEW_ADDR                -1U

We use it like this:

	block_t src, dest;

	if (src != dest && dest != NEW_ADDR && dest != NULL_ADDR) {

block_t is 64 bits so probably the macro should probably be:

#define NEW_ADDR               ((block_t)-1)

I'm not able to test this so there may be some reason why changing
this breaks something.  In that case we could do:

#define NEW_ADDR               ((u32)(block_t)-1) /* explanation */

regards,
dan carpenter

Re: [patch] f2fs: dereferencing an ERR_PTR

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

2013-05-23 (목), 13:12 +0300, Dan Carpenter:
> Btw, Linus's new Sparse changes don't like the f2fs's NEW_ADDR
> macro.
> 
> #define NEW_ADDR                -1U
> 
> We use it like this:
> 
> 	block_t src, dest;
> 
> 	if (src != dest && dest != NEW_ADDR && dest != NULL_ADDR) {
> 
> block_t is 64 bits so probably the macro should probably be:
> 
> #define NEW_ADDR               ((block_t)-1)

Hi,
Agreed that it needs to synchronize on-disk and in-memory block address
types, __le32 and u64.
How about the below patch?

> 
> I'm not able to test this so there may be some reason why changing
> this breaks something.  In that case we could do:
> 
> #define NEW_ADDR               ((u32)(block_t)-1) /* explanation */
> 
> regards,
> dan carpenter
> 

From ded3c78fcb6dd8c39a8664471544942e51ff9faf Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk.kim@samsung.com>
Date: Fri, 24 May 2013 12:41:04 +0900
Subject: [PATCH] f2fs: align data types between on-disk and in-memory
block
 addresses
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net

The on-disk block address is defined as __le32, but in-memory block
address,
block_t, does as u64.

Let's synchronize them to 32 bits.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/f2fs.h          | 5 ++++-
 include/linux/f2fs_fs.h | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7b05029..92fd4e9 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -37,7 +37,10 @@
 		typecheck(unsigned long long, b) &&			\
 		((long long)((a) - (b)) > 0))
 
-typedef u64 block_t;
+typedef u32 block_t;	/*
+			 * should not change u32, since it is the on-disk block
+			 * address format, __le32.
+			 */
 typedef u32 nid_t;
 
 struct f2fs_mount_info {
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index df6fab8..383d5e3 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -20,8 +20,8 @@
 #define F2FS_BLKSIZE			4096	/* support only 4KB block */
 #define F2FS_MAX_EXTENSION		64	/* # of extension entries */
 
-#define NULL_ADDR		0x0U
-#define NEW_ADDR		-1U
+#define NULL_ADDR		((block_t)0)	/* used as block_t addresses */
+#define NEW_ADDR		((block_t)-1)	/* used as block_t addresses */
 
 #define F2FS_ROOT_INO(sbi)	(sbi->root_ino_num)
 #define F2FS_NODE_INO(sbi)	(sbi->node_ino_num)
-- 
1.8.1.3.566.gaa39828



-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] f2fs: dereferencing an ERR_PTR

[Dan Carpenter]

On Fri, May 24, 2013 at 12:46:08PM +0900, Jaegeuk Kim wrote:
> From ded3c78fcb6dd8c39a8664471544942e51ff9faf Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> Date: Fri, 24 May 2013 12:41:04 +0900
> Subject: [PATCH] f2fs: align data types between on-disk and in-memory
> block
>  addresses
> Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-f2fs-devel@lists.sourceforge.net
> 
> The on-disk block address is defined as __le32, but in-memory block
> address,
> block_t, does as u64.
> 
> Let's synchronize them to 32 bits.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>

What you say sounds good to me.  Could you give me the reported-by
tag instead of the Signed-off-by?  The Signed-off-by means it went
through my hands and it's intended as sort of a legal thing like
signing a document.

regards,
dan carpenter

Re: [patch] f2fs: dereferencing an ERR_PTR

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

2013-05-24 (금), 14:23 +0300, Dan Carpenter:
> On Fri, May 24, 2013 at 12:46:08PM +0900, Jaegeuk Kim wrote:
> > From ded3c78fcb6dd8c39a8664471544942e51ff9faf Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> > Date: Fri, 24 May 2013 12:41:04 +0900
> > Subject: [PATCH] f2fs: align data types between on-disk and in-memory
> > block
> >  addresses
> > Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
> > linux-f2fs-devel@lists.sourceforge.net
> > 
> > The on-disk block address is defined as __le32, but in-memory block
> > address,
> > block_t, does as u64.
> > 
> > Let's synchronize them to 32 bits.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> 
> What you say sounds good to me.  Could you give me the reported-by
> tag instead of the Signed-off-by?  The Signed-off-by means it went
> through my hands and it's intended as sort of a legal thing like
> signing a document.

Ok, I see.
Thank you very much for the notification. :)

> 
> regards,
> dan carpenter

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] f2fs: dereferencing an ERR_PTR

[walter harms]

Am 23.05.2013 12:02, schrieb Dan Carpenter:
> There is an error path where "dir" is an ERR_PTR.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 5148d90..921aede 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -71,7 +71,8 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
>  out:
>  	f2fs_msg(inode->i_sb, KERN_NOTICE, "recover_inode and its dentry: "
>  			"ino = %x, name = %s, dir = %lx, err = %d",
> -			ino_of_node(ipage), raw_inode->i_name, dir->i_ino, err);
> +			ino_of_node(ipage), raw_inode->i_name,
> +			IS_ERR(dir) ? 0 : dir->i_ino, err);
>  	return err;
>  }
>  
I am not an expert on  this matter so a simple question:
dir->i_ino=0 is not valid ?

re,
 wh


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

Re: [patch] f2fs: dereferencing an ERR_PTR

[Dan Carpenter]

On Sun, May 26, 2013 at 05:05:15PM +0200, walter harms wrote:
> 
> 
> Am 23.05.2013 12:02, schrieb Dan Carpenter:
> > There is an error path where "dir" is an ERR_PTR.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index 5148d90..921aede 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -71,7 +71,8 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
> >  out:
> >  	f2fs_msg(inode->i_sb, KERN_NOTICE, "recover_inode and its dentry: "
> >  			"ino = %x, name = %s, dir = %lx, err = %d",
> > -			ino_of_node(ipage), raw_inode->i_name, dir->i_ino, err);
> > +			ino_of_node(ipage), raw_inode->i_name,
> > +			IS_ERR(dir) ? 0 : dir->i_ino, err);
> >  	return err;
> >  }
> >  
> I am not an expert on  this matter so a simple question:
> dir->i_ino=0 is not valid ?

That is a valid question.  The trick is that we also print the err
code so error conditions should be pretty obvious.

regards,
dan carpenter

Re: [patch] f2fs: dereferencing an ERR_PTR

[walter harms]

Am 26.05.2013 22:41, schrieb Dan Carpenter:
> On Sun, May 26, 2013 at 05:05:15PM +0200, walter harms wrote:
>>
>>
>> Am 23.05.2013 12:02, schrieb Dan Carpenter:
>>> There is an error path where "dir" is an ERR_PTR.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>> index 5148d90..921aede 100644
>>> --- a/fs/f2fs/recovery.c
>>> +++ b/fs/f2fs/recovery.c
>>> @@ -71,7 +71,8 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
>>>  out:
>>>  	f2fs_msg(inode->i_sb, KERN_NOTICE, "recover_inode and its dentry: "
>>>  			"ino = %x, name = %s, dir = %lx, err = %d",
>>> -			ino_of_node(ipage), raw_inode->i_name, dir->i_ino, err);
>>> +			ino_of_node(ipage), raw_inode->i_name,
>>> +			IS_ERR(dir) ? 0 : dir->i_ino, err);
>>>  	return err;
>>>  }
>>>  
>> I am not an expert on  this matter so a simple question:
>> dir->i_ino=0 is not valid ?
> 
> That is a valid question.  The trick is that we also print the err
> code so error conditions should be pretty obvious.
> 
hi dan,
people never read error messages propperly, i would go for -1
or at least something obvoius like 57005 (0xdead :).

re,
 wh