2014-02-17 12:25:57

by sougata

Subject: [PATCH] hfsplus: fix concurrent acess of alloc_blocks

Concurrent access to alloc_blocks in hfsplus_inode_info is
protected by extents_lock mutex. This patch fixes two
instances where alloc_blocks modification was not protected
with this lock. This fixes possible allocation bitmap
corruption in race conditions while extending and truncating
files.

Signed-off-by: Sougata Santra <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/hfsplus/extents.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index fbb212f..314c858 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -498,11 +498,13 @@ int hfsplus_file_extend(struct inode *inode)
goto insert_extent;
}
out:
- mutex_unlock(&hip->extents_lock);
if (!res) {
hip->alloc_blocks += len;
+ mutex_unlock(&hip->extents_lock);
hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
+ return 0;
}
+ mutex_unlock(&hip->extents_lock);
return res;

insert_extent:
@@ -592,9 +594,9 @@ void hfsplus_file_truncate(struct inode *inode)
hfs_brec_remove(&fd);
}
hfs_find_exit(&fd);
- mutex_unlock(&hip->extents_lock);

hip->alloc_blocks = blk_cnt;
+ mutex_unlock(&hip->extents_lock);
out:
hip->phys_size = inode->i_size;
hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >>
--
1.8.1.4

2014-02-18 22:06:45

by Andrew Morton

Subject: Re: [PATCH] hfsplus: fix concurrent acess of alloc_blocks

On Mon, 17 Feb 2014 14:20:47 +0200 Sougata Santra <[email protected]> wrote:

>
> Concurrent access to alloc_blocks in hfsplus_inode_info is
> protected by extents_lock mutex. This patch fixes two
> instances where alloc_blocks modification was not protected
> with this lock. This fixes possible allocation bitmap
> corruption in race conditions while extending and truncating
> files.
>
> ...
>
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -498,11 +498,13 @@ int hfsplus_file_extend(struct inode *inode)
> goto insert_extent;
> }
> out:
> - mutex_unlock(&hip->extents_lock);
> if (!res) {
> hip->alloc_blocks += len;
> + mutex_unlock(&hip->extents_lock);
> hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
> + return 0;
> }
> + mutex_unlock(&hip->extents_lock);
> return res;
>

This looks OK.

> @@ -592,9 +594,9 @@ void hfsplus_file_truncate(struct inode *inode)
> hfs_brec_remove(&fd);
> }
> hfs_find_exit(&fd);
> - mutex_unlock(&hip->extents_lock);
>
> hip->alloc_blocks = blk_cnt;
> + mutex_unlock(&hip->extents_lock);
> out:
> hip->phys_size = inode->i_size;
> hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >>

But this does not. To provide locking for
hfsplus_inode_info.alloc_blocks, we must take the lock *before* taking
a local copy of ->alloc_blocks.

Please review:

