[Gluster-devel] libgfapi usage issues: overwrites 'THIS' and use after free

Raghavendra Talur rtalur at redhat.com
Fri Apr 17 11:21:29 UTC 2015


On Friday 17 April 2015 03:53 PM, Poornima Gurusiddaiah wrote:
> Hi,
>
> There are two concerns in the usage of libgfapi which have been
> present from day one, but now
> with new users of libgfapi its a necessity to fix these:
>
> 1. When libgfapi is used by the GlusterFS internal xlators, 'THIS'
> gets overwritten:
> Eg: when snapview-server creates a new fs instance for every snap that
> is created.
> Currently any libgfapi calls inside the xlator overwrites the THIS
> value to contain glfs-master(gfapi xlator).
> Hence after the api exits, any further code in the parent xlator
> referring to THIS will refer
> to the glfs-master(gfapi xlator).
>
> Proposed solutions:
> - Store and restore THIS in every API exposed by libgfapi, patch for
> the same can be found at http://review.gluster.org/#/c/9797/
> - Other solution suggested by Niels was to not have internal xlators
> calling libgfapi and
> move the core functionality to libglusterfs. But even with this the
> nested mount/ctx issue can still exist.


Restating the problem to make sure I understand it correctly:
When a "glusterfs" process wants to access a volume using libgfapi we 
have a problem at hand where THIS->ctx of original glusterfs process and 
libgfapi cause conflicts.
Examples could be
1. Snapd which is a glusterfs brick process wants to become a glusterfs 
client process for a different volume using libgfapi.
2. Someday glusterd may choose to become client to a management volume 
using libgfapi.
3. Someone writes an application in libgfapi which gives unified view of 
multiple glusterfs volumes using libgfapi.
4. This is can *possibly* happen without libgfapi if One glusterfs 
process does more than one glusterfs_ctx_new in a nested fashion. (I am 
not sure it is technically possible with current infra or if it is 
already done somewhere).

So the question boils down to "Can we have two ctx in a nested fashion 
in a single process and switch as and when required?, If so how?"

With Poornima's patch here, we have taken care of all cases where it is 
libgfapi encapsulated in another (glusterfs|libgfapi) process.

What we have not solved is the case where a glusterfsd process creates 
another nested ctx as mentioned in case 4 above.

If we think we will never encounter case 4 above then it may just be 
enough to go ahead with this patch and solve this for libgfapi.

I am not sure if I understand Niels' suggestion completely. How can a
gluster xlator access a different volume than what it is part of, if not
through libgfapi?


>
> 2. When libgfapi APIs are called by the application on a fs object,
> that is already closed(glfs_fini()'ed):
> Ideally it is the applications responsibility to take care, to not do
> such things. But its also good
> to not crash libgfapi when such ops are performed by the application.
> We have already seen these issues in snapview server.
>
> Proposed Solutions/workarounds:
> - Do not free the fs object(leaks few bytes) have a state bit to say
> valid or invalid. In every API check
> for the fs validity before proceeding. Patch for same
> @http://review.gluster.org/#/c/9797/
> - As suggested by Niels, have a fs global pool which tracks
> allocated/freed fs objects.
> - Have the applications fix it, so that they do not call fops on
> closed fs object(unmounted fs).
> This mandates multithreaded/asynchronous applications to have some
> synchronization mechanism.
>

IMHO, allocation and free of memory of fs objects should
never be libgfapi's responsibility. They must be allocated by the
application as per struct defined in glfs.h.

Once a fini() is called on a fs, it is like a close() on a fd.
As Poornima suggests, just set the invalid flag which is part of the
struct. It should be application's responsibilty to free the fs memory
after fini returns.

The above patch checks for validity of fs in entry of
every fop and just returns EINVAL whenever the invalid flag is set.

The only downside to this is that it breaks api backward compatibility.
glfs_new currently takes only volname as its input.
new glfs_new would take a preallocated fs and a volname as its input.


> Please let me know your comments on the same.
>
> Regards,
> Poornima
>
>
>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
>



More information about the Gluster-devel mailing list