[PATCH]: initialize parts of udf inode earlier in create

[PATCH]: initialize parts of udf inode earlier in create

[Eric Sandeen]

I saw an oops down this path when trying to create a new file on a UDF 
filesystem which was internally marked as readonly, but mounted rw:

udf_create
        udf_new_inode
                new_inode
                        alloc_inode
                        	udf_alloc_inode
                udf_new_block
                        returns EIO due to readonlyness
                iput (on error)
                        udf_put_inode
                                udf_discard_prealloc
                                        udf_next_aext
                                                udf_current_aext
                                                        udf_get_fileshortad
                                                                OOPS

the udf_discard_prealloc() path was examining uninitialized fields of the 
udf inode.

udf_discard_prealloc() already has this code to short-circuit the discard 
path if no extents are preallocated:

        if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_IN_ICB ||
                inode->i_size == UDF_I_LENEXTENTS(inode))
        {
                return;
        }

so if we initialize UDF_I_LENEXTENTS(inode) = 0 earlier in udf_new_inode, we
won't try to free the (not) preallocated blocks, since this will match
the i_size = 0 set when the inode was initialized.

Thanks,

-Eric

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Index: linux-2.6.17/fs/udf/ialloc.c
===================================================================
--- linux-2.6.17.orig/fs/udf/ialloc.c
+++ linux-2.6.17/fs/udf/ialloc.c
@@ -75,6 +75,12 @@ struct inode * udf_new_inode (struct ino
 	}
 	*err = -ENOSPC;
 
+	UDF_I_UNIQUE(inode) = 0;
+	UDF_I_LENEXTENTS(inode) = 0;
+	UDF_I_NEXT_ALLOC_BLOCK(inode) = 0;
+	UDF_I_NEXT_ALLOC_GOAL(inode) = 0;
+	UDF_I_STRAT4096(inode) = 0;
+
 	block = udf_new_block(dir->i_sb, NULL, UDF_I_LOCATION(dir).partitionReferenceNum,
 		start, err);
 	if (*err)
@@ -84,11 +90,6 @@ struct inode * udf_new_inode (struct ino
 	}
 
 	mutex_lock(&sbi->s_alloc_mutex);
-	UDF_I_UNIQUE(inode) = 0;
-	UDF_I_LENEXTENTS(inode) = 0;
-	UDF_I_NEXT_ALLOC_BLOCK(inode) = 0;
-	UDF_I_NEXT_ALLOC_GOAL(inode) = 0;
-	UDF_I_STRAT4096(inode) = 0;
 	if (UDF_SB_LVIDBH(sb))
 	{
 		struct logicalVolHeaderDesc *lvhd;
 

Re: [PATCH]: initialize parts of udf inode earlier in create

[dan]

> I saw an oops down this path when trying to create a new file on a UDF 
> filesystem which was internally marked as readonly, but mounted rw:
> 
> udf_create
>         udf_new_inode
>                 new_inode
>                         alloc_inode
>                         	udf_alloc_inode
>                 udf_new_block
>                         returns EIO due to readonlyness
>                 iput (on error)

I ran into the same issue today, but when listing a directory with
invalid/corrupt entries:

udf_lookup
        udf_iget
                get_new_inode_fast
                        alloc_inode
                                udf_alloc_inode
                __udf_read_inode
                        fails for any reason
                iput (on error)
                        ...

The following patch to udf_alloc_inode() should take care of both (and
other similar) cases, but I've only tested it with udf_lookup().

Dan

--

Signed-off-by: Dan Bastone <dan@pwienterprises.com>

--- linux-2.6.17.7/fs/udf/super.c.orig
+++ linux-2.6.17.7/fs/udf/super.c
@@ -116,6 +116,13 @@
        ei = (struct udf_inode_info *)kmem_cache_alloc(udf_inode_cachep,
        SLAB_KERNEL);
        if (!ei)
                return NULL;
+
+       ei->i_unique = 0;
+       ei->i_lenExtents = 0;
+       ei->i_next_alloc_block = 0;
+       ei->i_next_alloc_goal = 0;
+       ei->i_strat4096 = 0;
+
        return &ei->vfs_inode;
 }


