isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Jiri Slaby]

Hi,

this happened, while playing with broken dvd.

ISO 9660 Extensions: Microsoft Joliet Level 3
ISO 9660 Extensions: RRIP_1991A
end_request: I/O error, dev sr0, sector 21760
Buffer I/O error on device sr0, logical block 5440
Buffer I/O error on device sr0, logical block 5441
Buffer I/O error on device sr0, logical block 5442
Buffer I/O error on device sr0, logical block 5443
Buffer I/O error on device sr0, logical block 5444
Buffer I/O error on device sr0, logical block 5445
Buffer I/O error on device sr0, logical block 5446
Buffer I/O error on device sr0, logical block 5447
Buffer I/O error on device sr0, logical block 5448
Buffer I/O error on device sr0, logical block 5449
end_request: I/O error, dev sr0, sector 1447344
end_request: I/O error, dev sr0, sector 1447344
end_request: I/O error, dev sr0, sector 21808
end_request: I/O error, dev sr0, sector 21812
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21764
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21764
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21764
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21764
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21764
end_request: I/O error, dev sr0, sector 21760
end_request: I/O error, dev sr0, sector 21764
end_request: I/O error, dev sr0, sector 21760
printk: 40 messages suppressed.
Buffer I/O error on device sr0, logical block 5440
Buffer I/O error on device sr0, logical block 5441
end_request: I/O error, dev sr0, sector 136
ISOFS: unable to read i-node block
Unable to handle kernel NULL pointer dereference at 00000000000000ad RIP:
 [<ffffffff802a679f>] d_splice_alias+0x1f/0x100
PGD 67f1a067 PUD 67e15067 PMD 0
Oops: 0000 [1] SMP
last sysfs file: /sys/devices/platform/coretemp.1/temp1_input
CPU 0
Modules linked in: isofs tun bitrev ipv6 arc4 ecb crypto_blkcipher cryptomgr cry
pto_algapi ath5k mac80211 cfg80211 rtc_cmos rtc_core rtc_lib crc32 floppy ehci_h
cd sr_mod cdrom
Pid: 3096, comm: nautilus Not tainted 2.6.24-rc5-mm1_64 #354
RIP: 0010:[<ffffffff802a679f>]  [<ffffffff802a679f>] d_splice_alias+0x1f/0x100
RSP: 0000:ffff810061543b08  EFLAGS: 00010282
RAX: 00000000ffffffff RBX: fffffffffffffffb RCX: ffffffff880d3454
RDX: ffff810048542750 RSI: ffff81005e114e10 RDI: fffffffffffffffb
RBP: ffff810061543b28 R08: 0000000000000049 R09: 8000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: ffff81005e114e10
R13: ffff81005e2b11c0 R14: 0000000000000003 R15: 000000000000013c
FS:  0000000042123950(0063) GS:ffffffff80650000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000000000ad CR3: 0000000067d2f000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process nautilus (pid: 3096, threadinfo ffff810061542000, task ffff810048542750)
Stack:  ffff81007c0799c0 fffffffffffffffb ffff81007c0799c0 ffff81005e2b11c0
 ffff810061543bd8 ffffffff880d2395 ffffffff802a565b ffff81005e114e10
 ffff81005e2ec030 ffff8100638a3000 ffffe200015c63a8 0000000000000022
