Re: [PATCH v4 4/7] ntfs3: add directory index tracepoints

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 [off-list ref]

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.

[ ... ]

[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)) {

[ ... ]

[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);

[ ... ]

[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/20260820090504.950475-1-liubaolin12138@163.com?part=4