* [PATCH 4.19 1/5] staging: erofs: add error handling for xattr submodule
@ 2019-03-11 6:08 ` Gao Xiang
0 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
commit cadf1ccf1b0021d0b7a9347e102ac5258f9f98c8 upstream.
This patch enhances the missing error handling code for
xattr submodule, which improves the stability for the rare cases.
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
This series resolves the following conflicts:
FAILED: patch "[PATCH] staging: erofs: fix fast symlink w/o xattr when fs xattr is" failed to apply to 4.19-stable tree
FAILED: patch "[PATCH] staging: erofs: fix memleak of inode's shared xattr array" failed to apply to 4.19-stable tree
FAILED: patch "[PATCH] staging: erofs: fix race of initializing xattrs of a inode at" failed to apply to 4.19-stable tree
FAILED: patch "[PATCH] staging: erofs: keep corrupted fs from crashing kernel in" failed to apply to 4.19-stable tree
In addition, there is another patch called
staging: erofs: add error handling for xattr submodule
that needs to be backported to 4.19 as well (4.20+ already has this patch.)
drivers/staging/erofs/internal.h | 5 +-
drivers/staging/erofs/xattr.c | 112 +++++++++++++++++++++++++++------------
2 files changed, 81 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 9f44ed8f0023..1c048c412fb2 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -485,8 +485,9 @@ struct erofs_map_blocks_iter {
};
-static inline struct page *erofs_get_inline_page(struct inode *inode,
- erofs_blk_t blkaddr)
+static inline struct page *
+erofs_get_inline_page(struct inode *inode,
+ erofs_blk_t blkaddr)
{
return erofs_get_meta_page(inode->i_sb,
blkaddr, S_ISDIR(inode->i_mode));
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 0e9cfeccdf99..d4bccb57e7c3 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -24,16 +24,25 @@ struct xattr_iter {
static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
{
- /* only init_inode_xattrs use non-atomic once */
+ /* the only user of kunmap() is 'init_inode_xattrs' */
if (unlikely(!atomic))
kunmap(it->page);
else
kunmap_atomic(it->kaddr);
+
unlock_page(it->page);
put_page(it->page);
}
-static void init_inode_xattrs(struct inode *inode)
+static inline void xattr_iter_end_final(struct xattr_iter *it)
+{
+ if (!it->page)
+ return;
+
+ xattr_iter_end(it, true);
+}
+
+static int init_inode_xattrs(struct inode *inode)
{
struct xattr_iter it;
unsigned i;
@@ -43,7 +52,7 @@ static void init_inode_xattrs(struct inode *inode)
bool atomic_map;
if (likely(inode_has_inited_xattr(inode)))
- return;
+ return 0;
vi = EROFS_V(inode);
BUG_ON(!vi->xattr_isize);
@@ -53,7 +62,8 @@ static void init_inode_xattrs(struct inode *inode)
it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
it.page = erofs_get_inline_page(inode, it.blkaddr);
- BUG_ON(IS_ERR(it.page));
+ if (IS_ERR(it.page))
+ return PTR_ERR(it.page);
/* read in shared xattr array (non-atomic, see kmalloc below) */
it.kaddr = kmap(it.page);
@@ -62,9 +72,12 @@ static void init_inode_xattrs(struct inode *inode)
ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
vi->xattr_shared_count = ih->h_shared_count;
- vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
- vi->xattr_shared_count, sizeof(unsigned),
- GFP_KERNEL | __GFP_NOFAIL);
+ vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
+ sizeof(uint), GFP_KERNEL);
+ if (!vi->xattr_shared_xattrs) {
+ xattr_iter_end(&it, atomic_map);
+ return -ENOMEM;
+ }
/* let's skip ibody header */
it.ofs += sizeof(struct erofs_xattr_ibody_header);
@@ -77,7 +90,8 @@ static void init_inode_xattrs(struct inode *inode)
it.page = erofs_get_meta_page(inode->i_sb,
++it.blkaddr, S_ISDIR(inode->i_mode));
- BUG_ON(IS_ERR(it.page));
+ if (IS_ERR(it.page))
+ return PTR_ERR(it.page);
it.kaddr = kmap_atomic(it.page);
atomic_map = true;
@@ -90,6 +104,7 @@ static void init_inode_xattrs(struct inode *inode)
xattr_iter_end(&it, atomic_map);
inode_set_inited_xattr(inode);
+ return 0;
}
struct xattr_iter_handlers {
@@ -99,18 +114,25 @@ struct xattr_iter_handlers {
void (*value)(struct xattr_iter *, unsigned, char *, unsigned);
};
-static void xattr_iter_fixup(struct xattr_iter *it)
+static inline int xattr_iter_fixup(struct xattr_iter *it)
{
- if (unlikely(it->ofs >= EROFS_BLKSIZ)) {
- xattr_iter_end(it, true);
+ if (it->ofs < EROFS_BLKSIZ)
+ return 0;
- it->blkaddr += erofs_blknr(it->ofs);
- it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
- BUG_ON(IS_ERR(it->page));
+ xattr_iter_end(it, true);
- it->kaddr = kmap_atomic(it->page);
- it->ofs = erofs_blkoff(it->ofs);
+ it->blkaddr += erofs_blknr(it->ofs);
+ it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
+ if (IS_ERR(it->page)) {
+ int err = PTR_ERR(it->page);
+
+ it->page = NULL;
+ return err;
}
+
+ it->kaddr = kmap_atomic(it->page);
+ it->ofs = erofs_blkoff(it->ofs);
+ return 0;
}
static int inline_xattr_iter_begin(struct xattr_iter *it,
@@ -132,21 +154,24 @@ static int inline_xattr_iter_begin(struct xattr_iter *it,
it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
it->page = erofs_get_inline_page(inode, it->blkaddr);
- BUG_ON(IS_ERR(it->page));
- it->kaddr = kmap_atomic(it->page);
+ if (IS_ERR(it->page))
+ return PTR_ERR(it->page);
+ it->kaddr = kmap_atomic(it->page);
return vi->xattr_isize - xattr_header_sz;
}
static int xattr_foreach(struct xattr_iter *it,
- struct xattr_iter_handlers *op, unsigned *tlimit)
+ const struct xattr_iter_handlers *op, unsigned int *tlimit)
{
struct erofs_xattr_entry entry;
unsigned value_sz, processed, slice;
int err;
/* 0. fixup blkaddr, ofs, ipage */
- xattr_iter_fixup(it);
+ err = xattr_iter_fixup(it);
+ if (err)
+ return err;
/*
* 1. read xattr entry to the memory,
@@ -178,7 +203,9 @@ static int xattr_foreach(struct xattr_iter *it,
if (it->ofs >= EROFS_BLKSIZ) {
BUG_ON(it->ofs > EROFS_BLKSIZ);
- xattr_iter_fixup(it);
+ err = xattr_iter_fixup(it);
+ if (err)
+ goto out;
it->ofs = 0;
}
@@ -210,7 +237,10 @@ static int xattr_foreach(struct xattr_iter *it,
while (processed < value_sz) {
if (it->ofs >= EROFS_BLKSIZ) {
BUG_ON(it->ofs > EROFS_BLKSIZ);
- xattr_iter_fixup(it);
+
+ err = xattr_iter_fixup(it);
+ if (err)
+ goto out;
it->ofs = 0;
}
@@ -270,7 +300,7 @@ static void xattr_copyvalue(struct xattr_iter *_it,
memcpy(it->buffer + processed, buf, len);
}
-static struct xattr_iter_handlers find_xattr_handlers = {
+static const struct xattr_iter_handlers find_xattr_handlers = {
.entry = xattr_entrymatch,
.name = xattr_namematch,
.alloc_buffer = xattr_checkbuffer,
@@ -291,8 +321,11 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it)
ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
if (ret >= 0)
break;
+
+ if (ret != -ENOATTR) /* -ENOMEM, -EIO, etc. */
+ break;
}
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_size;
}
@@ -315,8 +348,10 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
xattr_iter_end(&it->it, true);
it->it.page = erofs_get_meta_page(inode->i_sb,
- blkaddr, false);
- BUG_ON(IS_ERR(it->it.page));
+ blkaddr, false);
+ if (IS_ERR(it->it.page))
+ return PTR_ERR(it->it.page);
+
it->it.kaddr = kmap_atomic(it->it.page);
it->it.blkaddr = blkaddr;
}
@@ -324,9 +359,12 @@ static int shared_getxattr(struct inode *inode, struct getxattr_iter *it)
ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
if (ret >= 0)
break;
+
+ if (ret != -ENOATTR) /* -ENOMEM, -EIO, etc. */
+ break;
}
if (vi->xattr_shared_count)
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_size;
}
@@ -351,7 +389,9 @@ int erofs_getxattr(struct inode *inode, int index,
if (unlikely(name == NULL))
return -EINVAL;
- init_inode_xattrs(inode);
+ ret = init_inode_xattrs(inode);
+ if (ret)
+ return ret;
it.index = index;
@@ -494,7 +534,7 @@ static int xattr_skipvalue(struct xattr_iter *_it,
return 1;
}
-static struct xattr_iter_handlers list_xattr_handlers = {
+static const struct xattr_iter_handlers list_xattr_handlers = {
.entry = xattr_entrylist,
.name = xattr_namelist,
.alloc_buffer = xattr_skipvalue,
@@ -516,7 +556,7 @@ static int inline_listxattr(struct listxattr_iter *it)
if (ret < 0)
break;
}
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_ofs;
}
@@ -538,8 +578,10 @@ static int shared_listxattr(struct listxattr_iter *it)
xattr_iter_end(&it->it, true);
it->it.page = erofs_get_meta_page(inode->i_sb,
- blkaddr, false);
- BUG_ON(IS_ERR(it->it.page));
+ blkaddr, false);
+ if (IS_ERR(it->it.page))
+ return PTR_ERR(it->it.page);
+
it->it.kaddr = kmap_atomic(it->it.page);
it->it.blkaddr = blkaddr;
}
@@ -549,7 +591,7 @@ static int shared_listxattr(struct listxattr_iter *it)
break;
}
if (vi->xattr_shared_count)
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_ofs;
}
@@ -560,7 +602,9 @@ ssize_t erofs_listxattr(struct dentry *dentry,
int ret;
struct listxattr_iter it;
- init_inode_xattrs(d_inode(dentry));
+ ret = init_inode_xattrs(d_inode(dentry));
+ if (ret)
+ return ret;
it.dentry = dentry;
it.buffer = buffer;
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4.19 2/5] staging: erofs: fix fast symlink w/o xattr when fs xattr is on
2019-03-11 6:08 ` Gao Xiang
@ 2019-03-11 6:08 ` Gao Xiang
-1 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, LKML, linux-erofs, Chao Yu, Chao Yu,
Miao Xie, Fang Wei, Gao Xiang
commit 7077fffcb0b0b65dc75e341306aeef4d0e7f2ec6 upstream.
Currently, this will hit a BUG_ON for these symlinks as follows:
- kernel message
------------[ cut here ]------------
kernel BUG at drivers/staging/erofs/xattr.c:59!
SMP PTI
CPU: 1 PID: 1170 Comm: getllxattr Not tainted 4.20.0-rc6+ #92
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
RIP: 0010:init_inode_xattrs+0x22b/0x270
Code: 48 0f 45 ea f0 ff 4d 34 74 0d 41 83 4c 24 e0 01 31 c0 e9 00 fe ff ff 48 89 ef e8 e0 31 9e ff eb e9 89 e8 e9 ef fd ff ff 0f 0$
<0f> 0b 48 89 ef e8 fb f6 9c ff 48 8b 45 08 a8 01 75 24 f0 ff 4d 34
RSP: 0018:ffffa03ac026bdf8 EFLAGS: 00010246
------------[ cut here ]------------
...
Call Trace:
erofs_listxattr+0x30/0x2c0
? selinux_inode_listxattr+0x5a/0x80
? kmem_cache_alloc+0x33/0x170
? security_inode_listxattr+0x27/0x40
listxattr+0xaf/0xc0
path_listxattr+0x5a/0xa0
do_syscall_64+0x43/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
...
---[ end trace 3c24b49408dc0c72 ]---
Fix it by checking ->xattr_isize in init_inode_xattrs(),
and it also fixes improper return value -ENOTSUPP
(it should be -ENODATA if xattr is enabled) for those inodes.
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable@vger.kernel.org> # 4.19+
Reported-by: Li Guifu <bluce.liguifu@huawei.com>
Tested-by: Li Guifu <bluce.liguifu@huawei.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
drivers/staging/erofs/inode.c | 8 ++++----
drivers/staging/erofs/xattr.c | 25 ++++++++++++++++++++-----
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index 9e7815f55a17..7448744cc515 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -184,16 +184,16 @@ static int fill_inode(struct inode *inode, int isdir)
/* setup the new inode */
if (S_ISREG(inode->i_mode)) {
#ifdef CONFIG_EROFS_FS_XATTR
- if (vi->xattr_isize)
- inode->i_op = &erofs_generic_xattr_iops;
+ inode->i_op = &erofs_generic_xattr_iops;
#endif
inode->i_fop = &generic_ro_fops;
} else if (S_ISDIR(inode->i_mode)) {
inode->i_op =
#ifdef CONFIG_EROFS_FS_XATTR
- vi->xattr_isize ? &erofs_dir_xattr_iops :
-#endif
+ &erofs_dir_xattr_iops;
+#else
&erofs_dir_iops;
+#endif
inode->i_fop = &erofs_dir_fops;
} else if (S_ISLNK(inode->i_mode)) {
/* by default, page_get_link is used for symlink */
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index d4bccb57e7c3..ac2567576335 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -55,7 +55,26 @@ static int init_inode_xattrs(struct inode *inode)
return 0;
vi = EROFS_V(inode);
- BUG_ON(!vi->xattr_isize);
+
+ /*
+ * bypass all xattr operations if ->xattr_isize is not greater than
+ * sizeof(struct erofs_xattr_ibody_header), in detail:
+ * 1) it is not enough to contain erofs_xattr_ibody_header then
+ * ->xattr_isize should be 0 (it means no xattr);
+ * 2) it is just to contain erofs_xattr_ibody_header, which is on-disk
+ * undefined right now (maybe use later with some new sb feature).
+ */
+ if (vi->xattr_isize == sizeof(struct erofs_xattr_ibody_header)) {
+ errln("xattr_isize %d of nid %llu is not supported yet",
+ vi->xattr_isize, vi->nid);
+ return -ENOTSUPP;
+ } else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
+ if (unlikely(vi->xattr_isize)) {
+ DBG_BUGON(1);
+ return -EIO; /* xattr ondisk layout error */
+ }
+ return -ENOATTR;
+ }
sbi = EROFS_I_SB(inode);
it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
@@ -414,7 +433,6 @@ static int erofs_xattr_generic_get(const struct xattr_handler *handler,
struct dentry *unused, struct inode *inode,
const char *name, void *buffer, size_t size)
{
- struct erofs_vnode *const vi = EROFS_V(inode);
struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
switch (handler->flags) {
@@ -432,9 +450,6 @@ static int erofs_xattr_generic_get(const struct xattr_handler *handler,
return -EINVAL;
}
- if (!vi->xattr_isize)
- return -ENOATTR;
-
return erofs_getxattr(inode, handler->flags, name, buffer, size);
}
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4.19 2/5] staging: erofs: fix fast symlink w/o xattr when fs xattr is on
@ 2019-03-11 6:08 ` Gao Xiang
0 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
commit 7077fffcb0b0b65dc75e341306aeef4d0e7f2ec6 upstream.
Currently, this will hit a BUG_ON for these symlinks as follows:
- kernel message
------------[ cut here ]------------
kernel BUG at drivers/staging/erofs/xattr.c:59!
SMP PTI
CPU: 1 PID: 1170 Comm: getllxattr Not tainted 4.20.0-rc6+ #92
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
RIP: 0010:init_inode_xattrs+0x22b/0x270
Code: 48 0f 45 ea f0 ff 4d 34 74 0d 41 83 4c 24 e0 01 31 c0 e9 00 fe ff ff 48 89 ef e8 e0 31 9e ff eb e9 89 e8 e9 ef fd ff ff 0f 0$
<0f> 0b 48 89 ef e8 fb f6 9c ff 48 8b 45 08 a8 01 75 24 f0 ff 4d 34
RSP: 0018:ffffa03ac026bdf8 EFLAGS: 00010246
------------[ cut here ]------------
...
Call Trace:
erofs_listxattr+0x30/0x2c0
? selinux_inode_listxattr+0x5a/0x80
? kmem_cache_alloc+0x33/0x170
? security_inode_listxattr+0x27/0x40
listxattr+0xaf/0xc0
path_listxattr+0x5a/0xa0
do_syscall_64+0x43/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
...
---[ end trace 3c24b49408dc0c72 ]---
Fix it by checking ->xattr_isize in init_inode_xattrs(),
and it also fixes improper return value -ENOTSUPP
(it should be -ENODATA if xattr is enabled) for those inodes.
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable at vger.kernel.org> # 4.19+
Reported-by: Li Guifu <bluce.liguifu at huawei.com>
Tested-by: Li Guifu <bluce.liguifu at huawei.com>
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
drivers/staging/erofs/inode.c | 8 ++++----
drivers/staging/erofs/xattr.c | 25 ++++++++++++++++++++-----
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index 9e7815f55a17..7448744cc515 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -184,16 +184,16 @@ static int fill_inode(struct inode *inode, int isdir)
/* setup the new inode */
if (S_ISREG(inode->i_mode)) {
#ifdef CONFIG_EROFS_FS_XATTR
- if (vi->xattr_isize)
- inode->i_op = &erofs_generic_xattr_iops;
+ inode->i_op = &erofs_generic_xattr_iops;
#endif
inode->i_fop = &generic_ro_fops;
} else if (S_ISDIR(inode->i_mode)) {
inode->i_op =
#ifdef CONFIG_EROFS_FS_XATTR
- vi->xattr_isize ? &erofs_dir_xattr_iops :
-#endif
+ &erofs_dir_xattr_iops;
+#else
&erofs_dir_iops;
+#endif
inode->i_fop = &erofs_dir_fops;
} else if (S_ISLNK(inode->i_mode)) {
/* by default, page_get_link is used for symlink */
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index d4bccb57e7c3..ac2567576335 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -55,7 +55,26 @@ static int init_inode_xattrs(struct inode *inode)
return 0;
vi = EROFS_V(inode);
- BUG_ON(!vi->xattr_isize);
+
+ /*
+ * bypass all xattr operations if ->xattr_isize is not greater than
+ * sizeof(struct erofs_xattr_ibody_header), in detail:
+ * 1) it is not enough to contain erofs_xattr_ibody_header then
+ * ->xattr_isize should be 0 (it means no xattr);
+ * 2) it is just to contain erofs_xattr_ibody_header, which is on-disk
+ * undefined right now (maybe use later with some new sb feature).
+ */
+ if (vi->xattr_isize == sizeof(struct erofs_xattr_ibody_header)) {
+ errln("xattr_isize %d of nid %llu is not supported yet",
+ vi->xattr_isize, vi->nid);
+ return -ENOTSUPP;
+ } else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
+ if (unlikely(vi->xattr_isize)) {
+ DBG_BUGON(1);
+ return -EIO; /* xattr ondisk layout error */
+ }
+ return -ENOATTR;
+ }
sbi = EROFS_I_SB(inode);
it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
@@ -414,7 +433,6 @@ static int erofs_xattr_generic_get(const struct xattr_handler *handler,
struct dentry *unused, struct inode *inode,
const char *name, void *buffer, size_t size)
{
- struct erofs_vnode *const vi = EROFS_V(inode);
struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
switch (handler->flags) {
@@ -432,9 +450,6 @@ static int erofs_xattr_generic_get(const struct xattr_handler *handler,
return -EINVAL;
}
- if (!vi->xattr_isize)
- return -ENOATTR;
-
return erofs_getxattr(inode, handler->flags, name, buffer, size);
}
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Patch "staging: erofs: fix fast symlink w/o xattr when fs xattr is on" has been added to the 4.19-stable tree
2019-03-11 6:08 ` Gao Xiang
(?)
@ 2019-03-12 12:57 ` gregkh
-1 siblings, 0 replies; 17+ messages in thread
From: gregkh @ 2019-03-12 12:57 UTC (permalink / raw)
This is a note to let you know that I've just added the patch titled
staging: erofs: fix fast symlink w/o xattr when fs xattr is on
to the 4.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
staging-erofs-fix-fast-symlink-w-o-xattr-when-fs-xattr-is-on.patch
and it can be found in the queue-4.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable at vger.kernel.org> know about it.
>From foo at baz Tue Mar 12 05:46:41 PDT 2019
From: Gao Xiang <gaoxiang25@huawei.com>
Date: Mon, 11 Mar 2019 14:08:55 +0800
Subject: staging: erofs: fix fast symlink w/o xattr when fs xattr is on
To: <stable at vger.kernel.org>
Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>, LKML <linux-kernel at vger.kernel.org>, <linux-erofs at lists.ozlabs.org>, Chao Yu <yuchao0 at huawei.com>, Chao Yu <chao at kernel.org>, Miao Xie <miaoxie at huawei.com>, Fang Wei <fangwei1 at huawei.com>, Gao Xiang <gaoxiang25 at huawei.com>
Message-ID: <20190311060858.28654-2-gaoxiang25 at huawei.com>
From: Gao Xiang <gaoxiang25@huawei.com>
commit 7077fffcb0b0b65dc75e341306aeef4d0e7f2ec6 upstream.
Currently, this will hit a BUG_ON for these symlinks as follows:
- kernel message
------------[ cut here ]------------
kernel BUG at drivers/staging/erofs/xattr.c:59!
SMP PTI
CPU: 1 PID: 1170 Comm: getllxattr Not tainted 4.20.0-rc6+ #92
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
RIP: 0010:init_inode_xattrs+0x22b/0x270
Code: 48 0f 45 ea f0 ff 4d 34 74 0d 41 83 4c 24 e0 01 31 c0 e9 00 fe ff ff 48 89 ef e8 e0 31 9e ff eb e9 89 e8 e9 ef fd ff ff 0f 0$
<0f> 0b 48 89 ef e8 fb f6 9c ff 48 8b 45 08 a8 01 75 24 f0 ff 4d 34
RSP: 0018:ffffa03ac026bdf8 EFLAGS: 00010246
------------[ cut here ]------------
...
Call Trace:
erofs_listxattr+0x30/0x2c0
? selinux_inode_listxattr+0x5a/0x80
? kmem_cache_alloc+0x33/0x170
? security_inode_listxattr+0x27/0x40
listxattr+0xaf/0xc0
path_listxattr+0x5a/0xa0
do_syscall_64+0x43/0xf0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
...
---[ end trace 3c24b49408dc0c72 ]---
Fix it by checking ->xattr_isize in init_inode_xattrs(),
and it also fixes improper return value -ENOTSUPP
(it should be -ENODATA if xattr is enabled) for those inodes.
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable at vger.kernel.org> # 4.19+
Reported-by: Li Guifu <bluce.liguifu at huawei.com>
Tested-by: Li Guifu <bluce.liguifu at huawei.com>
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
---
drivers/staging/erofs/inode.c | 8 ++++----
drivers/staging/erofs/xattr.c | 25 ++++++++++++++++++++-----
2 files changed, 24 insertions(+), 9 deletions(-)
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -184,16 +184,16 @@ static int fill_inode(struct inode *inod
/* setup the new inode */
if (S_ISREG(inode->i_mode)) {
#ifdef CONFIG_EROFS_FS_XATTR
- if (vi->xattr_isize)
- inode->i_op = &erofs_generic_xattr_iops;
+ inode->i_op = &erofs_generic_xattr_iops;
#endif
inode->i_fop = &generic_ro_fops;
} else if (S_ISDIR(inode->i_mode)) {
inode->i_op =
#ifdef CONFIG_EROFS_FS_XATTR
- vi->xattr_isize ? &erofs_dir_xattr_iops :
-#endif
+ &erofs_dir_xattr_iops;
+#else
&erofs_dir_iops;
+#endif
inode->i_fop = &erofs_dir_fops;
} else if (S_ISLNK(inode->i_mode)) {
/* by default, page_get_link is used for symlink */
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -55,7 +55,26 @@ static int init_inode_xattrs(struct inod
return 0;
vi = EROFS_V(inode);
- BUG_ON(!vi->xattr_isize);
+
+ /*
+ * bypass all xattr operations if ->xattr_isize is not greater than
+ * sizeof(struct erofs_xattr_ibody_header), in detail:
+ * 1) it is not enough to contain erofs_xattr_ibody_header then
+ * ->xattr_isize should be 0 (it means no xattr);
+ * 2) it is just to contain erofs_xattr_ibody_header, which is on-disk
+ * undefined right now (maybe use later with some new sb feature).
+ */
+ if (vi->xattr_isize == sizeof(struct erofs_xattr_ibody_header)) {
+ errln("xattr_isize %d of nid %llu is not supported yet",
+ vi->xattr_isize, vi->nid);
+ return -ENOTSUPP;
+ } else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
+ if (unlikely(vi->xattr_isize)) {
+ DBG_BUGON(1);
+ return -EIO; /* xattr ondisk layout error */
+ }
+ return -ENOATTR;
+ }
sbi = EROFS_I_SB(inode);
it.blkaddr = erofs_blknr(iloc(sbi, vi->nid) + vi->inode_isize);
@@ -414,7 +433,6 @@ static int erofs_xattr_generic_get(const
struct dentry *unused, struct inode *inode,
const char *name, void *buffer, size_t size)
{
- struct erofs_vnode *const vi = EROFS_V(inode);
struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
switch (handler->flags) {
@@ -432,9 +450,6 @@ static int erofs_xattr_generic_get(const
return -EINVAL;
}
- if (!vi->xattr_isize)
- return -ENOATTR;
-
return erofs_getxattr(inode, handler->flags, name, buffer, size);
}
Patches currently in stable-queue which might be from gaoxiang25 at huawei.com are
queue-4.19/staging-erofs-fix-fast-symlink-w-o-xattr-when-fs-xattr-is-on.patch
queue-4.19/staging-erofs-fix-race-of-initializing-xattrs-of-a-inode-at-the-same-time.patch
queue-4.19/staging-erofs-add-error-handling-for-xattr-submodule.patch
queue-4.19/staging-erofs-keep-corrupted-fs-from-crashing-kernel-in-erofs_namei.patch
queue-4.19/staging-erofs-fix-memleak-of-inode-s-shared-xattr-array.patch
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4.19 3/5] staging: erofs: fix memleak of inode's shared xattr array
2019-03-11 6:08 ` Gao Xiang
@ 2019-03-11 6:08 ` Gao Xiang
-1 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, LKML, linux-erofs, Chao Yu, Chao Yu,
Miao Xie, Fang Wei, Sheng Yong, Gao Xiang
From: Sheng Yong <shengyong1@huawei.com>
commit 3b1b5291f79d040d549d7c746669fc30e8045b9b upstream.
If it fails to read a shared xattr page, the inode's shared xattr array
is not freed. The next time the inode's xattr is accessed, the previously
allocated array is leaked.
Signed-off-by: Sheng Yong <shengyong1@huawei.com>
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable@vger.kernel.org> # 4.19+
Reviewed-by: Gao Xiang <gaoxiang25@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
drivers/staging/erofs/xattr.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index ac2567576335..890fa06bbcbb 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -109,8 +109,11 @@ static int init_inode_xattrs(struct inode *inode)
it.page = erofs_get_meta_page(inode->i_sb,
++it.blkaddr, S_ISDIR(inode->i_mode));
- if (IS_ERR(it.page))
+ if (IS_ERR(it.page)) {
+ kfree(vi->xattr_shared_xattrs);
+ vi->xattr_shared_xattrs = NULL;
return PTR_ERR(it.page);
+ }
it.kaddr = kmap_atomic(it.page);
atomic_map = true;
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4.19 3/5] staging: erofs: fix memleak of inode's shared xattr array
@ 2019-03-11 6:08 ` Gao Xiang
0 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
From: Sheng Yong <shengyong1@huawei.com>
commit 3b1b5291f79d040d549d7c746669fc30e8045b9b upstream.
If it fails to read a shared xattr page, the inode's shared xattr array
is not freed. The next time the inode's xattr is accessed, the previously
allocated array is leaked.
Signed-off-by: Sheng Yong <shengyong1 at huawei.com>
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable at vger.kernel.org> # 4.19+
Reviewed-by: Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
drivers/staging/erofs/xattr.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index ac2567576335..890fa06bbcbb 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -109,8 +109,11 @@ static int init_inode_xattrs(struct inode *inode)
it.page = erofs_get_meta_page(inode->i_sb,
++it.blkaddr, S_ISDIR(inode->i_mode));
- if (IS_ERR(it.page))
+ if (IS_ERR(it.page)) {
+ kfree(vi->xattr_shared_xattrs);
+ vi->xattr_shared_xattrs = NULL;
return PTR_ERR(it.page);
+ }
it.kaddr = kmap_atomic(it.page);
atomic_map = true;
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Patch "staging: erofs: fix memleak of inode's shared xattr array" has been added to the 4.19-stable tree
2019-03-11 6:08 ` Gao Xiang
(?)
@ 2019-03-12 12:57 ` gregkh
-1 siblings, 0 replies; 17+ messages in thread
From: gregkh @ 2019-03-12 12:57 UTC (permalink / raw)
This is a note to let you know that I've just added the patch titled
staging: erofs: fix memleak of inode's shared xattr array
to the 4.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
staging-erofs-fix-memleak-of-inode-s-shared-xattr-array.patch
and it can be found in the queue-4.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable at vger.kernel.org> know about it.
>From foo at baz Tue Mar 12 05:46:41 PDT 2019
From: Gao Xiang <gaoxiang25@huawei.com>
Date: Mon, 11 Mar 2019 14:08:56 +0800
Subject: staging: erofs: fix memleak of inode's shared xattr array
To: <stable at vger.kernel.org>
Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>, LKML <linux-kernel at vger.kernel.org>, <linux-erofs at lists.ozlabs.org>, Chao Yu <yuchao0 at huawei.com>, Chao Yu <chao at kernel.org>, Miao Xie <miaoxie at huawei.com>, Fang Wei <fangwei1 at huawei.com>, Sheng Yong <shengyong1 at huawei.com>, Gao Xiang <gaoxiang25 at huawei.com>
Message-ID: <20190311060858.28654-3-gaoxiang25 at huawei.com>
From: Gao Xiang <gaoxiang25@huawei.com>
From: Sheng Yong <shengyong1@huawei.com>
commit 3b1b5291f79d040d549d7c746669fc30e8045b9b upstream.
If it fails to read a shared xattr page, the inode's shared xattr array
is not freed. The next time the inode's xattr is accessed, the previously
allocated array is leaked.
Signed-off-by: Sheng Yong <shengyong1 at huawei.com>
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable at vger.kernel.org> # 4.19+
Reviewed-by: Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
---
drivers/staging/erofs/xattr.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -109,8 +109,11 @@ static int init_inode_xattrs(struct inod
it.page = erofs_get_meta_page(inode->i_sb,
++it.blkaddr, S_ISDIR(inode->i_mode));
- if (IS_ERR(it.page))
+ if (IS_ERR(it.page)) {
+ kfree(vi->xattr_shared_xattrs);
+ vi->xattr_shared_xattrs = NULL;
return PTR_ERR(it.page);
+ }
it.kaddr = kmap_atomic(it.page);
atomic_map = true;
Patches currently in stable-queue which might be from gaoxiang25 at huawei.com are
queue-4.19/staging-erofs-fix-fast-symlink-w-o-xattr-when-fs-xattr-is-on.patch
queue-4.19/staging-erofs-fix-race-of-initializing-xattrs-of-a-inode-at-the-same-time.patch
queue-4.19/staging-erofs-add-error-handling-for-xattr-submodule.patch
queue-4.19/staging-erofs-keep-corrupted-fs-from-crashing-kernel-in-erofs_namei.patch
queue-4.19/staging-erofs-fix-memleak-of-inode-s-shared-xattr-array.patch
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4.19 4/5] staging: erofs: fix race of initializing xattrs of a inode at the same time
2019-03-11 6:08 ` Gao Xiang
@ 2019-03-11 6:08 ` Gao Xiang
-1 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, LKML, linux-erofs, Chao Yu, Chao Yu,
Miao Xie, Fang Wei, Gao Xiang
commit 62dc45979f3f8cb0ea67302a93bff686f0c46c5a upstream.
In real scenario, there could be several threads accessing xattrs
of the same xattr-uninitialized inode, and init_inode_xattrs()
almost at the same time.
That's actually an unexpected behavior, this patch closes the race.
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable@vger.kernel.org> # 4.19+
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
drivers/staging/erofs/internal.h | 11 ++++++++---
drivers/staging/erofs/xattr.c | 41 ++++++++++++++++++++++++++++------------
2 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 1c048c412fb2..c70f0c5237ea 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -327,12 +327,17 @@ static inline erofs_off_t iloc(struct erofs_sb_info *sbi, erofs_nid_t nid)
return blknr_to_addr(sbi->meta_blkaddr) + (nid << sbi->islotbits);
}
-#define inode_set_inited_xattr(inode) (EROFS_V(inode)->flags |= 1)
-#define inode_has_inited_xattr(inode) (EROFS_V(inode)->flags & 1)
+/* atomic flag definitions */
+#define EROFS_V_EA_INITED_BIT 0
+
+/* bitlock definitions (arranged in reverse order) */
+#define EROFS_V_BL_XATTR_BIT (BITS_PER_LONG - 1)
struct erofs_vnode {
erofs_nid_t nid;
- unsigned int flags;
+
+ /* atomic flags (including bitlocks) */
+ unsigned long flags;
unsigned char data_mapping_mode;
/* inline size in bytes */
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 890fa06bbcbb..955d024bf5a9 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -44,17 +44,24 @@ static inline void xattr_iter_end_final(struct xattr_iter *it)
static int init_inode_xattrs(struct inode *inode)
{
+ struct erofs_vnode *const vi = EROFS_V(inode);
struct xattr_iter it;
unsigned i;
struct erofs_xattr_ibody_header *ih;
struct erofs_sb_info *sbi;
- struct erofs_vnode *vi;
bool atomic_map;
+ int ret = 0;
- if (likely(inode_has_inited_xattr(inode)))
+ /* the most case is that xattrs of this inode are initialized. */
+ if (test_bit(EROFS_V_EA_INITED_BIT, &vi->flags))
return 0;
- vi = EROFS_V(inode);
+ if (wait_on_bit_lock(&vi->flags, EROFS_V_BL_XATTR_BIT, TASK_KILLABLE))
+ return -ERESTARTSYS;
+
+ /* someone has initialized xattrs for us? */
+ if (test_bit(EROFS_V_EA_INITED_BIT, &vi->flags))
+ goto out_unlock;
/*
* bypass all xattr operations if ->xattr_isize is not greater than
@@ -67,13 +74,16 @@ static int init_inode_xattrs(struct inode *inode)
if (vi->xattr_isize == sizeof(struct erofs_xattr_ibody_header)) {
errln("xattr_isize %d of nid %llu is not supported yet",
vi->xattr_isize, vi->nid);
- return -ENOTSUPP;
+ ret = -ENOTSUPP;
+ goto out_unlock;
} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
if (unlikely(vi->xattr_isize)) {
DBG_BUGON(1);
- return -EIO; /* xattr ondisk layout error */
+ ret = -EIO;
+ goto out_unlock; /* xattr ondisk layout error */
}
- return -ENOATTR;
+ ret = -ENOATTR;
+ goto out_unlock;
}
sbi = EROFS_I_SB(inode);
@@ -81,8 +91,10 @@ static int init_inode_xattrs(struct inode *inode)
it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
it.page = erofs_get_inline_page(inode, it.blkaddr);
- if (IS_ERR(it.page))
- return PTR_ERR(it.page);
+ if (IS_ERR(it.page)) {
+ ret = PTR_ERR(it.page);
+ goto out_unlock;
+ }
/* read in shared xattr array (non-atomic, see kmalloc below) */
it.kaddr = kmap(it.page);
@@ -95,7 +107,8 @@ static int init_inode_xattrs(struct inode *inode)
sizeof(uint), GFP_KERNEL);
if (vi->xattr_shared_xattrs == NULL) {
xattr_iter_end(&it, atomic_map);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_unlock;
}
/* let's skip ibody header */
@@ -112,7 +125,8 @@ static int init_inode_xattrs(struct inode *inode)
if (IS_ERR(it.page)) {
kfree(vi->xattr_shared_xattrs);
vi->xattr_shared_xattrs = NULL;
- return PTR_ERR(it.page);
+ ret = PTR_ERR(it.page);
+ goto out_unlock;
}
it.kaddr = kmap_atomic(it.page);
@@ -125,8 +139,11 @@ static int init_inode_xattrs(struct inode *inode)
}
xattr_iter_end(&it, atomic_map);
- inode_set_inited_xattr(inode);
- return 0;
+ set_bit(EROFS_V_EA_INITED_BIT, &vi->flags);
+
+out_unlock:
+ clear_and_wake_up_bit(EROFS_V_BL_XATTR_BIT, &vi->flags);
+ return ret;
}
struct xattr_iter_handlers {
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4.19 4/5] staging: erofs: fix race of initializing xattrs of a inode at the same time
@ 2019-03-11 6:08 ` Gao Xiang
0 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
commit 62dc45979f3f8cb0ea67302a93bff686f0c46c5a upstream.
In real scenario, there could be several threads accessing xattrs
of the same xattr-uninitialized inode, and init_inode_xattrs()
almost at the same time.
That's actually an unexpected behavior, this patch closes the race.
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable at vger.kernel.org> # 4.19+
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
drivers/staging/erofs/internal.h | 11 ++++++++---
drivers/staging/erofs/xattr.c | 41 ++++++++++++++++++++++++++++------------
2 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index 1c048c412fb2..c70f0c5237ea 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -327,12 +327,17 @@ static inline erofs_off_t iloc(struct erofs_sb_info *sbi, erofs_nid_t nid)
return blknr_to_addr(sbi->meta_blkaddr) + (nid << sbi->islotbits);
}
-#define inode_set_inited_xattr(inode) (EROFS_V(inode)->flags |= 1)
-#define inode_has_inited_xattr(inode) (EROFS_V(inode)->flags & 1)
+/* atomic flag definitions */
+#define EROFS_V_EA_INITED_BIT 0
+
+/* bitlock definitions (arranged in reverse order) */
+#define EROFS_V_BL_XATTR_BIT (BITS_PER_LONG - 1)
struct erofs_vnode {
erofs_nid_t nid;
- unsigned int flags;
+
+ /* atomic flags (including bitlocks) */
+ unsigned long flags;
unsigned char data_mapping_mode;
/* inline size in bytes */
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 890fa06bbcbb..955d024bf5a9 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -44,17 +44,24 @@ static inline void xattr_iter_end_final(struct xattr_iter *it)
static int init_inode_xattrs(struct inode *inode)
{
+ struct erofs_vnode *const vi = EROFS_V(inode);
struct xattr_iter it;
unsigned i;
struct erofs_xattr_ibody_header *ih;
struct erofs_sb_info *sbi;
- struct erofs_vnode *vi;
bool atomic_map;
+ int ret = 0;
- if (likely(inode_has_inited_xattr(inode)))
+ /* the most case is that xattrs of this inode are initialized. */
+ if (test_bit(EROFS_V_EA_INITED_BIT, &vi->flags))
return 0;
- vi = EROFS_V(inode);
+ if (wait_on_bit_lock(&vi->flags, EROFS_V_BL_XATTR_BIT, TASK_KILLABLE))
+ return -ERESTARTSYS;
+
+ /* someone has initialized xattrs for us? */
+ if (test_bit(EROFS_V_EA_INITED_BIT, &vi->flags))
+ goto out_unlock;
/*
* bypass all xattr operations if ->xattr_isize is not greater than
@@ -67,13 +74,16 @@ static int init_inode_xattrs(struct inode *inode)
if (vi->xattr_isize == sizeof(struct erofs_xattr_ibody_header)) {
errln("xattr_isize %d of nid %llu is not supported yet",
vi->xattr_isize, vi->nid);
- return -ENOTSUPP;
+ ret = -ENOTSUPP;
+ goto out_unlock;
} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
if (unlikely(vi->xattr_isize)) {
DBG_BUGON(1);
- return -EIO; /* xattr ondisk layout error */
+ ret = -EIO;
+ goto out_unlock; /* xattr ondisk layout error */
}
- return -ENOATTR;
+ ret = -ENOATTR;
+ goto out_unlock;
}
sbi = EROFS_I_SB(inode);
@@ -81,8 +91,10 @@ static int init_inode_xattrs(struct inode *inode)
it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
it.page = erofs_get_inline_page(inode, it.blkaddr);
- if (IS_ERR(it.page))
- return PTR_ERR(it.page);
+ if (IS_ERR(it.page)) {
+ ret = PTR_ERR(it.page);
+ goto out_unlock;
+ }
/* read in shared xattr array (non-atomic, see kmalloc below) */
it.kaddr = kmap(it.page);
@@ -95,7 +107,8 @@ static int init_inode_xattrs(struct inode *inode)
sizeof(uint), GFP_KERNEL);
if (vi->xattr_shared_xattrs == NULL) {
xattr_iter_end(&it, atomic_map);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_unlock;
}
/* let's skip ibody header */
@@ -112,7 +125,8 @@ static int init_inode_xattrs(struct inode *inode)
if (IS_ERR(it.page)) {
kfree(vi->xattr_shared_xattrs);
vi->xattr_shared_xattrs = NULL;
- return PTR_ERR(it.page);
+ ret = PTR_ERR(it.page);
+ goto out_unlock;
}
it.kaddr = kmap_atomic(it.page);
@@ -125,8 +139,11 @@ static int init_inode_xattrs(struct inode *inode)
}
xattr_iter_end(&it, atomic_map);
- inode_set_inited_xattr(inode);
- return 0;
+ set_bit(EROFS_V_EA_INITED_BIT, &vi->flags);
+
+out_unlock:
+ clear_and_wake_up_bit(EROFS_V_BL_XATTR_BIT, &vi->flags);
+ return ret;
}
struct xattr_iter_handlers {
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Patch "staging: erofs: fix race of initializing xattrs of a inode at the same time" has been added to the 4.19-stable tree
2019-03-11 6:08 ` Gao Xiang
(?)
@ 2019-03-12 12:57 ` gregkh
-1 siblings, 0 replies; 17+ messages in thread
From: gregkh @ 2019-03-12 12:57 UTC (permalink / raw)
This is a note to let you know that I've just added the patch titled
staging: erofs: fix race of initializing xattrs of a inode at the same time
to the 4.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
staging-erofs-fix-race-of-initializing-xattrs-of-a-inode-at-the-same-time.patch
and it can be found in the queue-4.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable at vger.kernel.org> know about it.
>From foo at baz Tue Mar 12 05:46:41 PDT 2019
From: Gao Xiang <gaoxiang25@huawei.com>
Date: Mon, 11 Mar 2019 14:08:57 +0800
Subject: staging: erofs: fix race of initializing xattrs of a inode at the same time
To: <stable at vger.kernel.org>
Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>, LKML <linux-kernel at vger.kernel.org>, <linux-erofs at lists.ozlabs.org>, Chao Yu <yuchao0 at huawei.com>, Chao Yu <chao at kernel.org>, Miao Xie <miaoxie at huawei.com>, Fang Wei <fangwei1 at huawei.com>, Gao Xiang <gaoxiang25 at huawei.com>
Message-ID: <20190311060858.28654-4-gaoxiang25 at huawei.com>
From: Gao Xiang <gaoxiang25@huawei.com>
commit 62dc45979f3f8cb0ea67302a93bff686f0c46c5a upstream.
In real scenario, there could be several threads accessing xattrs
of the same xattr-uninitialized inode, and init_inode_xattrs()
almost at the same time.
That's actually an unexpected behavior, this patch closes the race.
Fixes: b17500a0fdba ("staging: erofs: introduce xattr & acl support")
Cc: <stable at vger.kernel.org> # 4.19+
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
---
drivers/staging/erofs/internal.h | 11 +++++++---
drivers/staging/erofs/xattr.c | 41 +++++++++++++++++++++++++++------------
2 files changed, 37 insertions(+), 15 deletions(-)
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -327,12 +327,17 @@ static inline erofs_off_t iloc(struct er
return blknr_to_addr(sbi->meta_blkaddr) + (nid << sbi->islotbits);
}
-#define inode_set_inited_xattr(inode) (EROFS_V(inode)->flags |= 1)
-#define inode_has_inited_xattr(inode) (EROFS_V(inode)->flags & 1)
+/* atomic flag definitions */
+#define EROFS_V_EA_INITED_BIT 0
+
+/* bitlock definitions (arranged in reverse order) */
+#define EROFS_V_BL_XATTR_BIT (BITS_PER_LONG - 1)
struct erofs_vnode {
erofs_nid_t nid;
- unsigned int flags;
+
+ /* atomic flags (including bitlocks) */
+ unsigned long flags;
unsigned char data_mapping_mode;
/* inline size in bytes */
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -44,17 +44,24 @@ static inline void xattr_iter_end_final(
static int init_inode_xattrs(struct inode *inode)
{
+ struct erofs_vnode *const vi = EROFS_V(inode);
struct xattr_iter it;
unsigned i;
struct erofs_xattr_ibody_header *ih;
struct erofs_sb_info *sbi;
- struct erofs_vnode *vi;
bool atomic_map;
+ int ret = 0;
- if (likely(inode_has_inited_xattr(inode)))
+ /* the most case is that xattrs of this inode are initialized. */
+ if (test_bit(EROFS_V_EA_INITED_BIT, &vi->flags))
return 0;
- vi = EROFS_V(inode);
+ if (wait_on_bit_lock(&vi->flags, EROFS_V_BL_XATTR_BIT, TASK_KILLABLE))
+ return -ERESTARTSYS;
+
+ /* someone has initialized xattrs for us? */
+ if (test_bit(EROFS_V_EA_INITED_BIT, &vi->flags))
+ goto out_unlock;
/*
* bypass all xattr operations if ->xattr_isize is not greater than
@@ -67,13 +74,16 @@ static int init_inode_xattrs(struct inod
if (vi->xattr_isize == sizeof(struct erofs_xattr_ibody_header)) {
errln("xattr_isize %d of nid %llu is not supported yet",
vi->xattr_isize, vi->nid);
- return -ENOTSUPP;
+ ret = -ENOTSUPP;
+ goto out_unlock;
} else if (vi->xattr_isize < sizeof(struct erofs_xattr_ibody_header)) {
if (unlikely(vi->xattr_isize)) {
DBG_BUGON(1);
- return -EIO; /* xattr ondisk layout error */
+ ret = -EIO;
+ goto out_unlock; /* xattr ondisk layout error */
}
- return -ENOATTR;
+ ret = -ENOATTR;
+ goto out_unlock;
}
sbi = EROFS_I_SB(inode);
@@ -81,8 +91,10 @@ static int init_inode_xattrs(struct inod
it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
it.page = erofs_get_inline_page(inode, it.blkaddr);
- if (IS_ERR(it.page))
- return PTR_ERR(it.page);
+ if (IS_ERR(it.page)) {
+ ret = PTR_ERR(it.page);
+ goto out_unlock;
+ }
/* read in shared xattr array (non-atomic, see kmalloc below) */
it.kaddr = kmap(it.page);
@@ -95,7 +107,8 @@ static int init_inode_xattrs(struct inod
sizeof(uint), GFP_KERNEL);
if (!vi->xattr_shared_xattrs) {
xattr_iter_end(&it, atomic_map);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_unlock;
}
/* let's skip ibody header */
@@ -112,7 +125,8 @@ static int init_inode_xattrs(struct inod
if (IS_ERR(it.page)) {
kfree(vi->xattr_shared_xattrs);
vi->xattr_shared_xattrs = NULL;
- return PTR_ERR(it.page);
+ ret = PTR_ERR(it.page);
+ goto out_unlock;
}
it.kaddr = kmap_atomic(it.page);
@@ -125,8 +139,11 @@ static int init_inode_xattrs(struct inod
}
xattr_iter_end(&it, atomic_map);
- inode_set_inited_xattr(inode);
- return 0;
+ set_bit(EROFS_V_EA_INITED_BIT, &vi->flags);
+
+out_unlock:
+ clear_and_wake_up_bit(EROFS_V_BL_XATTR_BIT, &vi->flags);
+ return ret;
}
struct xattr_iter_handlers {
Patches currently in stable-queue which might be from gaoxiang25 at huawei.com are
queue-4.19/staging-erofs-fix-fast-symlink-w-o-xattr-when-fs-xattr-is-on.patch
queue-4.19/staging-erofs-fix-race-of-initializing-xattrs-of-a-inode-at-the-same-time.patch
queue-4.19/staging-erofs-add-error-handling-for-xattr-submodule.patch
queue-4.19/staging-erofs-keep-corrupted-fs-from-crashing-kernel-in-erofs_namei.patch
queue-4.19/staging-erofs-fix-memleak-of-inode-s-shared-xattr-array.patch
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4.19 5/5] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
2019-03-11 6:08 ` Gao Xiang
@ 2019-03-11 6:08 ` Gao Xiang
-1 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, LKML, linux-erofs, Chao Yu, Chao Yu,
Miao Xie, Fang Wei, Gao Xiang
commit 419d6efc50e94bcf5d6b35cd8c71f79edadec564 upstream.
As Al pointed out, "
... and while we are at it, what happens to
unsigned int nameoff = le16_to_cpu(de[mid].nameoff);
unsigned int matched = min(startprfx, endprfx);
struct qstr dname = QSTR_INIT(data + nameoff,
unlikely(mid >= ndirents - 1) ?
maxsize - nameoff :
le16_to_cpu(de[mid + 1].nameoff) - nameoff);
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
if le16_to_cpu(de[...].nameoff) is not monotonically increasing? I.e.
what's to prevent e.g. (unsigned)-1 ending up in dname.len?
Corrupted fs image shouldn't oops the kernel.. "
Revisit the related lookup flow to address the issue.
Fixes: d72d1ce60174 ("staging: erofs: add namei functions")
Cc: <stable@vger.kernel.org> # 4.19+
Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
drivers/staging/erofs/namei.c | 189 ++++++++++++++++++++++--------------------
1 file changed, 100 insertions(+), 89 deletions(-)
diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
index 546a47156101..023f64fa2c87 100644
--- a/drivers/staging/erofs/namei.c
+++ b/drivers/staging/erofs/namei.c
@@ -15,74 +15,77 @@
#include <trace/events/erofs.h>
-/* based on the value of qn->len is accurate */
-static inline int dirnamecmp(struct qstr *qn,
- struct qstr *qd, unsigned *matched)
+struct erofs_qstr {
+ const unsigned char *name;
+ const unsigned char *end;
+};
+
+/* based on the end of qn is accurate and it must have the trailing '\0' */
+static inline int dirnamecmp(const struct erofs_qstr *qn,
+ const struct erofs_qstr *qd,
+ unsigned int *matched)
{
- unsigned i = *matched, len = min(qn->len, qd->len);
-loop:
- if (unlikely(i >= len)) {
- *matched = i;
- if (qn->len < qd->len) {
- /*
- * actually (qn->len == qd->len)
- * when qd->name[i] == '\0'
- */
- return qd->name[i] == '\0' ? 0 : -1;
+ unsigned int i = *matched;
+
+ /*
+ * on-disk error, let's only BUG_ON in the debugging mode.
+ * otherwise, it will return 1 to just skip the invalid name
+ * and go on (in consideration of the lookup performance).
+ */
+ DBG_BUGON(qd->name > qd->end);
+
+ /* qd could not have trailing '\0' */
+ /* However it is absolutely safe if < qd->end */
+ while (qd->name + i < qd->end && qd->name[i] != '\0') {
+ if (qn->name[i] != qd->name[i]) {
+ *matched = i;
+ return qn->name[i] > qd->name[i] ? 1 : -1;
}
- return (qn->len > qd->len);
+ ++i;
}
-
- if (qn->name[i] != qd->name[i]) {
- *matched = i;
- return qn->name[i] > qd->name[i] ? 1 : -1;
- }
-
- ++i;
- goto loop;
+ *matched = i;
+ /* See comments in __d_alloc on the terminating NUL character */
+ return qn->name[i] == '\0' ? 0 : 1;
}
-static struct erofs_dirent *find_target_dirent(
- struct qstr *name,
- u8 *data, int maxsize)
+#define nameoff_from_disk(off, sz) (le16_to_cpu(off) & ((sz) - 1))
+
+static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
+ u8 *data,
+ unsigned int dirblksize,
+ const int ndirents)
{
- unsigned ndirents, head, back;
- unsigned startprfx, endprfx;
+ int head, back;
+ unsigned int startprfx, endprfx;
struct erofs_dirent *const de = (struct erofs_dirent *)data;
- /* make sure that maxsize is valid */
- BUG_ON(maxsize < sizeof(struct erofs_dirent));
-
- ndirents = le16_to_cpu(de->nameoff) / sizeof(*de);
-
- /* corrupted dir (may be unnecessary...) */
- BUG_ON(!ndirents);
-
- head = 0;
+ /* since the 1st dirent has been evaluated previously */
+ head = 1;
back = ndirents - 1;
startprfx = endprfx = 0;
while (head <= back) {
- unsigned mid = head + (back - head) / 2;
- unsigned nameoff = le16_to_cpu(de[mid].nameoff);
- unsigned matched = min(startprfx, endprfx);
-
- struct qstr dname = QSTR_INIT(data + nameoff,
- unlikely(mid >= ndirents - 1) ?
- maxsize - nameoff :
- le16_to_cpu(de[mid + 1].nameoff) - nameoff);
+ const int mid = head + (back - head) / 2;
+ const int nameoff = nameoff_from_disk(de[mid].nameoff,
+ dirblksize);
+ unsigned int matched = min(startprfx, endprfx);
+ struct erofs_qstr dname = {
+ .name = data + nameoff,
+ .end = unlikely(mid >= ndirents - 1) ?
+ data + dirblksize :
+ data + nameoff_from_disk(de[mid + 1].nameoff,
+ dirblksize)
+ };
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
- if (unlikely(!ret))
+ if (unlikely(!ret)) {
return de + mid;
- else if (ret > 0) {
+ } else if (ret > 0) {
head = mid + 1;
startprfx = matched;
- } else if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
- else {
+ } else {
back = mid - 1;
endprfx = matched;
}
@@ -91,12 +94,12 @@ static struct erofs_dirent *find_target_dirent(
return ERR_PTR(-ENOENT);
}
-static struct page *find_target_block_classic(
- struct inode *dir,
- struct qstr *name, int *_diff)
+static struct page *find_target_block_classic(struct inode *dir,
+ struct erofs_qstr *name,
+ int *_ndirents)
{
- unsigned startprfx, endprfx;
- unsigned head, back;
+ unsigned int startprfx, endprfx;
+ int head, back;
struct address_space *const mapping = dir->i_mapping;
struct page *candidate = ERR_PTR(-ENOENT);
@@ -105,41 +108,43 @@ static struct page *find_target_block_classic(
back = inode_datablocks(dir) - 1;
while (head <= back) {
- unsigned mid = head + (back - head) / 2;
+ const int mid = head + (back - head) / 2;
struct page *page = read_mapping_page(mapping, mid, NULL);
- if (IS_ERR(page)) {
-exact_out:
- if (!IS_ERR(candidate)) /* valid candidate */
- put_page(candidate);
- return page;
- } else {
- int diff;
- unsigned ndirents, matched;
- struct qstr dname;
+ if (!IS_ERR(page)) {
struct erofs_dirent *de = kmap_atomic(page);
- unsigned nameoff = le16_to_cpu(de->nameoff);
-
- ndirents = nameoff / sizeof(*de);
+ const int nameoff = nameoff_from_disk(de->nameoff,
+ EROFS_BLKSIZ);
+ const int ndirents = nameoff / sizeof(*de);
+ int diff;
+ unsigned int matched;
+ struct erofs_qstr dname;
- /* corrupted dir (should have one entry at least) */
- BUG_ON(!ndirents || nameoff > PAGE_SIZE);
+ if (unlikely(!ndirents)) {
+ DBG_BUGON(1);
+ kunmap_atomic(de);
+ put_page(page);
+ page = ERR_PTR(-EIO);
+ goto out;
+ }
matched = min(startprfx, endprfx);
dname.name = (u8 *)de + nameoff;
- dname.len = ndirents == 1 ?
- /* since the rest of the last page is 0 */
- EROFS_BLKSIZ - nameoff
- : le16_to_cpu(de[1].nameoff) - nameoff;
+ if (ndirents == 1)
+ dname.end = (u8 *)de + EROFS_BLKSIZ;
+ else
+ dname.end = (u8 *)de +
+ nameoff_from_disk(de[1].nameoff,
+ EROFS_BLKSIZ);
/* string comparison without already matched prefix */
diff = dirnamecmp(name, &dname, &matched);
kunmap_atomic(de);
if (unlikely(!diff)) {
- *_diff = 0;
- goto exact_out;
+ *_ndirents = 0;
+ goto out;
} else if (diff > 0) {
head = mid + 1;
startprfx = matched;
@@ -147,45 +152,51 @@ static struct page *find_target_block_classic(
if (likely(!IS_ERR(candidate)))
put_page(candidate);
candidate = page;
+ *_ndirents = ndirents;
} else {
put_page(page);
- if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
-
back = mid - 1;
endprfx = matched;
}
+ continue;
}
+out: /* free if the candidate is valid */
+ if (!IS_ERR(candidate))
+ put_page(candidate);
+ return page;
}
- *_diff = 1;
return candidate;
}
int erofs_namei(struct inode *dir,
- struct qstr *name,
- erofs_nid_t *nid, unsigned *d_type)
+ struct qstr *name,
+ erofs_nid_t *nid, unsigned int *d_type)
{
- int diff;
+ int ndirents;
struct page *page;
- u8 *data;
+ void *data;
struct erofs_dirent *de;
+ struct erofs_qstr qn;
if (unlikely(!dir->i_size))
return -ENOENT;
- diff = 1;
- page = find_target_block_classic(dir, name, &diff);
+ qn.name = name->name;
+ qn.end = name->name + name->len;
+
+ ndirents = 0;
+ page = find_target_block_classic(dir, &qn, &ndirents);
if (unlikely(IS_ERR(page)))
return PTR_ERR(page);
data = kmap_atomic(page);
/* the target page has been mapped */
- de = likely(diff) ?
- /* since the rest of the last page is 0 */
- find_target_dirent(name, data, EROFS_BLKSIZ) :
- (struct erofs_dirent *)data;
+ if (ndirents)
+ de = find_target_dirent(&qn, data, EROFS_BLKSIZ, ndirents);
+ else
+ de = (struct erofs_dirent *)data;
if (likely(!IS_ERR(de))) {
*nid = le64_to_cpu(de->nid);
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4.19 5/5] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
@ 2019-03-11 6:08 ` Gao Xiang
0 siblings, 0 replies; 17+ messages in thread
From: Gao Xiang @ 2019-03-11 6:08 UTC (permalink / raw)
commit 419d6efc50e94bcf5d6b35cd8c71f79edadec564 upstream.
As Al pointed out, "
... and while we are at it, what happens to
unsigned int nameoff = le16_to_cpu(de[mid].nameoff);
unsigned int matched = min(startprfx, endprfx);
struct qstr dname = QSTR_INIT(data + nameoff,
unlikely(mid >= ndirents - 1) ?
maxsize - nameoff :
le16_to_cpu(de[mid + 1].nameoff) - nameoff);
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
if le16_to_cpu(de[...].nameoff) is not monotonically increasing? I.e.
what's to prevent e.g. (unsigned)-1 ending up in dname.len?
Corrupted fs image shouldn't oops the kernel.. "
Revisit the related lookup flow to address the issue.
Fixes: d72d1ce60174 ("staging: erofs: add namei functions")
Cc: <stable at vger.kernel.org> # 4.19+
Suggested-by: Al Viro <viro at ZenIV.linux.org.uk>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
---
drivers/staging/erofs/namei.c | 189 ++++++++++++++++++++++--------------------
1 file changed, 100 insertions(+), 89 deletions(-)
diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
index 546a47156101..023f64fa2c87 100644
--- a/drivers/staging/erofs/namei.c
+++ b/drivers/staging/erofs/namei.c
@@ -15,74 +15,77 @@
#include <trace/events/erofs.h>
-/* based on the value of qn->len is accurate */
-static inline int dirnamecmp(struct qstr *qn,
- struct qstr *qd, unsigned *matched)
+struct erofs_qstr {
+ const unsigned char *name;
+ const unsigned char *end;
+};
+
+/* based on the end of qn is accurate and it must have the trailing '\0' */
+static inline int dirnamecmp(const struct erofs_qstr *qn,
+ const struct erofs_qstr *qd,
+ unsigned int *matched)
{
- unsigned i = *matched, len = min(qn->len, qd->len);
-loop:
- if (unlikely(i >= len)) {
- *matched = i;
- if (qn->len < qd->len) {
- /*
- * actually (qn->len == qd->len)
- * when qd->name[i] == '\0'
- */
- return qd->name[i] == '\0' ? 0 : -1;
+ unsigned int i = *matched;
+
+ /*
+ * on-disk error, let's only BUG_ON in the debugging mode.
+ * otherwise, it will return 1 to just skip the invalid name
+ * and go on (in consideration of the lookup performance).
+ */
+ DBG_BUGON(qd->name > qd->end);
+
+ /* qd could not have trailing '\0' */
+ /* However it is absolutely safe if < qd->end */
+ while (qd->name + i < qd->end && qd->name[i] != '\0') {
+ if (qn->name[i] != qd->name[i]) {
+ *matched = i;
+ return qn->name[i] > qd->name[i] ? 1 : -1;
}
- return (qn->len > qd->len);
+ ++i;
}
-
- if (qn->name[i] != qd->name[i]) {
- *matched = i;
- return qn->name[i] > qd->name[i] ? 1 : -1;
- }
-
- ++i;
- goto loop;
+ *matched = i;
+ /* See comments in __d_alloc on the terminating NUL character */
+ return qn->name[i] == '\0' ? 0 : 1;
}
-static struct erofs_dirent *find_target_dirent(
- struct qstr *name,
- u8 *data, int maxsize)
+#define nameoff_from_disk(off, sz) (le16_to_cpu(off) & ((sz) - 1))
+
+static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
+ u8 *data,
+ unsigned int dirblksize,
+ const int ndirents)
{
- unsigned ndirents, head, back;
- unsigned startprfx, endprfx;
+ int head, back;
+ unsigned int startprfx, endprfx;
struct erofs_dirent *const de = (struct erofs_dirent *)data;
- /* make sure that maxsize is valid */
- BUG_ON(maxsize < sizeof(struct erofs_dirent));
-
- ndirents = le16_to_cpu(de->nameoff) / sizeof(*de);
-
- /* corrupted dir (may be unnecessary...) */
- BUG_ON(!ndirents);
-
- head = 0;
+ /* since the 1st dirent has been evaluated previously */
+ head = 1;
back = ndirents - 1;
startprfx = endprfx = 0;
while (head <= back) {
- unsigned mid = head + (back - head) / 2;
- unsigned nameoff = le16_to_cpu(de[mid].nameoff);
- unsigned matched = min(startprfx, endprfx);
-
- struct qstr dname = QSTR_INIT(data + nameoff,
- unlikely(mid >= ndirents - 1) ?
- maxsize - nameoff :
- le16_to_cpu(de[mid + 1].nameoff) - nameoff);
+ const int mid = head + (back - head) / 2;
+ const int nameoff = nameoff_from_disk(de[mid].nameoff,
+ dirblksize);
+ unsigned int matched = min(startprfx, endprfx);
+ struct erofs_qstr dname = {
+ .name = data + nameoff,
+ .end = unlikely(mid >= ndirents - 1) ?
+ data + dirblksize :
+ data + nameoff_from_disk(de[mid + 1].nameoff,
+ dirblksize)
+ };
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
- if (unlikely(!ret))
+ if (unlikely(!ret)) {
return de + mid;
- else if (ret > 0) {
+ } else if (ret > 0) {
head = mid + 1;
startprfx = matched;
- } else if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
- else {
+ } else {
back = mid - 1;
endprfx = matched;
}
@@ -91,12 +94,12 @@ static struct erofs_dirent *find_target_dirent(
return ERR_PTR(-ENOENT);
}
-static struct page *find_target_block_classic(
- struct inode *dir,
- struct qstr *name, int *_diff)
+static struct page *find_target_block_classic(struct inode *dir,
+ struct erofs_qstr *name,
+ int *_ndirents)
{
- unsigned startprfx, endprfx;
- unsigned head, back;
+ unsigned int startprfx, endprfx;
+ int head, back;
struct address_space *const mapping = dir->i_mapping;
struct page *candidate = ERR_PTR(-ENOENT);
@@ -105,41 +108,43 @@ static struct page *find_target_block_classic(
back = inode_datablocks(dir) - 1;
while (head <= back) {
- unsigned mid = head + (back - head) / 2;
+ const int mid = head + (back - head) / 2;
struct page *page = read_mapping_page(mapping, mid, NULL);
- if (IS_ERR(page)) {
-exact_out:
- if (!IS_ERR(candidate)) /* valid candidate */
- put_page(candidate);
- return page;
- } else {
- int diff;
- unsigned ndirents, matched;
- struct qstr dname;
+ if (!IS_ERR(page)) {
struct erofs_dirent *de = kmap_atomic(page);
- unsigned nameoff = le16_to_cpu(de->nameoff);
-
- ndirents = nameoff / sizeof(*de);
+ const int nameoff = nameoff_from_disk(de->nameoff,
+ EROFS_BLKSIZ);
+ const int ndirents = nameoff / sizeof(*de);
+ int diff;
+ unsigned int matched;
+ struct erofs_qstr dname;
- /* corrupted dir (should have one entry at least) */
- BUG_ON(!ndirents || nameoff > PAGE_SIZE);
+ if (unlikely(!ndirents)) {
+ DBG_BUGON(1);
+ kunmap_atomic(de);
+ put_page(page);
+ page = ERR_PTR(-EIO);
+ goto out;
+ }
matched = min(startprfx, endprfx);
dname.name = (u8 *)de + nameoff;
- dname.len = ndirents == 1 ?
- /* since the rest of the last page is 0 */
- EROFS_BLKSIZ - nameoff
- : le16_to_cpu(de[1].nameoff) - nameoff;
+ if (ndirents == 1)
+ dname.end = (u8 *)de + EROFS_BLKSIZ;
+ else
+ dname.end = (u8 *)de +
+ nameoff_from_disk(de[1].nameoff,
+ EROFS_BLKSIZ);
/* string comparison without already matched prefix */
diff = dirnamecmp(name, &dname, &matched);
kunmap_atomic(de);
if (unlikely(!diff)) {
- *_diff = 0;
- goto exact_out;
+ *_ndirents = 0;
+ goto out;
} else if (diff > 0) {
head = mid + 1;
startprfx = matched;
@@ -147,45 +152,51 @@ static struct page *find_target_block_classic(
if (likely(!IS_ERR(candidate)))
put_page(candidate);
candidate = page;
+ *_ndirents = ndirents;
} else {
put_page(page);
- if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
-
back = mid - 1;
endprfx = matched;
}
+ continue;
}
+out: /* free if the candidate is valid */
+ if (!IS_ERR(candidate))
+ put_page(candidate);
+ return page;
}
- *_diff = 1;
return candidate;
}
int erofs_namei(struct inode *dir,
- struct qstr *name,
- erofs_nid_t *nid, unsigned *d_type)
+ struct qstr *name,
+ erofs_nid_t *nid, unsigned int *d_type)
{
- int diff;
+ int ndirents;
struct page *page;
- u8 *data;
+ void *data;
struct erofs_dirent *de;
+ struct erofs_qstr qn;
if (unlikely(!dir->i_size))
return -ENOENT;
- diff = 1;
- page = find_target_block_classic(dir, name, &diff);
+ qn.name = name->name;
+ qn.end = name->name + name->len;
+
+ ndirents = 0;
+ page = find_target_block_classic(dir, &qn, &ndirents);
if (unlikely(IS_ERR(page)))
return PTR_ERR(page);
data = kmap_atomic(page);
/* the target page has been mapped */
- de = likely(diff) ?
- /* since the rest of the last page is 0 */
- find_target_dirent(name, data, EROFS_BLKSIZ) :
- (struct erofs_dirent *)data;
+ if (ndirents)
+ de = find_target_dirent(&qn, data, EROFS_BLKSIZ, ndirents);
+ else
+ de = (struct erofs_dirent *)data;
if (likely(!IS_ERR(de))) {
*nid = le64_to_cpu(de->nid);
--
2.14.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Patch "staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()" has been added to the 4.19-stable tree
2019-03-11 6:08 ` Gao Xiang
(?)
@ 2019-03-12 12:57 ` gregkh
-1 siblings, 0 replies; 17+ messages in thread
From: gregkh @ 2019-03-12 12:57 UTC (permalink / raw)
This is a note to let you know that I've just added the patch titled
staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
to the 4.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
staging-erofs-keep-corrupted-fs-from-crashing-kernel-in-erofs_namei.patch
and it can be found in the queue-4.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable at vger.kernel.org> know about it.
>From foo at baz Tue Mar 12 05:46:41 PDT 2019
From: Gao Xiang <gaoxiang25@huawei.com>
Date: Mon, 11 Mar 2019 14:08:58 +0800
Subject: staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
To: <stable at vger.kernel.org>
Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>, LKML <linux-kernel at vger.kernel.org>, <linux-erofs at lists.ozlabs.org>, Chao Yu <yuchao0 at huawei.com>, Chao Yu <chao at kernel.org>, Miao Xie <miaoxie at huawei.com>, Fang Wei <fangwei1 at huawei.com>, Gao Xiang <gaoxiang25 at huawei.com>
Message-ID: <20190311060858.28654-5-gaoxiang25 at huawei.com>
From: Gao Xiang <gaoxiang25@huawei.com>
commit 419d6efc50e94bcf5d6b35cd8c71f79edadec564 upstream.
As Al pointed out, "
... and while we are at it, what happens to
unsigned int nameoff = le16_to_cpu(de[mid].nameoff);
unsigned int matched = min(startprfx, endprfx);
struct qstr dname = QSTR_INIT(data + nameoff,
unlikely(mid >= ndirents - 1) ?
maxsize - nameoff :
le16_to_cpu(de[mid + 1].nameoff) - nameoff);
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
if le16_to_cpu(de[...].nameoff) is not monotonically increasing? I.e.
what's to prevent e.g. (unsigned)-1 ending up in dname.len?
Corrupted fs image shouldn't oops the kernel.. "
Revisit the related lookup flow to address the issue.
Fixes: d72d1ce60174 ("staging: erofs: add namei functions")
Cc: <stable at vger.kernel.org> # 4.19+
Suggested-by: Al Viro <viro at ZenIV.linux.org.uk>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
---
drivers/staging/erofs/namei.c | 189 ++++++++++++++++++++++--------------------
1 file changed, 100 insertions(+), 89 deletions(-)
--- a/drivers/staging/erofs/namei.c
+++ b/drivers/staging/erofs/namei.c
@@ -15,74 +15,77 @@
#include <trace/events/erofs.h>
-/* based on the value of qn->len is accurate */
-static inline int dirnamecmp(struct qstr *qn,
- struct qstr *qd, unsigned *matched)
+struct erofs_qstr {
+ const unsigned char *name;
+ const unsigned char *end;
+};
+
+/* based on the end of qn is accurate and it must have the trailing '\0' */
+static inline int dirnamecmp(const struct erofs_qstr *qn,
+ const struct erofs_qstr *qd,
+ unsigned int *matched)
{
- unsigned i = *matched, len = min(qn->len, qd->len);
-loop:
- if (unlikely(i >= len)) {
- *matched = i;
- if (qn->len < qd->len) {
- /*
- * actually (qn->len == qd->len)
- * when qd->name[i] == '\0'
- */
- return qd->name[i] == '\0' ? 0 : -1;
- }
- return (qn->len > qd->len);
- }
+ unsigned int i = *matched;
- if (qn->name[i] != qd->name[i]) {
- *matched = i;
- return qn->name[i] > qd->name[i] ? 1 : -1;
+ /*
+ * on-disk error, let's only BUG_ON in the debugging mode.
+ * otherwise, it will return 1 to just skip the invalid name
+ * and go on (in consideration of the lookup performance).
+ */
+ DBG_BUGON(qd->name > qd->end);
+
+ /* qd could not have trailing '\0' */
+ /* However it is absolutely safe if < qd->end */
+ while (qd->name + i < qd->end && qd->name[i] != '\0') {
+ if (qn->name[i] != qd->name[i]) {
+ *matched = i;
+ return qn->name[i] > qd->name[i] ? 1 : -1;
+ }
+ ++i;
}
-
- ++i;
- goto loop;
+ *matched = i;
+ /* See comments in __d_alloc on the terminating NUL character */
+ return qn->name[i] == '\0' ? 0 : 1;
}
-static struct erofs_dirent *find_target_dirent(
- struct qstr *name,
- u8 *data, int maxsize)
+#define nameoff_from_disk(off, sz) (le16_to_cpu(off) & ((sz) - 1))
+
+static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
+ u8 *data,
+ unsigned int dirblksize,
+ const int ndirents)
{
- unsigned ndirents, head, back;
- unsigned startprfx, endprfx;
+ int head, back;
+ unsigned int startprfx, endprfx;
struct erofs_dirent *const de = (struct erofs_dirent *)data;
- /* make sure that maxsize is valid */
- BUG_ON(maxsize < sizeof(struct erofs_dirent));
-
- ndirents = le16_to_cpu(de->nameoff) / sizeof(*de);
-
- /* corrupted dir (may be unnecessary...) */
- BUG_ON(!ndirents);
-
- head = 0;
+ /* since the 1st dirent has been evaluated previously */
+ head = 1;
back = ndirents - 1;
startprfx = endprfx = 0;
while (head <= back) {
- unsigned mid = head + (back - head) / 2;
- unsigned nameoff = le16_to_cpu(de[mid].nameoff);
- unsigned matched = min(startprfx, endprfx);
-
- struct qstr dname = QSTR_INIT(data + nameoff,
- unlikely(mid >= ndirents - 1) ?
- maxsize - nameoff :
- le16_to_cpu(de[mid + 1].nameoff) - nameoff);
+ const int mid = head + (back - head) / 2;
+ const int nameoff = nameoff_from_disk(de[mid].nameoff,
+ dirblksize);
+ unsigned int matched = min(startprfx, endprfx);
+ struct erofs_qstr dname = {
+ .name = data + nameoff,
+ .end = unlikely(mid >= ndirents - 1) ?
+ data + dirblksize :
+ data + nameoff_from_disk(de[mid + 1].nameoff,
+ dirblksize)
+ };
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
- if (unlikely(!ret))
+ if (unlikely(!ret)) {
return de + mid;
- else if (ret > 0) {
+ } else if (ret > 0) {
head = mid + 1;
startprfx = matched;
- } else if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
- else {
+ } else {
back = mid - 1;
endprfx = matched;
}
@@ -91,12 +94,12 @@ static struct erofs_dirent *find_target_
return ERR_PTR(-ENOENT);
}
-static struct page *find_target_block_classic(
- struct inode *dir,
- struct qstr *name, int *_diff)
+static struct page *find_target_block_classic(struct inode *dir,
+ struct erofs_qstr *name,
+ int *_ndirents)
{
- unsigned startprfx, endprfx;
- unsigned head, back;
+ unsigned int startprfx, endprfx;
+ int head, back;
struct address_space *const mapping = dir->i_mapping;
struct page *candidate = ERR_PTR(-ENOENT);
@@ -105,41 +108,43 @@ static struct page *find_target_block_cl
back = inode_datablocks(dir) - 1;
while (head <= back) {
- unsigned mid = head + (back - head) / 2;
+ const int mid = head + (back - head) / 2;
struct page *page = read_mapping_page(mapping, mid, NULL);
- if (IS_ERR(page)) {
-exact_out:
- if (!IS_ERR(candidate)) /* valid candidate */
- put_page(candidate);
- return page;
- } else {
- int diff;
- unsigned ndirents, matched;
- struct qstr dname;
+ if (!IS_ERR(page)) {
struct erofs_dirent *de = kmap_atomic(page);
- unsigned nameoff = le16_to_cpu(de->nameoff);
-
- ndirents = nameoff / sizeof(*de);
+ const int nameoff = nameoff_from_disk(de->nameoff,
+ EROFS_BLKSIZ);
+ const int ndirents = nameoff / sizeof(*de);
+ int diff;
+ unsigned int matched;
+ struct erofs_qstr dname;
- /* corrupted dir (should have one entry at least) */
- BUG_ON(!ndirents || nameoff > PAGE_SIZE);
+ if (unlikely(!ndirents)) {
+ DBG_BUGON(1);
+ kunmap_atomic(de);
+ put_page(page);
+ page = ERR_PTR(-EIO);
+ goto out;
+ }
matched = min(startprfx, endprfx);
dname.name = (u8 *)de + nameoff;
- dname.len = ndirents == 1 ?
- /* since the rest of the last page is 0 */
- EROFS_BLKSIZ - nameoff
- : le16_to_cpu(de[1].nameoff) - nameoff;
+ if (ndirents == 1)
+ dname.end = (u8 *)de + EROFS_BLKSIZ;
+ else
+ dname.end = (u8 *)de +
+ nameoff_from_disk(de[1].nameoff,
+ EROFS_BLKSIZ);
/* string comparison without already matched prefix */
diff = dirnamecmp(name, &dname, &matched);
kunmap_atomic(de);
if (unlikely(!diff)) {
- *_diff = 0;
- goto exact_out;
+ *_ndirents = 0;
+ goto out;
} else if (diff > 0) {
head = mid + 1;
startprfx = matched;
@@ -147,45 +152,51 @@ exact_out:
if (likely(!IS_ERR(candidate)))
put_page(candidate);
candidate = page;
+ *_ndirents = ndirents;
} else {
put_page(page);
- if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
-
back = mid - 1;
endprfx = matched;
}
+ continue;
}
+out: /* free if the candidate is valid */
+ if (!IS_ERR(candidate))
+ put_page(candidate);
+ return page;
}
- *_diff = 1;
return candidate;
}
int erofs_namei(struct inode *dir,
- struct qstr *name,
- erofs_nid_t *nid, unsigned *d_type)
+ struct qstr *name,
+ erofs_nid_t *nid, unsigned int *d_type)
{
- int diff;
+ int ndirents;
struct page *page;
- u8 *data;
+ void *data;
struct erofs_dirent *de;
+ struct erofs_qstr qn;
if (unlikely(!dir->i_size))
return -ENOENT;
- diff = 1;
- page = find_target_block_classic(dir, name, &diff);
+ qn.name = name->name;
+ qn.end = name->name + name->len;
+
+ ndirents = 0;
+ page = find_target_block_classic(dir, &qn, &ndirents);
if (unlikely(IS_ERR(page)))
return PTR_ERR(page);
data = kmap_atomic(page);
/* the target page has been mapped */
- de = likely(diff) ?
- /* since the rest of the last page is 0 */
- find_target_dirent(name, data, EROFS_BLKSIZ) :
- (struct erofs_dirent *)data;
+ if (ndirents)
+ de = find_target_dirent(&qn, data, EROFS_BLKSIZ, ndirents);
+ else
+ de = (struct erofs_dirent *)data;
if (likely(!IS_ERR(de))) {
*nid = le64_to_cpu(de->nid);
Patches currently in stable-queue which might be from gaoxiang25 at huawei.com are
queue-4.19/staging-erofs-fix-fast-symlink-w-o-xattr-when-fs-xattr-is-on.patch
queue-4.19/staging-erofs-fix-race-of-initializing-xattrs-of-a-inode-at-the-same-time.patch
queue-4.19/staging-erofs-add-error-handling-for-xattr-submodule.patch
queue-4.19/staging-erofs-keep-corrupted-fs-from-crashing-kernel-in-erofs_namei.patch
queue-4.19/staging-erofs-fix-memleak-of-inode-s-shared-xattr-array.patch
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4.19 1/5] staging: erofs: add error handling for xattr submodule
2019-03-11 6:08 ` Gao Xiang
@ 2019-03-12 12:48 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-12 12:48 UTC (permalink / raw)
To: Gao Xiang; +Cc: stable, LKML, linux-erofs, Chao Yu, Chao Yu, Miao Xie, Fang Wei
On Mon, Mar 11, 2019 at 02:08:54PM +0800, Gao Xiang wrote:
> commit cadf1ccf1b0021d0b7a9347e102ac5258f9f98c8 upstream.
>
> This patch enhances the missing error handling code for
> xattr submodule, which improves the stability for the rare cases.
>
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>
> This series resolves the following conflicts:
> FAILED: patch "[PATCH] staging: erofs: fix fast symlink w/o xattr when fs xattr is" failed to apply to 4.19-stable tree
> FAILED: patch "[PATCH] staging: erofs: fix memleak of inode's shared xattr array" failed to apply to 4.19-stable tree
> FAILED: patch "[PATCH] staging: erofs: fix race of initializing xattrs of a inode at" failed to apply to 4.19-stable tree
> FAILED: patch "[PATCH] staging: erofs: keep corrupted fs from crashing kernel in" failed to apply to 4.19-stable tree
>
> In addition, there is another patch called
> staging: erofs: add error handling for xattr submodule
> that needs to be backported to 4.19 as well (4.20+ already has this patch.)
>
> drivers/staging/erofs/internal.h | 5 +-
> drivers/staging/erofs/xattr.c | 112 +++++++++++++++++++++++++++------------
> 2 files changed, 81 insertions(+), 36 deletions(-)
All now queued up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4.19 1/5] staging: erofs: add error handling for xattr submodule
@ 2019-03-12 12:48 ` Greg Kroah-Hartman
0 siblings, 0 replies; 17+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-12 12:48 UTC (permalink / raw)
On Mon, Mar 11, 2019@02:08:54PM +0800, Gao Xiang wrote:
> commit cadf1ccf1b0021d0b7a9347e102ac5258f9f98c8 upstream.
>
> This patch enhances the missing error handling code for
> xattr submodule, which improves the stability for the rare cases.
>
> Reviewed-by: Chao Yu <yuchao0 at huawei.com>
> Signed-off-by: Chao Yu <yuchao0 at huawei.com>
> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
> ---
>
> This series resolves the following conflicts:
> FAILED: patch "[PATCH] staging: erofs: fix fast symlink w/o xattr when fs xattr is" failed to apply to 4.19-stable tree
> FAILED: patch "[PATCH] staging: erofs: fix memleak of inode's shared xattr array" failed to apply to 4.19-stable tree
> FAILED: patch "[PATCH] staging: erofs: fix race of initializing xattrs of a inode at" failed to apply to 4.19-stable tree
> FAILED: patch "[PATCH] staging: erofs: keep corrupted fs from crashing kernel in" failed to apply to 4.19-stable tree
>
> In addition, there is another patch called
> staging: erofs: add error handling for xattr submodule
> that needs to be backported to 4.19 as well (4.20+ already has this patch.)
>
> drivers/staging/erofs/internal.h | 5 +-
> drivers/staging/erofs/xattr.c | 112 +++++++++++++++++++++++++++------------
> 2 files changed, 81 insertions(+), 36 deletions(-)
All now queued up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 17+ messages in thread
* Patch "staging: erofs: add error handling for xattr submodule" has been added to the 4.19-stable tree
2019-03-11 6:08 ` Gao Xiang
` (5 preceding siblings ...)
(?)
@ 2019-03-12 12:57 ` gregkh
-1 siblings, 0 replies; 17+ messages in thread
From: gregkh @ 2019-03-12 12:57 UTC (permalink / raw)
This is a note to let you know that I've just added the patch titled
staging: erofs: add error handling for xattr submodule
to the 4.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
staging-erofs-add-error-handling-for-xattr-submodule.patch
and it can be found in the queue-4.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable at vger.kernel.org> know about it.
>From foo at baz Tue Mar 12 05:46:41 PDT 2019
From: Gao Xiang <gaoxiang25@huawei.com>
Date: Mon, 11 Mar 2019 14:08:54 +0800
Subject: staging: erofs: add error handling for xattr submodule
To: <stable at vger.kernel.org>
Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>, LKML <linux-kernel at vger.kernel.org>, <linux-erofs at lists.ozlabs.org>, Chao Yu <yuchao0 at huawei.com>, Chao Yu <chao at kernel.org>, Miao Xie <miaoxie at huawei.com>, Fang Wei <fangwei1 at huawei.com>, Gao Xiang <gaoxiang25 at huawei.com>
Message-ID: <20190311060858.28654-1-gaoxiang25 at huawei.com>
From: Gao Xiang <gaoxiang25@huawei.com>
commit cadf1ccf1b0021d0b7a9347e102ac5258f9f98c8 upstream.
This patch enhances the missing error handling code for
xattr submodule, which improves the stability for the rare cases.
Reviewed-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Chao Yu <yuchao0 at huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
---
drivers/staging/erofs/internal.h | 5 +
drivers/staging/erofs/xattr.c | 112 +++++++++++++++++++++++++++------------
2 files changed, 81 insertions(+), 36 deletions(-)
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -485,8 +485,9 @@ struct erofs_map_blocks_iter {
};
-static inline struct page *erofs_get_inline_page(struct inode *inode,
- erofs_blk_t blkaddr)
+static inline struct page *
+erofs_get_inline_page(struct inode *inode,
+ erofs_blk_t blkaddr)
{
return erofs_get_meta_page(inode->i_sb,
blkaddr, S_ISDIR(inode->i_mode));
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -24,16 +24,25 @@ struct xattr_iter {
static inline void xattr_iter_end(struct xattr_iter *it, bool atomic)
{
- /* only init_inode_xattrs use non-atomic once */
+ /* the only user of kunmap() is 'init_inode_xattrs' */
if (unlikely(!atomic))
kunmap(it->page);
else
kunmap_atomic(it->kaddr);
+
unlock_page(it->page);
put_page(it->page);
}
-static void init_inode_xattrs(struct inode *inode)
+static inline void xattr_iter_end_final(struct xattr_iter *it)
+{
+ if (!it->page)
+ return;
+
+ xattr_iter_end(it, true);
+}
+
+static int init_inode_xattrs(struct inode *inode)
{
struct xattr_iter it;
unsigned i;
@@ -43,7 +52,7 @@ static void init_inode_xattrs(struct ino
bool atomic_map;
if (likely(inode_has_inited_xattr(inode)))
- return;
+ return 0;
vi = EROFS_V(inode);
BUG_ON(!vi->xattr_isize);
@@ -53,7 +62,8 @@ static void init_inode_xattrs(struct ino
it.ofs = erofs_blkoff(iloc(sbi, vi->nid) + vi->inode_isize);
it.page = erofs_get_inline_page(inode, it.blkaddr);
- BUG_ON(IS_ERR(it.page));
+ if (IS_ERR(it.page))
+ return PTR_ERR(it.page);
/* read in shared xattr array (non-atomic, see kmalloc below) */
it.kaddr = kmap(it.page);
@@ -62,9 +72,12 @@ static void init_inode_xattrs(struct ino
ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
vi->xattr_shared_count = ih->h_shared_count;
- vi->xattr_shared_xattrs = (unsigned *)kmalloc_array(
- vi->xattr_shared_count, sizeof(unsigned),
- GFP_KERNEL | __GFP_NOFAIL);
+ vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
+ sizeof(uint), GFP_KERNEL);
+ if (!vi->xattr_shared_xattrs) {
+ xattr_iter_end(&it, atomic_map);
+ return -ENOMEM;
+ }
/* let's skip ibody header */
it.ofs += sizeof(struct erofs_xattr_ibody_header);
@@ -77,7 +90,8 @@ static void init_inode_xattrs(struct ino
it.page = erofs_get_meta_page(inode->i_sb,
++it.blkaddr, S_ISDIR(inode->i_mode));
- BUG_ON(IS_ERR(it.page));
+ if (IS_ERR(it.page))
+ return PTR_ERR(it.page);
it.kaddr = kmap_atomic(it.page);
atomic_map = true;
@@ -90,6 +104,7 @@ static void init_inode_xattrs(struct ino
xattr_iter_end(&it, atomic_map);
inode_set_inited_xattr(inode);
+ return 0;
}
struct xattr_iter_handlers {
@@ -99,18 +114,25 @@ struct xattr_iter_handlers {
void (*value)(struct xattr_iter *, unsigned, char *, unsigned);
};
-static void xattr_iter_fixup(struct xattr_iter *it)
+static inline int xattr_iter_fixup(struct xattr_iter *it)
{
- if (unlikely(it->ofs >= EROFS_BLKSIZ)) {
- xattr_iter_end(it, true);
+ if (it->ofs < EROFS_BLKSIZ)
+ return 0;
- it->blkaddr += erofs_blknr(it->ofs);
- it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
- BUG_ON(IS_ERR(it->page));
+ xattr_iter_end(it, true);
- it->kaddr = kmap_atomic(it->page);
- it->ofs = erofs_blkoff(it->ofs);
+ it->blkaddr += erofs_blknr(it->ofs);
+ it->page = erofs_get_meta_page(it->sb, it->blkaddr, false);
+ if (IS_ERR(it->page)) {
+ int err = PTR_ERR(it->page);
+
+ it->page = NULL;
+ return err;
}
+
+ it->kaddr = kmap_atomic(it->page);
+ it->ofs = erofs_blkoff(it->ofs);
+ return 0;
}
static int inline_xattr_iter_begin(struct xattr_iter *it,
@@ -132,21 +154,24 @@ static int inline_xattr_iter_begin(struc
it->ofs = erofs_blkoff(iloc(sbi, vi->nid) + inline_xattr_ofs);
it->page = erofs_get_inline_page(inode, it->blkaddr);
- BUG_ON(IS_ERR(it->page));
- it->kaddr = kmap_atomic(it->page);
+ if (IS_ERR(it->page))
+ return PTR_ERR(it->page);
+ it->kaddr = kmap_atomic(it->page);
return vi->xattr_isize - xattr_header_sz;
}
static int xattr_foreach(struct xattr_iter *it,
- struct xattr_iter_handlers *op, unsigned *tlimit)
+ const struct xattr_iter_handlers *op, unsigned int *tlimit)
{
struct erofs_xattr_entry entry;
unsigned value_sz, processed, slice;
int err;
/* 0. fixup blkaddr, ofs, ipage */
- xattr_iter_fixup(it);
+ err = xattr_iter_fixup(it);
+ if (err)
+ return err;
/*
* 1. read xattr entry to the memory,
@@ -178,7 +203,9 @@ static int xattr_foreach(struct xattr_it
if (it->ofs >= EROFS_BLKSIZ) {
BUG_ON(it->ofs > EROFS_BLKSIZ);
- xattr_iter_fixup(it);
+ err = xattr_iter_fixup(it);
+ if (err)
+ goto out;
it->ofs = 0;
}
@@ -210,7 +237,10 @@ static int xattr_foreach(struct xattr_it
while (processed < value_sz) {
if (it->ofs >= EROFS_BLKSIZ) {
BUG_ON(it->ofs > EROFS_BLKSIZ);
- xattr_iter_fixup(it);
+
+ err = xattr_iter_fixup(it);
+ if (err)
+ goto out;
it->ofs = 0;
}
@@ -270,7 +300,7 @@ static void xattr_copyvalue(struct xattr
memcpy(it->buffer + processed, buf, len);
}
-static struct xattr_iter_handlers find_xattr_handlers = {
+static const struct xattr_iter_handlers find_xattr_handlers = {
.entry = xattr_entrymatch,
.name = xattr_namematch,
.alloc_buffer = xattr_checkbuffer,
@@ -291,8 +321,11 @@ static int inline_getxattr(struct inode
ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining);
if (ret >= 0)
break;
+
+ if (ret != -ENOATTR) /* -ENOMEM, -EIO, etc. */
+ break;
}
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_size;
}
@@ -315,8 +348,10 @@ static int shared_getxattr(struct inode
xattr_iter_end(&it->it, true);
it->it.page = erofs_get_meta_page(inode->i_sb,
- blkaddr, false);
- BUG_ON(IS_ERR(it->it.page));
+ blkaddr, false);
+ if (IS_ERR(it->it.page))
+ return PTR_ERR(it->it.page);
+
it->it.kaddr = kmap_atomic(it->it.page);
it->it.blkaddr = blkaddr;
}
@@ -324,9 +359,12 @@ static int shared_getxattr(struct inode
ret = xattr_foreach(&it->it, &find_xattr_handlers, NULL);
if (ret >= 0)
break;
+
+ if (ret != -ENOATTR) /* -ENOMEM, -EIO, etc. */
+ break;
}
if (vi->xattr_shared_count)
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_size;
}
@@ -351,7 +389,9 @@ int erofs_getxattr(struct inode *inode,
if (unlikely(name == NULL))
return -EINVAL;
- init_inode_xattrs(inode);
+ ret = init_inode_xattrs(inode);
+ if (ret)
+ return ret;
it.index = index;
@@ -494,7 +534,7 @@ static int xattr_skipvalue(struct xattr_
return 1;
}
-static struct xattr_iter_handlers list_xattr_handlers = {
+static const struct xattr_iter_handlers list_xattr_handlers = {
.entry = xattr_entrylist,
.name = xattr_namelist,
.alloc_buffer = xattr_skipvalue,
@@ -516,7 +556,7 @@ static int inline_listxattr(struct listx
if (ret < 0)
break;
}
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_ofs;
}
@@ -538,8 +578,10 @@ static int shared_listxattr(struct listx
xattr_iter_end(&it->it, true);
it->it.page = erofs_get_meta_page(inode->i_sb,
- blkaddr, false);
- BUG_ON(IS_ERR(it->it.page));
+ blkaddr, false);
+ if (IS_ERR(it->it.page))
+ return PTR_ERR(it->it.page);
+
it->it.kaddr = kmap_atomic(it->it.page);
it->it.blkaddr = blkaddr;
}
@@ -549,7 +591,7 @@ static int shared_listxattr(struct listx
break;
}
if (vi->xattr_shared_count)
- xattr_iter_end(&it->it, true);
+ xattr_iter_end_final(&it->it);
return ret < 0 ? ret : it->buffer_ofs;
}
@@ -560,7 +602,9 @@ ssize_t erofs_listxattr(struct dentry *d
int ret;
struct listxattr_iter it;
- init_inode_xattrs(d_inode(dentry));
+ ret = init_inode_xattrs(d_inode(dentry));
+ if (ret)
+ return ret;
it.dentry = dentry;
it.buffer = buffer;
Patches currently in stable-queue which might be from gaoxiang25 at huawei.com are
queue-4.19/staging-erofs-fix-fast-symlink-w-o-xattr-when-fs-xattr-is-on.patch
queue-4.19/staging-erofs-fix-race-of-initializing-xattrs-of-a-inode-at-the-same-time.patch
queue-4.19/staging-erofs-add-error-handling-for-xattr-submodule.patch
queue-4.19/staging-erofs-keep-corrupted-fs-from-crashing-kernel-in-erofs_namei.patch
queue-4.19/staging-erofs-fix-memleak-of-inode-s-shared-xattr-array.patch
^ permalink raw reply [flat|nested] 17+ messages in thread