Proper error handling on NULL pointers

Proper error handling on NULL pointers

[Andi Drebes]

Hi!
I recently posted a message that addresses the usage of BUG_ON for memory shortages concerning btrfs_alloc_path() (see [1]):

	...
	path = btrfs_alloc_path();
	BUG_ON(!path);
	ret = btrfs_search_slot(NULL, root, &search_key, path, 0, 0);
	...

It seems that this is not the only place where NULL pointers returned by allocation functions are not handled properly. For instance in start_transaction(), errors are not handled at all:

	struct btrfs_trans_handle *h =
		kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS);
	...
	/* Access h without check */
	...
	return h;

Repairing this would sometimes require to review a bunch of functions that directly or indirectly refer to it. As pointed out by Andi Kleen two weeks ago in a reply to message by Diego Calleja (see [2] and [3]), this might not be trivial, because these errors are sometimes not handled at all in highlevel functions.

However, is there any interest in patches fixing these problems? If yes: what would be the best strategy? Should we start fixing this "layer by layer" -- the low-level functions first and the high-level functions later on? Or should use come kind of "vertical approach" -- one low-level function and all of its callers at once?

If these issues currently don't have a very high priority, we could collect them in a TODO section in the Wiki or something. Any suggestions?

	Cheers,
		Andi

[1] http://www.spinics.net/lists/linux-btrfs/msg03221.html
[2] http://www.spinics.net/lists/linux-btrfs/msg03197.html
[3] http://www.spinics.net/lists/linux-btrfs/msg03198.html

Re: Proper error handling on NULL pointers

[Diego Calleja]

On Lunes 19 Octubre 2009 12:36:13 Andi Drebes escribi=C3=B3:
> However, is there any interest in patches fixing these problems? If y=
es: what would be the best strategy? Should we start fixing this "layer=
 by layer" -- the low-level functions first and the high-level function=
s later on? Or should use come kind of "vertical approach" -- one low-l=
evel function and all of its callers at once?

I don't know what is the developer plan to fix that - apparently it's
not in the high-priority list (but it must be certainly in the priority
list, anyone who gets out of memory using btrfs will have some chances =
of
getting an oops - but notice that most of the important paths are ready
to handle errors reliably and there aren't many bug reports due to bad
oom handling, so it doesn't seems to be that critical).

I realized that it isn't really helpful to add BUG_ONs to failed alloca=
tion
paths, the code will oops itself as soon as it tries to use the NULL po=
inter,
so adding BUG_ONs is redundant. Passing the error to the callers and ha=
ndling
all that properly is the real fix, but since it requires auditing the w=
hole FS
it's probably not an easy task. I tried to do that with a couple of fun=
ctions,
but Kleen's mail made me realize that it isn't that easy.=20
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Proper error handling on NULL pointers

[Andi Drebes]

> I don't know what is the developer plan to fix that - apparently it's
> not in the high-priority list (but it must be certainly in the priority
> list, anyone who gets out of memory using btrfs will have some chances of
> getting an oops [...]).
I'd really appreciate to see a TODO section somewhere in the wiki. All the stuff that doesn't have a very high priority, but that should be done sometime in the future, could be put there. Even the simplest things like search and replace operations. This would be a good point for people not yet familiar with the btrfs code (like me) to get involved and main developers would get rid of the things that don't have a high priority.

> [...] Passing the error to the callers and handling
> all that properly is the real fix, but since it requires auditing the whole
> FS it's probably not an easy task. I tried to do that with a couple of
> functions, but Kleen's mail made me realize that it isn't that easy.
Maybe it would be easier if we found some people helping us to fix this. In this case it would also be really helpful if one of the main developers reviewed the work with a "btrfs expert eye".

For now I'm going to analyze some dependencies and see how many and *which* functions are affected.

What about the main developers? How do you see things? I'd really like to hear your opinion before messing around in the project.

	Cheers,
		Andi

Re: Proper error handling on NULL pointers

[Diego Calleja]

On Jueves 22 Octubre 2009 12:15:59 Andi Drebes escribi=C3=B3:
> I'd really appreciate to see a TODO section somewhere in the wiki. [.=
=2E]

