From f191bb7bc1abd250bdf0a5a6972ce95fcbd3314b Mon Sep 17 00:00:00 2001 From: Raghavendra Bhat Date: Fri, 8 Jun 2018 09:54:00 -0400 Subject: features/snapview-server: validate the fs instance before doing fop there PROBLEM: ======== USS design depends on snapview-server translator communicating with each individual snapshot via gfapi. So, the snapview-server xlator maintains the glfs instance (thus the snapshot) to which a inode belongs to by storing it inside the inode context. Suppose, a file from a snapshot is opened by a application, and the fd is still valid from application's point of view (i.e. application has not yet closed fd). Now, if the snapshot to which the opened file belongs to is deleted, then the glfs_t instance corresponding to the snapshot is destroyed by snapview-server as part of snap deletion. But now, if the application does IO on the fd it has kept open, then snapview server tries to send that request to the corresponding snap via glfs instance for that snapshot stored in the inode context for the file on which the application is sending the fop. And this results in freed up glfs_t pointer being accessed and causes a segfault. FIX: === For fd based operations, check whether the glfs instance that the inode contains in its context, is still valid or not. For non fd based operations, usually lookup should guarantee that. But if the file was already looked up, and the client accessing the snap data (either NFS, or native glusterfs fuse) does not bother to send a lookup and directly sends a path based fop, then that path based fop should ensure that the fs instance is valid. Change-Id: I881be15ec46ecb51aa844d7fd41d5630f0d644fb updates: bz#1602070 Signed-off-by: Raghavendra Bhat --- .../snapview-server/src/snapview-server-helpers.c | 35 +++++++ .../features/snapview-server/src/snapview-server.c | 109 +++++++++++++++------ .../features/snapview-server/src/snapview-server.h | 6 ++ 3 files changed, 120 insertions(+), 30 deletions(-) (limited to 'xlators/features/snapview-server/src') diff --git a/xlators/features/snapview-server/src/snapview-server-helpers.c b/xlators/features/snapview-server/src/snapview-server-helpers.c index 6f305dbc2f..091feb5918 100644 --- a/xlators/features/snapview-server/src/snapview-server-helpers.c +++ b/xlators/features/snapview-server/src/snapview-server-helpers.c @@ -595,3 +595,38 @@ svs_get_latest_snapshot (xlator_t *this) out: return fs; } + +glfs_t * +svs_inode_ctx_glfs_mapping (xlator_t *this, svs_inode_t *inode_ctx) +{ + glfs_t *fs = NULL; + + GF_VALIDATE_OR_GOTO ("svs", this, out); + GF_VALIDATE_OR_GOTO (this->name, inode_ctx, out); + + fs = inode_ctx->fs; + + SVS_CHECK_VALID_SNAPSHOT_HANDLE (fs, this); + +out: + return fs; +} + +glfs_t * +svs_inode_glfs_mapping (xlator_t *this, inode_t *inode) +{ + svs_inode_t *inode_ctx = NULL; + glfs_t *fs = NULL; + + inode_ctx = svs_inode_ctx_get (this, inode); + if (!inode_ctx) { + gf_log (this->name, GF_LOG_ERROR, "inode context not found for" + " the inode %s", uuid_utoa (inode->gfid)); + goto out; + } + + fs = svs_inode_ctx_glfs_mapping (this, inode_ctx); + +out: + return fs; +} diff --git a/xlators/features/snapview-server/src/snapview-server.c b/xlators/features/snapview-server/src/snapview-server.c index 9e109e213c..ed56937aa7 100644 --- a/xlators/features/snapview-server/src/snapview-server.c +++ b/xlators/features/snapview-server/src/snapview-server.c @@ -543,7 +543,6 @@ svs_lookup (call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *xdata) svs_inode_t *parent_ctx = NULL; int32_t ret = -1; inode_t *parent = NULL; - snap_dirent_t *dirent = NULL; gf_boolean_t entry_point_key = _gf_false; gf_boolean_t entry_point = _gf_false; call_stack_t *root = NULL; @@ -592,13 +591,6 @@ svs_lookup (call_frame_t *frame, xlator_t *this, loc_t *loc, dict_t *xdata) inode_ctx = svs_inode_ctx_get (this, loc->inode); - /* Initialize latest snapshot, which is used for nameless lookups */ - dirent = svs_get_latest_snap_entry (this); - - if (dirent && !dirent->fs) { - svs_initialise_snapshot_volume (this, dirent->name, NULL); - } - if (xdata && !inode_ctx) { ret = dict_get_str_boolean (xdata, "entry-point", _gf_false); if (ret == -1) { @@ -996,6 +988,17 @@ svs_fgetxattr (call_frame_t *frame, xlator_t *this, fd_t *fd, const char *name, goto out; } + if (!(svs_inode_ctx_glfs_mapping (this, inode_ctx))) { + gf_log (this->name, GF_LOG_ERROR, "glfs instance " + "instance %p to which the inode %s belongs" + "to does not exist. That snapshot might have" + "been deleted or deactivated", inode_ctx->fs, + uuid_utoa (fd->inode->gfid)); + op_ret = -1; + op_errno = EBADF; + goto out; + } + sfd = svs_fd_ctx_get_or_new (this, fd); if (!sfd) { gf_log (this->name, GF_LOG_ERROR, "failed to get the fd " @@ -1130,9 +1133,12 @@ out: int32_t svs_releasedir (xlator_t *this, fd_t *fd) { - svs_fd_t *sfd = NULL; - uint64_t tmp_pfd = 0; - int ret = 0; + svs_fd_t *sfd = NULL; + uint64_t tmp_pfd = 0; + int ret = 0; + svs_inode_t *svs_inode = NULL; + glfs_t *fs = NULL; + inode_t *inode = NULL; GF_VALIDATE_OR_GOTO ("snapview-server", this, out); GF_VALIDATE_OR_GOTO (this->name, fd, out); @@ -1144,13 +1150,24 @@ svs_releasedir (xlator_t *this, fd_t *fd) goto out; } - sfd = (svs_fd_t *)(long)tmp_pfd; - if (sfd->fd) { - ret = glfs_closedir (sfd->fd); - if (ret) - gf_log (this->name, GF_LOG_WARNING, "failed to close " - "the glfd for directory %s", - uuid_utoa (fd->inode->gfid)); + + inode = fd->inode; + + svs_inode = svs_inode_ctx_get (this, inode); + if (svs_inode) { + fs = svs_inode->fs; /* should inode->lock be held for this? */ + SVS_CHECK_VALID_SNAPSHOT_HANDLE (fs, this); + if (fs) { + sfd = (svs_fd_t *)(long)tmp_pfd; + if (sfd->fd) { + ret = glfs_closedir (sfd->fd); + if (ret) + gf_log (this->name, GF_LOG_WARNING, + "failed to close the glfd" + "for directory %s", + uuid_utoa (fd->inode->gfid)); + } + } } GF_FREE (sfd); @@ -1209,9 +1226,12 @@ out: int32_t svs_release (xlator_t *this, fd_t *fd) { - svs_fd_t *sfd = NULL; - uint64_t tmp_pfd = 0; - int ret = 0; + svs_fd_t *sfd = NULL; + uint64_t tmp_pfd = 0; + int ret = 0; + inode_t *inode = NULL; + svs_inode_t *svs_inode = NULL; + glfs_t *fs = NULL; GF_VALIDATE_OR_GOTO ("snapview-server", this, out); GF_VALIDATE_OR_GOTO (this->name, fd, out); @@ -1223,13 +1243,22 @@ svs_release (xlator_t *this, fd_t *fd) goto out; } - sfd = (svs_fd_t *)(long)tmp_pfd; - if (sfd->fd) { - ret = glfs_close (sfd->fd); - if (ret) { - gf_log (this->name, GF_LOG_ERROR, "failed to close " - "the glfd for %s", - uuid_utoa (fd->inode->gfid)); + inode = fd->inode; + + svs_inode = svs_inode_ctx_get (this, inode); + if (svs_inode) { + fs = svs_inode->fs; /* should inode->lock be held for this? */ + SVS_CHECK_VALID_SNAPSHOT_HANDLE (fs, this); + if (fs) { + sfd = (svs_fd_t *)(long)tmp_pfd; + if (sfd->fd) { + ret = glfs_close (sfd->fd); + if (ret) + gf_log (this->name, GF_LOG_ERROR, + "failed to close " + "the glfd for %s", + uuid_utoa (fd->inode->gfid)); + } } } @@ -1893,8 +1922,18 @@ svs_fstat (call_frame_t *frame, xlator_t *this, fd_t *fd, dict_t *xdata) if (inode_ctx->type == SNAP_VIEW_ENTRY_POINT_INODE) { svs_iatt_fill (fd->inode->gfid, &buf); op_ret = 0; - } - else { + } else { + if (!(svs_inode_ctx_glfs_mapping (this, inode_ctx))) { + gf_log (this->name, GF_LOG_ERROR, "glfs instance " + "instance %p to which the inode %s belongs " + "to does not exist. That snapshot might have " + "been deleted or deactivated", inode_ctx->fs, + uuid_utoa (fd->inode->gfid)); + op_ret = -1; + op_errno = EBADF; + goto out; + } + sfd = svs_fd_ctx_get_or_new (this, fd); if (!sfd) { gf_log (this->name, GF_LOG_ERROR, "failed to get the " @@ -2084,6 +2123,16 @@ svs_readv (call_frame_t *frame, xlator_t *this, goto out; } + if (!svs_inode_glfs_mapping (this, fd->inode)) { + gf_log (this->name, GF_LOG_ERROR, "glfs instance to " + "which the inode %s receiving read request belongs, " + "does not exist anymore", + uuid_utoa (fd->inode->gfid)); + op_ret = -1; + op_errno = EBADF; /* should this be some other error? */ + goto out; + } + sfd = svs_fd_ctx_get_or_new (this, fd); if (!sfd) { gf_log (this->name, GF_LOG_ERROR, "failed to get the fd " diff --git a/xlators/features/snapview-server/src/snapview-server.h b/xlators/features/snapview-server/src/snapview-server.h index bacb627560..a805c7435b 100644 --- a/xlators/features/snapview-server/src/snapview-server.h +++ b/xlators/features/snapview-server/src/snapview-server.h @@ -242,4 +242,10 @@ int32_t svs_get_handle (xlator_t *this, loc_t *loc, svs_inode_t *inode_ctx, int32_t *op_errno); +glfs_t * +svs_inode_glfs_mapping (xlator_t *this, inode_t *inode); + +glfs_t * +svs_inode_ctx_glfs_mapping (xlator_t *this, svs_inode_t *inode_ctx); + #endif /* __SNAP_VIEW_H__ */ -- cgit