From 32bf92c5c35120674385e097a1ec829bd4b237d9 Mon Sep 17 00:00:00 2001 From: "chao.an" Date: Mon, 14 Dec 2020 14:59:17 +0800 Subject: [PATCH] fs/unionfs: remove excessive protection to avoid deadlock Deadlock during recursive access if unionfs overlays procfs, check the critical segment only and remove the useless protection part. |#0 unionfs_statfs (mountpt=0xf3df4540, buf=0xf3de2f0c) at unionfs/fs_unionfs.c:2136 ... |#6 0x08069429 in procfs_read (filep=0xf3df4574, buffer=0xf3df4610 "...", buflen=1024) at procfs/fs_procfs.c:412 |#7 0x0806c339 in unionfs_read (filep=0xf3de219c, buffer=0xf3df4610 "...", buflen=1024) at unionfs/fs_unionfs.c:1026 original call stack: (gdb) bt |#0 unionfs_statfs (mountpt=0xf3df4540, buf=0xf3de2f0c) at unionfs/fs_unionfs.c:2136 |#1 0x08071629 in mountpoint_filter (node=0xf3df4540, dirpath=0xf3df4a28 "/proc", arg=0xf3de2fc4) at mount/fs_foreachmountpoint.c:119 |#2 0x0807171b in foreach_inodelevel (node=0xf3df4540, info=0xf3df4a20) at inode/fs_foreachinode.c:90 |#3 0x08071898 in foreach_inode (handler=0x8071530 , arg=0xf3de2fc4) at inode/fs_foreachinode.c:193 |#4 0x080716c1 in foreach_mountpoint (handler=0x8070e2f , arg=0xf3de300c) at mount/fs_foreachmountpoint.c:169 |#5 0x08071399 in mount_read (filep=0xf3df4574, buffer=0xf3df4610 "...", buflen=1024) at mount/fs_procfs_mount.c:537 |#6 0x08069429 in procfs_read (filep=0xf3df4574, buffer=0xf3df4610 "...", buflen=1024) at procfs/fs_procfs.c:412 |#7 0x0806c339 in unionfs_read (filep=0xf3de219c, buffer=0xf3df4610 "...", buflen=1024) at unionfs/fs_unionfs.c:1026 |#8 0x080657a2 in file_read (filep=0xf3de219c, buf=0xf3df4610, nbytes=1024) at vfs/fs_read.c:110 |#9 0x0806581a in nx_read (fd=3, buf=0xf3df4610, nbytes=1024) at vfs/fs_read.c:175 |#10 0x08065847 in read (fd=3, buf=0xf3df4610, nbytes=1024) at vfs/fs_read.c:206 |#11 0x0805a242 in nsh_catfile (vtbl=0xf3df3f10, cmd=0xf3df4378 "df", filepath=0x808d5ed "/proc/fs/blocks") at nsh_fsutils.c:116 |#12 0x0805b1de in cmd_df (vtbl=0xf3df3f10, argc=1, argv=0xf3de32c0) at nsh_mntcmds.c:73 |#13 0x08056370 in nsh_command (vtbl=0xf3df3f10, argc=1, argv=0xf3de32c0) at nsh_command.c:1061 |#14 0x08053b16 in nsh_execute (vtbl=0xf3df3f10, argc=1, argv=0xf3de32c0, redirfile=0x0, oflags=0) at nsh_parse.c:741 |#15 0x08055998 in nsh_parse_command (vtbl=0xf3df3f10, cmdline=0xf3df4378 "df") at nsh_parse.c:2578 |#16 0x08055a7b in nsh_parse (vtbl=0xf3df3f10, cmdline=0xf3df4378 "df") at nsh_parse.c:2662 |#17 0x0805d691 in nsh_session (pstate=0xf3df3f10, login=1 '\001', argc=1, argv=0xf3de34b0) at nsh_session.c:191 |#18 0x0805b542 in nsh_consolemain (argc=1, argv=0xf3de34b0) at nsh_consolemain.c:115 |#19 0x0805346c in nsh_main (argc=1, argv=0xf3de34b0) at nsh_main.c:168 |#20 0x0805075a in nxtask_startup (entrypt=0x805340a , argc=1, argv=0xf3de34b0) at sched/task_startup.c:165 |#21 0x08049713 in nxtask_start () at task/task_start.c:144 |#22 0x00000000 in ?? () Change-Id: Ic4c7aff0ea50388a371c525745e817a787dabcca Signed-off-by: chao.an --- fs/unionfs/fs_unionfs.c | 272 ++++++---------------------------------- 1 file changed, 36 insertions(+), 236 deletions(-) diff --git a/fs/unionfs/fs_unionfs.c b/fs/unionfs/fs_unionfs.c index 7ccb354185b16..58ea391a2343e 100644 --- a/fs/unionfs/fs_unionfs.c +++ b/fs/unionfs/fs_unionfs.c @@ -979,7 +979,6 @@ static ssize_t unionfs_read(FAR struct file *filep, FAR char *buffer, FAR struct unionfs_file_s *uf; FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; - int ret = -EPERM; finfo("buflen: %lu\n", (unsigned long)buflen); @@ -988,14 +987,6 @@ static ssize_t unionfs_read(FAR struct file *filep, FAR char *buffer, DEBUGASSERT(filep != NULL && filep->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)filep->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && filep->f_priv != NULL); uf = (FAR struct unionfs_file_s *)filep->f_priv; @@ -1008,13 +999,7 @@ static ssize_t unionfs_read(FAR struct file *filep, FAR char *buffer, /* Perform the lower level read operation */ - if (ops->read != NULL) - { - ret = ops->read(&uf->uf_file, buffer, buflen); - } - - unionfs_semgive(ui); - return ret; + return ops->read ? ops->read(&uf->uf_file, buffer, buflen) : -EPERM; } /**************************************************************************** @@ -1028,7 +1013,6 @@ static ssize_t unionfs_write(FAR struct file *filep, FAR const char *buffer, FAR struct unionfs_file_s *uf; FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; - int ret = -EPERM; finfo("buflen: %lu\n", (unsigned long)buflen); @@ -1037,14 +1021,6 @@ static ssize_t unionfs_write(FAR struct file *filep, FAR const char *buffer, DEBUGASSERT(filep != NULL && filep->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)filep->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && filep->f_priv != NULL); uf = (FAR struct unionfs_file_s *)filep->f_priv; @@ -1057,13 +1033,7 @@ static ssize_t unionfs_write(FAR struct file *filep, FAR const char *buffer, /* Perform the lower level write operation */ - if (ops->write != NULL) - { - ret = ops->write(&uf->uf_file, buffer, buflen); - } - - unionfs_semgive(ui); - return ret; + return ops->write ? ops->write(&uf->uf_file, buffer, buflen) : -EPERM; } /**************************************************************************** @@ -1076,7 +1046,6 @@ static off_t unionfs_seek(FAR struct file *filep, off_t offset, int whence) FAR struct unionfs_file_s *uf; FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; - int ret; finfo("offset: %lu whence: %d\n", (unsigned long)offset, whence); @@ -1085,14 +1054,6 @@ static off_t unionfs_seek(FAR struct file *filep, off_t offset, int whence) DEBUGASSERT(filep != NULL && filep->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)filep->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && filep->f_priv != NULL); uf = (FAR struct unionfs_file_s *)filep->f_priv; @@ -1111,6 +1072,16 @@ static off_t unionfs_seek(FAR struct file *filep, off_t offset, int whence) } else { + int ret; + + /* Get exclusive access to the file system data structures */ + + ret = unionfs_semtake(ui, false); + if (ret < 0) + { + return ret; + } + /* No... Just set the common file position value */ switch (whence) @@ -1137,9 +1108,10 @@ static off_t unionfs_seek(FAR struct file *filep, off_t offset, int whence) offset = (off_t)-EINVAL; break; } + + unionfs_semgive(ui); } - unionfs_semgive(ui); return offset; } @@ -1153,7 +1125,6 @@ static int unionfs_ioctl(FAR struct file *filep, int cmd, unsigned long arg) FAR struct unionfs_file_s *uf; FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; - int ret = -ENOTTY; finfo("cmd: %d arg: %lu\n", cmd, arg); @@ -1162,14 +1133,6 @@ static int unionfs_ioctl(FAR struct file *filep, int cmd, unsigned long arg) DEBUGASSERT(filep != NULL && filep->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)filep->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && filep->f_priv != NULL); uf = (FAR struct unionfs_file_s *)filep->f_priv; @@ -1182,13 +1145,7 @@ static int unionfs_ioctl(FAR struct file *filep, int cmd, unsigned long arg) /* Perform the lower level ioctl operation */ - if (ops->ioctl != NULL) - { - ret = ops->ioctl(&uf->uf_file, cmd, arg); - } - - unionfs_semgive(ui); - return ret; + return ops->ioctl ? ops->ioctl(&uf->uf_file, cmd, arg) : -ENOTTY; } /**************************************************************************** @@ -1201,7 +1158,6 @@ static int unionfs_sync(FAR struct file *filep) FAR struct unionfs_file_s *uf; FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; - int ret = -EINVAL; finfo("filep=%p\n", filep); @@ -1210,14 +1166,6 @@ static int unionfs_sync(FAR struct file *filep) DEBUGASSERT(filep != NULL && filep->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)filep->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && filep->f_priv != NULL); uf = (FAR struct unionfs_file_s *)filep->f_priv; @@ -1230,13 +1178,7 @@ static int unionfs_sync(FAR struct file *filep) /* Perform the lower level sync operation */ - if (ops->sync != NULL) - { - ret = ops->sync(&uf->uf_file); - } - - unionfs_semgive(ui); - return ret; + return ops->sync ? ops->sync(&uf->uf_file) : -EINVAL; } /**************************************************************************** @@ -1259,14 +1201,6 @@ static int unionfs_dup(FAR const struct file *oldp, FAR struct file *newp) DEBUGASSERT(oldp != NULL && oldp->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)oldp->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && oldp->f_priv != NULL); oldpriv = (FAR struct unionfs_file_s *)oldp->f_priv; @@ -1326,7 +1260,6 @@ static int unionfs_fstat(FAR const struct file *filep, FAR struct stat *buf) FAR struct unionfs_file_s *uf; FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; - int ret = -EPERM; finfo("filep=%p buf=%p\n", filep, buf); @@ -1335,14 +1268,6 @@ static int unionfs_fstat(FAR const struct file *filep, FAR struct stat *buf) DEBUGASSERT(filep != NULL && filep->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)filep->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && filep->f_priv != NULL); uf = (FAR struct unionfs_file_s *)filep->f_priv; @@ -1355,13 +1280,7 @@ static int unionfs_fstat(FAR const struct file *filep, FAR struct stat *buf) /* Perform the lower level write operation */ - if (ops->fstat != NULL) - { - ret = ops->fstat(&uf->uf_file, buf); - } - - unionfs_semgive(ui); - return ret; + return ops->fstat ? ops->fstat(&uf->uf_file, buf) : -EPERM; } /**************************************************************************** @@ -1378,7 +1297,6 @@ static int unionfs_truncate(FAR struct file *filep, off_t length) FAR struct unionfs_file_s *uf; FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; - int ret = -EPERM; finfo("filep=%p length=%ld\n", filep, (long)length); @@ -1387,14 +1305,6 @@ static int unionfs_truncate(FAR struct file *filep, off_t length) DEBUGASSERT(filep != NULL && filep->f_inode != NULL); ui = (FAR struct unionfs_inode_s *)filep->f_inode->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(ui != NULL && filep->f_priv != NULL); uf = (FAR struct unionfs_file_s *)filep->f_priv; @@ -1407,13 +1317,7 @@ static int unionfs_truncate(FAR struct file *filep, off_t length) /* Perform the lower level write operation */ - if (ops->truncate != NULL) - { - ret = ops->truncate(&uf->uf_file, length); - } - - unionfs_semgive(ui); - return ret; + return ops->truncate ? ops->truncate(&uf->uf_file, length) : -EPERM; } /**************************************************************************** @@ -1940,7 +1844,7 @@ static int unionfs_rewinddir(struct inode *mountpt, struct fs_dirent_s *dir) FAR struct unionfs_mountpt_s *um; FAR const struct mountpt_operations *ops; FAR struct fs_unionfsdir_s *fu; - int ret; + int ret = -EINVAL; finfo("mountpt=%p dir=%p\n", mountpt, dir); @@ -1949,14 +1853,6 @@ static int unionfs_rewinddir(struct inode *mountpt, struct fs_dirent_s *dir) DEBUGASSERT(mountpt != NULL && mountpt->i_private != NULL); ui = (FAR struct unionfs_inode_s *)mountpt->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(dir); fu = &dir->u.unionfs; @@ -1988,13 +1884,8 @@ static int unionfs_rewinddir(struct inode *mountpt, struct fs_dirent_s *dir) ret = ops->rewinddir(um->um_node, fu->fu_lower[fu->fu_ndx]); dir->fd_position = fu->fu_lower[fu->fu_ndx]->fd_position; } - else - { - ret = -EINVAL; - } } - unionfs_semgive(ui); return ret; } @@ -2085,10 +1976,14 @@ static int unionfs_unbind(FAR void *handle, FAR struct inode **blkdriver, if (ui->ui_nopen <= 0) { + unionfs_semgive(ui); unionfs_destroy(ui); } + else + { + unionfs_semgive(ui); + } - unionfs_semgive(ui); return OK; } @@ -2119,14 +2014,6 @@ static int unionfs_statfs(FAR struct inode *mountpt, FAR struct statfs *buf) memset(buf, 0, sizeof(struct statfs)); - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - /* Get statfs info from file system 1. * * REVISIT: What would it mean if one file system did not support statfs? @@ -2149,7 +2036,7 @@ static int unionfs_statfs(FAR struct inode *mountpt, FAR struct statfs *buf) ret = ops1->statfs(um1->um_node, &buf1); if (ret < 0) { - goto errout_with_semaphore; + return ret; } /* Get stafs info from file system 2 */ @@ -2157,29 +2044,26 @@ static int unionfs_statfs(FAR struct inode *mountpt, FAR struct statfs *buf) ret = ops2->statfs(um2->um_node, &buf2); if (ret < 0) { - goto errout_with_semaphore; + return ret; } } else if (ops1->statfs != NULL) { /* We have statfs for file system 1 only */ - ret = ops1->statfs(um1->um_node, buf); - goto errout_with_semaphore; + return ops1->statfs(um1->um_node, buf); } else if (ops2->statfs != NULL) { /* We have statfs for file system 2 only */ - ret = ops2->statfs(um2->um_node, buf); - goto errout_with_semaphore; + return ops2->statfs(um2->um_node, buf); } else { /* We could not get stafs info from either file system */ - ret = -ENOSYS; - goto errout_with_semaphore; + return -ENOSYS; } /* We get here is we successfully obtained statfs info from both file @@ -2231,11 +2115,7 @@ static int unionfs_statfs(FAR struct inode *mountpt, FAR struct statfs *buf) buf->f_bfree = buf1.f_bfree + buf2.f_bfree; buf->f_bavail = buf1.f_bavail + buf2.f_bavail; - ret = OK; - -errout_with_semaphore: - unionfs_semgive(ui); - return ret; + return OK; } /**************************************************************************** @@ -2258,14 +2138,6 @@ static int unionfs_unlink(FAR struct inode *mountpt, relpath != NULL); ui = (FAR struct unionfs_inode_s *)mountpt->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - /* Check if some exists at this path on file system 1. This might be * a file or a directory */ @@ -2305,7 +2177,6 @@ static int unionfs_unlink(FAR struct inode *mountpt, } } - unionfs_semgive(ui); return ret; } @@ -2331,30 +2202,20 @@ static int unionfs_mkdir(FAR struct inode *mountpt, FAR const char *relpath, relpath != NULL); ui = (FAR struct unionfs_inode_s *)mountpt->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - /* Is there anything with this name on either file system? */ um = &ui->ui_fs[0]; ret = unionfs_trystat(um->um_node, relpath, um->um_prefix, &buf); if (ret >= 0) { - ret = -EEXIST; - goto errout_with_semaphore; + return -EEXIST; } um = &ui->ui_fs[1]; ret = unionfs_trystat(um->um_node, relpath, um->um_prefix, &buf); if (ret >= 0) { - ret = -EEXIST; - goto errout_with_semaphore; + return -EEXIST; } /* Try to create the directory on both file systems. */ @@ -2370,20 +2231,7 @@ static int unionfs_mkdir(FAR struct inode *mountpt, FAR const char *relpath, * read-only and the other is write-able? */ - if (ret1 >= 0 || ret2 >= 0) - { - ret = OK; - } - else - { - /* Otherwise, pick one */ - - ret = ret1; - } - -errout_with_semaphore: - unionfs_semgive(ui); - return ret; + return (ret1 >= 0 || ret2 >= 0) ? OK : ret1; } /**************************************************************************** @@ -2394,8 +2242,8 @@ static int unionfs_rmdir(FAR struct inode *mountpt, FAR const char *relpath) { FAR struct unionfs_inode_s *ui; FAR struct unionfs_mountpt_s *um; + int ret = -ENOENT; int tmp; - int ret; finfo("relpath: %s\n", relpath); @@ -2405,16 +2253,6 @@ static int unionfs_rmdir(FAR struct inode *mountpt, FAR const char *relpath) relpath != NULL); ui = (FAR struct unionfs_inode_s *)mountpt->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - - ret = -ENOENT; - /* We really don't know any better so we will try to remove the directory * from both file systems. */ @@ -2432,7 +2270,6 @@ static int unionfs_rmdir(FAR struct inode *mountpt, FAR const char *relpath) ret = unionfs_tryrmdir(um->um_node, relpath, um->um_prefix); if (ret < 0) { - unionfs_semgive(ui); return ret; } } @@ -2456,7 +2293,6 @@ static int unionfs_rmdir(FAR struct inode *mountpt, FAR const char *relpath) */ } - unionfs_semgive(ui); return ret; } @@ -2470,8 +2306,8 @@ static int unionfs_rename(FAR struct inode *mountpt, { FAR struct unionfs_inode_s *ui; FAR struct unionfs_mountpt_s *um; - int tmp; int ret = -ENOENT; + int tmp; finfo("oldrelpath: %s newrelpath: %s\n", oldrelpath, newrelpath); @@ -2480,14 +2316,6 @@ static int unionfs_rename(FAR struct inode *mountpt, DEBUGASSERT(mountpt != NULL && mountpt->i_private != NULL); ui = (FAR struct unionfs_inode_s *)mountpt->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - DEBUGASSERT(oldrelpath != NULL && oldrelpath != NULL); /* Is there a file with this name on file system 1 */ @@ -2510,7 +2338,6 @@ static int unionfs_rename(FAR struct inode *mountpt, * file of the same relative path will become visible. */ - unionfs_semgive(ui); return OK; } } @@ -2532,7 +2359,6 @@ static int unionfs_rename(FAR struct inode *mountpt, um->um_prefix); } - unionfs_semgive(ui); return ret; } @@ -2555,14 +2381,6 @@ static int unionfs_stat(FAR struct inode *mountpt, FAR const char *relpath, relpath != NULL); ui = (FAR struct unionfs_inode_s *)mountpt->i_private; - /* Get exclusive access to the file system data structures */ - - ret = unionfs_semtake(ui, false); - if (ret < 0) - { - return ret; - } - /* stat this path on file system 1 */ um = &ui->ui_fs[0]; @@ -2573,7 +2391,6 @@ static int unionfs_stat(FAR struct inode *mountpt, FAR const char *relpath, * shadow the second anyway. */ - unionfs_semgive(ui); return OK; } @@ -2587,7 +2404,6 @@ static int unionfs_stat(FAR struct inode *mountpt, FAR const char *relpath, * shadow the second anyway. */ - unionfs_semgive(ui); return OK; } @@ -2620,7 +2436,6 @@ static int unionfs_stat(FAR struct inode *mountpt, FAR const char *relpath, } } - unionfs_semgive(ui); return ret; } @@ -2818,17 +2633,6 @@ int unionfs_mount(FAR const char *fspath1, FAR const char *prefix1, * for now, however. */ - /* Insert a dummy node -- we need to hold the inode semaphore - * to do this because we will have a momentarily bad structure. - * NOTE that the inode will be created with a reference count of zero. - */ - - ret = inode_semtake(); - if (ret < 0) - { - return ret; - } - ret = inode_reserve(mountpt, &mpinode); if (ret < 0) { @@ -2841,7 +2645,7 @@ int unionfs_mount(FAR const char *fspath1, FAR const char *prefix1, */ ferr("ERROR: Failed to reserve inode\n"); - goto errout_with_semaphore; + return ret; } /* Populate the inode with driver specific information. */ @@ -2862,14 +2666,10 @@ int unionfs_mount(FAR const char *fspath1, FAR const char *prefix1, goto errout_with_mountpt; } - inode_semgive(); return OK; errout_with_mountpt: inode_release(mpinode); - -errout_with_semaphore: - inode_semgive(); return ret; } #endif /* !CONFIG_DISABLE_MOUNTPOINT && CONFIG_FS_UNIONFS */