* [PATCH] jfs: Add missing NULL pointer check in __get_metapage @ 2017-10-04 8:24 Juerg Haefliger 2017-10-25 7:50 ` Juerg Haefliger 0 siblings, 1 reply; 7+ messages in thread From: Juerg Haefliger @ 2017-10-04 8:24 UTC (permalink / raw) To: shaggy, jfs-discussion, linux-kernel; +Cc: Juerg Haefliger 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; -- 2.14.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] jfs: Add missing NULL pointer check in __get_metapage 2017-10-04 8:24 [PATCH] jfs: Add missing NULL pointer check in __get_metapage Juerg Haefliger @ 2017-10-25 7:50 ` Juerg Haefliger 2017-10-30 22:13 ` [Jfs-discussion] " Dave Kleikamp 0 siblings, 1 reply; 7+ messages in thread From: Juerg Haefliger @ 2017-10-25 7:50 UTC (permalink / raw) To: shaggy, jfs-discussion, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1452 bytes --] Is this a patch you might consider? 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; > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 845 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Jfs-discussion] [PATCH] jfs: Add missing NULL pointer check in __get_metapage 2017-10-25 7:50 ` Juerg Haefliger @ 2017-10-30 22:13 ` Dave Kleikamp 2017-11-02 6:59 ` Juerg Haefliger 0 siblings, 1 reply; 7+ messages in thread From: Dave Kleikamp @ 2017-10-30 22:13 UTC (permalink / raw) To: Juerg Haefliger, jfs-discussion, linux-kernel 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; >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Jfs-discussion] [PATCH] jfs: Add missing NULL pointer check in __get_metapage 2017-10-30 22:13 ` [Jfs-discussion] " Dave Kleikamp @ 2017-11-02 6:59 ` Juerg Haefliger 2017-11-02 13:15 ` Dave Kleikamp 0 siblings, 1 reply; 7+ messages in thread From: Juerg Haefliger @ 2017-11-02 6:59 UTC (permalink / raw) To: Dave Kleikamp, jfs-discussion, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2784 bytes --] On 10/30/2017 11:13 PM, Dave Kleikamp wrote: > 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; I don't understand. This is part of the patch that I sent. > > 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. I don't understand this either :-) Yes, the callers do check for a null pointer but things blow up (in __get_metapage) before that check without the above fix. ...Juerg > > 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; >>> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 845 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Jfs-discussion] [PATCH] jfs: Add missing NULL pointer check in __get_metapage 2017-11-02 6:59 ` Juerg Haefliger @ 2017-11-02 13:15 ` Dave Kleikamp 2017-11-02 13:43 ` Juerg Haefliger 0 siblings, 1 reply; 7+ messages in thread From: Dave Kleikamp @ 2017-11-02 13:15 UTC (permalink / raw) To: Juerg Haefliger, jfs-discussion, linux-kernel On 11/02/2017 01:59 AM, Juerg Haefliger wrote: > > > On 10/30/2017 11:13 PM, Dave Kleikamp wrote: >> 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; > > I don't understand. This is part of the patch that I sent. Doh! How'd I miss that? > > >> >> 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. > > I don't understand this either :-) Yes, the callers do check for a null > pointer but things blow up (in __get_metapage) before that check without > the above fix. Yeah, the fix to __get_metapage() is necessary. I'm not convinced the first part of the patch, to alloc_metapage(), is necessary. > > ...Juerg > > >> >> 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; >>>> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Jfs-discussion] [PATCH] jfs: Add missing NULL pointer check in __get_metapage 2017-11-02 13:15 ` Dave Kleikamp @ 2017-11-02 13:43 ` Juerg Haefliger 2017-11-02 14:44 ` Dave Kleikamp 0 siblings, 1 reply; 7+ messages in thread From: Juerg Haefliger @ 2017-11-02 13:43 UTC (permalink / raw) To: Dave Kleikamp, jfs-discussion, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 3436 bytes --] On 11/02/2017 02:15 PM, Dave Kleikamp wrote: > On 11/02/2017 01:59 AM, Juerg Haefliger wrote: >> >> >> On 10/30/2017 11:13 PM, Dave Kleikamp wrote: >>> 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; >> >> I don't understand. This is part of the patch that I sent. > > Doh! How'd I miss that? :-) >> >> >>> >>> 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. >> >> I don't understand this either :-) Yes, the callers do check for a null >> pointer but things blow up (in __get_metapage) before that check without >> the above fix. > > Yeah, the fix to __get_metapage() is necessary. I'm not convinced the > first part of the patch, to alloc_metapage(), is necessary. It's not. I just thought it'd be nice to get some sort of notification in the log when the alloc fails. But if the callers log it then that's fine. ...Juerg >> >> ...Juerg >> >> >>> >>> 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; >>>>> >> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 845 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Jfs-discussion] [PATCH] jfs: Add missing NULL pointer check in __get_metapage 2017-11-02 13:43 ` Juerg Haefliger @ 2017-11-02 14:44 ` Dave Kleikamp 0 siblings, 0 replies; 7+ messages in thread From: Dave Kleikamp @ 2017-11-02 14:44 UTC (permalink / raw) To: Juerg Haefliger, jfs-discussion, linux-kernel On 11/02/2017 08:43 AM, Juerg Haefliger wrote: >>>> 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. >>> >>> I don't understand this either :-) Yes, the callers do check for a null >>> pointer but things blow up (in __get_metapage) before that check without >>> the above fix. >> >> Yeah, the fix to __get_metapage() is necessary. I'm not convinced the >> first part of the patch, to alloc_metapage(), is necessary. > > It's not. I just thought it'd be nice to get some sort of notification > in the log when the alloc fails. But if the callers log it then that's fine. Okay. I'll push just the __get_metapage part of your patch. Thanks, Shaggy ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-02 14:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-04 8:24 [PATCH] jfs: Add missing NULL pointer check in __get_metapage Juerg Haefliger 2017-10-25 7:50 ` Juerg Haefliger 2017-10-30 22:13 ` [Jfs-discussion] " Dave Kleikamp 2017-11-02 6:59 ` Juerg Haefliger 2017-11-02 13:15 ` Dave Kleikamp 2017-11-02 13:43 ` Juerg Haefliger 2017-11-02 14:44 ` Dave Kleikamp
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®