On Tue, 15 Jul 2008 15:09:02 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Tue, Jul 15, 2008 at 02:48:11PM -0400, J. Bruce Fields wrote:
> > From: Jeff Layton <jlayton@xxxxxxxxxx>
> >
> > nlmsvc_testlock calls nlmsvc_lookup_host to find a nlm_host struct. The
> > callers of this functions, however, call nlmsvc_retrieve_args or
> > nlm4svc_retrieve_args, which also return a nlm_host struct.
> >
> > Change nlmsvc_testlock to take a host arg instead of calling
> > nlmsvc_lookup_host itself and change the callers to pass a pointer to
> > the nlm_host they've already found.
> >
> > We take a reference to host in the place where nlmsvc_testlock()
> > previous did a new lookup, so the reference counting is unchanged from
> > before.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxxxxxx>
> > ---
> > fs/lockd/svc4proc.c | 2 +-
> > fs/lockd/svclock.c | 12 +++---------
> > fs/lockd/svcproc.c | 2 +-
> > include/linux/lockd/lockd.h | 3 ++-
> > 4 files changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > index 006a832..8cfb9da 100644
> > --- a/fs/lockd/svc4proc.c
> > +++ b/fs/lockd/svc4proc.c
> > @@ -99,7 +99,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
> > return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
> >
> > /* Now check for conflicting locks */
> > - resp->status = nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie);
> > + resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
> > if (resp->status == nlm_drop_reply)
> > rc = rpc_drop_reply;
> > else
> > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > index 81aca85..f40afb3 100644
> > --- a/fs/lockd/svclock.c
> > +++ b/fs/lockd/svclock.c
> > @@ -460,8 +460,8 @@ out:
> > */
> > __be32
> > nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> > - struct nlm_lock *lock, struct nlm_lock *conflock,
> > - struct nlm_cookie *cookie)
> > + struct nlm_host *host, struct nlm_lock *lock,
> > + struct nlm_lock *conflock, struct nlm_cookie *cookie)
> > {
> > struct nlm_block *block = NULL;
> > int error;
> > @@ -479,16 +479,10 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> >
> > if (block == NULL) {
> > struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > - struct nlm_host *host;
> >
> > if (conf == NULL)
> > return nlm_granted;
> > - /* Create host handle for callback */
> > - host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
> > - if (host == NULL) {
> > - kfree(conf);
> > - return nlm_lck_denied_nolocks;
> > - }
> > + nlm_get_host(host);
> > block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> > if (block == NULL) {
> > kfree(conf);
>
> By the way, it'd seem clearer if nlmsvc_create_block() did the job of
> taking whatever reference it needed on "host" itself.
>
That does seem like the best thing to do here...
> Seems like you could do the same for nlm_alloc_host() as well.
>
> --b.
>
> commit cc8c1b0a6514c670b1ab568241210bbdbece7923
> Author: J. Bruce Fields <bfields@xxxxxxxxxxxxxx>
> Date: Tue Jul 15 15:05:45 2008 -0400
>
> lockd: get host reference in nlmsvc_create_block() instead of callers
>
> As it is it looks like there's an obvious reference count leak until you
> track down the definition of nlm_alloc_host() and see that it's
> guaranteed to consume a reference, success or failure.
>
I'm not sure I follow this. I don't see an nlm_alloc_host(). Do you mean
nlm_alloc_call()? If so, then it looks like it should only consume a
reference on failure (when signaled).
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxxxxxx>
>
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 92c49d7..80d4f2e 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -180,6 +180,7 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
> struct nlm_block *block;
> struct nlm_rqst *call = NULL;
>
> + nlm_get_host(host);
> call = nlm_alloc_call(host);
> if (call == NULL)
> return NULL;
> @@ -380,8 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> */
> block = nlmsvc_lookup_block(file, lock);
> if (block == NULL) {
> - block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
> - lock, cookie);
> + block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> ret = nlm_lck_denied_nolocks;
> if (block == NULL)
> goto out;
> @@ -476,7 +476,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>
> if (conf == NULL)
> return nlm_granted;
> - nlm_get_host(host);
> block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> if (block == NULL) {
> kfree(conf);
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
