[PATCH] Btrfs: protect the pending_snapshots list with trans_lock

[PATCH] Btrfs: protect the pending_snapshots list with trans_lock

[Josef Bacik]

Currently there is nothing protecting the pending_snapshots list on the
transaction.  We only hold the directory mutex that we are snapshotting and a
read lock on the subvol_sem, so we could race with somebody else creating a
snapshot in a different directory and end up with list corruption.  So protect
this list with the trans_lock.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/btrfs/ioctl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b793d11..a3c4751 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -482,8 +482,10 @@ static int create_snapshot(struct btrfs_root *root, struct dentry *dentry,
 	ret = btrfs_snap_reserve_metadata(trans, pending_snapshot);
 	BUG_ON(ret);
 
+	spin_lock(&root->fs_info->trans_lock);
 	list_add(&pending_snapshot->list,
 		 &trans->transaction->pending_snapshots);
+	spin_unlock(&root->fs_info->trans_lock);
 	if (async_transid) {
 		*async_transid = trans->transid;
 		ret = btrfs_commit_transaction_async(trans,
-- 
1.7.5.2

Re: [PATCH] Btrfs: protect the pending_snapshots list with trans_lock

[David Sterba]

Hi,

On Tue, Jun 14, 2011 at 03:17:47PM -0400, Josef Bacik wrote:
> Currently there is nothing protecting the pending_snapshots list on the
> transaction.  We only hold the directory mutex that we are snapshotting and a
> read lock on the subvol_sem, so we could race with somebody else creating a
> snapshot in a different directory and end up with list corruption.  So protect
> this list with the trans_lock.  Thanks,

it's correct that the pending_snapshots list is not protected, but I do
not see a way how to corrupt the list.

The callchain goes like this:

<user called ioctl>
  btrfs_ioctl_snap_create_transid
    btrfs_mksubvol
      create_snapshot

the interesting stuff happens inside create_snapshot:

* alloc pending snapshot struct
* start transaction (btrfs_start_transaction)
* reserve metadata
* list_add pending_snapshot to the transaction
* commit the transaction (sync or async)
* free pending snapshot struct

the snapshots are really created from btrfs_commit_transaction,
create_pending_snapshots(), which does not protect the
trans->pending_snapshots list, just traverses it. There is a potential
corruption too but ...

... there is the 'start transaction' call, which tries very hard to
start a new transaction, ie. this call returns a new transaction handle.
To prove this point needs more space, I will skip it now. The important
thing for our case is that new transaction waits for the current to
unblock.

Then, each snap create will have its own transaction and own
pending_snapshots list, and will create just one snapshot per
transaction, because any concurrently running ioctl snap create will block
in the start_transaction call.

Therefore I think that either

1) there is no locking needed around pending_snapshots (under assumption
one snapshot per transaction), or

2) you need to protect every access to the pending_snapshot:
- add in create_snapshot
- splice in btrfs_destroy_pending_snapshots
- list foreach in create_pending_snapshots
- list_empty in btrfs_commit_transaction


david

Re: [PATCH] Btrfs: protect the pending_snapshots list with trans_lock

[Chris Mason]

Excerpts from David Sterba's message of 2011-06-15 05:53:29 -0400:
> Hi,
> 
> On Tue, Jun 14, 2011 at 03:17:47PM -0400, Josef Bacik wrote:
> > Currently there is nothing protecting the pending_snapshots list on the
> > transaction.  We only hold the directory mutex that we are snapshotting and a
> > read lock on the subvol_sem, so we could race with somebody else creating a
> > snapshot in a different directory and end up with list corruption.  So protect
> > this list with the trans_lock.  Thanks,
> 
> it's correct that the pending_snapshots list is not protected, but I do
> not see a way how to corrupt the list.
> 
> The callchain goes like this:
> 
> <user called ioctl>
>   btrfs_ioctl_snap_create_transid
>     btrfs_mksubvol
>       create_snapshot
> 
> the interesting stuff happens inside create_snapshot:
> 
> * alloc pending snapshot struct
> * start transaction (btrfs_start_transaction)
> * reserve metadata
> * list_add pending_snapshot to the transaction
> * commit the transaction (sync or async)
> * free pending snapshot struct
> 
> the snapshots are really created from btrfs_commit_transaction,
> create_pending_snapshots(), which does not protect the
> trans->pending_snapshots list, just traverses it. There is a potential
> corruption too but ...
> 
> ... there is the 'start transaction' call, which tries very hard to
> start a new transaction, ie. this call returns a new transaction handle.
> To prove this point needs more space, I will skip it now. The important
> thing for our case is that new transaction waits for the current to
> unblock.
> 
> Then, each snap create will have its own transaction and own
> pending_snapshots list, and will create just one snapshot per
> transaction, because any concurrently running ioctl snap create will block
> in the start_transaction call.
> 
> Therefore I think that either
> 
> 1) there is no locking needed around pending_snapshots (under assumption
> one snapshot per transaction), or

There is definitely a window where two procs can be inside
create_snapshot() at the same time in the same transaction.  You're
correct that the window is small, but its there.  We're not racing with
the bulk of the work done at transaction commit time though.

Going down to your list:

> 
> 2) you need to protect every access to the pending_snapshot:
> - add in create_snapshot

create_snapshot is racey.

> - splice in btrfs_destroy_pending_snapshots

This can only happen during unmount while there are no writers.

> - list foreach in create_pending_snapshots

This can only happen during commit while there are no writers

> - list_empty in btrfs_commit_transaction

This is there to make sure that any delayed allocation from the source
subvol is actually done in the snapshot.  You're right that we should
have a better check here.  The worst possible case in getting this wrong
is that the snapshot misses some recent file writes in the original, but
it will be completely consistent.

-chris

Re: [PATCH] Btrfs: protect the pending_snapshots list with trans_lock

[David Sterba]

On Wed, Jun 15, 2011 at 07:41:37AM -0400, Chris Mason wrote:
> There is definitely a window where two procs can be inside
> create_snapshot() at the same time in the same transaction.

I trust you on that. I was trying to follow the callgraph from
btrfs_strat_transaction called from create_snapshot to see whether and
how two calls may access the list and came to conclusion that there must
be a fresh transaction after the call. There are lots of
join_transaction, commit_transaction calls and I could have lost track.
I'm leaving this to me as a homework, it was very educative anyway.

> Going down to your list:
> 
> > 
> > 2) you need to protect every access to the pending_snapshot:
> > - add in create_snapshot
> 
> create_snapshot is racey.

agreed, the patch fixes it

> 
> > - splice in btrfs_destroy_pending_snapshots
> 
> This can only happen during unmount while there are no writers.
>
> > - list foreach in create_pending_snapshots
> 
> This can only happen during commit while there are no writers

this counts as protection, for both

> > - list_empty in btrfs_commit_transaction
> 
> This is there to make sure that any delayed allocation from the source
> subvol is actually done in the snapshot.  You're right that we should
> have a better check here.  The worst possible case in getting this wrong
> is that the snapshot misses some recent file writes in the original, but
> it will be completely consistent.

I think this is ok and acceptable.


Thanks,
david