From 467033f4a261bef280bfdb29f57aeba49a767ae7 Mon Sep 17 00:00:00 2001 From: Isaku Yamahata Date: Wed, 12 Feb 2020 08:56:25 -0800 Subject: [PATCH] [LibOS] Remove mmap logic on read/write in chroot FS Previously, the LibOS logic in chroot FS's read/write emulation tried to be (too) smart: it mmapped regular files and performed a memcpy on the user-requested file offset + size. This mmap trick required the use of DkStreamMap() which is inefficient on Linux-SGX PAL, since for SGX, the PAL must memcpy from untrusted file buffer into the trusted enclave buffer. Thus, reads/writes in chroot FS resulted in two memcpys under SGX. This commit removes this complicated mmap logic and simply calls DkStreamRead/Write() for regular files. In other words, the mmap optimization is moved out of the LibOS layer and into the PAL layer. --- LibOS/shim/include/shim_handle.h | 5 - LibOS/shim/src/fs/chroot/fs.c | 205 +------------------------------ 2 files changed, 5 insertions(+), 205 deletions(-) diff --git a/LibOS/shim/include/shim_handle.h b/LibOS/shim/include/shim_handle.h index a344f70e..8e1dca12 100644 --- a/LibOS/shim/include/shim_handle.h +++ b/LibOS/shim/include/shim_handle.h @@ -86,11 +86,6 @@ struct shim_file_handle { enum shim_file_type type; off_t size; off_t marker; - - enum { FILEBUF_MAP, FILEBUF_NONE } buf_type; - size_t mapsize; - off_t mapoffset; - void* mapbuf; }; #define FILE_HANDLE_DATA(hdl) ((hdl)->info.file.data) diff --git a/LibOS/shim/src/fs/chroot/fs.c b/LibOS/shim/src/fs/chroot/fs.c index adee8f5d..76e77db0 100644 --- a/LibOS/shim/src/fs/chroot/fs.c +++ b/LibOS/shim/src/fs/chroot/fs.c @@ -473,7 +473,6 @@ static int chroot_open (struct shim_handle * hdl, struct shim_dentry * dent, hdl->type = TYPE_FILE; file->marker = (flags & O_APPEND) ? size : 0; file->size = size; - file->buf_type = (data->type == FILE_REGULAR) ? FILEBUF_MAP : FILEBUF_NONE; hdl->flags = flags; hdl->acc_mode = ACC_MODE(flags & O_ACCMODE); qstrcopy(&hdl->uri, &data->host_uri); @@ -503,7 +502,6 @@ static int chroot_creat (struct shim_handle * hdl, struct shim_dentry * dir, hdl->type = TYPE_FILE; file->marker = (flags & O_APPEND) ? size : 0; file->size = size; - file->buf_type = (data->type == FILE_REGULAR) ? FILEBUF_MAP : FILEBUF_NONE; hdl->flags = flags; hdl->acc_mode = ACC_MODE(flags & O_ACCMODE); qstrcopy(&hdl->uri, &data->host_uri); @@ -617,7 +615,7 @@ static int chroot_hstat (struct shim_handle * hdl, struct stat * stat) stat->st_dev = mdata ? (dev_t) mdata->ino_base : 0; stat->st_ino = dent ? (ino_t) dent->ino : 0; stat->st_size = file->size; - stat->st_mode |= (file->buf_type == FILEBUF_MAP) ? S_IFREG : S_IFCHR; + stat->st_mode |= (file->type == FILE_REGULAR) ? S_IFREG : S_IFCHR; } return 0; @@ -626,190 +624,18 @@ static int chroot_hstat (struct shim_handle * hdl, struct stat * stat) return query_dentry(hdl->dentry, hdl->pal_handle, NULL, stat); } -static void chroot_flush_map(struct shim_handle* hdl) { - struct shim_file_handle* file = &hdl->info.file; - - if (file->buf_type == FILEBUF_MAP) { - lock(&hdl->lock); - void* mapbuf = file->mapbuf; - size_t mapsize = file->mapsize; - file->mapoffset = 0; - file->mapbuf = NULL; - unlock(&hdl->lock); - - if (mapbuf) { - DkStreamUnmap(mapbuf, mapsize); - - if (bkeep_munmap(mapbuf, mapsize, VMA_INTERNAL) < 0) - BUG(); - } - } -} - static int chroot_flush(struct shim_handle* hdl) { int ret = DkStreamFlush(hdl->pal_handle); if (ret < 0) return ret; - - chroot_flush_map(hdl); return 0; } static int chroot_close(struct shim_handle* hdl) { - chroot_flush_map(hdl); + __UNUSED(hdl); return 0; } -static inline int __map_buffer (struct shim_handle * hdl, size_t size) -{ - struct shim_file_handle * file = &hdl->info.file; - - if (file->mapbuf) { - if (file->marker >= file->mapoffset && - file->marker + size <= file->mapoffset + file->mapsize) - return 0; - - DkStreamUnmap(file->mapbuf, file->mapsize); - - if (bkeep_munmap(file->mapbuf, file->mapsize, VMA_INTERNAL) < 0) - BUG(); - - file->mapbuf = NULL; - file->mapoffset = 0; - } - - /* second, reallocate the buffer */ - size_t bufsize = file->mapsize ? : FILE_BUFMAP_SIZE; - assert(IS_POWER_OF_2(bufsize)); - off_t mapoff = ALIGN_DOWN_POW2(file->marker, bufsize); - size_t maplen = bufsize; - int flags = MAP_FILE | MAP_PRIVATE | VMA_INTERNAL; - int prot = PROT_READ; - - if (hdl->acc_mode & MAY_WRITE) { - flags = MAP_FILE | MAP_SHARED | VMA_INTERNAL; - prot |= PROT_WRITE; - } - - while (mapoff + maplen < file->marker + size) - maplen *= 2; - - /* Create the bookkeeping before allocating the memory. */ - void * mapbuf = bkeep_unmapped_any(maplen, prot, flags, mapoff, "filebuf"); - if (!mapbuf) - return -ENOMEM; - - PAL_PTR mapped = DkStreamMap(hdl->pal_handle, mapbuf, PAL_PROT(prot, flags), - mapoff, maplen); - - if (!mapped) { - bkeep_munmap(mapbuf, maplen, flags); - return -PAL_ERRNO; - } - - assert((void*)mapped == mapbuf); - - file->mapbuf = mapbuf; - file->mapoffset = mapoff; - file->mapsize = maplen; - - return 0; -} - -static ssize_t map_read (struct shim_handle * hdl, void * buf, size_t count) -{ - struct shim_file_handle * file = &hdl->info.file; - ssize_t ret = 0; - lock(&hdl->lock); - - struct shim_file_data * data = FILE_HANDLE_DATA(hdl); - off_t size = atomic_read(&data->size); - - if (check_version(hdl) && - file->size < size) - file->size = size; - - off_t marker = file->marker; - - if (marker >= file->size) { - count = 0; - goto out; - } - - if ((ret = __map_buffer(hdl, count)) < 0) { - unlock(&hdl->lock); - return ret; - } - - size_t bytes_left; - if (!__builtin_sub_overflow(file->size, marker, &bytes_left) && bytes_left < count) - count = bytes_left; - - if (count) { - memcpy(buf, file->mapbuf + (marker - file->mapoffset), count); - file->marker = marker + count; - } - -out: - unlock(&hdl->lock); - return count; -} - -static ssize_t map_write (struct shim_handle * hdl, const void * buf, size_t count) -{ - struct shim_file_handle * file = &hdl->info.file; - ssize_t ret = 0; - lock(&hdl->lock); - - struct shim_file_data * data = FILE_HANDLE_DATA(hdl); - off_t marker = file->marker; - - off_t new_marker; - if (__builtin_add_overflow(marker, count, &new_marker)) { - // We can't handle this case reasonably. - ret = -EFBIG; - goto out; - } - - if (new_marker > file->size) { - file->size = new_marker; - - PAL_NUM pal_ret = DkStreamWrite(hdl->pal_handle, file->marker, count, (void *) buf, NULL); - - if (pal_ret == PAL_STREAM_ERROR) { - ret = -PAL_ERRNO; - goto out; - } - - if (pal_ret < count) { - file->size -= count - pal_ret; - } - - chroot_update_size(hdl, file, data); - - if (__builtin_add_overflow(marker, pal_ret, &file->marker)) { - // Should never happen. Even if it would, we couldn't recover from this condition. - BUG(); - } - ret = (ssize_t) pal_ret; - goto out; - } - - if ((ret = __map_buffer(hdl, count)) < 0) - goto out; - - - if (count) { - memcpy(file->mapbuf + (marker - file->mapoffset), buf, count); - file->marker = new_marker; - } - - ret = count; -out: - unlock(&hdl->lock); - return ret; -} - static ssize_t chroot_read (struct shim_handle * hdl, void * buf, size_t count) { ssize_t ret = 0; @@ -834,16 +660,7 @@ static ssize_t chroot_read (struct shim_handle * hdl, void * buf, size_t count) goto out; } - if (file->buf_type == FILEBUF_MAP) { - ret = map_read(hdl, buf, count); - if (ret != -EACCES) - goto out; - - lock(&hdl->lock); - file->buf_type = FILEBUF_NONE; - } else { - lock(&hdl->lock); - } + lock(&hdl->lock); PAL_NUM pal_ret = DkStreamRead(hdl->pal_handle, file->marker, count, buf, NULL, 0); if (pal_ret != PAL_STREAM_ERROR) { @@ -884,16 +701,7 @@ static ssize_t chroot_write (struct shim_handle * hdl, const void * buf, size_t goto out; } - if (hdl->info.file.buf_type == FILEBUF_MAP) { - ret = map_write(hdl, buf, count); - if (ret != -EACCES) - goto out; - - lock(&hdl->lock); - file->buf_type = FILEBUF_NONE; - } else { - lock(&hdl->lock); - } + lock(&hdl->lock); PAL_NUM pal_ret = DkStreamWrite(hdl->pal_handle, file->marker, count, (void *) buf, NULL); if (pal_ret != PAL_STREAM_ERROR) { @@ -1186,9 +994,6 @@ static int chroot_checkout (struct shim_handle * hdl) hdl->pal_handle = NULL; } - hdl->info.file.mapsize = 0; - hdl->info.file.mapoffset = 0; - hdl->info.file.mapbuf = NULL; return 0; } @@ -1268,7 +1073,7 @@ static off_t chroot_poll (struct shim_handle * hdl, int poll_type) off_t marker = file->marker; - if (file->buf_type == FILEBUF_MAP) { + if (file->type == FILE_REGULAR) { ret = poll_type & FS_POLL_WR; if ((poll_type & FS_POLL_RD) && file->size > marker) ret |= FS_POLL_RD;