All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Layton <jeff.layton@primarydata.com>
To: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, linux-nfs@vger.kernel.org,
	trond.myklebust@primarydata.com
Subject: Re: [PATCH 0/5] locks: move most locks_release_private calls outside of i_lock
Date: Tue, 12 Aug 2014 13:43:13 -0400	[thread overview]
Message-ID: <20140812134313.1273dc63@tlielax.poochiereds.net> (raw)
In-Reply-To: <1407854893-2698-1-git-send-email-jlayton@primarydata.com>

On Tue, 12 Aug 2014 10:48:08 -0400
Jeff Layton <jlayton@primarydata.com> wrote:

> In the days of yore, the file locking code was primarily protected by
> the BKL. That changed in commit 72f98e72551fa (locks: turn lock_flocks
> into a spinlock), at which point the code was changed to be protected
> by a conventional spinlock (mostly due to a push to finally eliminate
> the BKL). Since then, the code has been changed to use the i_lock
> instead of a global spinlock, but it's still under a spinlock.
> 
> With that change, several functions now no longer can block when they
> originally could. This is a particular problem with the
> fl_release_private operation. In NFSv4, that operation is used to kick
> off a RELEASE_LOCKOWNER or FREE_STATEID call, and that requires being
> able to do an allocation.
> 
> This was reported by Josh Stone here:
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=1089092
> 
> My initial stab at fixing this involved moving this to a workqueue, but
> Trond pointed out the above change was technically a regression with the
> way the spinlocking in the file locking code works, and suggested an
> alternate approach to fixing it.
> 
> This set focuses on moving most of the locks_release_private calls
> outside of the inode->i_lock. There are still a few that are done
> under the i_lock in the lease handling code. Cleaning up the use of
> the i_lock in the lease code is a larger project which we'll have to
> tackle at some point, but there are some other cleanups that will
> need to happen first.
> 
> Absent any objections, I'll plan to merge these for 3.18.
> 

Erm...make that v3.17...

As Trond points out, the fact that we end up doing sleeping allocations
under spinlock can allow an unprivileged user to crash a NFSv4 client.
So I may see about merging these sooner rather than later after they've
had a little soak time in linux-next.

-- 
Jeff Layton <jlayton@primarydata.com>

WARNING: multiple messages have this Message-ID (diff)
From: Jeff Layton <jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
To: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org
Subject: Re: [PATCH 0/5] locks: move most locks_release_private calls outside of i_lock
Date: Tue, 12 Aug 2014 13:43:13 -0400	[thread overview]
Message-ID: <20140812134313.1273dc63@tlielax.poochiereds.net> (raw)
In-Reply-To: <1407854893-2698-1-git-send-email-jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>

On Tue, 12 Aug 2014 10:48:08 -0400
Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org> wrote:

> In the days of yore, the file locking code was primarily protected by
> the BKL. That changed in commit 72f98e72551fa (locks: turn lock_flocks
> into a spinlock), at which point the code was changed to be protected
> by a conventional spinlock (mostly due to a push to finally eliminate
> the BKL). Since then, the code has been changed to use the i_lock
> instead of a global spinlock, but it's still under a spinlock.
> 
> With that change, several functions now no longer can block when they
> originally could. This is a particular problem with the
> fl_release_private operation. In NFSv4, that operation is used to kick
> off a RELEASE_LOCKOWNER or FREE_STATEID call, and that requires being
> able to do an allocation.
> 
> This was reported by Josh Stone here:
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=1089092
> 
> My initial stab at fixing this involved moving this to a workqueue, but
> Trond pointed out the above change was technically a regression with the
> way the spinlocking in the file locking code works, and suggested an
> alternate approach to fixing it.
> 
> This set focuses on moving most of the locks_release_private calls
> outside of the inode->i_lock. There are still a few that are done
> under the i_lock in the lease handling code. Cleaning up the use of
> the i_lock in the lease code is a larger project which we'll have to
> tackle at some point, but there are some other cleanups that will
> need to happen first.
> 
> Absent any objections, I'll plan to merge these for 3.18.
> 

Erm...make that v3.17...

As Trond points out, the fact that we end up doing sleeping allocations
under spinlock can allow an unprivileged user to crash a NFSv4 client.
So I may see about merging these sooner rather than later after they've
had a little soak time in linux-next.

-- 
Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  parent reply	other threads:[~2014-08-12 17:43 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-12 14:48 [PATCH 0/5] locks: move most locks_release_private calls outside of i_lock Jeff Layton
2014-08-12 14:48 ` [PATCH 1/5] locks: don't call locks_release_private from locks_copy_lock Jeff Layton
2014-08-12 14:48 ` [PATCH 2/5] locks: don't reuse file_lock in __posix_lock_file Jeff Layton
2014-08-12 14:48 ` [PATCH 3/5] locks: defer freeing locks in locks_delete_lock until after i_lock has been dropped Jeff Layton
2014-08-12 14:48 ` [PATCH 4/5] locks: move locks_free_lock calls in do_fcntl_add_lease outside spinlock Jeff Layton
2014-08-12 14:48 ` [PATCH 5/5] locks: update Locking documentation to clarify fl_release_private behavior Jeff Layton
2014-08-12 15:32 ` [PATCH 0/5] locks: move most locks_release_private calls outside of i_lock Christoph Hellwig
2014-08-12 16:21   ` Jeff Layton
2014-08-12 17:43 ` Jeff Layton [this message]
2014-08-12 17:43   ` Jeff Layton
2014-08-12 17:53   ` J. Bruce Fields
2014-08-12 17:53     ` J. Bruce Fields
2014-08-12 22:28 ` Stephen Rothwell
2014-08-12 23:47   ` Jeff Layton
2014-08-12 23:47     ` Jeff Layton
2014-08-13  2:09     ` Stephen Rothwell
2014-08-13 15:51   ` J. Bruce Fields

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140812134313.1273dc63@tlielax.poochiereds.net \
    --to=jeff.layton@primarydata.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trond.myklebust@primarydata.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.