There's one (needs updating): http://btrfs.wiki.kernel.org/index.php/De=
velopment_timeline
Also, http://btrfs.wiki.kernel.org/index.php/Project_ideas has some ide=
as.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Proper error handling on NULL pointers

[Andi Drebes]

> > I'd really appreciate to see a TODO section somewhere in the wiki. [..]
> 
> There's one (needs updating):
> http://btrfs.wiki.kernel.org/index.php/Development_timeline 
> Also, http://btrfs.wiki.kernel.org/index.php/Project_ideas has some ideas.
Yes, I've already seen them. They are quite feature-/highlevel-oriented. I thought of something more detailed focusing on cleanups / errors / etc.

	Cheers,
		Andi

Re: Proper error handling on NULL pointers

[Jeff Mahoney]

On 10/22/2009 06:15 AM, Andi Drebes wrote:
>> I don't know what is the developer plan to fix that - apparently it's
>> not in the high-priority list (but it must be certainly in the priority
>> list, anyone who gets out of memory using btrfs will have some chances of
>> getting an oops [...]).
> I'd really appreciate to see a TODO section somewhere in the wiki. All the stuff that doesn't have a very high priority, but that should be done sometime in the future, could be put there. Even the simplest things like search and replace operations. This would be a good point for people not yet familiar with the btrfs code (like me) to get involved and main developers would get rid of the things that don't have a high priority.
> 
>> [...] Passing the error to the callers and handling
>> all that properly is the real fix, but since it requires auditing the whole
>> FS it's probably not an easy task. I tried to do that with a couple of
>> functions, but Kleen's mail made me realize that it isn't that easy.
> Maybe it would be easier if we found some people helping us to fix this. In this case it would also be really helpful if one of the main developers reviewed the work with a "btrfs expert eye".
> 
> For now I'm going to analyze some dependencies and see how many and *which* functions are affected.
> 
> What about the main developers? How do you see things? I'd really like to hear your opinion before messing around in the project.

Hi Andi -

I actually have a patch set that addresses these. I submitted it several
months ago, but nothing happened. The problem with fixes like these is
that they change context *everywhere* so the best time to add them is
really when there is a lull in the project. Otherwise, you end up
forcing people working on other features or fixes to redo a bunch of
work. When's the last time you saw a lull in btrfs development? :)

I'll refresh it when I get a chance either today or early next week and
post it again.

-Jeff

-- 
Jeff Mahoney
SUSE Labs

Re: Proper error handling on NULL pointers

[Chris Mason]

On Fri, Oct 23, 2009 at 02:08:48PM -0400, Jeff Mahoney wrote:
> On 10/22/2009 06:15 AM, Andi Drebes wrote:
> >> I don't know what is the developer plan to fix that - apparently it's
> >> not in the high-priority list (but it must be certainly in the priority
> >> list, anyone who gets out of memory using btrfs will have some chances of
> >> getting an oops [...]).
> > I'd really appreciate to see a TODO section somewhere in the wiki. All the stuff that doesn't have a very high priority, but that should be done sometime in the future, could be put there. Even the simplest things like search and replace operations. This would be a good point for people not yet familiar with the btrfs code (like me) to get involved and main developers would get rid of the things that don't have a high priority.
> > 
> >> [...] Passing the error to the callers and handling
> >> all that properly is the real fix, but since it requires auditing the whole
> >> FS it's probably not an easy task. I tried to do that with a couple of
> >> functions, but Kleen's mail made me realize that it isn't that easy.
> > Maybe it would be easier if we found some people helping us to fix this. In this case it would also be really helpful if one of the main developers reviewed the work with a "btrfs expert eye".
> > 
> > For now I'm going to analyze some dependencies and see how many and *which* functions are affected.
> > 
> > What about the main developers? How do you see things? I'd really like to hear your opinion before messing around in the project.
> 
> Hi Andi -
> 
> I actually have a patch set that addresses these. I submitted it several
> months ago, but nothing happened.

