Re: old->umask copying without spin_lock, in copy_fs_struct()

Re: old->umask copying without spin_lock, in copy_fs_struct()

[Al Viro]

On Sun, Apr 07, 2013 at 11:37:27AM +0600, Rakib Mullick wrote:
> Hello,
> 
> In copy_fs_struct(), old->umask is assigned to fs->umask outside of
> spin_lock(&old->lock). Shouldn't it be inside spin_lock()? Since we're
> dealing with  fs_struct *old ? Isn't it unsafe? Following lines -
> 
>               fs->umask = old->umask;
> 
>                 spin_lock(&old->lock);

What would moving it down buy us?  Root, pwd and umask are all modified
independently; the *only* reason why we hold old->lock for root and
pwd (and we might drop and regain it between copying those - it would
be pointless, so we don't bother, but it wouldn't have affected correctness)
is that we want the values of root.mnt and root.dentry taken at the same
time and we want to grab extra references on those while they are still
valid.  The same goes for pwd, of course.  That's what old->lock
protects - we want the damn thing atomic wrt set_fs_root() and set_fs_pwd().
umask is an integer; its updates are atomic anyway, so it's not as if we
could see a half-updated value or needed to do anything with refcounts.

Re: old->umask copying without spin_lock, in copy_fs_struct()

[Rakib Mullick]

On Sun, Apr 7, 2013 at 1:56 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Apr 07, 2013 at 11:37:27AM +0600, Rakib Mullick wrote:
>> Hello,
>>
>> In copy_fs_struct(), old->umask is assigned to fs->umask outside of
>> spin_lock(&old->lock). Shouldn't it be inside spin_lock()? Since we're
>> dealing with  fs_struct *old ? Isn't it unsafe? Following lines -
>>
>>               fs->umask = old->umask;
>>
>>                 spin_lock(&old->lock);
>
> What would moving it down buy us?  Root, pwd and umask are all modified
> independently; the *only* reason why we hold old->lock for root and
> pwd (and we might drop and regain it between copying those - it would
> be pointless, so we don't bother, but it wouldn't have affected correctness)
> is that we want the values of root.mnt and root.dentry taken at the same
> time and we want to grab extra references on those while they are still
> valid.  The same goes for pwd, of course.  That's what old->lock
> protects - we want the damn thing atomic wrt set_fs_root() and set_fs_pwd().
> umask is an integer; its updates are atomic anyway, so it's not as if we
> could see a half-updated value or needed to do anything with refcounts.

Thanks for your explanation! The ->umask operation is trivial and as
you've explained (I was also looking at the code),
it seems that code execution order makes sure that nothing goes wrong.
fs_struct's data are protected with the ->lock, that's what I was
thinking in that way and was just making sure it wasn't missed out
accidentally.

Thanks
Rakib.