linux-kernel - Re: [PATCH 1/1] Btrfs: Code Cleanup

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20160324150307.GR29764@twin.jikos.cz>
Date:	Thu, 24 Mar 2016 16:03:07 +0100
From:	David Sterba <dsterba@...e.cz>
To:	Flex Liu <fliu@...ell.com>
Cc:	David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
	Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
	linux-kernel@...r.kernel.org, Petr Tesarik <ptesarik@...e.com>,
	Flex Liu <fliu@...e.com>
Subject: Re: [PATCH 1/1] Btrfs: Code Cleanup

On Sun, Mar 20, 2016 at 03:11:11PM +0800, Flex Liu wrote:
> From: Flex Liu <fliu@...e.com>
> 
> In fs/btrfs/volumes.c:2328
> 
>         if (seeding_dev) {
>                 sb->s_flags &= ~MS_RDONLY;
>                 ret = btrfs_prepare_sprout(root);
>                 BUG_ON(ret); /* -ENOMEM */
>         }
> 
> the error code would be return from:
> 
>         fs_devs = kzalloc(sizeof(*fs_devs), GFP_NOFS);
>         if (!fs_devs)
>                 return ERR_PTR(-ENOMEM);

> Insufficient memory in btrfs would let the kernel panic, it suboptimal.
> instead, we should return the error code in btrfs_init_new_device to
> btrfs_ioctl.
> 
> Hello kernel list.
> This is my first patch for kernel, so if I missed some of the guidelines,
> please be patient :) I hope everything is explained in the commit message.

So a few comments:

- the subject line should say something like

  "handle errors in btrfs_init_new_device"
  or "replace BUG_ON with proper error handling",

  because it's not a code cleanup in the sense we commonly use

- you don't need to quote the code in the changelog, we can see it in
  the sources (unless you want to point out something very specific that
  might not be obvious)

> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2325,7 +2325,10 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>  	if (seeding_dev) {
>  		sb->s_flags &= ~MS_RDONLY;
>  		ret = btrfs_prepare_sprout(root);
> -		BUG_ON(ret); /* -ENOMEM */
> +		if (ret) {
> +			btrfs_abort_transaction(trans, root, ret);

The transaction abort seems a bit heavy as it will take down the whole
filesystem. It's called from the device add ioctl, this is a restartable
operation.

Unfortunatelly btrfs_prepare_sprout is called after the transaction
start so btrfs_abort_transaction must be called. To avoid it, the code
would need to be reorganized, so the memory allocations happen in
advance.

Fixing the error handling might need more patches. btrfs_prepare_sprout
can be split into parts that do just allocations and initialization and
do not touch the filesystem state (like dropping the seeding flag).

The first part will hopefully cover all failure possibilities, before
the transaction stargs, the second shall not fail at all and the error
checking can be avoided completely.