[Gluster-devel] crypt xlator bug

[Niels de Vos]

On Thu, Apr 02, 2015 at 01:58:39PM +0530, Raghavendra Bhat wrote:
> On Thursday 02 April 2015 01:00 PM, Pranith Kumar Karampuri wrote:
> >
> >On 04/02/2015 12:27 AM, Raghavendra Talur wrote:
> >>
> >>
> >>On Wed, Apr 1, 2015 at 10:34 PM, Justin Clift <justin at gluster.org
> >><mailto:justin at gluster.org>> wrote:
> >>
> >>    On 1 Apr 2015, at 10:57, Emmanuel Dreyfus <manu at netbsd.org
> >>    <mailto:manu at netbsd.org>> wrote:
> >>    > Hi
> >>    >
> >>    > crypt.t was recently broken in NetBSD regression. The glusterfs
> >>    returns
> >>    > a node with file type invalid to FUSE, and that breaks the test.
> >>    >
> >>    > After running a git bisect, I found the offending commit after
> >>    which
> >>    > this behavior appeared:
> >>    >    8a2e2b88fc21dc7879f838d18cd0413dd88023b7
> >>    >    mem-pool: invalidate memory on GF_FREE to aid debugging
> >>    >
> >>    > This means the bug has always been there, but this debugging aid
> >>    > caused it to be reliable.
> >>
> >>    Sounds like that commit is a good win then. :)
> >>
> >>    Harsha/Pranith/Lala, your names are on the git blame for crypt.c...
> >>    any ideas? :)
> >>
> >>
> >>I found one issue that local is not allocated using GF_CALLOC and with a
> >>mem-type.
> >>This is a patch which *might* fix it.
> >>
> >>diff --git a/xlators/encryption/crypt/src/crypt-mem-types.h
> >>b/xlators/encryption/crypt/src/crypt-mem-types.h
> >>index 2eab921..c417b67 100644
> >>--- a/xlators/encryption/crypt/src/crypt-mem-types.h
> >>+++ b/xlators/encryption/crypt/src/crypt-mem-types.h
> >>@@ -24,6 +24,7 @@ enum gf_crypt_mem_types_ {
> >>        gf_crypt_mt_key,
> >>        gf_crypt_mt_iovec,
> >>        gf_crypt_mt_char,
> >>+        gf_crypt_mt_local,
> >>        gf_crypt_mt_end,
> >> };
> >>diff --git a/xlators/encryption/crypt/src/crypt.c
> >>b/xlators/encryption/crypt/src/crypt.c
> >>index ae8cdb2..63c0977 100644
> >>--- a/xlators/encryption/crypt/src/crypt.c
> >>+++ b/xlators/encryption/crypt/src/crypt.c
> >>@@ -48,7 +48,7 @@ static crypt_local_t *crypt_alloc_local(call_frame_t
> >>*frame, xlator_t *this,
> >> {
> >>        crypt_local_t *local = NULL;
> >>-       local = mem_get0(this->local_pool);
> >>+        local = GF_CALLOC (sizeof (*local), 1, gf_crypt_mt_local);
> >local is using the memory from pool earlier(i.e. with mem_get0()). Which
> >seems ok to me. Changing it this way will include memory allocation in fop
> >I/O path which is why xlators generally use the mem-pool approach.
> >
> >Pranith
> 
> I think, crypt xlator should do a mem_put of local after doing STACK_UNWIND
> like other xlators which also use mem_get for local (such as AFR). I am
> suspecting crypt not doing mem_put might be the reason for the bug
> mentioned.

I've looked at this now too. The use of mem_get0() seems fine to me. But
indeed, calling mem_put() is missing. Whenever the crypt_local_t should
be released, mem_put() should get called, just like any
GF_CALLOC/GF_FREE combinations.

HTH,
Niels

> 
> Regards,
> Raghavendra Bat
> 
> >>        if (!local) {
> >>                gf_log(this->name, GF_LOG_ERROR, "out of memory");
> >>                return NULL;
> >>
> >>
> >>Niels should be able to recognize if this is sufficient fix or not.
> >>
> >>Thanks,
> >>Raghavendra Talur
> >>
> >>    + Justin
> >>
> >>    --
> >>    GlusterFS - http://www.gluster.org
> >>
> >>    An open source, distributed file system scaling to several
> >>    petabytes, and handling thousands of clients.
> >>
> >>    My personal twitter: twitter.com/realjustinclift
> >>    <http://twitter.com/realjustinclift>
> >>
> >>    _______________________________________________
> >>    Gluster-devel mailing list
> >>    Gluster-devel at gluster.org <mailto:Gluster-devel at gluster.org>
> >>    http://www.gluster.org/mailman/listinfo/gluster-devel
> >>
> >>
> >>
> >>
> >>-- 
> >>*Raghavendra Talur *
> >>
> >
> >
> >
> >_______________________________________________
> >Gluster-devel mailing list
> >Gluster-devel at gluster.org
> >http://www.gluster.org/mailman/listinfo/gluster-devel
> 

> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20150402/fe2802f5/attachment.sig>