[3/3] staging: erofs: xattr.c: avoid BUG_ON
diff mbox series

Message ID 20190813023054.73126-3-gaoxiang25@huawei.com
State New
Headers show
Series
  • [1/3] staging: erofs: inline erofs_inode_is_data_compressed()
Related show

Commit Message

Gao Xiang Aug. 13, 2019, 2:30 a.m. UTC
Kill all the remaining BUG_ON in EROFS:
 - one BUG_ON was used to detect xattr on-disk corruption,
   proper error handling should be added for it instead;
 - the other BUG_ONs are used to detect potential issues,
   use DBG_BUGON only in (eng) debugging version.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/xattr.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Chao Yu Aug. 13, 2019, 3:20 a.m. UTC | #1
On 2019/8/13 10:30, Gao Xiang wrote:
> Kill all the remaining BUG_ON in EROFS:
>  - one BUG_ON was used to detect xattr on-disk corruption,
>    proper error handling should be added for it instead;
>  - the other BUG_ONs are used to detect potential issues,
>    use DBG_BUGON only in (eng) debugging version.

BTW, do we need add WARN_ON() into DBG_BUGON() to show some details function or
call stack in where we encounter the issue?

> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,
Gao Xiang Aug. 13, 2019, 3:57 a.m. UTC | #2
Hi Chao,

On Tue, Aug 13, 2019 at 11:20:22AM +0800, Chao Yu wrote:
> On 2019/8/13 10:30, Gao Xiang wrote:
> > Kill all the remaining BUG_ON in EROFS:
> >  - one BUG_ON was used to detect xattr on-disk corruption,
> >    proper error handling should be added for it instead;
> >  - the other BUG_ONs are used to detect potential issues,
> >    use DBG_BUGON only in (eng) debugging version.
> 
> BTW, do we need add WARN_ON() into DBG_BUGON() to show some details function or
> call stack in where we encounter the issue?

Thanks for kindly review :)

Agreed, it seems much better. If there are no other considerations
here, I can submit another patch addressing it later or maybe we
can change it in the next linux version since I'd like to focusing
on moving out of staging for this round...

Thanks,
Gao Xiang

> 
> > 
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks,
Chao Yu Aug. 13, 2019, 6:10 a.m. UTC | #3
Hi Xiang,

On 2019/8/13 11:57, Gao Xiang wrote:
> Hi Chao,
> 
> On Tue, Aug 13, 2019 at 11:20:22AM +0800, Chao Yu wrote:
>> On 2019/8/13 10:30, Gao Xiang wrote:
>>> Kill all the remaining BUG_ON in EROFS:
>>>  - one BUG_ON was used to detect xattr on-disk corruption,
>>>    proper error handling should be added for it instead;
>>>  - the other BUG_ONs are used to detect potential issues,
>>>    use DBG_BUGON only in (eng) debugging version.
>>
>> BTW, do we need add WARN_ON() into DBG_BUGON() to show some details function or
>> call stack in where we encounter the issue?
> 
> Thanks for kindly review :)
> 
> Agreed, it seems much better. If there are no other considerations
> here, I can submit another patch addressing it later or maybe we
> can change it in the next linux version since I'd like to focusing
> on moving out of staging for this round...

No problem, we can change it in a proper time.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>
>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>>
>> Thanks,
> .
>

Patch
diff mbox series

diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index b29177a17347..289c7850ec96 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -115,7 +115,7 @@  static int init_inode_xattrs(struct inode *inode)
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
 		if (unlikely(it.ofs >= EROFS_BLKSIZ)) {
 			/* cannot be unaligned */
-			BUG_ON(it.ofs != EROFS_BLKSIZ);
+			DBG_BUGON(it.ofs != EROFS_BLKSIZ);
 			xattr_iter_end(&it, atomic_map);
 
 			it.page = erofs_get_meta_page(sb, ++it.blkaddr,
@@ -191,7 +191,7 @@  static int inline_xattr_iter_begin(struct xattr_iter *it,
 
 	xattr_header_sz = inlinexattr_header_size(inode);
 	if (unlikely(xattr_header_sz >= vi->xattr_isize)) {
-		BUG_ON(xattr_header_sz > vi->xattr_isize);
+		DBG_BUGON(xattr_header_sz > vi->xattr_isize);
 		return -ENOATTR;
 	}
 
@@ -234,7 +234,11 @@  static int xattr_foreach(struct xattr_iter *it,
 	if (tlimit) {
 		unsigned int entry_sz = EROFS_XATTR_ENTRY_SIZE(&entry);
 
-		BUG_ON(*tlimit < entry_sz);
+		/* xattr on-disk corruption: xattr entry beyond xattr_isize */
+		if (unlikely(*tlimit < entry_sz)) {
+			DBG_BUGON(1);
+			return -EIO;
+		}
 		*tlimit -= entry_sz;
 	}
 
@@ -253,7 +257,7 @@  static int xattr_foreach(struct xattr_iter *it,
 
 	while (processed < entry.e_name_len) {
 		if (it->ofs >= EROFS_BLKSIZ) {
-			BUG_ON(it->ofs > EROFS_BLKSIZ);
+			DBG_BUGON(it->ofs > EROFS_BLKSIZ);
 
 			err = xattr_iter_fixup(it);
 			if (err)
@@ -288,7 +292,7 @@  static int xattr_foreach(struct xattr_iter *it,
 
 	while (processed < value_sz) {
 		if (it->ofs >= EROFS_BLKSIZ) {
-			BUG_ON(it->ofs > EROFS_BLKSIZ);
+			DBG_BUGON(it->ofs > EROFS_BLKSIZ);
 
 			err = xattr_iter_fixup(it);
 			if (err)