Call Trace:
 [<ffffffff880d2395>] :isofs:isofs_lookup+0x395/0x4a0
 [<ffffffff802a565b>] d_alloc+0x2b/0x1d0
 [<ffffffff8029a97c>] do_lookup+0x1ac/0x200
 [<ffffffff8029c747>] __link_path_walk+0x827/0xd10
 [<ffffffff8029cc93>] link_path_walk+0x63/0xe0
 [<ffffffff8028469f>] free_pages_and_swap_cache+0x9f/0xd0
 [<ffffffff8027d7f5>] unmap_region+0x135/0x150
 [<ffffffff8028ea65>] kmem_cache_free+0x15/0xa0
 [<ffffffff8026f686>] __pagevec_free+0x26/0x30
 [<ffffffff8029cd2c>] path_walk+0x1c/0x20
 [<ffffffff8029cf53>] do_path_lookup+0x83/0x1b0
 [<ffffffff8029dc2c>] __user_walk_fd+0x4c/0x80
 [<ffffffff80295b5b>] vfs_lstat_fd+0x2b/0x70
 [<ffffffff8028469f>] free_pages_and_swap_cache+0x9f/0xd0
 [<ffffffff8027d7f5>] unmap_region+0x135/0x150
 [<ffffffff8028ea65>] kmem_cache_free+0x15/0xa0
 [<ffffffff80295df7>] sys_newlstat+0x27/0x50
 [<ffffffff802521f9>] up_write+0x9/0x10
 [<ffffffff8027e755>] sys_munmap+0x55/0x70
 [<ffffffff8020ba4e>] system_call+0x7e/0x83


Code: 0f b7 87 b2 00 00 00 25 00 f0 00 00 3d 00 40 00 00 74 27 48
RIP  [<ffffffff802a679f>] d_splice_alias+0x1f/0x100
 RSP <ffff810061543b08>
CR2: 00000000000000ad

regards,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Pekka J Enberg]

Hi Jiri,

On Thu, 3 Jan 2008, Jiri Slaby wrote:
> this happened, while playing with broken dvd.

[snip]

> Buffer I/O error on device sr0, logical block 5441
> end_request: I/O error, dev sr0, sector 136
> ISOFS: unable to read i-node block
> Unable to handle kernel NULL pointer dereference at 00000000000000ad RIP:
>  [<ffffffff802a679f>] d_splice_alias+0x1f/0x100

[snip]

> Call Trace:
>  [<ffffffff880d2395>] :isofs:isofs_lookup+0x395/0x4a0
>  [<ffffffff802a565b>] d_alloc+0x2b/0x1d0
>  [<ffffffff8029a97c>] do_lookup+0x1ac/0x200

Does the following patch fix it?

			Pekka

[PATCH] isofs: check for bad inode in isofs_lookup
From: Pekka Enberg <penberg@cs.helsinki.fi>

If isofs_read_inode() fails to read one of the inode blocks from disk, it
returns a bad inode.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 fs/isofs/namei.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-2.6/fs/isofs/namei.c
===================================================================
--- linux-2.6.orig/fs/isofs/namei.c
+++ linux-2.6/fs/isofs/namei.c
@@ -183,6 +183,11 @@ struct dentry *isofs_lookup(struct inode
 			unlock_kernel();
 			return ERR_PTR(-EACCES);
 		}
+		if (is_bad_inode(inode)) {
+			unlock_kernel();
+			iput(inode);
+			return ERR_PTR(-ENOENT);
+		}
 	}
 	unlock_kernel();
 	return d_splice_alias(inode, dentry);

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Al Viro]

On Thu, Jan 03, 2008 at 02:23:44PM +0100, Jiri Slaby wrote:

> ISOFS: unable to read i-node block

isofs_read_inode() failing, about to do make_bad_inode()

> Unable to handle kernel NULL pointer dereference at 00000000000000ad RIP:
>  [<ffffffff802a679f>] d_splice_alias+0x1f/0x100

struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
{
        struct dentry *new = NULL;

        if (inode && S_ISDIR(inode->i_mode)) {
	                     ^^^^^^^^^^^^^

> RIP: 0010:[<ffffffff802a679f>]  [<ffffffff802a679f>] d_splice_alias+0x1f/0x100
> RSP: 0000:ffff810061543b08  EFLAGS: 00010282
> RAX: 00000000ffffffff RBX: fffffffffffffffb RCX: ffffffff880d3454
> RDX: ffff810048542750 RSI: ffff81005e114e10 RDI: fffffffffffffffb

with inode equal to (struct inode *)-5.  Which is ERR_PTR(-EIO)...

>  [<ffffffff880d2395>] :isofs:isofs_lookup+0x395/0x4a0

        inode = NULL;
        if (found) {
                inode = isofs_iget(dir->i_sb, block, offset);
                if (!inode) {
                        unlock_kernel();
                        return ERR_PTR(-EACCES);
                }
        }
        unlock_kernel();
        return d_splice_alias(inode, dentry);

So we've got ERR_PTR(-EIO) from isofs_iget().  Bloody odd, seeing that
isofs_iget() either explicitly returns NULL or does

        if (inode && (inode->i_state & I_NEW)) {
                sb->s_op->read_inode(inode);
                unlock_new_inode(inode);
        }

        return inode;

which would not manage to return ERR_PTR(-EIO), no matter what - it would
die on access to inode->i_state.  I don't have -mm tree at hand, check if
there's anything affected in these areas.  Perhaps somebody tried to pass
error values from isofs_iget() and forgot to update callers?

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Ingo Molnar]

* Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

>  			return ERR_PTR(-EACCES);
>  		}
> +		if (is_bad_inode(inode)) {
> +			unlock_kernel();
> +			iput(inode);
> +			return ERR_PTR(-ENOENT);
> +		}

fs/isofs/rock.c:474 parse_rock_ridge_inode_internal() seems buggy too:

                        reloc =
                            isofs_iget(inode->i_sb,
                                       ISOFS_I(inode)->i_first_extent,
                                       0);
                        if (!reloc)
                                goto out;

it should probably do "!reloc || is_bad_inode(inode)" as well.

and there are about 5 other callsites as well that only check for a NULL 
return.

perhaps the better fix would be to add this to inode.c:isofs_iget():

        if (inode && (inode->i_state & I_NEW)) {
                sb->s_op->read_inode(inode);
                unlock_new_inode(inode);
                if (s_bad_inode(inode))
			inode = NULL;
        }

?

	Ingo

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Al Viro]

On Thu, Jan 03, 2008 at 03:51:28PM +0200, Pekka J Enberg wrote:

> Does the following patch fix it?

Highly doubtful, and doesn't deal with the real problem, AFAICS...

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Al Viro]

On Thu, Jan 03, 2008 at 03:11:20PM +0100, Ingo Molnar wrote:
> and there are about 5 other callsites as well that only check for a NULL 
> return.

... except that this is *not* getting NULL or make_bad_inode(); that's
getting ERR_PTR(), which can't happen in mainline.  So that looks like
an -mm patch changing calling conventions and not updating callers...

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Jiri Slaby]

On 01/03/2008 02:51 PM, Pekka J Enberg wrote:
> Hi Jiri,
> 
> On Thu, 3 Jan 2008, Jiri Slaby wrote:
>> this happened, while playing with broken dvd.
> 
> [snip]
> 
>> Buffer I/O error on device sr0, logical block 5441
>> end_request: I/O error, dev sr0, sector 136
>> ISOFS: unable to read i-node block
>> Unable to handle kernel NULL pointer dereference at 00000000000000ad RIP:
>>  [<ffffffff802a679f>] d_splice_alias+0x1f/0x100
> 
> [snip]
> 
>> Call Trace:
>>  [<ffffffff880d2395>] :isofs:isofs_lookup+0x395/0x4a0
>>  [<ffffffff802a565b>] d_alloc+0x2b/0x1d0
>>  [<ffffffff8029a97c>] do_lookup+0x1ac/0x200
> 
> Does the following patch fix it?
> 
> 			Pekka
> 
> [PATCH] isofs: check for bad inode in isofs_lookup
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> If isofs_read_inode() fails to read one of the inode blocks from disk, it
> returns a bad inode.
> 
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>  fs/isofs/namei.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> Index: linux-2.6/fs/isofs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/isofs/namei.c
> +++ linux-2.6/fs/isofs/namei.c
> @@ -183,6 +183,11 @@ struct dentry *isofs_lookup(struct inode
>  			unlock_kernel();
>  			return ERR_PTR(-EACCES);
>  		}
> +		if (is_bad_inode(inode)) {
> +			unlock_kernel();
> +			iput(inode);
> +			return ERR_PTR(-ENOENT);
> +		}
>  	}
>  	unlock_kernel();
>  	return d_splice_alias(inode, dentry);