-- 
  
  diegogarcia@cluemail.com

-- 
http://www.fastmail.fm - Email service worth paying for. Try it for free

Re: [PATCH]: initialize parts of udf inode earlier in create

[Eric Sandeen]

dan@pwienterprises.com wrote:
> I ran into the same issue today, but when listing a directory with
> invalid/corrupt entries:

...

> The following patch to udf_alloc_inode() should take care of both (and
> other similar) cases, but I've only tested it with udf_lookup().
> 
> Dan
> 
> --
> 
> Signed-off-by: Dan Bastone <dan@pwienterprises.com>
> 
> --- linux-2.6.17.7/fs/udf/super.c.orig
> +++ linux-2.6.17.7/fs/udf/super.c
> @@ -116,6 +116,13 @@
>         ei = (struct udf_inode_info *)kmem_cache_alloc(udf_inode_cachep,
>         SLAB_KERNEL);
>         if (!ei)
>                 return NULL;
> +
> +       ei->i_unique = 0;
> +       ei->i_lenExtents = 0;
> +       ei->i_next_alloc_block = 0;
> +       ei->i_next_alloc_goal = 0;
> +       ei->i_strat4096 = 0;
> +
>         return &ei->vfs_inode;
>  }

That looks fine to me, but I wonder if there's a cleaner way, rather 
than sprinkling these initializations in the code.  If __udf_read_inode 
fails, then it calls mark_bad_inode; maybe the code should check for 
that before trying to discard prealloced blocks?  I don't really know 
enough about all the UDF codepaths (by far!) to know for sure what the 
best solution is, here.

I do notice that for example ext2_put_inode() checks for bad_inode 
before calling ext2_discard_prealloc.  And it looks like the udf code 
may have a little ext2 history in it :)

-Eric

(hm, just realized that my original patch in this thread isn't strictly 
necessary for the reasons I originally proposed; udf_clear_inode checks 
for MS_RDONLY before discarding the prealloc, and my first UDF patch set 
the MS_RDONLY flag on these read-only-marked filesystems... ah well)

Re: [PATCH]: initialize parts of udf inode earlier in create

[Dan Bastone]

On Mon, 07 Aug 2006 17:45:03 -0500, "Eric Sandeen" <sandeen@sandeen.net>
said:
> That looks fine to me, but I wonder if there's a cleaner way, rather
> than sprinkling these initializations in the code.  If __udf_read_inode
> fails, then it calls mark_bad_inode; maybe the code should check for
> that before trying to discard prealloced blocks?  I don't really know
> enough about all the UDF codepaths (by far!) to know for sure what the
> best solution is, here.

I'm certainly not an expert on this code either, but it seems like doing
the initializations once in udf_alloc_inode() makes the most sense.  As
I said it should fix both of the scenarios you & I experienced as well
as any others that assume the udf_inode_info structs are zeroed.  Now
that I look at it again, I think it also makes the initializations in
udf_new_inode() redundant.

So, assuming my previous patch is applied and yours is not, I think the
following is right:

---

Signed-off-by: Dan Bastone <dan@pwienterprises.com>

--- linux-2.6.17.7/fs/udf/ialloc.c.orig
+++ linux-2.6.17.7/fs/udf/ialloc.c
@@ -84,11 +84,6 @@
        }

        mutex_lock(&sbi->s_alloc_mutex);
-       UDF_I_UNIQUE(inode) = 0;
-       UDF_I_LENEXTENTS(inode) = 0;
-       UDF_I_NEXT_ALLOC_BLOCK(inode) = 0;
-       UDF_I_NEXT_ALLOC_GOAL(inode) = 0;
-       UDF_I_STRAT4096(inode) = 0;
        if (UDF_SB_LVIDBH(sb))
        {
                struct logicalVolHeaderDesc *lvhd;

---

Dan

-- 
http://www.fastmail.fm - Faster than the air-speed velocity of an
                          unladen european swallow