Re: [PATCH] fs-writeback: fix NULL pointer dereference in__mark_inode_dirty

[Sergey Senozhatsky]

On (02/28/11 16:59), Andreas Bießmann wrote:
> Dear Sergey Senozhatsky,
> 
> Am 28.02.2011 16:43, schrieb Sergey Senozhatsky:
> > On (02/28/11 16:25), Andreas Bießmann wrote:
> 
> >> The reference to sb->s_bdi may be deleted from mmc_blk_remove() ->
> >> del_gendisk() -> unlink_gendisk() -> bdi_unregister() -> bdi_prune_sb() while
> >> another instance try to write some data to the device.
> >>
> >> Signed-off-by: Andreas Bießmann <biessmann@xxxxxxxxxxxxx>
> >> ---
> >>  fs/fs-writeback.c |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> >> index cdbf7ac..96b4b25 100644
> >> --- a/fs/fs-writeback.c
> >> +++ b/fs/fs-writeback.c
> >> @@ -1047,6 +1047,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> >>  		if (!was_dirty) {
> >>  			bdi = inode_to_bdi(inode);
> >>  
> >> +			if (!bdi)
> >> +				goto out;
> >> +
> >>  			if (bdi_cap_writeback_dirty(bdi)) {
> >>  				WARN(!test_bit(BDI_registered, &bdi->state),
> >>  				     "bdi-%s not registered\n", bdi->name);
> > 
> > Hello,
> > I had something very similar to this some time ago
> > https://lkml.org/lkml/2010/12/9/436
> 
> Sorry, I did not see that patch.
>
No problem :-)
 
> > However, I'm not sure that this check is sufficient.
> 
> Why are you think this is not sufficient?
> If an instance try to write that specific inode to an physical device
> which is not longer available how should we react then?
> 

I think the path which `kills' the device should take care of that.
Otherwise, IMHO, we have:
- ok, we're falling on line 42 -- let's fix line 42

ignoring the fact that there are reasons which led to faulty line 42,
which are, for example:
#0 
 604     spin_lock(&sb_lock);
 605     list_for_each_entry(sb, &super_blocks, s_list) {
 606         if (sb->s_bdi == bdi)
 607             sb->s_bdi = NULL;
 608     }
 609     spin_unlock(&sb_lock);

#1 bdi_prune_sb                                         
#2 bdi_unregister                
#3 del_gendisk                   

Of course, I may be wrong.


> Another solution could be to clean up all instances referring to that
> superblock in del_/unlink_gendisk(). But I think to check the return of
> inode_to_bdi() is needed in any case.
> 


	Sergey

Attachment: pgp00000.pgp
Description: PGP signature