Can't say, the DVD seems to be OK, I don't know what was wrong (as I can say,
this happened several times in the past yet and after reboot everything OK; I
suspect gnome auto mounter -- multiple machines, several DVD ROMs, same disk
with OS, similar errors, but that's too few to report).

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Al Viro]

On Thu, Jan 03, 2008 at 03:15:56PM +0100, Jiri Slaby wrote:

> Can't say, the DVD seems to be OK, I don't know what was wrong (as I can say,
> this happened several times in the past yet and after reboot everything OK; I
> suspect gnome auto mounter -- multiple machines, several DVD ROMs, same disk
> with OS, similar errors, but that's too few to report).

It is -mm-specific, all right - fallout from
	iget-stop-isofs-from-using-read_inode.patch
not covered in
	iget-stop-isofs-from-using-read_inode-fix*

Callers in fs/isofs/namei.c are missed.

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Dave Young]

On Fri, Jan 04, 2008 at 10:47:55AM +0000, Al Viro wrote:
> On Thu, Jan 03, 2008 at 03:15:56PM +0100, Jiri Slaby wrote:
> 
> > Can't say, the DVD seems to be OK, I don't know what was wrong (as I can say,
> > this happened several times in the past yet and after reboot everything OK; I
> > suspect gnome auto mounter -- multiple machines, several DVD ROMs, same disk
> > with OS, similar errors, but that's too few to report).
> 
> It is -mm-specific, all right - fallout from
> 	iget-stop-isofs-from-using-read_inode.patch
> not covered in
> 	iget-stop-isofs-from-using-read_inode-fix*
> 
> Callers in fs/isofs/namei.c are missed.

Yes, some isofs_iget return value should be handled, it is missed.

Maybe this:

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
fs/isofs/export.c |    8 ++++----
fs/isofs/namei.c  |    4 ++--
fs/isofs/rock.c   |    4 +++-
3 files changed, 9 insertions(+), 7 deletions(-)

diff -upr linux/fs/isofs/export.c linux.new/fs/isofs/export.c
--- linux/fs/isofs/export.c	2008-01-04 18:54:40.000000000 +0800
+++ linux.new/fs/isofs/export.c	2008-01-04 19:04:31.000000000 +0800
@@ -26,8 +26,8 @@ isofs_export_iget(struct super_block *sb
 	if (block == 0)
 		return ERR_PTR(-ESTALE);
 	inode = isofs_iget(sb, block, offset);
-	if (inode == NULL)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(inode))
+		return inode;
 	if (is_bad_inode(inode)
 	    || (generation && inode->i_generation != generation))
 	{
@@ -110,8 +110,8 @@ static struct dentry *isofs_export_get_p
 	parent_inode = isofs_iget(child_inode->i_sb,
 				  parent_block,
 				  parent_offset);
-	if (parent_inode == NULL) {
-		rv = ERR_PTR(-EACCES);
+	if (IS_ERR(parent_inode)) {
+		rv = parent_inode;
 		goto out;
 	}
 
diff -upr linux/fs/isofs/namei.c linux.new/fs/isofs/namei.c
--- linux/fs/isofs/namei.c	2008-01-04 18:54:40.000000000 +0800
+++ linux.new/fs/isofs/namei.c	2008-01-04 18:57:27.000000000 +0800
@@ -179,9 +179,9 @@ struct dentry *isofs_lookup(struct inode
 	inode = NULL;
 	if (found) {
 		inode = isofs_iget(dir->i_sb, block, offset);
-		if (!inode) {
+		if (IS_ERR(inode)) {
 			unlock_kernel();
-			return ERR_PTR(-EACCES);
+			return inode;
 		}
 	}
 	unlock_kernel();
diff -upr linux/fs/isofs/rock.c linux.new/fs/isofs/rock.c
--- linux/fs/isofs/rock.c	2008-01-04 18:54:40.000000000 +0800
+++ linux.new/fs/isofs/rock.c	2008-01-04 19:01:10.000000000 +0800
@@ -474,8 +474,10 @@ repeat:
 			    isofs_iget(inode->i_sb,
 				       ISOFS_I(inode)->i_first_extent,
 				       0);
-			if (!reloc)
+			if (IS_ERR(reloc)) {
+				err = PTR_ERR(reloc);
 				goto out;
+			}
 			inode->i_mode = reloc->i_mode;
 			inode->i_nlink = reloc->i_nlink;
 			inode->i_uid = reloc->i_uid;

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Dave Young]

On Fri, Jan 04, 2008 at 07:13:57PM +0800, Dave Young wrote:
> On Fri, Jan 04, 2008 at 10:47:55AM +0000, Al Viro wrote:
> > On Thu, Jan 03, 2008 at 03:15:56PM +0100, Jiri Slaby wrote:
> > 
> > > Can't say, the DVD seems to be OK, I don't know what was wrong (as I can say,
> > > this happened several times in the past yet and after reboot everything OK; I
> > > suspect gnome auto mounter -- multiple machines, several DVD ROMs, same disk
> > > with OS, similar errors, but that's too few to report).
> > 
> > It is -mm-specific, all right - fallout from
> > 	iget-stop-isofs-from-using-read_inode.patch
> > not covered in
> > 	iget-stop-isofs-from-using-read_inode-fix*
> > 
> > Callers in fs/isofs/namei.c are missed.
> 
> Yes, some isofs_iget return value should be handled, it is missed.
> 
> Maybe this:
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 
> 
> ---
> fs/isofs/export.c |    8 ++++----
> fs/isofs/namei.c  |    4 ++--
> fs/isofs/rock.c   |    4 +++-
> 3 files changed, 9 insertions(+), 7 deletions(-)
> 
[snip]

I'm sorry for patch of building error, following one instead:

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
fs/isofs/export.c |    8 ++++----
fs/isofs/namei.c  |    4 ++--
fs/isofs/rock.c   |    4 +++-
3 files changed, 9 insertions(+), 7 deletions(-)

diff -upr linux/fs/isofs/export.c linux.new/fs/isofs/export.c
--- linux/fs/isofs/export.c	2008-01-04 18:54:40.000000000 +0800
+++ linux.new/fs/isofs/export.c	2008-01-04 19:18:45.000000000 +0800
@@ -26,8 +26,8 @@ isofs_export_iget(struct super_block *sb
 	if (block == 0)
 		return ERR_PTR(-ESTALE);
 	inode = isofs_iget(sb, block, offset);
-	if (inode == NULL)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(inode))
+		return (struct dentry *)inode;
 	if (is_bad_inode(inode)
 	    || (generation && inode->i_generation != generation))
 	{
@@ -110,8 +110,8 @@ static struct dentry *isofs_export_get_p
 	parent_inode = isofs_iget(child_inode->i_sb,
 				  parent_block,
 				  parent_offset);
-	if (parent_inode == NULL) {
-		rv = ERR_PTR(-EACCES);
+	if (IS_ERR(parent_inode)) {
+		rv = (struct dentry *)parent_inode;
 		goto out;
 	}
 
diff -upr linux/fs/isofs/namei.c linux.new/fs/isofs/namei.c
--- linux/fs/isofs/namei.c	2008-01-04 18:54:40.000000000 +0800
+++ linux.new/fs/isofs/namei.c	2008-01-04 19:18:45.000000000 +0800
@@ -179,9 +179,9 @@ struct dentry *isofs_lookup(struct inode
 	inode = NULL;
 	if (found) {
 		inode = isofs_iget(dir->i_sb, block, offset);
-		if (!inode) {
+		if (IS_ERR(inode)) {
 			unlock_kernel();
-			return ERR_PTR(-EACCES);
+			return inode;
 		}
 	}
 	unlock_kernel();
diff -upr linux/fs/isofs/rock.c linux.new/fs/isofs/rock.c
--- linux/fs/isofs/rock.c	2008-01-04 18:54:40.000000000 +0800
+++ linux.new/fs/isofs/rock.c	2008-01-04 19:18:45.000000000 +0800
@@ -474,8 +474,10 @@ repeat:
 			    isofs_iget(inode->i_sb,
 				       ISOFS_I(inode)->i_first_extent,
 				       0);
-			if (!reloc)
+			if (IS_ERR(reloc)) {
+				ret = PTR_ERR(reloc);
 				goto out;
+			}
 			inode->i_mode = reloc->i_mode;
 			inode->i_nlink = reloc->i_nlink;
 			inode->i_uid = reloc->i_uid;

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Al Viro]

On Fri, Jan 04, 2008 at 07:13:57PM +0800, Dave Young wrote:
> > 	iget-stop-isofs-from-using-read_inode-fix*
> > 
> > Callers in fs/isofs/namei.c are missed.
> 
> Yes, some isofs_iget return value should be handled, it is missed.
> 
> Maybe this:

Not enough.  isofs_iget() still can return NULL; that needs to be converted to
ERR_PTR().

BTW, ERR_CAST is there for purpose...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff -urN foo1/fs/isofs/export.c foo2/fs/isofs/export.c
--- foo1/fs/isofs/export.c	2008-01-04 06:23:10.000000000 -0500
+++ foo2/fs/isofs/export.c	2008-01-04 06:18:52.000000000 -0500
@@ -26,11 +26,9 @@
 	if (block == 0)
 		return ERR_PTR(-ESTALE);
 	inode = isofs_iget(sb, block, offset);
-	if (inode == NULL)
-		return ERR_PTR(-ENOMEM);
-	if (is_bad_inode(inode)
-	    || (generation && inode->i_generation != generation))
-	{
+	if (ERR_PTR(inode))
+		return ERR_CAST(inode);
+	if (generation && inode->i_generation != generation) {
 		iput(inode);
 		return ERR_PTR(-ESTALE);
 	}
@@ -110,7 +108,7 @@
 	parent_inode = isofs_iget(child_inode->i_sb,
 				  parent_block,
 				  parent_offset);
-	if (parent_inode == NULL) {
+	if (ERR_PTR(parent_inode)) {
 		rv = ERR_PTR(-EACCES);
 		goto out;
 	}
diff -urN foo1/fs/isofs/inode.c foo2/fs/isofs/inode.c
--- foo1/fs/isofs/inode.c	2008-01-04 06:23:33.000000000 -0500
+++ foo2/fs/isofs/inode.c	2008-01-04 06:15:43.000000000 -0500
@@ -1408,7 +1408,7 @@
 	long ret;
 
 	if (offset >= 1ul << sb->s_blocksize_bits)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	data.block = block;
 	data.offset = offset;
@@ -1418,7 +1418,10 @@
 	inode = iget5_locked(sb, hashval, &isofs_iget5_test,
 				&isofs_iget5_set, &data);
 
-	if (inode && (inode->i_state & I_NEW)) {
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+
+	if (inode->i_state & I_NEW) {
 		ret = isofs_read_inode(inode);
 		if (ret < 0) {
 			iget_failed(inode);
diff -urN foo1/fs/isofs/namei.c foo2/fs/isofs/namei.c
--- foo1/fs/isofs/namei.c	2008-01-04 06:23:10.000000000 -0500
+++ foo2/fs/isofs/namei.c	2008-01-04 06:19:20.000000000 -0500
@@ -179,9 +179,9 @@
 	inode = NULL;
 	if (found) {
 		inode = isofs_iget(dir->i_sb, block, offset);
-		if (!inode) {
+		if (ERR_PTR(inode)) {
 			unlock_kernel();
-			return ERR_PTR(-EACCES);
+			return ERR_CAST(inode);
 		}
 	}
 	unlock_kernel();
diff -urN foo1/fs/isofs/rock.c foo2/fs/isofs/rock.c
--- foo1/fs/isofs/rock.c	2008-01-04 06:23:10.000000000 -0500
+++ foo2/fs/isofs/rock.c	2008-01-04 06:20:41.000000000 -0500
@@ -474,7 +474,7 @@
 			    isofs_iget(inode->i_sb,
 				       ISOFS_I(inode)->i_first_extent,
 				       0);
-			if (!reloc)
+			if (ERR_PTR(reloc))
 				goto out;
 			inode->i_mode = reloc->i_mode;
 			inode->i_nlink = reloc->i_nlink;

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[David Howells]

Al Viro <viro@ZenIV.linux.org.uk> wrote:

> Not enough.  isofs_iget() still can return NULL; that needs to be converted to
> ERR_PTR().
> 
> BTW, ERR_CAST is there for purpose...

This patch should probably be added on top of that one.

David
---
IGET: Add another couple of fixes to ISOFS error handling

From: David Howells <dhowells@redhat.com>

Add some more fixes to ISOFS error handling on top of Al Viro's patch:

 (1) Use IS_ERR() rather than ERR_PTR() to test for errors.

 (2) Return the error from isofs_iget() in parse_rock_ridge_inode_internal().

 (3) In isofs_export_get_parent() return -ENOMEM if that was the error rather
     than -EACCES.  Should we return -EIO if that is the error rather than
     -EACCES?

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/isofs/export.c |    8 +++++---
 fs/isofs/namei.c  |    2 +-
 fs/isofs/rock.c   |    4 +++-
 3 files changed, 9 insertions(+), 5 deletions(-)


diff --git a/fs/isofs/export.c b/fs/isofs/export.c
index 63a2113..bb21913 100644
--- a/fs/isofs/export.c
+++ b/fs/isofs/export.c
@@ -26,7 +26,7 @@ isofs_export_iget(struct super_block *sb,
 	if (block == 0)
 		return ERR_PTR(-ESTALE);
 	inode = isofs_iget(sb, block, offset);
-	if (ERR_PTR(inode))
+	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 	if (generation && inode->i_generation != generation) {
 		iput(inode);
@@ -108,8 +108,10 @@ static struct dentry *isofs_export_get_parent(struct dentry *child)
 	parent_inode = isofs_iget(child_inode->i_sb,
 				  parent_block,
 				  parent_offset);
-	if (ERR_PTR(parent_inode)) {
-		rv = ERR_PTR(-EACCES);
+	if (IS_ERR(parent_inode)) {
+		rv = ERR_CAST(parent_inode);
+		if (rv != ERR_PTR(-ENOMEM))
+			rv = ERR_PTR(-EACCES);
 		goto out;
 	}
 
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index beee021..344b247 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -179,7 +179,7 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
 	inode = NULL;
 	if (found) {
 		inode = isofs_iget(dir->i_sb, block, offset);
-		if (ERR_PTR(inode)) {
+		if (IS_ERR(inode)) {
 			unlock_kernel();
 			return ERR_CAST(inode);
 		}
diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
index c62b650..6bd48f0 100644
--- a/fs/isofs/rock.c
+++ b/fs/isofs/rock.c
@@ -474,8 +474,10 @@ repeat:
 			    isofs_iget(inode->i_sb,
 				       ISOFS_I(inode)->i_first_extent,
 				       0);
-			if (ERR_PTR(reloc))
+			if (IS_ERR(reloc)) {
+				ret = PTR_ERR(reloc);
 				goto out;
+			}
 			inode->i_mode = reloc->i_mode;
 			inode->i_nlink = reloc->i_nlink;
 			inode->i_uid = reloc->i_uid;

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Al Viro]

On Fri, Jan 04, 2008 at 12:24:01PM +0000, David Howells wrote:
> Add some more fixes to ISOFS error handling on top of Al Viro's patch:
> 
>  (1) Use IS_ERR() rather than ERR_PTR() to test for errors.

*blush*

My apologies, should've had coffee before posting.  FWIW, this patch
pile is getting ridiculous - it's what, original + 2 fixes in -mm +
mine + this one?  Could you post the updated patch with all fixes
and fixes to fixes folded into it?

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[David Howells]

Al Viro <viro@ZenIV.linux.org.uk> wrote:

> My apologies, should've had coffee before posting.

Me too, probably.

> FWIW, this patch pile is getting ridiculous - it's what, original + 2 fixes
> in -mm + mine + this one?  Could you post the updated patch with all fixes
> and fixes to fixes folded into it?

I can, though Andrew usually objects to that.

David

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Ingo Molnar]

* David Howells <dhowells@redhat.com> wrote:

> > FWIW, this patch pile is getting ridiculous - it's what, original + 
> > 2 fixes in -mm + mine + this one?  Could you post the updated patch 
> > with all fixes and fixes to fixes folded into it?
> 
> I can, though Andrew usually objects to that.

folding often results in commit messages and hence credit lost - and 
thus discourages test/review feedback and bugfixing. It's also easier to 
see the track record/history of a patch if the fixes are in separate 
patches. It's also easier to undo a 'fix' (and track the state/version 
of changes) if it turns out to be a bad fix. If things get spaghetti 
then a new splitup indeed helps (for larger patchsets), and Andrew 
usually asks for a new splitup in that case. Andrew often folds patches 
together right before sending it off to Linus (while constructing 
combined credit - so credit is not lost).

	Ingo

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Andrew Morton]

On Thu, 3 Jan 2008 14:10:33 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:

> 
> which would not manage to return ERR_PTR(-EIO), no matter what - it would
> die on access to inode->i_state.  I don't have -mm tree at hand, check if
> there's anything affected in these areas.  Perhaps somebody tried to pass
> error values from isofs_iget() and forgot to update callers?

Yep.  Unlike 2.6.24-rc5-mm1, 2.6.24-rc6-mm1 has

iget-stop-isofs-from-using-read_inode-fix-2.patch
iget-stop-isofs-from-using-read_inode-fix-2-update.patch

so hopefully this was all fixed.

Re: isofs oops - d_splice_alias+0x1f (2.6.24-rc5-mm1)

[Al Viro]

On Sat, Jan 05, 2008 at 01:31:26AM -0800, Andrew Morton wrote:
> On Thu, 3 Jan 2008 14:10:33 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > 
> > which would not manage to return ERR_PTR(-EIO), no matter what - it would
> > die on access to inode->i_state.  I don't have -mm tree at hand, check if
> > there's anything affected in these areas.  Perhaps somebody tried to pass
> > error values from isofs_iget() and forgot to update callers?
> 
> Yep.  Unlike 2.6.24-rc5-mm1, 2.6.24-rc6-mm1 has
> 
> iget-stop-isofs-from-using-read_inode-fix-2.patch
> iget-stop-isofs-from-using-read_inode-fix-2-update.patch
> 
> so hopefully this was all fixed.

Nope; missed by both; fix (and fix to fix) had been posted, but IMO it's
better to replace the entire sorry pile with new variant of patch dhowells
had posted - it has all fixes folded in.