Messages in this thread |  | | | Date | Mon, 28 Feb 2011 18:29:09 +0200 | | From | Sergey Senozhatsky <> | | Subject | Re: [PATCH] fs-writeback: fix NULL pointer dereference in __mark_inode_dirty |
| |
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@corscience.de> > >> --- > >> 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 [unhandled content-type:application/pgp-signature] |  |