linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: old->umask copying without spin_lock, in copy_fs_struct()
       [not found] <CADZ9YHiQBips5DB=xo0Kh-Nx_s60K+W+5RK-kCrHMfy6AGkhyA@mail.gmail.com>
@ 2013-04-07  7:56 ` Al Viro
  2013-04-07  8:49   ` Rakib Mullick
  0 siblings, 1 reply; 2+ messages in thread
From: Al Viro @ 2013-04-07  7:56 UTC (permalink / raw)
  To: Rakib Mullick; +Cc: LKML

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.

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: old->umask copying without spin_lock, in copy_fs_struct()
  2013-04-07  7:56 ` old->umask copying without spin_lock, in copy_fs_struct() Al Viro
@ 2013-04-07  8:49   ` Rakib Mullick
  0 siblings, 0 replies; 2+ messages in thread
From: Rakib Mullick @ 2013-04-07  8:49 UTC (permalink / raw)
  To: Al Viro; +Cc: LKML

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.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2013-04-07  8:49 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CADZ9YHiQBips5DB=xo0Kh-Nx_s60K+W+5RK-kCrHMfy6AGkhyA@mail.gmail.com>
2013-04-07  7:56 ` old->umask copying without spin_lock, in copy_fs_struct() Al Viro
2013-04-07  8:49   ` Rakib Mullick

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).