Re: [PATCH v3 3/4] alloc_tag: Extend allocinfo_filter to support tracing queries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] alloc_tag: UAPI ABI break in allocinfo_ioctl() from growing allocinfo_filter
--

commit 93a7b2baed26770ea553f10bf7e51ee61056c1ff
Author: Abhishek Bapat <abhishekbapat@google.com>

alloc_tag: Extend allocinfo_filter to support tracing queries

This commit extends the /proc/allocinfo ioctl interface to report whether a
codetag has tracing enabled and to filter tags based on their trace state.

> diff --git a/include/uapi/linux/alloc_tag.h b/include/uapi/linux/alloc_tag.h
> index dafcba2f7e2c1..a96054ac22376 100644
> --- a/include/uapi/linux/alloc_tag.h
> +++ b/include/uapi/linux/alloc_tag.h
> @@ -38,7 +38,8 @@ struct allocinfo_counter {
>  	__u64 bytes;
>  	__u64 calls;
>  	__u8 accurate;
> -	__u8 pad[7];
> +	__u8 trace_on;
> +	__u8 pad[6];
>  } __attribute__((aligned(8)));
>  
>  struct allocinfo_tag_data {
> @@ -54,7 +55,8 @@ enum {
>  	ALLOCINFO_FILTER_INACCURATE,
>  	ALLOCINFO_FILTER_MIN_SIZE,
>  	ALLOCINFO_FILTER_MAX_SIZE,
> -	__ALLOCINFO_FILTER_LAST = ALLOCINFO_FILTER_MAX_SIZE
> +	ALLOCINFO_FILTER_TRACE_ON,
> +	__ALLOCINFO_FILTER_LAST = ALLOCINFO_FILTER_TRACE_ON
>  };
>  
>  #define ALLOCINFO_FILTER_MASK_MODNAME		(1 << ALLOCINFO_FILTER_MODNAME)
> @@ -64,6 +66,7 @@ enum {
>  #define ALLOCINFO_FILTER_MASK_INACCURATE	(1 << ALLOCINFO_FILTER_INACCURATE)
>  #define ALLOCINFO_FILTER_MASK_MIN_SIZE		(1 << ALLOCINFO_FILTER_MIN_SIZE)
>  #define ALLOCINFO_FILTER_MASK_MAX_SIZE		(1 << ALLOCINFO_FILTER_MAX_SIZE)
> +#define ALLOCINFO_FILTER_MASK_TRACE_ON		(1 << ALLOCINFO_FILTER_TRACE_ON)
>  
>  #define ALLOCINFO_FILTER_MASKS \
>  	((1 << (__ALLOCINFO_FILTER_LAST + 1)) - 1)
> @@ -75,6 +78,7 @@ struct allocinfo_filter {
>  	__u64 max_size;
>  	/* filter criteria only; see allocinfo_counter.accurate for actual accuracy */
>  	__u64 inaccurate;
> +	__u64 tracing;
>  };
>  
>  struct allocinfo_get_at {

[Severity: High]
Does appending tracing to struct allocinfo_filter break the UAPI ABI for
ALLOCINFO_IOC_GET_AT?

struct allocinfo_filter and struct allocinfo_get_at were introduced in commit
1d581ab2348c ("alloc_tag: add ioctl to /proc/allocinfo"), commit 5732a4e4c18a
("alloc_tag: add ioctl filters to /proc/allocinfo"), commit 6f6769ea88f8
("alloc_tag: add size based filtering to ioctl"), and commit 33588e0b81df
("alloc_tag: add accuracy based filtering to ioctl").

Unlike struct allocinfo_counter, where trace_on consumes a byte of pad[] to
keep the structure size unchanged, struct allocinfo_filter has no reserved
padding. Appending __u64 tracing increases sizeof(struct allocinfo_filter)
from 232 to 240 bytes.

Because struct allocinfo_get_at embeds struct allocinfo_filter by value before
the output field data:

include/uapi/linux/alloc_tag.h:
    struct allocinfo_get_at {
            /* inputs */
            __u64 pos;
            struct allocinfo_filter filter;
            /* output */
            struct allocinfo_tag_data data;
    };
    ...
    #define ALLOCINFO_IOC_GET_AT _IOWR(ALLOCINFO_IOC_BASE, _ALLOCINFO_IOC_GET_AT, \
                                       struct allocinfo_get_at)

growing struct allocinfo_filter shifts the offset of data by 8 bytes,
increases sizeof(struct allocinfo_get_at) from 464 to 472 bytes, and changes
the size encoded into ALLOCINFO_IOC_GET_AT (as well as struct
allocinfo_toggle_traces and ALLOCINFO_IOC_TOGGLE_TRACE).

In mm/alloc_tag.c, allocinfo_ioctl() -> allocinfo_ioctl_get_at() only matches
the new ALLOCINFO_IOC_GET_AT command value and copies sizeof(params):

mm/alloc_tag.c:allocinfo_ioctl() {
    ...
    switch (cmd) {
    ...
    case ALLOCINFO_IOC_GET_AT:
            ret = allocinfo_ioctl_get_at(file->private_data, arg);
            break;
    ...
    default:
            ret = -ENOIOCTLCMD;
            break;
    }
}

mm/alloc_tag.c:allocinfo_ioctl_get_at() {
    ...
    if (copy_from_user(&params, arg, sizeof(params)))
            return -EFAULT;
    ...
}

When existing userspace binaries built against the previous UAPI header invoke
ioctl(fd, ALLOCINFO_IOC_GET_AT, &arg) on /proc/allocinfo, won't
allocinfo_ioctl() fail to match cmd and return -ENOIOCTLCMD (-ENOTTY)?

Since matches_filter() only evaluates filter->inaccurate as a boolean via
!!(filter->inaccurate), could the boolean filter criteria be encoded without
growing struct allocinfo_filter, or could allocinfo_ioctl() handle the legacy
command size so existing binaries continue to work?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792439751.git.abhishekbapat@google.com?part=3