* [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 related [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;
as well as URLs for NNTP newsgroup(s).