Hi Jeff,

If you're able to refresh this, I'll push it immediately into the .32
queue.  That way we won't end up having to rediff it again.

Thanks!

-chris

Re: Proper error handling on NULL pointers

[Jeff Mahoney]

On 10/26/2009 05:14 AM, Chris Mason wrote:
> On Fri, Oct 23, 2009 at 02:08:48PM -0400, Jeff Mahoney wrote:
>> On 10/22/2009 06:15 AM, Andi Drebes wrote:
>>>> I don't know what is the developer plan to fix that - apparently it's
>>>> not in the high-priority list (but it must be certainly in the priority
>>>> list, anyone who gets out of memory using btrfs will have some chances of
>>>> getting an oops [...]).
>>> I'd really appreciate to see a TODO section somewhere in the wiki. All the stuff that doesn't have a very high priority, but that should be done sometime in the future, could be put there. Even the simplest things like search and replace operations. This would be a good point for people not yet familiar with the btrfs code (like me) to get involved and main developers would get rid of the things that don't have a high priority.
>>>
>>>> [...] Passing the error to the callers and handling
>>>> all that properly is the real fix, but since it requires auditing the whole
>>>> FS it's probably not an easy task. I tried to do that with a couple of
>>>> functions, but Kleen's mail made me realize that it isn't that easy.
>>> Maybe it would be easier if we found some people helping us to fix this. In this case it would also be really helpful if one of the main developers reviewed the work with a "btrfs expert eye".
>>>
>>> For now I'm going to analyze some dependencies and see how many and *which* functions are affected.
>>>
>>> What about the main developers? How do you see things? I'd really like to hear your opinion before messing around in the project.
>>
>> Hi Andi -
>>
>> I actually have a patch set that addresses these. I submitted it several
>> months ago, but nothing happened.
> 
> Hi Jeff,
> 
> If you're able to refresh this, I'll push it immediately into the .32
> queue.  That way we won't end up having to rediff it again.

Hi Chris -

I spent some time refreshing it this weekend. I'm doing some review now
and will hopefully post it later today.

-Jeff

-- 
Jeff Mahoney
SUSE Labs

Re: Proper error handling on NULL pointers

[Jeff Mahoney]

On 10/26/2009 05:14 AM, Chris Mason wrote:
> On Fri, Oct 23, 2009 at 02:08:48PM -0400, Jeff Mahoney wrote:
>> On 10/22/2009 06:15 AM, Andi Drebes wrote:
>>>> I don't know what is the developer plan to fix that - apparently it's
>>>> not in the high-priority list (but it must be certainly in the priority
>>>> list, anyone who gets out of memory using btrfs will have some chances of
>>>> getting an oops [...]).
>>> I'd really appreciate to see a TODO section somewhere in the wiki. All the stuff that doesn't have a very high priority, but that should be done sometime in the future, could be put there. Even the simplest things like search and replace operations. This would be a good point for people not yet familiar with the btrfs code (like me) to get involved and main developers would get rid of the things that don't have a high priority.
>>>
>>>> [...] Passing the error to the callers and handling
>>>> all that properly is the real fix, but since it requires auditing the whole
>>>> FS it's probably not an easy task. I tried to do that with a couple of
>>>> functions, but Kleen's mail made me realize that it isn't that easy.
>>> Maybe it would be easier if we found some people helping us to fix this. In this case it would also be really helpful if one of the main developers reviewed the work with a "btrfs expert eye".
>>>
>>> For now I'm going to analyze some dependencies and see how many and *which* functions are affected.
>>>
>>> What about the main developers? How do you see things? I'd really like to hear your opinion before messing around in the project.
>>
>> Hi Andi -
>>
>> I actually have a patch set that addresses these. I submitted it several
>> months ago, but nothing happened.
> 
> Hi Jeff,
> 
> If you're able to refresh this, I'll push it immediately into the .32
> queue.  That way we won't end up having to rediff it again.

