f2fs: dereferencing an ERR_PTR

PATCH In review Dan Carpenter 3 participants 8 messages May 23, 2013 – May 27, 2013

Subsystems (2)

Kernel janitors F2FS file system

lore

Share & tools

lore

Dan Carpenter

author

2013-05-23 10:02 UTC #0 lore raw

PATCH 1/1 f2fs: dereferencing an ERR_PTR

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

Signed-off-by Dan Carpenter <[email protected]>

fs/f2fs/recovery.c +2 -1

index 5148d90..921aede 100644
@@ -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;
 }

Dan Carpenter

author

2013-05-23 10:12 UTC #1 lore raw

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

Jaegeuk Kim

2013-05-24 03:46 UTC #2 lore raw

2013-05-23 (목), 13:12 +0300, Dan Carpenter:

Quoted 7 lines

Btw, Linus's new Sparse changes don't like the f2fs's NEW_ADDR macro.
#define NEW_ADDR -1U
We use it like this:

Quoted 7 lines

	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?

Quoted 9 lines

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 <[email protected]>
Date: Fri, 24 May 2013 12:41:04 +0900
Subject: [PATCH] f2fs: align data types between on-disk and in-memory block
addresses
Cc: [email protected], [email protected], [email protected]

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 <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h          | 5 ++++-
 include/linux/f2fs_fs.h | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

fs/f2fs/f2fs.h +4 -1

index 7b05029..92fd4e9 100644
@@ -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 {

include/linux/f2fs_fs.h +2 -2

index df6fab8..383d5e3 100644
@@ -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)

Signature · 7 lines

1.8.1.3.566.gaa39828



-- 
Jaegeuk Kim
Samsung

Dan Carpenter

author

2013-05-24 11:23 UTC #3 lore raw

Quoted 9 lines · Jaegeuk Kim

From ded3c78fcb6dd8c39a8664471544942e51ff9faf Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 24 May 2013 12:41:04 +0900
Subject: [PATCH] f2fs: align data types between on-disk and in-memory block
addresses
Cc: [email protected], [email protected], [email protected]

Quoted 8 lines

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 <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>

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

Jaegeuk Kim

2013-05-24 11:32 UTC #4 lore raw

2013-05-24 (금), 14:23 +0300, Dan Carpenter:

Quoted 16 lines · Jaegeuk Kim

Quoted 15 lines
From ded3c78fcb6dd8c39a8664471544942e51ff9faf Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 24 May 2013 12:41:04 +0900
Subject: [PATCH] f2fs: align data types between on-disk and in-memory block
addresses
Cc: [email protected], [email protected], [email protected]
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.

Quoted 7 lines

Quoted 2 lines
Signed-off-by Dan Carpenter <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
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. :)

Quoted 3 lines

regards,
dan carpenter

Signature · 2 lines

Jaegeuk Kim
Samsung

walter harms

2013-05-26 15:05 UTC #5 lore raw

In reply to Dan Carpenter

Am 23.05.2013 12:02, schrieb Dan Carpenter:

Quoted 9 lines

There is an error path where "dir" is an ERR_PTR.
Signed-off-by Dan Carpenter <[email protected]>
fs/f2fs/recovery.c +0 -0
index 5148d90..921aede 100644

fs/f2fs/recovery.c static int recover_dentry(struct page *ipage, struct inode *inode) · line 78

Quoted 9 lines

fs/f2fs/recovery.c +2 -1

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

Quoted 5 lines

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

Dan Carpenter

author

2013-05-26 20:41 UTC #6 lore raw

Quoted 7 lines · walter harms

Am 23.05.2013 12:02, schrieb Dan Carpenter:
Quoted 4 lines
There is an error path where "dir" is an ERR_PTR.
Signed-off-by Dan Carpenter <[email protected]>

Quoted 16 lines

Quoted 5 lines
fs/f2fs/recovery.c +0 -0
index 5148d90..921aede 100644
fs/f2fs/recovery.c static int recover_dentry(struct page *ipage, struct inode *inode) · line 78
Quoted 9 lines
fs/f2fs/recovery.c +2 -1
@@ -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

walter harms

2013-05-27 07:13 UTC #7 lore raw

Am 26.05.2013 22:41, schrieb Dan Carpenter:

Quoted 25 lines · walter harms

Quoted 7 lines
Am 23.05.2013 12:02, schrieb Dan Carpenter:
Quoted 4 lines
There is an error path where "dir" is an ERR_PTR.
Signed-off-by Dan Carpenter <[email protected]>
Quoted 16 lines
Quoted 5 lines
fs/f2fs/recovery.c +0 -0
index 5148d90..921aede 100644
fs/f2fs/recovery.c static int recover_dentry(struct page *ipage, struct inode *inode) · line 78
Quoted 9 lines
fs/f2fs/recovery.c +2 -1
@@ -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 ?

Quoted 3 lines

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