Re: [Nfs-ganesha-devel] [ntirpc] refcount issue ?
Hi Swen, Yes, please do. Matt - "Swen Schillig" wrote: > On Mo, 2016-10-10 at 11:46 -0400, Matt Benjamin wrote: > > Hi Swen, > > > > Are you going to re-push your recent closed PR as 2 new PRs as > > planned? > > > > Dan has made some time this week to review and test, so if you have > > time, it will be easier to get to merge. > > > > Cheers, > > > > Matt > Hi Matt > > It is re-pushed , I just didn't create a new PR yet. > I was thinking we should test and review before creating a new PR. > > But if you prefer to have a PR right away, I can do that. > > Cheers Swen > > > > - Original Message - > > > > > > From: "Matt Benjamin" > > > To: "Swen Schillig" > > > Cc: "Daniel Gryniewicz" , "nfs-ganesha-devel" > > > -ganesha-de...@lists.sourceforge.net> > > > Sent: Tuesday, September 6, 2016 4:16:39 PM > > > Subject: Re: [ntirpc] refcount issue ? > > > > > > Hi, > > > > > > inline > > > > > > - Original Message - > > > > > > > > From: "Swen Schillig" > > > > To: "Matt Benjamin" , "Daniel Gryniewicz" > > > > > > > > Cc: "nfs-ganesha-devel" > > > > > Sent: Tuesday, September 6, 2016 4:10:32 PM > > > > Subject: [ntirpc] refcount issue ? > > > > > > > > Matt, Dan. > > > > > > > > Could you please have a look at the following code areas and > > > > verify > > > > what I think is a refcount issue. > > > > > > > > clnt_vc_ncreate2() > > > > { > > > > ... > > > > if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) { > > > > xd = rec->hdl.xd = alloc_x_vc_data(); > > > > ... > > > > } else { > > > > xd = rec->hdl.xd; > > > > ++(xd->refcnt); <=== this is not right. we're not > > > > taking an addtl' > > > > ref > > > > > > Aren't we? We now share a ref to previously allocated > rec->hdl.xd. > > > > > > > > > > > here. > > > > } > > > > ... > > > > clnt->cl_p1 = xd; <=== but here we should increment > > > > the > > > > refocunt. > > > > } > > > > > > > > another code section with the same handling. > > > > > > > > makefd_xprt() > > > > { > > > > ... > > > > if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) { > > > > newxd = true; > > > > xd = rec->hdl.xd = alloc_x_vc_data(); > > > > ... > > > > } else { > > > > xd = (struct x_vc_data *)rec->hdl.xd; > > > > /* dont return destroyed xprts */ > > > > if (!(xd->flags & X_VC_DATA_FLAG_SVC_DESTROYED)) { > > > > if (rec->hdl.xprt) { > > > > xprt = rec->hdl.xprt; > > > > /* inc xprt refcnt */ > > > > SVC_REF(xprt, SVC_REF_FLAG_NONE); > > > > } else > > > > ++(xd->refcnt); < not right, no > > > > addtl' ref to xd taken. > > > > > > so this looks more likely to be incorrect, need to review > > > > > > > > > > > } > > > > /* return extra ref */ > > > > rpc_dplx_unref(rec, > > > > RPC_DPLX_FLAG_LOCKED | > > > > RPC_DPLX_FLAG_UNLOCK); > > > > *allocated = FALSE; > > > > > > > > /* return ref'd xprt */ > > > > goto done_xprt; > > > > } > > > > ... > > > > xprt->xp_p1 = xd; < but here we should increment the > > > > refcount > > > > > > > > ... > > > > } > > > > > > > > Both areas handle the refcount'ing wrong, but it might balance > > > > out > > > > sometimes. > > > > > > > > > > > > What do you think ? > > > > > > > > Cheers Swen > > > > > > > > > > > > > > -- > > > Matt Benjamin > > > Red Hat, Inc. > > > 315 West Huron Street, Suite 140A > > > Ann Arbor, Michigan 48103 > > > > > > http://www.redhat.com/en/technologies/storage > > > > > > tel. 734-707-0660 > > > fax. 734-769-8938 > > > cel. 734-216-5309 > > > > > > > > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > ___ > Nfs-ganesha-devel mailing list > Nfs-ganesha-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel -- Matt Benjamin CohortFS, LLC. 315 West Huron Street, Suite 140A Ann Arbor, Michigan 48103 http://cohortfs.com tel. 734-761-4689 fax. 734-769-8938 cel. 734-216-5309 -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lis
Re: [Nfs-ganesha-devel] [ntirpc] refcount issue ?
On Mo, 2016-10-10 at 11:46 -0400, Matt Benjamin wrote: > Hi Swen, > > Are you going to re-push your recent closed PR as 2 new PRs as > planned? > > Dan has made some time this week to review and test, so if you have > time, it will be easier to get to merge. > > Cheers, > > Matt Hi Matt It is re-pushed , I just didn't create a new PR yet. I was thinking we should test and review before creating a new PR. But if you prefer to have a PR right away, I can do that. Cheers Swen > > - Original Message - > > > > From: "Matt Benjamin" > > To: "Swen Schillig" > > Cc: "Daniel Gryniewicz" , "nfs-ganesha-devel" > -ganesha-de...@lists.sourceforge.net> > > Sent: Tuesday, September 6, 2016 4:16:39 PM > > Subject: Re: [ntirpc] refcount issue ? > > > > Hi, > > > > inline > > > > - Original Message - > > > > > > From: "Swen Schillig" > > > To: "Matt Benjamin" , "Daniel Gryniewicz" > > > > > > Cc: "nfs-ganesha-devel" > > > Sent: Tuesday, September 6, 2016 4:10:32 PM > > > Subject: [ntirpc] refcount issue ? > > > > > > Matt, Dan. > > > > > > Could you please have a look at the following code areas and > > > verify > > > what I think is a refcount issue. > > > > > > clnt_vc_ncreate2() > > > { > > > ... > > > if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) { > > > xd = rec->hdl.xd = alloc_x_vc_data(); > > > ... > > > } else { > > > xd = rec->hdl.xd; > > > ++(xd->refcnt); <=== this is not right. we're not > > > taking an addtl' > > > ref > > > > Aren't we? We now share a ref to previously allocated rec->hdl.xd. > > > > > > > > here. > > > } > > > ... > > > clnt->cl_p1 = xd; <=== but here we should increment > > > the > > > refocunt. > > > } > > > > > > another code section with the same handling. > > > > > > makefd_xprt() > > > { > > > ... > > > if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) { > > > newxd = true; > > > xd = rec->hdl.xd = alloc_x_vc_data(); > > > ... > > > } else { > > > xd = (struct x_vc_data *)rec->hdl.xd; > > > /* dont return destroyed xprts */ > > > if (!(xd->flags & X_VC_DATA_FLAG_SVC_DESTROYED)) { > > > if (rec->hdl.xprt) { > > > xprt = rec->hdl.xprt; > > > /* inc xprt refcnt */ > > > SVC_REF(xprt, SVC_REF_FLAG_NONE); > > > } else > > > ++(xd->refcnt); < not right, no > > > addtl' ref to xd taken. > > > > so this looks more likely to be incorrect, need to review > > > > > > > > } > > > /* return extra ref */ > > > rpc_dplx_unref(rec, > > > RPC_DPLX_FLAG_LOCKED | > > > RPC_DPLX_FLAG_UNLOCK); > > > *allocated = FALSE; > > > > > > /* return ref'd xprt */ > > > goto done_xprt; > > > } > > > ... > > > xprt->xp_p1 = xd; < but here we should increment the > > > refcount > > > > > > ... > > > } > > > > > > Both areas handle the refcount'ing wrong, but it might balance > > > out > > > sometimes. > > > > > > > > > What do you think ? > > > > > > Cheers Swen > > > > > > > > > > -- > > Matt Benjamin > > Red Hat, Inc. > > 315 West Huron Street, Suite 140A > > Ann Arbor, Michigan 48103 > > > > http://www.redhat.com/en/technologies/storage > > > > tel. 734-707-0660 > > fax. 734-769-8938 > > cel. 734-216-5309 > > > -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
Re: [Nfs-ganesha-devel] [PATCH] Fix refcount leak in svc_rpc_gss_data under RPCSEC_GSS_DESTROY
I see it already merged, thank you Matt. On Mon, Oct 10, 2016 at 9:40 PM, Malahal Naineni wrote: > I wanted people to review first. My tests did pass with this fix. If > no review comments, then yes, I will do a pull request. > > Regards, Malahal > > On Mon, Oct 10, 2016 at 7:40 PM, Matt W. Benjamin wrote: >> Oh, it is. >> >> Matt >> >> - "Matt W. Benjamin" wrote: >> >>> Hi Malahal, >>> >>> Is this intended to be a PR? >>> >>> Matt >>> >>> - "Malahal Naineni" wrote: >>> >>> > A umount/mount loop for a krb5 mount was sending RPCSEC_GSS_DESTROY >>> > calls at times. We end up a huge number of contexts that are >>> expired >>> > but >>> > not destroyed, eventually failing mounts after some time. >>> > >>> > Release reference we got on GSS data object after deleting it from >>> > hash >>> > table. >>> > >>> > Signed-off-by: Malahal Naineni >>> > --- >>> > src/authgss_hash.c | 2 +- >>> > src/svc_auth_gss.c | 7 ++- >>> > 2 files changed, 7 insertions(+), 2 deletions(-) >>> > >>> > diff --git a/src/authgss_hash.c b/src/authgss_hash.c >>> > index d88a378..21ecaf9 100644 >>> > --- a/src/authgss_hash.c >>> > +++ b/src/authgss_hash.c >>> > @@ -187,7 +187,7 @@ authgss_ctx_hash_set(struct svc_rpc_gss_data >>> *gd) >>> > gss_ctx = (gss_union_ctx_id_desc *) (gd->ctx); >>> > gd->hk.k = gss_ctx_hash(gss_ctx); >>> > >>> > - ++(gd->refcnt); /* locked */ >>> > + (void)atomic_inc_uint32_t(&gd->refcnt); >>> > t = rbtx_partition_of_scalar(&authgss_hash_st.xt, gd->hk.k); >>> > mutex_lock(&t->mtx); >>> > rslt = >>> > diff --git a/src/svc_auth_gss.c b/src/svc_auth_gss.c >>> > index 3322ab5..cc7b1f4 100644 >>> > --- a/src/svc_auth_gss.c >>> > +++ b/src/svc_auth_gss.c >>> > @@ -612,7 +612,7 @@ _svcauth_gss(struct svc_req *req, struct >>> rpc_msg >>> > *msg, >>> > >>> > *no_dispatch = true; >>> > >>> > - (void)authgss_ctx_hash_del(gd); /* unrefs, can destroy gd */ >>> > + (void)authgss_ctx_hash_del(gd); >>> > >>> > /* avoid lock order reversal gd->lock, xprt->xp_lock */ >>> > mutex_unlock(&gd->lock); >>> > @@ -622,6 +622,11 @@ _svcauth_gss(struct svc_req *req, struct >>> rpc_msg >>> > *msg, >>> > svc_sendreply(req->rq_xprt, req, (xdrproc_t) xdr_void, >>> > (caddr_t) NULL); >>> > >>> > + /* We acquired a reference on gd with authgss_ctx_hash_get >>> > +* call. Time to release the reference as we don't need >>> > +* gd anymore. >>> > +*/ >>> > + unref_svc_rpc_gss_data(gd, SVC_RPC_GSS_FLAG_NONE); >>> > req->rq_auth = &svc_auth_none; >>> > >>> > break; >>> > -- >>> > 1.8.3.1 >>> > >>> > >>> > >>> -- >>> > Check out the vibrant tech community on one of the world's most >>> > engaging tech sites, SlashDot.org! http://sdm.link/slashdot >>> > ___ >>> > Nfs-ganesha-devel mailing list >>> > Nfs-ganesha-devel@lists.sourceforge.net >>> > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel >>> >>> -- >>> Matt Benjamin >>> CohortFS, LLC. >>> 315 West Huron Street, Suite 140A >>> Ann Arbor, Michigan 48103 >>> >>> http://cohortfs.com >>> >>> tel. 734-761-4689 >>> fax. 734-769-8938 >>> cel. 734-216-5309 >>> >>> -- >>> Check out the vibrant tech community on one of the world's most >>> engaging tech sites, SlashDot.org! http://sdm.link/slashdot >>> ___ >>> Nfs-ganesha-devel mailing list >>> Nfs-ganesha-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel >> >> -- >> Matt Benjamin >> CohortFS, LLC. >> 315 West Huron Street, Suite 140A >> Ann Arbor, Michigan 48103 >> >> http://cohortfs.com >> >> tel. 734-761-4689 >> fax. 734-769-8938 >> cel. 734-216-5309 >> >> -- >> Check out the vibrant tech community on one of the world's most >> engaging tech sites, SlashDot.org! http://sdm.link/slashdot >> ___ >> Nfs-ganesha-devel mailing list >> Nfs-ganesha-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
Re: [Nfs-ganesha-devel] [PATCH] Fix refcount leak in svc_rpc_gss_data under RPCSEC_GSS_DESTROY
I wanted people to review first. My tests did pass with this fix. If no review comments, then yes, I will do a pull request. Regards, Malahal On Mon, Oct 10, 2016 at 7:40 PM, Matt W. Benjamin wrote: > Oh, it is. > > Matt > > - "Matt W. Benjamin" wrote: > >> Hi Malahal, >> >> Is this intended to be a PR? >> >> Matt >> >> - "Malahal Naineni" wrote: >> >> > A umount/mount loop for a krb5 mount was sending RPCSEC_GSS_DESTROY >> > calls at times. We end up a huge number of contexts that are >> expired >> > but >> > not destroyed, eventually failing mounts after some time. >> > >> > Release reference we got on GSS data object after deleting it from >> > hash >> > table. >> > >> > Signed-off-by: Malahal Naineni >> > --- >> > src/authgss_hash.c | 2 +- >> > src/svc_auth_gss.c | 7 ++- >> > 2 files changed, 7 insertions(+), 2 deletions(-) >> > >> > diff --git a/src/authgss_hash.c b/src/authgss_hash.c >> > index d88a378..21ecaf9 100644 >> > --- a/src/authgss_hash.c >> > +++ b/src/authgss_hash.c >> > @@ -187,7 +187,7 @@ authgss_ctx_hash_set(struct svc_rpc_gss_data >> *gd) >> > gss_ctx = (gss_union_ctx_id_desc *) (gd->ctx); >> > gd->hk.k = gss_ctx_hash(gss_ctx); >> > >> > - ++(gd->refcnt); /* locked */ >> > + (void)atomic_inc_uint32_t(&gd->refcnt); >> > t = rbtx_partition_of_scalar(&authgss_hash_st.xt, gd->hk.k); >> > mutex_lock(&t->mtx); >> > rslt = >> > diff --git a/src/svc_auth_gss.c b/src/svc_auth_gss.c >> > index 3322ab5..cc7b1f4 100644 >> > --- a/src/svc_auth_gss.c >> > +++ b/src/svc_auth_gss.c >> > @@ -612,7 +612,7 @@ _svcauth_gss(struct svc_req *req, struct >> rpc_msg >> > *msg, >> > >> > *no_dispatch = true; >> > >> > - (void)authgss_ctx_hash_del(gd); /* unrefs, can destroy gd */ >> > + (void)authgss_ctx_hash_del(gd); >> > >> > /* avoid lock order reversal gd->lock, xprt->xp_lock */ >> > mutex_unlock(&gd->lock); >> > @@ -622,6 +622,11 @@ _svcauth_gss(struct svc_req *req, struct >> rpc_msg >> > *msg, >> > svc_sendreply(req->rq_xprt, req, (xdrproc_t) xdr_void, >> > (caddr_t) NULL); >> > >> > + /* We acquired a reference on gd with authgss_ctx_hash_get >> > +* call. Time to release the reference as we don't need >> > +* gd anymore. >> > +*/ >> > + unref_svc_rpc_gss_data(gd, SVC_RPC_GSS_FLAG_NONE); >> > req->rq_auth = &svc_auth_none; >> > >> > break; >> > -- >> > 1.8.3.1 >> > >> > >> > >> -- >> > Check out the vibrant tech community on one of the world's most >> > engaging tech sites, SlashDot.org! http://sdm.link/slashdot >> > ___ >> > Nfs-ganesha-devel mailing list >> > Nfs-ganesha-devel@lists.sourceforge.net >> > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel >> >> -- >> Matt Benjamin >> CohortFS, LLC. >> 315 West Huron Street, Suite 140A >> Ann Arbor, Michigan 48103 >> >> http://cohortfs.com >> >> tel. 734-761-4689 >> fax. 734-769-8938 >> cel. 734-216-5309 >> >> -- >> Check out the vibrant tech community on one of the world's most >> engaging tech sites, SlashDot.org! http://sdm.link/slashdot >> ___ >> Nfs-ganesha-devel mailing list >> Nfs-ganesha-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel > > -- > Matt Benjamin > CohortFS, LLC. > 315 West Huron Street, Suite 140A > Ann Arbor, Michigan 48103 > > http://cohortfs.com > > tel. 734-761-4689 > fax. 734-769-8938 > cel. 734-216-5309 > > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > ___ > Nfs-ganesha-devel mailing list > Nfs-ganesha-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
Re: [Nfs-ganesha-devel] [ntirpc] refcount issue ?
Hi Swen, Are you going to re-push your recent closed PR as 2 new PRs as planned? Dan has made some time this week to review and test, so if you have time, it will be easier to get to merge. Cheers, Matt - Original Message - > From: "Matt Benjamin" > To: "Swen Schillig" > Cc: "Daniel Gryniewicz" , "nfs-ganesha-devel" > > Sent: Tuesday, September 6, 2016 4:16:39 PM > Subject: Re: [ntirpc] refcount issue ? > > Hi, > > inline > > - Original Message - > > From: "Swen Schillig" > > To: "Matt Benjamin" , "Daniel Gryniewicz" > > > > Cc: "nfs-ganesha-devel" > > Sent: Tuesday, September 6, 2016 4:10:32 PM > > Subject: [ntirpc] refcount issue ? > > > > Matt, Dan. > > > > Could you please have a look at the following code areas and verify > > what I think is a refcount issue. > > > > clnt_vc_ncreate2() > > { > > ... > > if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) { > > xd = rec->hdl.xd = alloc_x_vc_data(); > > ... > > } else { > > xd = rec->hdl.xd; > > ++(xd->refcnt); <=== this is not right. we're not taking an > > addtl' > > ref > > Aren't we? We now share a ref to previously allocated rec->hdl.xd. > > > here. > > } > > ... > > clnt->cl_p1 = xd; <=== but here we should increment the > > refocunt. > > } > > > > another code section with the same handling. > > > > makefd_xprt() > > { > > ... > > if ((oflags & RPC_DPLX_LKP_OFLAG_ALLOC) || (!rec->hdl.xd)) { > > newxd = true; > > xd = rec->hdl.xd = alloc_x_vc_data(); > > ... > > } else { > > xd = (struct x_vc_data *)rec->hdl.xd; > > /* dont return destroyed xprts */ > > if (!(xd->flags & X_VC_DATA_FLAG_SVC_DESTROYED)) { > > if (rec->hdl.xprt) { > > xprt = rec->hdl.xprt; > > /* inc xprt refcnt */ > > SVC_REF(xprt, SVC_REF_FLAG_NONE); > > } else > > ++(xd->refcnt); < not right, no addtl' > > ref to xd taken. > > so this looks more likely to be incorrect, need to review > > > } > > /* return extra ref */ > > rpc_dplx_unref(rec, > > RPC_DPLX_FLAG_LOCKED | RPC_DPLX_FLAG_UNLOCK); > > *allocated = FALSE; > > > > /* return ref'd xprt */ > > goto done_xprt; > > } > > ... > > xprt->xp_p1 = xd; < but here we should increment the refcount > > > > ... > > } > > > > Both areas handle the refcount'ing wrong, but it might balance out > > sometimes. > > > > > > What do you think ? > > > > Cheers Swen > > > > > > -- > Matt Benjamin > Red Hat, Inc. > 315 West Huron Street, Suite 140A > Ann Arbor, Michigan 48103 > > http://www.redhat.com/en/technologies/storage > > tel. 734-707-0660 > fax. 734-769-8938 > cel. 734-216-5309 > -- Matt Benjamin Red Hat, Inc. 315 West Huron Street, Suite 140A Ann Arbor, Michigan 48103 http://www.redhat.com/en/technologies/storage tel. 734-707-0660 fax. 734-769-8938 cel. 734-216-5309 -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
[Nfs-ganesha-devel] Change in ffilz/nfs-ganesha[next]: Fix display_lock_cookie_key to pass right args to display_lo...
>From Malahal : Malahal has uploaded a new change for review. https://review.gerrithub.io/297744 Change subject: Fix display_lock_cookie_key to pass right args to display_lock_cookie .. Fix display_lock_cookie_key to pass right args to display_lock_cookie Change-Id: I0dc666949a32a48c43827fea298676239f8a569f Signed-off-by: Malahal Naineni --- M src/SAL/state_lock.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.gerrithub.io:29418/ffilz/nfs-ganesha refs/changes/44/297744/1 -- To view, visit https://review.gerrithub.io/297744 To unsubscribe, visit https://review.gerrithub.io/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I0dc666949a32a48c43827fea298676239f8a569f Gerrit-PatchSet: 1 Gerrit-Project: ffilz/nfs-ganesha Gerrit-Branch: next Gerrit-Owner: Malahal -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
Re: [Nfs-ganesha-devel] [PATCH] Fix refcount leak in svc_rpc_gss_data under RPCSEC_GSS_DESTROY
Hi Malahal, Is this intended to be a PR? Matt - "Malahal Naineni" wrote: > A umount/mount loop for a krb5 mount was sending RPCSEC_GSS_DESTROY > calls at times. We end up a huge number of contexts that are expired > but > not destroyed, eventually failing mounts after some time. > > Release reference we got on GSS data object after deleting it from > hash > table. > > Signed-off-by: Malahal Naineni > --- > src/authgss_hash.c | 2 +- > src/svc_auth_gss.c | 7 ++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/src/authgss_hash.c b/src/authgss_hash.c > index d88a378..21ecaf9 100644 > --- a/src/authgss_hash.c > +++ b/src/authgss_hash.c > @@ -187,7 +187,7 @@ authgss_ctx_hash_set(struct svc_rpc_gss_data *gd) > gss_ctx = (gss_union_ctx_id_desc *) (gd->ctx); > gd->hk.k = gss_ctx_hash(gss_ctx); > > - ++(gd->refcnt); /* locked */ > + (void)atomic_inc_uint32_t(&gd->refcnt); > t = rbtx_partition_of_scalar(&authgss_hash_st.xt, gd->hk.k); > mutex_lock(&t->mtx); > rslt = > diff --git a/src/svc_auth_gss.c b/src/svc_auth_gss.c > index 3322ab5..cc7b1f4 100644 > --- a/src/svc_auth_gss.c > +++ b/src/svc_auth_gss.c > @@ -612,7 +612,7 @@ _svcauth_gss(struct svc_req *req, struct rpc_msg > *msg, > > *no_dispatch = true; > > - (void)authgss_ctx_hash_del(gd); /* unrefs, can destroy gd */ > + (void)authgss_ctx_hash_del(gd); > > /* avoid lock order reversal gd->lock, xprt->xp_lock */ > mutex_unlock(&gd->lock); > @@ -622,6 +622,11 @@ _svcauth_gss(struct svc_req *req, struct rpc_msg > *msg, > svc_sendreply(req->rq_xprt, req, (xdrproc_t) xdr_void, > (caddr_t) NULL); > > + /* We acquired a reference on gd with authgss_ctx_hash_get > + * call. Time to release the reference as we don't need > + * gd anymore. > + */ > + unref_svc_rpc_gss_data(gd, SVC_RPC_GSS_FLAG_NONE); > req->rq_auth = &svc_auth_none; > > break; > -- > 1.8.3.1 > > > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > ___ > Nfs-ganesha-devel mailing list > Nfs-ganesha-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel -- Matt Benjamin CohortFS, LLC. 315 West Huron Street, Suite 140A Ann Arbor, Michigan 48103 http://cohortfs.com tel. 734-761-4689 fax. 734-769-8938 cel. 734-216-5309 -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
Re: [Nfs-ganesha-devel] [PATCH] Fix refcount leak in svc_rpc_gss_data under RPCSEC_GSS_DESTROY
Oh, it is. Matt - "Matt W. Benjamin" wrote: > Hi Malahal, > > Is this intended to be a PR? > > Matt > > - "Malahal Naineni" wrote: > > > A umount/mount loop for a krb5 mount was sending RPCSEC_GSS_DESTROY > > calls at times. We end up a huge number of contexts that are > expired > > but > > not destroyed, eventually failing mounts after some time. > > > > Release reference we got on GSS data object after deleting it from > > hash > > table. > > > > Signed-off-by: Malahal Naineni > > --- > > src/authgss_hash.c | 2 +- > > src/svc_auth_gss.c | 7 ++- > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/src/authgss_hash.c b/src/authgss_hash.c > > index d88a378..21ecaf9 100644 > > --- a/src/authgss_hash.c > > +++ b/src/authgss_hash.c > > @@ -187,7 +187,7 @@ authgss_ctx_hash_set(struct svc_rpc_gss_data > *gd) > > gss_ctx = (gss_union_ctx_id_desc *) (gd->ctx); > > gd->hk.k = gss_ctx_hash(gss_ctx); > > > > - ++(gd->refcnt); /* locked */ > > + (void)atomic_inc_uint32_t(&gd->refcnt); > > t = rbtx_partition_of_scalar(&authgss_hash_st.xt, gd->hk.k); > > mutex_lock(&t->mtx); > > rslt = > > diff --git a/src/svc_auth_gss.c b/src/svc_auth_gss.c > > index 3322ab5..cc7b1f4 100644 > > --- a/src/svc_auth_gss.c > > +++ b/src/svc_auth_gss.c > > @@ -612,7 +612,7 @@ _svcauth_gss(struct svc_req *req, struct > rpc_msg > > *msg, > > > > *no_dispatch = true; > > > > - (void)authgss_ctx_hash_del(gd); /* unrefs, can destroy gd */ > > + (void)authgss_ctx_hash_del(gd); > > > > /* avoid lock order reversal gd->lock, xprt->xp_lock */ > > mutex_unlock(&gd->lock); > > @@ -622,6 +622,11 @@ _svcauth_gss(struct svc_req *req, struct > rpc_msg > > *msg, > > svc_sendreply(req->rq_xprt, req, (xdrproc_t) xdr_void, > > (caddr_t) NULL); > > > > + /* We acquired a reference on gd with authgss_ctx_hash_get > > +* call. Time to release the reference as we don't need > > +* gd anymore. > > +*/ > > + unref_svc_rpc_gss_data(gd, SVC_RPC_GSS_FLAG_NONE); > > req->rq_auth = &svc_auth_none; > > > > break; > > -- > > 1.8.3.1 > > > > > > > -- > > Check out the vibrant tech community on one of the world's most > > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > > ___ > > Nfs-ganesha-devel mailing list > > Nfs-ganesha-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel > > -- > Matt Benjamin > CohortFS, LLC. > 315 West Huron Street, Suite 140A > Ann Arbor, Michigan 48103 > > http://cohortfs.com > > tel. 734-761-4689 > fax. 734-769-8938 > cel. 734-216-5309 > > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > ___ > Nfs-ganesha-devel mailing list > Nfs-ganesha-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel -- Matt Benjamin CohortFS, LLC. 315 West Huron Street, Suite 140A Ann Arbor, Michigan 48103 http://cohortfs.com tel. 734-761-4689 fax. 734-769-8938 cel. 734-216-5309 -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
[Nfs-ganesha-devel] [PATCH] Fix refcount leak in svc_rpc_gss_data under RPCSEC_GSS_DESTROY
A umount/mount loop for a krb5 mount was sending RPCSEC_GSS_DESTROY calls at times. We end up a huge number of contexts that are expired but not destroyed, eventually failing mounts after some time. Release reference we got on GSS data object after deleting it from hash table. Signed-off-by: Malahal Naineni --- src/authgss_hash.c | 2 +- src/svc_auth_gss.c | 7 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/authgss_hash.c b/src/authgss_hash.c index d88a378..21ecaf9 100644 --- a/src/authgss_hash.c +++ b/src/authgss_hash.c @@ -187,7 +187,7 @@ authgss_ctx_hash_set(struct svc_rpc_gss_data *gd) gss_ctx = (gss_union_ctx_id_desc *) (gd->ctx); gd->hk.k = gss_ctx_hash(gss_ctx); - ++(gd->refcnt); /* locked */ + (void)atomic_inc_uint32_t(&gd->refcnt); t = rbtx_partition_of_scalar(&authgss_hash_st.xt, gd->hk.k); mutex_lock(&t->mtx); rslt = diff --git a/src/svc_auth_gss.c b/src/svc_auth_gss.c index 3322ab5..cc7b1f4 100644 --- a/src/svc_auth_gss.c +++ b/src/svc_auth_gss.c @@ -612,7 +612,7 @@ _svcauth_gss(struct svc_req *req, struct rpc_msg *msg, *no_dispatch = true; - (void)authgss_ctx_hash_del(gd); /* unrefs, can destroy gd */ + (void)authgss_ctx_hash_del(gd); /* avoid lock order reversal gd->lock, xprt->xp_lock */ mutex_unlock(&gd->lock); @@ -622,6 +622,11 @@ _svcauth_gss(struct svc_req *req, struct rpc_msg *msg, svc_sendreply(req->rq_xprt, req, (xdrproc_t) xdr_void, (caddr_t) NULL); + /* We acquired a reference on gd with authgss_ctx_hash_get +* call. Time to release the reference as we don't need +* gd anymore. +*/ + unref_svc_rpc_gss_data(gd, SVC_RPC_GSS_FLAG_NONE); req->rq_auth = &svc_auth_none; break; -- 1.8.3.1 -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel