: initialize parts of udf inode earlier in create
PATCH 1/1 : initialize parts of udf inode earlier in create
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
fs/udf/ialloc.c +0 -0
@@ -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;Quoted 11 lines
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
--
fs/udf/super.c renamed from fs/udf/super.c.orig +7 -0
@@ -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; }
Signature · 4 lines
[email protected] -- http://www.fastmail.fm - Email service worth paying for. Try it for free
Quoted 2 lines · [email protected] wrote:
I ran into the same issue today, but when listing a directory with invalid/corrupt entries:
...
Quoted 15 lines
The following patch to
udf_alloc_inode()should take care of both (and other similar) cases, but I've only tested it withudf_lookup().Dan
--
Signed-off-by Dan Bastone <[email protected]>fs/udf/super.c renamed from fs/udf/super.c.orig +0
-0@@ -116,6 +116,13 @@ ei = (struct udf_inode_info *)kmem_cache_alloc (udf_inode_cachep,SLAB_KERNEL );if (!ei)
fs/udf/super.c line 128
Quoted 10 lines
fs/udf/super.c renamed from fs/udf/super.c.orig +7
-0return 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)
Quoted 6 lines · Eric Sandeen
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_inodefails, then it callsmark_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:
---
fs/udf/ialloc.c renamed from fs/udf/ialloc.c.orig +0 -5
@@ -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
Signature · 2 lines
http://www.fastmail.fm - Faster than the air-speed velocity of an
unladen european swallow