Re: [2.6.26 PATCH, RESEND]: fs_stack/eCryptfs: fsstack_copy_*updates

[Andrew Morton]

On Mon, 21 Apr 2008 02:50:42 -0400
Erez Zadok <ezk@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Erez Zadok <ezk@xxxxxxxxxxxxx>
>
> ...
>
>  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)
}


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