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
| ||
|
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."
Powered by blists - more mailing lists