Messages in this thread | | | Date | Wed, 30 Apr 2008 10:17:04 -0700 | From | Andrew Morton <> | Subject | Re: [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) }
| |