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