Messages in this thread Patch in this message | | | Subject | Re: [Jfs-discussion] [PATCH] jfs: Add missing NULL pointer check in __get_metapage | From | Dave Kleikamp <> | Date | Mon, 30 Oct 2017 17:13:07 -0500 |
| |
On 10/25/2017 02:50 AM, Juerg Haefliger wrote: > Is this a patch you might consider?
Sorry it's taken me so long to respond.
I don't think this is the right fix. A failed allocation will still result in a null pointer dereference by the caller, __get_metapage(). I think the check needs to be put there. Like this:
--- a/fs/jfs/jfs_metapage.c +++ b/fs/jfs/jfs_metapage.c @@ -663,6 +663,8 @@ struct metapage *__get_metapage(struct inode *inode, unsigned long lblock, } else { INCREMENT(mpStat.pagealloc); mp = alloc_metapage(GFP_NOFS); + if (!mp) + goto unlock; mp->page = page; mp->sb = inode->i_sb; mp->flag = 0;
Furthermore, it looks like all the callers of __get_metapage() check for a null return, so I'm not sure we need to handle the error at this point. I might have to look a bit harder at that, since there are many callers.
Thanks, Shaggy
> > Thanks > ...Juerg > > > On 10/04/2017 10:24 AM, Juerg Haefliger wrote: >> alloc_metapage can return a NULL pointer so check for that. And also emit >> an error message if that happens. >> >> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com> >> --- >> fs/jfs/jfs_metapage.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c >> index 1c4b9ad4d7ab..00f21af66872 100644 >> --- a/fs/jfs/jfs_metapage.c >> +++ b/fs/jfs/jfs_metapage.c >> @@ -187,14 +187,18 @@ static inline struct metapage *alloc_metapage(gfp_t gfp_mask) >> { >> struct metapage *mp = mempool_alloc(metapage_mempool, gfp_mask); >> >> - if (mp) { >> - mp->lid = 0; >> - mp->lsn = 0; >> - mp->data = NULL; >> - mp->clsn = 0; >> - mp->log = NULL; >> - init_waitqueue_head(&mp->wait); >> + if (!mp) { >> + jfs_err("mempool_alloc failed!\n"); >> + return NULL; >> } >> + >> + mp->lid = 0; >> + mp->lsn = 0; >> + mp->data = NULL; >> + mp->clsn = 0; >> + mp->log = NULL; >> + init_waitqueue_head(&mp->wait); >> + >> return mp; >> } >> >> @@ -663,6 +667,8 @@ struct metapage *__get_metapage(struct inode *inode, unsigned long lblock, >> } else { >> INCREMENT(mpStat.pagealloc); >> mp = alloc_metapage(GFP_NOFS); >> + if (!mp) >> + goto unlock; >> mp->page = page; >> mp->sb = inode->i_sb; >> mp->flag = 0; >>
| |