OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> writes: > Namjae Jeon <linkinjeon@xxxxxxxxx> writes: > >>> Yes, we can't use fat_search_long() as is, of course. However, we can >>> share the basic algorithm and code. >>> >>> The both are doing, >>> >>> 1) traverse the blocks chained by ->i_start. >>> 2) get the record (dirent) from blocks. >>> 3) check the detail of record >>> >>> The difference is only (3), right? I know, the code has many differences >>> though. The actual logic are almost same. >>> >>> And see, e.g., fat_get_cluster() is checking several unexpected >>> state. We have to care about corrupting data. It is not only >>> "infinite-loop" case. And why I'm saying it is better to share code. >> >> Regarding unexpected conditions, >> When we compare the unexpected conditions in fat_get_cluster: >> We get these as conditions which can arise: >> fat_end_read() returns: >> 1) < 0 >> 2) FAT_ENT_FREE >> 3) FAT_ENT_EOF >> And the last is >> 4) Preventing the infinite looping of cluster chain. >> >> Our patch is already covering the cases: >> case 1) , 2) => if (search_clus < 0 || search_clus == FAT_ENT_FREE) >> about case 3) => while (search_clus != FAT_ENT_EOF); >> >> while for Case 4: >> We can make changes like this: >> >> @@ -179,8 +179,9 @@ struct dentry *fat_get_parent(struct dentry *child_dir) >> struct inode *parent_inode = NULL; >> struct msdos_sb_info *sbi = MSDOS_SB(sb); >> int parent_logstart; >> - int search_clus, clus_to_match; >> + int search_clus, clus_to_match, clus_count = 0; >> sector_t blknr; >> + const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits; >> >> if (!fat_get_dotdot_entry(child_dir->d_inode, &dotdot_bh, &de)) { >> parent_logstart = fat_get_start(sbi, de); >> @@ -223,6 +224,14 @@ struct dentry *fat_get_parent(struct dentry *child_dir) >> search_clus, clus_to_match); >> if (IS_ERR(parent_inode) || parent_inode) >> break; >> + if (++clus_count > limit) { >> + fat_fs_error_ratelimit(sb, >> + "%s: detected the cluster chain loop" >> + " while reading directory entries from" >> + " cluster %d", __func__, search_clus); >> + parent_inode = ERR_PTR(-EIO); >> + break; >> + } >> fatent_init(&fatent); >> search_clus = fat_ent_read(sb, &fatent, >> search_clus); > > and it is copy of fat_get_cluster(), right? It would be sane as start > though, it is true to increase maintain cost, and it makes more similar > to fat_search_long() path. It is why I said, copy at first, and > refactoring after that. BTW, fat_search_long() was wrong as similar function. Actually it would be fat_scan(), because we don't care longname entry. -- OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html