lkml.org 
[lkml]   [2008]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [2.6.26 PATCH, RESEND]: fs_stack/eCryptfs: fsstack_copy_* updates
On Mon, 21 Apr 2008 02:50:42 -0400
Erez Zadok <ezk@cs.sunysb.edu> wrote:

>
> 1. remove the 3rd arg to fsstack_copy_attr_all. There are no users for it:
> ecryptfs never used the 3rd arg; unionfs stopped using it a long time
> ago. Halcrow ok'ed this patch some time ago.
>
> 2. add necessary locking for 32-bit smp systems in fsstack_copy_inode_size
> (courtesy Hugh Dickins).
>
> 3. minor commenting style changes, and addition of copyrights which were
> missing.
>
> Acked-by: Mike Halcrow <mhalcrow@us.ibm.com>
> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
>
> ...
>
> void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
> {
> - i_size_write(dst, i_size_read((struct inode *)src));
> +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> + spin_lock(&dst->i_lock);
> +#endif

The defined(CONFIG_SMP) is wrong. The spinlock is here to protect
dst->i_blocks, but it can be corrupted via preemption on uniprocessor as
well. So a plain old

#if BITS_PER_LONG == 32

would fix that.


> + i_size_write(dst, i_size_read(src));
> dst->i_blocks = src->i_blocks;
> +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> + spin_unlock(&dst->i_lock);
> +#endif
> }

However, what about src->i_blocks? It is protected by src->i_lock. The
code as you have it here could read transient values.

Furthermore, i_lock is defined as an innermost lock, for protection of
inode internals. But here we're proposing "taking" inode->i_size_seqcount
inside i_lock. Not necessarily a problem, but it broke the old rule.

We're also doing a read_seqlock of a _different_ inode inside this inode's
i_lock. Again, this is not necessarily a problem (but it might be!) but it
adds complexity and needs thought.


Can we avoid having to think?

void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
{
blkcnt_t i_blocks;
loff_t i_size;

i_size = i_size_read(src);
spin_lock_32bit(&src->i_lock);
i_blocks = src->i_blocks;
spin_unlock_32bit(&src->i_lock);

i_size_write(dst, i_size);
spin_lock_32bit(&dst->i_lock)
dst->i_blocks = i_blocks;
spin_unlock_32bit(&dst->i_lock)
}




\
 
 \ /
  Last update: 2008-04-30 19:31    [W:0.085 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site