linux-kernel - Re: locking problems: Btrfs: be more selective in the defrag ioctl

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: <20100322134720.GA2383@localhost.localdomain>
Date:	Mon, 22 Mar 2010 09:47:21 -0400
From:	Josef Bacik <josef@...hat.com>
To:	Dan Carpenter <error27@...il.com>, chris.mason@...cle.com,
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: locking problems: Btrfs: be more selective in the defrag ioctl

On Sat, Mar 20, 2010 at 02:38:51PM +0300, Dan Carpenter wrote:
> Hi Chris,
> 
> There is a locking problem in 
> 	940100a4a7b78 "Btrfs: be more selective in the defrag ioctl"
> 
> There are two places where we break out of the while loop under the 
> lock.
> 
> fs/btrfs/ioctl.c +708 btrfs_defrag_file(159) error: double lock 'mutex:&inode->i_mutex'
>    600                  mutex_lock(&inode->i_mutex);
>    601                  if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)
>    602                          BTRFS_I(inode)->force_compress = 1;
>    603  
>    604                  ret = btrfs_check_data_free_space(root, inode, PAGE_CACHE_SIZE);
>    605                  if (ret) {
>    606                          ret = -ENOSPC;
>    607                          break;
> 
> 	Here.
> 
>    608                  }
>    609  
>    610                  ret = btrfs_reserve_metadata_for_delalloc(root, inode, 1);
>    611                  if (ret) {
>    612                          btrfs_free_reserved_data_space(root, inode,
>    613                                                         PAGE_CACHE_SIZE);
>    614                          ret = -ENOSPC;
>    615                          break;
> 
> 	And here.
> 
>    616                  }
> 
> Maybe we should have "goto err_reservations;" instead of break?  I
> don't know the code well enough to say.

No, everything is accounted for correctly.  If the metadata reservation fails,
we free the data space reservation and break.  If the data space reservation
fails, we're good to go and can just exit.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/