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 <[email protected]> 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(¶ms, 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/[email protected]?part=3