--- a/fs/hfsplus/extents.c~hfsplus-fix-concurrent-acess-of-alloc_blocks-fix
+++ a/fs/hfsplus/extents.c
@@ -556,11 +556,13 @@ void hfsplus_file_truncate(struct inode

blk_cnt = (inode->i_size + HFSPLUS_SB(sb)->alloc_blksz - 1) >>
HFSPLUS_SB(sb)->alloc_blksz_shift;
+
+ mutex_lock(&hip->extents_lock);
+
alloc_cnt = hip->alloc_blocks;
if (blk_cnt == alloc_cnt)
- goto out;
+ goto out_unlock;

- mutex_lock(&hip->extents_lock);
res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
if (res) {
mutex_unlock(&hip->extents_lock);
@@ -594,6 +596,7 @@ void hfsplus_file_truncate(struct inode
hfs_find_exit(&fd);

hip->alloc_blocks = blk_cnt;
+out_unlock:
mutex_unlock(&hip->extents_lock);
out:
hip->phys_size = inode->i_size;
_

2014-02-19 09:36:47

by sougata

Subject: Re: [PATCH] hfsplus: fix concurrent acess of alloc_blocks

On Tue, 2014-02-18 at 14:06 -0800, Andrew Morton wrote:
> On Mon, 17 Feb 2014 14:20:47 +0200 Sougata Santra <[email protected]> wrote:
>
> >
> > Concurrent access to alloc_blocks in hfsplus_inode_info is
> > protected by extents_lock mutex. This patch fixes two
> > instances where alloc_blocks modification was not protected
> > with this lock. This fixes possible allocation bitmap
> > corruption in race conditions while extending and truncating
> > files.
> >
> > ...
> >
> > --- a/fs/hfsplus/extents.c
> > +++ b/fs/hfsplus/extents.c
> > @@ -498,11 +498,13 @@ int hfsplus_file_extend(struct inode *inode)
> > goto insert_extent;
> > }
> > out:
> > - mutex_unlock(&hip->extents_lock);
> > if (!res) {
> > hip->alloc_blocks += len;
> > + mutex_unlock(&hip->extents_lock);
> > hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
> > + return 0;
> > }
> > + mutex_unlock(&hip->extents_lock);
> > return res;
> >
>
> This looks OK.
>
> > @@ -592,9 +594,9 @@ void hfsplus_file_truncate(struct inode *inode)
> > hfs_brec_remove(&fd);
> > }
> > hfs_find_exit(&fd);
> > - mutex_unlock(&hip->extents_lock);
> >
> > hip->alloc_blocks = blk_cnt;
> > + mutex_unlock(&hip->extents_lock);
> > out:
> > hip->phys_size = inode->i_size;
> > hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >>
>
> But this does not. To provide locking for
> hfsplus_inode_info.alloc_blocks, we must take the lock *before* taking
> a local copy of ->alloc_blocks.
>
> Please review:
>
> --- a/fs/hfsplus/extents.c~hfsplus-fix-concurrent-acess-of-alloc_blocks-fix
> +++ a/fs/hfsplus/extents.c
> @@ -556,11 +556,13 @@ void hfsplus_file_truncate(struct inode
>
> blk_cnt = (inode->i_size + HFSPLUS_SB(sb)->alloc_blksz - 1) >>
> HFSPLUS_SB(sb)->alloc_blksz_shift;
> +
> + mutex_lock(&hip->extents_lock);
> +
> alloc_cnt = hip->alloc_blocks;
> if (blk_cnt == alloc_cnt)
> - goto out;
> + goto out_unlock;
>
> - mutex_lock(&hip->extents_lock);
> res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
> if (res) {
> mutex_unlock(&hip->extents_lock);
> @@ -594,6 +596,7 @@ void hfsplus_file_truncate(struct inode
> hfs_find_exit(&fd);
>
> hip->alloc_blocks = blk_cnt;
> +out_unlock:
> mutex_unlock(&hip->extents_lock);
> out:
> hip->phys_size = inode->i_size;
> _
>
This is good, Missed it. Thank you.

2014-02-19 09:53:24

by sougata

Subject: Re: [PATCH] hfsplus: fix concurrent acess of alloc_blocks

On 02/19/2014 12:06 AM, Andrew Morton wrote:
> On Mon, 17 Feb 2014 14:20:47 +0200 Sougata Santra <[email protected]> wrote:
>
>>
>> Concurrent access to alloc_blocks in hfsplus_inode_info is
>> protected by extents_lock mutex. This patch fixes two
>> instances where alloc_blocks modification was not protected
>> with this lock. This fixes possible allocation bitmap
>> corruption in race conditions while extending and truncating
>> files.
>>
>> ...
>>
>> --- a/fs/hfsplus/extents.c
>> +++ b/fs/hfsplus/extents.c
>> @@ -498,11 +498,13 @@ int hfsplus_file_extend(struct inode *inode)
>> goto insert_extent;
>> }
>> out:
>> - mutex_unlock(&hip->extents_lock);
>> if (!res) {
>> hip->alloc_blocks += len;
>> + mutex_unlock(&hip->extents_lock);
>> hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
>> + return 0;
>> }
>> + mutex_unlock(&hip->extents_lock);
>> return res;
>>
>
> This looks OK.
>
>> @@ -592,9 +594,9 @@ void hfsplus_file_truncate(struct inode *inode)
>> hfs_brec_remove(&fd);
>> }
>> hfs_find_exit(&fd);
>> - mutex_unlock(&hip->extents_lock);
>>
>> hip->alloc_blocks = blk_cnt;
>> + mutex_unlock(&hip->extents_lock);
>> out:
>> hip->phys_size = inode->i_size;
>> hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >>
>
> But this does not. To provide locking for
> hfsplus_inode_info.alloc_blocks, we must take the lock *before* taking
> a local copy of ->alloc_blocks.
>
> Please review:
>
> --- a/fs/hfsplus/extents.c~hfsplus-fix-concurrent-acess-of-alloc_blocks-fix
> +++ a/fs/hfsplus/extents.c
> @@ -556,11 +556,13 @@ void hfsplus_file_truncate(struct inode
>
> blk_cnt = (inode->i_size + HFSPLUS_SB(sb)->alloc_blksz - 1) >>
> HFSPLUS_SB(sb)->alloc_blksz_shift;
> +
> + mutex_lock(&hip->extents_lock);
> +
> alloc_cnt = hip->alloc_blocks;
> if (blk_cnt == alloc_cnt)
> - goto out;
> + goto out_unlock;
>
> - mutex_lock(&hip->extents_lock);
> res = hfs_find_init(HFSPLUS_SB(sb)->ext_tree, &fd);
> if (res) {
> mutex_unlock(&hip->extents_lock);
> @@ -594,6 +596,7 @@ void hfsplus_file_truncate(struct inode
> hfs_find_exit(&fd);
>
> hip->alloc_blocks = blk_cnt;
> +out_unlock:
> mutex_unlock(&hip->extents_lock);
> out:
We can also remove this label ?
> hip->phys_size = inode->i_size;
> _
>