I had to push this to next week. It builds and I expect it be stable,
but since there's been 6+ months of development on btrfs since I last
refreshed it, I want to make sure that I'm not missing call paths. I
quick check shows that I am and I just haven't had the time to update it
this week.

-Jeff

-- 
Jeff Mahoney
SUSE Labs

Re: Proper error handling on NULL pointers

[Chris Mason]

On Fri, Oct 30, 2009 at 09:27:36AM -0400, Jeff Mahoney wrote:
> On 10/26/2009 05:14 AM, Chris Mason wrote:
> > On Fri, Oct 23, 2009 at 02:08:48PM -0400, Jeff Mahoney wrote:
> >> On 10/22/2009 06:15 AM, Andi Drebes wrote:
> >>>> I don't know what is the developer plan to fix that - apparently it's
> >>>> not in the high-priority list (but it must be certainly in the priority
> >>>> list, anyone who gets out of memory using btrfs will have some chances of
> >>>> getting an oops [...]).
> >>> I'd really appreciate to see a TODO section somewhere in the wiki. All the stuff that doesn't have a very high priority, but that should be done sometime in the future, could be put there. Even the simplest things like search and replace operations. This would be a good point for people not yet familiar with the btrfs code (like me) to get involved and main developers would get rid of the things that don't have a high priority.
> >>>
> >>>> [...] Passing the error to the callers and handling
> >>>> all that properly is the real fix, but since it requires auditing the whole
> >>>> FS it's probably not an easy task. I tried to do that with a couple of
> >>>> functions, but Kleen's mail made me realize that it isn't that easy.
> >>> Maybe it would be easier if we found some people helping us to fix this. In this case it would also be really helpful if one of the main developers reviewed the work with a "btrfs expert eye".
> >>>
> >>> For now I'm going to analyze some dependencies and see how many and *which* functions are affected.
> >>>
> >>> What about the main developers? How do you see things? I'd really like to hear your opinion before messing around in the project.
> >>
> >> Hi Andi -
> >>
> >> I actually have a patch set that addresses these. I submitted it several
> >> months ago, but nothing happened.
> > 
> > Hi Jeff,
> > 
> > If you're able to refresh this, I'll push it immediately into the .32
> > queue.  That way we won't end up having to rediff it again.
> 
> I had to push this to next week. It builds and I expect it be stable,
> but since there's been 6+ months of development on btrfs since I last
> refreshed it, I want to make sure that I'm not missing call paths. I
> quick check shows that I am and I just haven't had the time to update it
> this week.

While I'm sure you'll have to update parts of the patch to catch the
last 6 months of work, I'd take an incomplete patch as long as it
doesn't break things.  No sense in needing to update the baseline code
over and over again.

-chris

Re: Proper error handling on NULL pointers

[Chris Mason]

On Wed, Oct 21, 2009 at 07:43:51PM +0200, Diego Calleja wrote:
> On Lunes 19 Octubre 2009 12:36:13 Andi Drebes escribi=F3:
> > However, is there any interest in patches fixing these problems? If=
 yes: what would be the best strategy? Should we start fixing this "lay=
er by layer" -- the low-level functions first and the high-level functi=
ons later on? Or should use come kind of "vertical approach" -- one low=
-level function and all of its callers at once?
>=20
> I don't know what is the developer plan to fix that - apparently it's
> not in the high-priority list (but it must be certainly in the priori=
ty
> list, anyone who gets out of memory using btrfs will have some chance=
s of
> getting an oops - but notice that most of the important paths are rea=
dy
> to handle errors reliably and there aren't many bug reports due to ba=
d
> oom handling, so it doesn't seems to be that critical).

On thing to remember on the allocator is that when you're asking for
one page or less in kmalloc the allocator will generally oom the box be=
fore it
returns null.

So, I'm not suggesting we aren't going to fix these but that's why they
haven't turned into any bug reports yet.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html