Alexey Asemov (Alex/AT)
2022-Nov-18 09:47 UTC
[Ocfs2-devel] dinode link count inconsistency in ocfs2_read_links_count() logic
Hello, While diagnosing unlink hang issue, I have found an inconsistency in dinode hardlink count handling. If we take a look at ocfs2.h, sections below the text, we see ocfs2_set_links_count() always stores high portion of link count into dinode, but ocfs2_read_links_count() retrieves and adds high portion *only* when inode has OCFS2_INDEXED_DIR_FL flag set. The problem is, ocfs2_read_links_count() is used throughout all the code and not directories only. For files, OCFS2_INDEXED_DIR_FL flag is never present, so when number of hardlinks for file spills over 65535, it will be written as 65536 correctly to dinode, but then will always be read as 0 to inode due to the check. This causes wrong link count of 0 in stat() and total hang on attempt to unlink the 'parent' inode directory entry somehow (this is the issue I hit but I have not diagnosed how it happens because I stumbled on wrong link count immediately and dug that direction). While not sure about the internals, I suggest removing - if (di->i_dyn_features & cpu_to_le16(OCFS2_INDEXED_DIR_FL)) check for the time being inside ocfs2_read_links_count() so it always reads the correct link count from dinode, be it file or directory. I do not see dinode link count parts used outside ocfs2.h API, so it should be semantically correct thing to do, but please someone acquainted with the logic confirm. KR, Alex --- static inline unsigned int ocfs2_read_links_count(struct ocfs2_dinode *di) { ??????? u32 nlink = le16_to_cpu(di->i_links_count); ??????? u32 hi = le16_to_cpu(di->i_links_count_hi); ??????? if (di->i_dyn_features & cpu_to_le16(OCFS2_INDEXED_DIR_FL)) ??????????????? nlink |= (hi << OCFS2_LINKS_HI_SHIFT); ??????? return nlink; } static inline void ocfs2_set_links_count(struct ocfs2_dinode *di, u32 nlink) { ??????? u16 lo, hi; ??????? lo = nlink; ??????? hi = nlink >> OCFS2_LINKS_HI_SHIFT; ??????? di->i_links_count = cpu_to_le16(lo); ??????? di->i_links_count_hi = cpu_to_le16(hi); }
Joseph Qi
2022-Nov-21 01:31 UTC
[Ocfs2-devel] dinode link count inconsistency in ocfs2_read_links_count() logic
Hi, ocfs2_mknod()/ocfs2_link()/ocfs2_rename() will always check ocfs2_link_max(), so it seems that it won't overflow. I wonder if you've encounter another bug that leads to dinode link count leaking. Thanks, Joseph On 11/18/22 5:47 PM, Alexey Asemov (Alex/AT) via Ocfs2-devel wrote:> Hello, > > While diagnosing unlink hang issue, I have found an inconsistency in dinode hardlink count handling. > > If we take a look at ocfs2.h, sections below the text, we see ocfs2_set_links_count() always stores high portion of link count into dinode, but ocfs2_read_links_count() retrieves and adds high portion *only* when inode has OCFS2_INDEXED_DIR_FL flag set. > > The problem is, ocfs2_read_links_count() is used throughout all the code and not directories only. For files, OCFS2_INDEXED_DIR_FL flag is never present, so when number of hardlinks for file spills over 65535, it will be written as 65536 correctly to dinode, but then will always be read as 0 to inode due to the check. This causes wrong link count of 0 in stat() and total hang on attempt to unlink the 'parent' inode directory entry somehow (this is the issue I hit but I have not diagnosed how it happens because I stumbled on wrong link count immediately and dug that direction). > > While not sure about the internals, I suggest removing > > - if (di->i_dyn_features & cpu_to_le16(OCFS2_INDEXED_DIR_FL)) > > check for the time being inside ocfs2_read_links_count() so it always reads the correct link count from dinode, be it file or directory. > > I do not see dinode link count parts used outside ocfs2.h API, so it should be semantically correct thing to do, but please someone acquainted with the logic confirm. > > KR, > Alex > > --- > > static inline unsigned int ocfs2_read_links_count(struct ocfs2_dinode *di) > { > ??????? u32 nlink = le16_to_cpu(di->i_links_count); > ??????? u32 hi = le16_to_cpu(di->i_links_count_hi); > > ??????? if (di->i_dyn_features & cpu_to_le16(OCFS2_INDEXED_DIR_FL)) > ??????????????? nlink |= (hi << OCFS2_LINKS_HI_SHIFT); > > ??????? return nlink; > } > > static inline void ocfs2_set_links_count(struct ocfs2_dinode *di, u32 nlink) > { > ??????? u16 lo, hi; > > ??????? lo = nlink; > ??????? hi = nlink >> OCFS2_LINKS_HI_SHIFT; > > ??????? di->i_links_count = cpu_to_le16(lo); > ??????? di->i_links_count_hi = cpu_to_le16(hi); > } > > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel at oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel