From: Qu Wenruo <[email protected]>
commit 496245cac57e26d8b738d85c7a29cf9a47610f3f upstream.
There is a report in kernel bugzilla about mismatch file type in dir
item and inode item.
This inspires us to check inode mode in inode item.
This patch will check the following members:
- inode key objectid
Should be ROOT_DIR_DIR or [256, (u64)-256] or FREE_INO.
- inode key offset
Should be 0
- inode item generation
- inode item transid
No newer than sb generation + 1.
The +1 is for log tree.
- inode item mode
No unknown bits.
No invalid S_IF* bit.
NOTE: S_IFMT check is not enough, need to check every know type.
- inode item nlink
Dir should have no more link than 1.
- inode item flags
Reviewed-by: Nikolay Borisov <[email protected]>
Reviewed-by: Johannes Thumshirn <[email protected]>
Signed-off-by: Qu Wenruo <[email protected]>
Reviewed-by: David Sterba <[email protected]>
Signed-off-by: David Sterba <[email protected]>
Signed-off-by: Ben Hutchings <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
fs/btrfs/ctree.h | 15 +++++++
fs/btrfs/tree-checker.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 109 insertions(+)
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1459,6 +1459,21 @@ do {
#define BTRFS_INODE_ROOT_ITEM_INIT (1 << 31)
+#define BTRFS_INODE_FLAG_MASK \
+ (BTRFS_INODE_NODATASUM | \
+ BTRFS_INODE_NODATACOW | \
+ BTRFS_INODE_READONLY | \
+ BTRFS_INODE_NOCOMPRESS | \
+ BTRFS_INODE_PREALLOC | \
+ BTRFS_INODE_SYNC | \
+ BTRFS_INODE_IMMUTABLE | \
+ BTRFS_INODE_APPEND | \
+ BTRFS_INODE_NODUMP | \
+ BTRFS_INODE_NOATIME | \
+ BTRFS_INODE_DIRSYNC | \
+ BTRFS_INODE_COMPRESS | \
+ BTRFS_INODE_ROOT_ITEM_INIT)
+
struct btrfs_map_token {
const struct extent_buffer *eb;
char *kaddr;
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -671,6 +671,97 @@ static int check_dev_item(struct btrfs_f
return 0;
}
+/* Inode item error output has the same format as dir_item_err() */
+#define inode_item_err(fs_info, eb, slot, fmt, ...) \
+ dir_item_err(fs_info, eb, slot, fmt, __VA_ARGS__)
+
+static int check_inode_item(struct btrfs_fs_info *fs_info,
+ struct extent_buffer *leaf,
+ struct btrfs_key *key, int slot)
+{
+ struct btrfs_inode_item *iitem;
+ u64 super_gen = btrfs_super_generation(fs_info->super_copy);
+ u32 valid_mask = (S_IFMT | S_ISUID | S_ISGID | S_ISVTX | 0777);
+ u32 mode;
+
+ if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID ||
+ key->objectid > BTRFS_LAST_FREE_OBJECTID) &&
+ key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
+ key->objectid != BTRFS_FREE_INO_OBJECTID) {
+ generic_err(fs_info, leaf, slot,
+ "invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu",
+ key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID,
+ BTRFS_FIRST_FREE_OBJECTID,
+ BTRFS_LAST_FREE_OBJECTID,
+ BTRFS_FREE_INO_OBJECTID);
+ return -EUCLEAN;
+ }
+ if (key->offset != 0) {
+ inode_item_err(fs_info, leaf, slot,
+ "invalid key offset: has %llu expect 0",
+ key->offset);
+ return -EUCLEAN;
+ }
+ iitem = btrfs_item_ptr(leaf, slot, struct btrfs_inode_item);
+
+ /* Here we use super block generation + 1 to handle log tree */
+ if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
+ inode_item_err(fs_info, leaf, slot,
+ "invalid inode generation: has %llu expect (0, %llu]",
+ btrfs_inode_generation(leaf, iitem),
+ super_gen + 1);
+ return -EUCLEAN;
+ }
+ /* Note for ROOT_TREE_DIR_ITEM, mkfs could set its transid 0 */
+ if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
+ inode_item_err(fs_info, leaf, slot,
+ "invalid inode generation: has %llu expect [0, %llu]",
+ btrfs_inode_transid(leaf, iitem), super_gen + 1);
+ return -EUCLEAN;
+ }
+
+ /*
+ * For size and nbytes it's better not to be too strict, as for dir
+ * item its size/nbytes can easily get wrong, but doesn't affect
+ * anything in the fs. So here we skip the check.
+ */
+ mode = btrfs_inode_mode(leaf, iitem);
+ if (mode & ~valid_mask) {
+ inode_item_err(fs_info, leaf, slot,
+ "unknown mode bit detected: 0x%x",
+ mode & ~valid_mask);
+ return -EUCLEAN;
+ }
+
+ /*
+ * S_IFMT is not bit mapped so we can't completely rely on is_power_of_2,
+ * but is_power_of_2() can save us from checking FIFO/CHR/DIR/REG.
+ * Only needs to check BLK, LNK and SOCKS
+ */
+ if (!is_power_of_2(mode & S_IFMT)) {
+ if (!S_ISLNK(mode) && !S_ISBLK(mode) && !S_ISSOCK(mode)) {
+ inode_item_err(fs_info, leaf, slot,
+ "invalid mode: has 0%o expect valid S_IF* bit(s)",
+ mode & S_IFMT);
+ return -EUCLEAN;
+ }
+ }
+ if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
+ inode_item_err(fs_info, leaf, slot,
+ "invalid nlink: has %u expect no more than 1 for dir",
+ btrfs_inode_nlink(leaf, iitem));
+ return -EUCLEAN;
+ }
+ if (btrfs_inode_flags(leaf, iitem) & ~BTRFS_INODE_FLAG_MASK) {
+ inode_item_err(fs_info, leaf, slot,
+ "unknown flags detected: 0x%llx",
+ btrfs_inode_flags(leaf, iitem) &
+ ~BTRFS_INODE_FLAG_MASK);
+ return -EUCLEAN;
+ }
+ return 0;
+}
+
/*
* Common point to switch the item-specific validation.
*/
@@ -704,6 +795,9 @@ static int check_leaf_item(struct btrfs_
case BTRFS_DEV_ITEM_KEY:
ret = check_dev_item(fs_info, leaf, key, slot);
break;
+ case BTRFS_INODE_ITEM_KEY:
+ ret = check_inode_item(fs_info, leaf, key, slot);
+ break;
}
return ret;
}
Hi!
> From: Qu Wenruo <[email protected]>
>
> commit 496245cac57e26d8b738d85c7a29cf9a47610f3f upstream.
>
> There is a report in kernel bugzilla about mismatch file type in dir
> item and inode item.
>
> This inspires us to check inode mode in inode item.
>
> This patch will check the following members:
> + /* Here we use super block generation + 1 to handle log tree */
> + if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
> + inode_item_err(fs_info, leaf, slot,
> + "invalid inode generation: has %llu expect (0, %llu]",
> + btrfs_inode_generation(leaf, iitem),
> + super_gen + 1);
> + return -EUCLEAN;
> + }
Printk suggests btrfs_inode_generation() may not be zero, but the
condition does not actually check that. Should that be added?
> + /* Note for ROOT_TREE_DIR_ITEM, mkfs could set its transid 0 */
> + if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
> + inode_item_err(fs_info, leaf, slot,
> + "invalid inode generation: has %llu expect [0, %llu]",
> + btrfs_inode_transid(leaf, iitem), super_gen + 1);
> + return -EUCLEAN;
> + }
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
Hi!
> >> From: Qu Wenruo <[email protected]>
> >>
> >> commit 496245cac57e26d8b738d85c7a29cf9a47610f3f upstream.
> >>
> >> There is a report in kernel bugzilla about mismatch file type in dir
> >> item and inode item.
> >>
> >> This inspires us to check inode mode in inode item.
> >>
> >> This patch will check the following members:
> >
> >> + /* Here we use super block generation + 1 to handle log tree */
> >> + if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
> >> + inode_item_err(fs_info, leaf, slot,
> >> + "invalid inode generation: has %llu expect (0, %llu]",
> >> + btrfs_inode_generation(leaf, iitem),
> >> + super_gen + 1);
> >> + return -EUCLEAN;
> >> + }
> >
> > Printk suggests btrfs_inode_generation() may not be zero, but the
> > condition does not actually check that. Should that be added?
>
> Sorry, btrfs_inode_generation() here is exactly what we're checking
> here, so what's wrong?
Quoted message says "(0, ...]", while message below says "[0, ...]". I
assume that means that btrfs_inode_generation() may not be zero in the
first case, but may be zero in the second case. But the code does not
test for zero here.
Best regards,
Pavel
> >> + /* Note for ROOT_TREE_DIR_ITEM, mkfs could set its transid 0 */
> >> + if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
> >> + inode_item_err(fs_info, leaf, slot,
> >> + "invalid inode generation: has %llu expect [0, %llu]",
> >> + btrfs_inode_transid(leaf, iitem), super_gen + 1);
> >> + return -EUCLEAN;
> >> + }
--
http://www.livejournal.com/~pavelmachek
On 2020/11/11 下午9:13, Pavel Machek wrote:
> Hi!
>
>> From: Qu Wenruo <[email protected]>
>>
>> commit 496245cac57e26d8b738d85c7a29cf9a47610f3f upstream.
>>
>> There is a report in kernel bugzilla about mismatch file type in dir
>> item and inode item.
>>
>> This inspires us to check inode mode in inode item.
>>
>> This patch will check the following members:
>
>> + /* Here we use super block generation + 1 to handle log tree */
>> + if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
>> + inode_item_err(fs_info, leaf, slot,
>> + "invalid inode generation: has %llu expect (0, %llu]",
>> + btrfs_inode_generation(leaf, iitem),
>> + super_gen + 1);
>> + return -EUCLEAN;
>> + }
>
> Printk suggests btrfs_inode_generation() may not be zero, but the
> condition does not actually check that. Should that be added?
Sorry, btrfs_inode_generation() here is exactly what we're checking
here, so what's wrong?
Or did you mean the next chunk of btrfs_inode_transid() check?
That error message is wrong, and we had upstream fix for it:
f96d6960abbc ("btrfs: tree-checker: fix the error message for transid
error")
Thanks,
Qu
>
>> + /* Note for ROOT_TREE_DIR_ITEM, mkfs could set its transid 0 */
>> + if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
>> + inode_item_err(fs_info, leaf, slot,
>> + "invalid inode generation: has %llu expect [0, %llu]",
>> + btrfs_inode_transid(leaf, iitem), super_gen + 1);
>> + return -EUCLEAN;
>> + }
>
> Best regards,
> Pavel
>
On 2020/11/11 下午9:38, Pavel Machek wrote:
> Hi!
>
>>>> From: Qu Wenruo <[email protected]>
>>>>
>>>> commit 496245cac57e26d8b738d85c7a29cf9a47610f3f upstream.
>>>>
>>>> There is a report in kernel bugzilla about mismatch file type in dir
>>>> item and inode item.
>>>>
>>>> This inspires us to check inode mode in inode item.
>>>>
>>>> This patch will check the following members:
>>>
>>>> + /* Here we use super block generation + 1 to handle log tree */
>>>> + if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
>>>> + inode_item_err(fs_info, leaf, slot,
>>>> + "invalid inode generation: has %llu expect (0, %llu]",
>>>> + btrfs_inode_generation(leaf, iitem),
>>>> + super_gen + 1);
>>>> + return -EUCLEAN;
>>>> + }
>>>
>>> Printk suggests btrfs_inode_generation() may not be zero, but the
>>> condition does not actually check that. Should that be added?
>>
>> Sorry, btrfs_inode_generation() here is exactly what we're checking
>> here, so what's wrong?
>
> Quoted message says "(0, ...]", while message below says "[0, ...]". I
> assume that means that btrfs_inode_generation() may not be zero in the
> first case, but may be zero in the second case. But the code does not
> test for zero here.
Zero for inode generation is more or less in the grey zone.
For inodes which can be accessed by users, inode 0 may cause small
problems for send, but despite that, no obvious problem.
For btrfs internal generations, it can be 0 and cause nothing wrong.
So here we don't check inode_generation == 0 case at all, or we could
lead to too many false alerts for older btrfs.
Thanks,
Q
>
> Best regards,
> Pavel
>
>>>> + /* Note for ROOT_TREE_DIR_ITEM, mkfs could set its transid 0 */
>>>> + if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
>>>> + inode_item_err(fs_info, leaf, slot,
>>>> + "invalid inode generation: has %llu expect [0, %llu]",
>>>> + btrfs_inode_transid(leaf, iitem), super_gen + 1);
>>>> + return -EUCLEAN;
>>>> + }
>