* [PATCH] hfsplus: fix concurrent acess of alloc_blocks
@ 2014-02-17 12:20 Sougata Santra
2014-02-18 22:06 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Sougata Santra @ 2014-02-17 12:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Christoph Hellwig, linux-fsdevel, linux-kernel,
Vyacheslav Dubeyko, Joe Perches, Alexey Khoroshilov
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 <sougata@tuxera.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hfsplus: fix concurrent acess of alloc_blocks
2014-02-17 12:20 [PATCH] hfsplus: fix concurrent acess of alloc_blocks Sougata Santra
@ 2014-02-18 22:06 ` Andrew Morton
2014-02-19 9:36 ` Sougata Santra
2014-02-19 9:53 ` sougata santra
0 siblings, 2 replies; 4+ messages in thread
From: Andrew Morton @ 2014-02-18 22:06 UTC (permalink / raw)
To: sougata
Cc: Christoph Hellwig, linux-fsdevel, linux-kernel,
Vyacheslav Dubeyko, Joe Perches, Alexey Khoroshilov
On Mon, 17 Feb 2014 14:20:47 +0200 Sougata Santra <sougata@tuxera.com> 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;
_
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hfsplus: fix concurrent acess of alloc_blocks
2014-02-18 22:06 ` Andrew Morton
@ 2014-02-19 9:36 ` Sougata Santra
2014-02-19 9:53 ` sougata santra
1 sibling, 0 replies; 4+ messages in thread
From: Sougata Santra @ 2014-02-19 9:36 UTC (permalink / raw)
To: Andrew Morton
Cc: Christoph Hellwig, linux-fsdevel, linux-kernel,
Vyacheslav Dubeyko, Joe Perches, Alexey Khoroshilov
On Tue, 2014-02-18 at 14:06 -0800, Andrew Morton wrote:
> On Mon, 17 Feb 2014 14:20:47 +0200 Sougata Santra <sougata@tuxera.com> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hfsplus: fix concurrent acess of alloc_blocks
2014-02-18 22:06 ` Andrew Morton
2014-02-19 9:36 ` Sougata Santra
@ 2014-02-19 9:53 ` sougata santra
1 sibling, 0 replies; 4+ messages in thread
From: sougata santra @ 2014-02-19 9:53 UTC (permalink / raw)
To: Andrew Morton
Cc: Christoph Hellwig, linux-fsdevel, linux-kernel,
Vyacheslav Dubeyko, Joe Perches, Alexey Khoroshilov
On 02/19/2014 12:06 AM, Andrew Morton wrote:
> On Mon, 17 Feb 2014 14:20:47 +0200 Sougata Santra <sougata@tuxera.com> 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;
> _
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-02-19 9:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-17 12:20 [PATCH] hfsplus: fix concurrent acess of alloc_blocks Sougata Santra
2014-02-18 22:06 ` Andrew Morton
2014-02-19 9:36 ` Sougata Santra
2014-02-19 9:53 ` sougata santra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.