linux-kernel - Re: [PATCH] btrfs:Change BUG_ON to new error path in __clear_extent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL3q7H5GLadiLA9PWBXyfamRUb4o=8vHiPd16e9XYBAbWjSKiA@mail.gmail.com>
Date:	Wed, 6 Apr 2016 17:10:58 +0100
From:	Filipe Manana <fdmanana@...il.com>
To:	Bastien Philbert <bastienphilbert@...il.com>
Cc:	Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
	David Sterba <dsterba@...e.com>,
	"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] btrfs:Change BUG_ON to new error path in __clear_extent_bit

On Wed, Apr 6, 2016 at 4:56 PM, Bastien Philbert
<bastienphilbert@...il.com> wrote:
> This remove the unnessary BUG_ON if the allocation with
> alloc_extent_state_atomic fails due to this function
> failure not being unrecoverable. Instead we now change
> this BUG_ON into a new error path that jumps to the goto
> label, out from freeing previously allocated resources
> before returning the error code -ENOMEM to signal callers
> that the call to __clear_extent_bit failed due to a memory
> allocation failure.
>
> Signed-off-by: Bastien Philbert <bastienphilbert@...il.com>
> ---
>  fs/btrfs/extent_io.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index d247fc0..4c87b77 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -682,7 +682,10 @@ hit_next:
>
>         if (state->start < start) {
>                 prealloc = alloc_extent_state_atomic(prealloc);
> -               BUG_ON(!prealloc);
> +               if (!prealloc) {
> +                       err = -ENOMEM;
> +                       goto out;
> +               }

Why only in this particular place? We do this in other places in this function.

Second, setting err to -ENOMEM is not enough. Under the out label we
always return 0, so you're not propagating the error to callers.

Now, most importantly, you didn't check if callers handle errors from
this function (__clear_extent_bit()) at all. A failure in this
function is critical.
For example, it can cause a range in an inode's io tree to become
locked forever, blocking any other tasks that want to operate on the
range, and we won't ever know what happened.
So it's far from trivial to handle errors from this function and
that's why the BUG_ON is there.

If you really want to get rid of the BUG_ON() calls you need to make
sure all callers don't ignore the errors and that they deal with them
properly.

>                 err = split_state(tree, state, prealloc, start);
>                 if (err)
>                         extent_io_tree_panic(tree, err);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."