Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Low] Tracepoints silently truncate 64-bit inode numbers on 32-bit platforms. Pre-existing issues: - [High] Pre-existing memory leaks of `struct ntfs_fnd` objects in the error paths of index modification functions. -- --- Patch [4]: [PATCH v4 4/7] ntfs3: add directory index tracepoints --- commit c766c411cdb54b2e6d2b021851b232bc13e68d08 Author: Baolin Liu <[email protected]> ntfs3: add directory index tracepoints Add ntfs3 tracepoints for directory index operations. This adds trace events for dir_search_u(), indx_find(), indx_insert_entry(), and indx_delete_entry() to help observe directory index lookup, insert, and delete activity. > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c > index 2b439ac043563..d4ca7b1bb5249 100644 > --- a/fs/ntfs3/index.c > +++ b/fs/ntfs3/index.c [ ... ] > @@ -2051,6 +2054,9 @@ int indx_insert_entry(struct ntfs_index *indx, struct > ntfs_inode *ni, > return -EINVAL; > } [Severity: High] This isn't a bug introduced by this patch, but does this direct return in indx_insert_entry() leak the fnd_a memory? Earlier in this function, memory is allocated: if (!fnd) { fnd_a = fnd_get(); ... And then later when checking the collation rule: cmp = get_cmp_func(root); if (unlikely(!cmp)) { WARN_ON_ONCE(1); return -EINVAL; } If get_cmp_func() returns NULL due to an unrecognized collation rule, shouldn't this jump to the `out` label so that fnd_a can be properly freed? > > + trace_ntfs3_indx_insert_entry(&ni->vfs_inode, indx->type, > + le16_to_cpu(new_de->key_size), undo); > + > if (fnd_is_empty(fnd)) { [ ... ] > @@ -2409,6 +2415,8 @@ int indx_delete_entry(struct ntfs_index *indx, struct > ntfs_inode *ni, > return -EINVAL; > } [Severity: High] This is a pre-existing issue, but does this direct return in indx_delete_entry() leak both the fnd and fnd2 memory? Similar to indx_insert_entry(), this function allocates memory: fnd = fnd_get(); ... fnd2 = fnd_get(); Then if get_cmp_func() returns NULL: cmp = get_cmp_func(root); if (unlikely(!cmp)) { WARN_ON_ONCE(1); return -EINVAL; } Shouldn't this go to the `out` label to clean up the memory instead of returning directly? > > + trace_ntfs3_indx_delete_entry(&ni->vfs_inode, indx->type, key_len); > + > /* Locate the entry to remove. */ > err = indx_find(indx, ni, root, key, key_len, ctx, &diff, &e, fnd); > diff --git a/include/trace/events/ntfs3.h b/include/trace/events/ntfs3.h > index 67a2bde0266df..4ac05ed29e49d 100644 > --- a/include/trace/events/ntfs3.h > +++ b/include/trace/events/ntfs3.h [ ... ] > @@ -142,6 +142,87 @@ TRACE_EVENT(ntfs3_create_inode, > __entry->parent_ino, __get_str(name)) > ); > > +TRACE_EVENT(ntfs3_dir_search_u, > + TP_PROTO(struct inode *dir, unsigned int name_len), > + TP_ARGS(dir, name_len), > + TP_STRUCT__entry( > + __field(unsigned long, dir_ino) [Severity: Low] Since the i_ino field in struct inode is a u64, will declaring this field as unsigned long truncate 64-bit inode numbers on 32-bit architectures? > + __field(dev_t, dev) > + __field(unsigned int, name_len) > + ), > + TP_fast_assign( > + __entry->dir_ino = dir->i_ino; > + __entry->dev = dir->i_sb->s_dev; > + __entry->name_len = name_len; > + ), > + TP_printk("dev=(%d,%d) dir=%lu name_len=%u", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + __entry->dir_ino, __entry->name_len) > +); > + > +TRACE_EVENT(ntfs3_indx_find, > + TP_PROTO(struct inode *inode, u8 type, size_t key_len), > + TP_ARGS(inode, type, key_len), > + TP_STRUCT__entry( > + __field(unsigned long, ino) [Severity: Low] Will this also truncate 64-bit inode numbers on 32-bit architectures? > + __field(size_t, key_len) > + __field(dev_t, dev) > + __field(u8, type) > + ), > + TP_fast_assign( > + __entry->ino = inode->i_ino; > + __entry->key_len = key_len; > + __entry->dev = inode->i_sb->s_dev; > + __entry->type = type; > + ), > + TP_printk("dev=(%d,%d) ino=%lu type=%u key_len=%zu", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + __entry->ino, __entry->type, __entry->key_len) > +); > + > +TRACE_EVENT(ntfs3_indx_insert_entry, > + TP_PROTO(struct inode *inode, u8 type, u16 key_len, bool undo), > + TP_ARGS(inode, type, key_len, undo), > + TP_STRUCT__entry( > + __field(unsigned long, ino) [Severity: Low] Will this also truncate 64-bit inode numbers on 32-bit architectures? > + __field(dev_t, dev) > + __field(u16, key_len) > + __field(u8, type) > + __field(bool, undo) > + ), > + TP_fast_assign( > + __entry->ino = inode->i_ino; > + __entry->dev = inode->i_sb->s_dev; > + __entry->key_len = key_len; > + __entry->type = type; > + __entry->undo = undo; > + ), > + TP_printk("dev=(%d,%d) ino=%lu type=%u key_len=%u undo=%d", > + MAJOR(__entry->dev), MINOR(__entry->dev), > + __entry->ino, __entry->type, __entry->key_len, > + __entry->undo) > +); > + > +TRACE_EVENT(ntfs3_indx_delete_entry, > + TP_PROTO(struct inode *inode, u8 type, u32 key_len), > + TP_ARGS(inode, type, key_len), > + TP_STRUCT__entry( > + __field(unsigned long, ino) [Severity: Low] Will this also truncate 64-bit inode numbers on 32-bit architectures? > + __field(u32, key_len) > + __field(dev_t, dev) > + __field(u8, type) > + ), -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
