From 5faf3750bc2957147fa3c54558737aa93d51bb8b Mon Sep 17 00:00:00 2001 From: Dominik Ermel Date: Thu, 15 Oct 2020 13:12:41 +0000 Subject: [PATCH] fs: fs_mount parameter verification before mutex locked block The commit moves fs_mount parameter verification above mutex lock. The list of mount points is now checked before attempting to obtain file system API pointer. All modifications to mount point data structure, given as a parameter to the fs_mount, are only applied after every other operation needed have completed successfully, immediately before adding the mount point to the list of mount points. The fs_mount will a warning when mounted file system does not support unmount. When a file system does not provide mount function, the -ENOTSUP error will be returned instead of -EINVAL. Signed-off-by: Dominik Ermel --- include/fs/fs.h | 11 ++++- subsys/fs/fs.c | 61 +++++++++++++++------------- tests/subsys/fs/fs_api/src/test_fs.c | 4 +- 3 files changed, 45 insertions(+), 31 deletions(-) diff --git a/include/fs/fs.h b/include/fs/fs.h index 75ffd77ef75..06543a700da 100644 --- a/include/fs/fs.h +++ b/include/fs/fs.h @@ -392,10 +392,17 @@ int fs_closedir(struct fs_dir_t *zdp); * calling the file system specific mount function and adding * the mount point to mounted file system list. * - * @param mp Pointer to the fs_mount_t structure + * @param mp Pointer to the fs_mount_t structure. Referenced object + * is not changed if the mount operation failed. + * A reference is captured in the fs infrastructure if the + * mount operation succeeds, and the application must not + * mutate the structure contents until fs_unmount is + * successfully invoked on the same pointer. * * @retval 0 on success; - * @retval <0 a negative errno code on error. + * @retval -ENOENT when file system type has not been registered; + * @retval -ENOTSUP when not supported by underlying file system driver; + * @retval <0 a other negative errno code on error. */ int fs_mount(struct fs_mount_t *mp); diff --git a/subsys/fs/fs.c b/subsys/fs/fs.c index d19cf7ba902..4854d80fedf 100644 --- a/subsys/fs/fs.c +++ b/subsys/fs/fs.c @@ -581,14 +581,40 @@ int fs_mount(struct fs_mount_t *mp) const struct fs_file_system_t *fs; sys_dnode_t *node; int rc = -EINVAL; + size_t len = 0; + /* Do all the mp checks prior to locking the mutex on the file + * subsystem. + */ if ((mp == NULL) || (mp->mnt_point == NULL)) { LOG_ERR("mount point not initialized!!"); return -EINVAL; } + len = strlen(mp->mnt_point); + + if ((len <= 1) || (mp->mnt_point[0] != '/')) { + LOG_ERR("invalid mount point!!"); + return -EINVAL; + } + k_mutex_lock(&mutex, K_FOREVER); + /* Check if mount point already exists */ + SYS_DLIST_FOR_EACH_NODE(&fs_mnt_list, node) { + itr = CONTAINER_OF(node, struct fs_mount_t, node); + /* continue if length does not match */ + if (len != itr->mountp_len) { + continue; + } + + if (strncmp(mp->mnt_point, itr->mnt_point, len) == 0) { + LOG_ERR("mount point already exists!!"); + rc = -EBUSY; + goto mount_err; + } + } + /* Get file system information */ fs = fs_type_get(mp->type); if (fs == NULL) { @@ -597,48 +623,27 @@ int fs_mount(struct fs_mount_t *mp) goto mount_err; } - mp->mountp_len = strlen(mp->mnt_point); - - if ((mp->mnt_point[0] != '/') || - (strlen(mp->mnt_point) <= 1)) { - LOG_ERR("invalid mount point!!"); - rc = -EINVAL; - goto mount_err; - } - if (fs->mount == NULL) { - LOG_ERR("fs ops functions not set!!"); - rc = -EINVAL; + LOG_ERR("fs type %d does not support mounting", mp->type); + rc = -ENOTSUP; goto mount_err; } - /* Check if mount point already exists */ - SYS_DLIST_FOR_EACH_NODE(&fs_mnt_list, node) { - itr = CONTAINER_OF(node, struct fs_mount_t, node); - /* continue if length does not match */ - if (mp->mountp_len != itr->mountp_len) { - continue; - } - - if (strncmp(mp->mnt_point, itr->mnt_point, - mp->mountp_len) == 0) { - LOG_ERR("mount Point already exists!!"); - rc = -EBUSY; - goto mount_err; - } + if (fs->unmount == NULL) { + LOG_WRN("mount path %s is not unmountable", + log_strdup(mp->mnt_point)); } - rc = fs->mount(mp); if (rc < 0) { LOG_ERR("fs mount error (%d)", rc); goto mount_err; } - /* set mount point fs interface */ + /* Update mount point data and append it to the list */ + mp->mountp_len = len; mp->fs = fs; - /* append to the mount list */ sys_dlist_append(&fs_mnt_list, &mp->node); LOG_DBG("fs mounted at %s", log_strdup(mp->mnt_point)); diff --git a/tests/subsys/fs/fs_api/src/test_fs.c b/tests/subsys/fs/fs_api/src/test_fs.c index a3681e54fca..a1733513173 100644 --- a/tests/subsys/fs/fs_api/src/test_fs.c +++ b/tests/subsys/fs/fs_api/src/test_fs.c @@ -295,11 +295,13 @@ static int temp_statvfs(struct fs_mount_t *mountp, static int temp_mount(struct fs_mount_t *mountp) { + size_t len = strlen(mountp->mnt_point); + if (mountp == NULL) { return -EINVAL; } - if (mountp->mnt_point[mountp->mountp_len - 1] != ':') { + if (mountp->mnt_point[len - 1] != ':') { return -EINVAL; } mp[mountp->type] = mountp;