missed inode->i_hash cleanup in prune_icache()

missed inode->i_hash cleanup in prune_icache()

[Nikita Danilov]

Hello,

fs/inode.c:prune_icache() does list_del(&inode->i_hash), and then calls
destroy_inode(). Inode is returned to the slab with ->i_hash still
containing dangling pointers. Probably this wasn't observed so far,
because prune_icache() is called during memory pressure and slab page
where inode is returned back into, is almost immediately released.

2.4 explicitly calls INIT_LIST_HEAD(&inode->i_hash) in prune_icache().

Following patch re-initializes ->i_hash.

Nikita.
===== fs/inode.c 1.84 vs edited =====
--- 1.84/fs/inode.c	Mon Dec 16 09:38:48 2002
+++ edited/fs/inode.c	Wed Dec 25 16:19:10 2002
@@ -248,7 +248,7 @@
 		struct inode *inode;
 
 		inode = list_entry(head->next, struct inode, i_list);
-		list_del(&inode->i_list);
+		list_del_init(&inode->i_list);
 
 		if (inode->i_data.nrpages)
 			truncate_inode_pages(&inode->i_data, 0);

Re: missed inode->i_hash cleanup in prune_icache()

[Andrew Morton]

Nikita Danilov wrote:
> 
> Hello,
> 
> fs/inode.c:prune_icache() does list_del(&inode->i_hash), and then calls
> destroy_inode(). Inode is returned to the slab with ->i_hash still
> containing dangling pointers. Probably this wasn't observed so far,
> because prune_icache() is called during memory pressure and slab page
> where inode is returned back into, is almost immediately released.
> 
> 2.4 explicitly calls INIT_LIST_HEAD(&inode->i_hash) in prune_icache().
> 
> Following patch re-initializes ->i_hash.
> 
> Nikita.
> ===== fs/inode.c 1.84 vs edited =====
> --- 1.84/fs/inode.c     Mon Dec 16 09:38:48 2002
> +++ edited/fs/inode.c   Wed Dec 25 16:19:10 2002
> @@ -248,7 +248,7 @@
>                 struct inode *inode;
> 
>                 inode = list_entry(head->next, struct inode, i_list);
> -               list_del(&inode->i_list);
> +               list_del_init(&inode->i_list);
> 
>                 if (inode->i_data.nrpages)
>                         truncate_inode_pages(&inode->i_data, 0);
> 

That's i_list, not i_hash.

Yes, it's a bit sloppy to leave the i_list pointers dangling but
fs/inode.c:new_inode() will just overwrite i_list and all is well.

Could you please double-check or clarify the need for this change?

Re: missed inode->i_hash cleanup in prune_icache()

[Nikita Danilov]

Andrew Morton writes:
 > Nikita Danilov wrote:
 > > 

[...]

 > >                 struct inode *inode;
 > > 
 > >                 inode = list_entry(head->next, struct inode, i_list);
 > > -               list_del(&inode->i_list);
 > > +               list_del_init(&inode->i_list);
 > > 
 > >                 if (inode->i_data.nrpages)
 > >                         truncate_inode_pages(&inode->i_data, 0);
 > > 
 > 
 > That's i_list, not i_hash.
 > 
 > Yes, it's a bit sloppy to leave the i_list pointers dangling but
 > fs/inode.c:new_inode() will just overwrite i_list and all is well.
 > 
 > Could you please double-check or clarify the need for this change?

You are right, sorry. Probably I stared at these lists for too long or
too short a time. We are seeing garbage on sb->s_io in sync_sb_inodes(),
but probably this is some reiser4 problem after all.

Nikita.