* [PATCH] nfsd41: free_session/free_client must be called under the client_lock
@ 2012-02-24 1:40 Benny Halevy
2012-03-06 23:11 ` J. Bruce Fields
0 siblings, 1 reply; 2+ messages in thread
From: Benny Halevy @ 2012-02-24 1:40 UTC (permalink / raw)
To: bfields; +Cc: linux-nfs
The session client is manipulated under the client_lock hence
both free_session and nfsd4_del_conns must be called under this lock.
This patch adds a BUG_ON that checks this condition in the
respective functions and implements the missing locks.
nfsd4_{get,put}_session helpers were moved to the C file that uses them
so to prevent use from external files and an unlocked version of
nfsd4_put_session is provided for external use from nfs4xdr.c
Signed-off-by: Benny Halevy <bhalevy@tonian.com>
---
fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++++++++++----
fs/nfsd/state.h | 13 +------------
2 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3876c75..cc89f0d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -93,6 +93,19 @@
static struct kmem_cache *stateid_slab = NULL;
static struct kmem_cache *deleg_slab = NULL;
+static void free_session(struct kref *);
+
+/* Must be called under the client_lock */
+static void nfsd4_put_session_locked(struct nfsd4_session *ses)
+{
+ kref_put(&ses->se_ref, free_session);
+}
+
+static void nfsd4_get_session(struct nfsd4_session *ses)
+{
+ kref_get(&ses->se_ref);
+}
+
void
__nfs4_lock_state(const char *func)
{
@@ -860,11 +873,12 @@ static void nfsd4_del_conns(struct nfsd4_session *s)
spin_unlock(&clp->cl_lock);
}
-void free_session(struct kref *kref)
+static void free_session(struct kref *kref)
{
struct nfsd4_session *ses;
int mem;
+ BUG_ON(!spin_is_locked(&client_lock));
ses = container_of(kref, struct nfsd4_session, se_ref);
nfsd4_del_conns(ses);
spin_lock(&nfsd_drc_lock);
@@ -875,6 +889,13 @@ void free_session(struct kref *kref)
kfree(ses);
}
+void nfsd4_put_session(struct nfsd4_session *ses)
+{
+ spin_lock(&client_lock);
+ nfsd4_put_session_locked(ses);
+ spin_unlock(&client_lock);
+}
+
static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct nfs4_client *clp, struct nfsd4_create_session *cses)
{
struct nfsd4_session *new;
@@ -922,7 +943,9 @@ void free_session(struct kref *kref)
status = nfsd4_new_conn_from_crses(rqstp, new);
/* whoops: benny points out, status is ignored! (err, or bogus) */
if (status) {
+ spin_lock(&client_lock);
free_session(&new->se_ref);
+ spin_unlock(&client_lock);
return NULL;
}
if (cses->flags & SESSION4_BACK_CHAN) {
@@ -1034,12 +1057,13 @@ void free_session(struct kref *kref)
static inline void
free_client(struct nfs4_client *clp)
{
+ BUG_ON(!spin_is_locked(&client_lock));
while (!list_empty(&clp->cl_sessions)) {
struct nfsd4_session *ses;
ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
se_perclnt);
list_del(&ses->se_perclnt);
- nfsd4_put_session(ses);
+ nfsd4_put_session_locked(ses);
}
if (clp->cl_cred.cr_group_info)
put_group_info(clp->cl_cred.cr_group_info);
@@ -1212,7 +1236,9 @@ struct nfs4_stid *find_stateid(struct nfs4_client *cl, stateid_t *t)
if (princ) {
clp->cl_principal = kstrdup(princ, GFP_KERNEL);
if (clp->cl_principal == NULL) {
+ spin_lock(&client_lock);
free_client(clp);
+ spin_unlock(&client_lock);
return NULL;
}
}
@@ -1877,9 +1903,10 @@ static bool nfsd4_compound_in_session(struct nfsd4_session *session, struct nfs4
nfsd4_probe_callback_sync(ses->se_client);
nfs4_unlock_state();
+ spin_lock(&client_lock);
nfsd4_del_conns(ses);
-
- nfsd4_put_session(ses);
+ nfsd4_put_session_locked(ses);
+ spin_unlock(&client_lock);
status = nfs_ok;
out:
dprintk("%s returns %d\n", __func__, ntohl(status));
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e2dd1c6..d381508 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -195,18 +195,7 @@ struct nfsd4_session {
struct nfsd4_slot *se_slots[]; /* forward channel slots */
};
-static inline void
-nfsd4_put_session(struct nfsd4_session *ses)
-{
- extern void free_session(struct kref *kref);
- kref_put(&ses->se_ref, free_session);
-}
-
-static inline void
-nfsd4_get_session(struct nfsd4_session *ses)
-{
- kref_get(&ses->se_ref);
-}
+extern void nfsd4_put_session(struct nfsd4_session *ses);
/* formatted contents of nfs4_sessionid */
struct nfsd4_sessionid {
--
1.7.6.5
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] nfsd41: free_session/free_client must be called under the client_lock
2012-02-24 1:40 [PATCH] nfsd41: free_session/free_client must be called under the client_lock Benny Halevy
@ 2012-03-06 23:11 ` J. Bruce Fields
0 siblings, 0 replies; 2+ messages in thread
From: J. Bruce Fields @ 2012-03-06 23:11 UTC (permalink / raw)
To: Benny Halevy; +Cc: bfields, linux-nfs
On Thu, Feb 23, 2012 at 05:40:52PM -0800, Benny Halevy wrote:
> The session client is manipulated under the client_lock hence
> both free_session and nfsd4_del_conns must be called under this lock.
I can't see any problem with this, applying. Thanks!
The rules for using client_lock, client_mutex, cl_lock, etc. risk
getting a little murky.
--b.
>
> This patch adds a BUG_ON that checks this condition in the
> respective functions and implements the missing locks.
>
> nfsd4_{get,put}_session helpers were moved to the C file that uses them
> so to prevent use from external files and an unlocked version of
> nfsd4_put_session is provided for external use from nfs4xdr.c
>
> Signed-off-by: Benny Halevy <bhalevy@tonian.com>
> ---
>
> fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++++++++++----
> fs/nfsd/state.h | 13 +------------
> 2 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3876c75..cc89f0d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -93,6 +93,19 @@
> static struct kmem_cache *stateid_slab = NULL;
> static struct kmem_cache *deleg_slab = NULL;
>
> +static void free_session(struct kref *);
> +
> +/* Must be called under the client_lock */
> +static void nfsd4_put_session_locked(struct nfsd4_session *ses)
> +{
> + kref_put(&ses->se_ref, free_session);
> +}
> +
> +static void nfsd4_get_session(struct nfsd4_session *ses)
> +{
> + kref_get(&ses->se_ref);
> +}
> +
> void
> __nfs4_lock_state(const char *func)
> {
> @@ -860,11 +873,12 @@ static void nfsd4_del_conns(struct nfsd4_session *s)
> spin_unlock(&clp->cl_lock);
> }
>
> -void free_session(struct kref *kref)
> +static void free_session(struct kref *kref)
> {
> struct nfsd4_session *ses;
> int mem;
>
> + BUG_ON(!spin_is_locked(&client_lock));
> ses = container_of(kref, struct nfsd4_session, se_ref);
> nfsd4_del_conns(ses);
> spin_lock(&nfsd_drc_lock);
> @@ -875,6 +889,13 @@ void free_session(struct kref *kref)
> kfree(ses);
> }
>
> +void nfsd4_put_session(struct nfsd4_session *ses)
> +{
> + spin_lock(&client_lock);
> + nfsd4_put_session_locked(ses);
> + spin_unlock(&client_lock);
> +}
> +
> static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct nfs4_client *clp, struct nfsd4_create_session *cses)
> {
> struct nfsd4_session *new;
> @@ -922,7 +943,9 @@ void free_session(struct kref *kref)
> status = nfsd4_new_conn_from_crses(rqstp, new);
> /* whoops: benny points out, status is ignored! (err, or bogus) */
> if (status) {
> + spin_lock(&client_lock);
> free_session(&new->se_ref);
> + spin_unlock(&client_lock);
> return NULL;
> }
> if (cses->flags & SESSION4_BACK_CHAN) {
> @@ -1034,12 +1057,13 @@ void free_session(struct kref *kref)
> static inline void
> free_client(struct nfs4_client *clp)
> {
> + BUG_ON(!spin_is_locked(&client_lock));
> while (!list_empty(&clp->cl_sessions)) {
> struct nfsd4_session *ses;
> ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
> se_perclnt);
> list_del(&ses->se_perclnt);
> - nfsd4_put_session(ses);
> + nfsd4_put_session_locked(ses);
> }
> if (clp->cl_cred.cr_group_info)
> put_group_info(clp->cl_cred.cr_group_info);
> @@ -1212,7 +1236,9 @@ struct nfs4_stid *find_stateid(struct nfs4_client *cl, stateid_t *t)
> if (princ) {
> clp->cl_principal = kstrdup(princ, GFP_KERNEL);
> if (clp->cl_principal == NULL) {
> + spin_lock(&client_lock);
> free_client(clp);
> + spin_unlock(&client_lock);
> return NULL;
> }
> }
> @@ -1877,9 +1903,10 @@ static bool nfsd4_compound_in_session(struct nfsd4_session *session, struct nfs4
> nfsd4_probe_callback_sync(ses->se_client);
> nfs4_unlock_state();
>
> + spin_lock(&client_lock);
> nfsd4_del_conns(ses);
> -
> - nfsd4_put_session(ses);
> + nfsd4_put_session_locked(ses);
> + spin_unlock(&client_lock);
> status = nfs_ok;
> out:
> dprintk("%s returns %d\n", __func__, ntohl(status));
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e2dd1c6..d381508 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -195,18 +195,7 @@ struct nfsd4_session {
> struct nfsd4_slot *se_slots[]; /* forward channel slots */
> };
>
> -static inline void
> -nfsd4_put_session(struct nfsd4_session *ses)
> -{
> - extern void free_session(struct kref *kref);
> - kref_put(&ses->se_ref, free_session);
> -}
> -
> -static inline void
> -nfsd4_get_session(struct nfsd4_session *ses)
> -{
> - kref_get(&ses->se_ref);
> -}
> +extern void nfsd4_put_session(struct nfsd4_session *ses);
>
> /* formatted contents of nfs4_sessionid */
> struct nfsd4_sessionid {
> --
> 1.7.6.5
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2012-03-06 23:11 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-24 1:40 [PATCH] nfsd41: free_session/free_client must be called under the client_lock Benny Halevy
2012-03-06 23:11 ` J. Bruce Fields
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).