Re: [PATCH v2 1/6] alloc_tag: add ioctl to /proc/allocinfo
From: Suren Baghdasaryan
Date: Wed Jun 03 2026 - 15:57:07 EST
On Fri, May 22, 2026 at 1:11 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 22 May 2026 17:45:33 +0000 Abhishek Bapat <abhishekbapat@xxxxxxxxxx> wrote:
>
> > From: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> >
> > Add the following ioctl commands for /proc/allocinfo file:
> >
> > ALLOCINFO_IOC_CONTENT_ID - gets content identifier which can be used
> > to check whether the file content has changed specifically due to module
> > load/unload. Every time a module is loaded / unloaded, the returned
> > value will be different. By comparing the identifier value at the
> > beginning and at the end of the content retrieval operation, users can
> > validate retrieved information for consistency.
> >
> > ALLOCINFO_IOC_GET_AT - gets the record at the specified position. This
> > is the position of a record in /proc/allocinfo.
> >
> > ALLOCINFO_IOC_GET_NEXT - gets the record next to the last retrieved
> > one. If no records were previously retrieved, returns the first
> > record.
> >
> > index 000000000000..e9a5b55fcc7a
> > --- /dev/null
> > +++ b/include/uapi/linux/alloc_tag.h
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * include/linux/alloc_tag.h
> > + */
> > +
> > +#ifndef _UAPI_ALLOC_TAG_H
> > +#define _UAPI_ALLOC_TAG_H
> > +
> > +#include <linux/types.h>
> > +
> > +#define ALLOCINFO_STR_SIZE 64
> > +
> > +struct allocinfo_content_id {
> > + __u64 id;
> > +};
> > +
> > +struct allocinfo_tag {
> > + /* Longer names are trimmed */
> > + char modname[ALLOCINFO_STR_SIZE];
> > + char function[ALLOCINFO_STR_SIZE];
> > + char filename[ALLOCINFO_STR_SIZE];
> > + __u64 lineno;
> > +};
> > +
> > +struct allocinfo_counter {
> > + __u64 bytes;
> > + __u64 calls;
> > + __u8 accurate;
> > + __u8 pad[7]; /* Add alignment to not break the 32-bit compatible interface */
>
> This seems rather fragile, and makes assumptions about compiler layout?
>
> Can't we use __attribute__((aligned)) in some fashion?
Ack. I think we can.
>
> > +};
> > +
> > +struct allocinfo_tag_data {
> > + struct allocinfo_tag tag;
> > + struct allocinfo_counter counter;
> > +};
> > +
> > +struct allocinfo_get_at {
> > + __u64 pos; /* input */
> > + struct allocinfo_tag_data data;
> > +};
> > +
> > +#define _ALLOCINFO_IOC_CONTENT_ID 0
> > +#define _ALLOCINFO_IOC_GET_AT 1
> > +#define _ALLOCINFO_IOC_GET_NEXT 2
> > +
> > +#define ALLOCINFO_IOC_BASE 0xA6
> > +#define ALLOCINFO_IOC_CONTENT_ID _IOR(ALLOCINFO_IOC_BASE, _ALLOCINFO_IOC_CONTENT_ID, \
> > + struct allocinfo_content_id)
> > +#define ALLOCINFO_IOC_GET_AT _IOWR(ALLOCINFO_IOC_BASE, _ALLOCINFO_IOC_GET_AT, \
> > + struct allocinfo_get_at)
> > +#define ALLOCINFO_IOC_GET_NEXT _IOR(ALLOCINFO_IOC_BASE, _ALLOCINFO_IOC_GET_NEXT, \
> > + struct allocinfo_tag_data)
> > +
> > +#endif /* _UAPI_ALLOC_TAG_H */
> > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> > index b9ca95d1f506..3598735b6c93 100644
> > --- a/lib/alloc_tag.c
> > +++ b/lib/alloc_tag.c
> > @@ -5,6 +5,7 @@
> > #include <linux/gfp.h>
> > #include <linux/kallsyms.h>
> > #include <linux/module.h>
> > +#include <linux/mutex.h>
> > #include <linux/page_ext.h>
> > #include <linux/pgalloc_tag.h>
> > #include <linux/proc_fs.h>
> > @@ -14,6 +15,7 @@
> > #include <linux/string_choices.h>
> > #include <linux/vmalloc.h>
> > #include <linux/kmemleak.h>
> > +#include <uapi/linux/alloc_tag.h>
> >
> > #define ALLOCINFO_FILE_NAME "allocinfo"
> > #define MODULE_ALLOC_TAG_VMAP_SIZE (100000UL * sizeof(struct alloc_tag))
> > @@ -46,6 +48,10 @@ int alloc_tag_ref_offs;
> > struct allocinfo_private {
> > struct codetag_iterator iter;
> > bool print_header;
> > + /* ioctl uses a separate iterator not to interfere with reads */
> > + struct codetag_iterator ioctl_iter;
> > + bool positioned; /* seq_open_private() sets to 0 */
> > + struct mutex ioctl_lock;
> > };
> >
> > static void *allocinfo_start(struct seq_file *m, loff_t *pos)
> > @@ -125,6 +131,190 @@ static const struct seq_operations allocinfo_seq_op = {
> > .show = allocinfo_show,
> > };
> >
> > +static int allocinfo_open(struct inode *inode, struct file *file)
> > +{
> > + int ret;
> > +
> > + ret = seq_open_private(file, &allocinfo_seq_op,
> > + sizeof(struct allocinfo_private));
> > + if (!ret) {
> > + struct seq_file *m = file->private_data;
> > + struct allocinfo_private *priv = m->private;
> > +
> > + mutex_init(&priv->ioctl_lock);
> > + }
> > + return ret;
> > +}
>
> Generally, the commenting in here is very thin. Add some explanations
> of what the various functions do and, especially, why they do it?
Ack. Will add.
>
> > +static int allocinfo_release(struct inode *inode, struct file *file)
> > +{
> > + return seq_release_private(inode, file);
> > +}
> > +
> > +static const char *allocinfo_str(const char *str)
> > +{
> > + size_t len = strlen(str);
> > +
> > + /* Keep an extra space for the trailing NULL. */
> > + if (len >= ALLOCINFO_STR_SIZE)
> > + str += (len - ALLOCINFO_STR_SIZE) + 1;
> > + return str;
> > +}
> > +
> > +/* Copy a string and trim from the beginning if it's too long */
> > +static void allocinfo_copy_str(char *dest, const char *src)
> > +{
> > + strscpy(dest, allocinfo_str(src), ALLOCINFO_STR_SIZE);
> > +}
>
> See, even these two little functions are unnecessarily difficult to
> review when one doesn"t know what they are setting out to do. One has
> to first reverse engineer their intent from the implementation, then
> check that the implementation implements that intent.
Ack.
>
> > +static int allocinfo_ioctl_get_at(struct seq_file *m, void __user *arg)
> > +{
> > + struct allocinfo_private *priv;
> > + struct codetag *ct;
> > + __u64 pos;
> > + struct allocinfo_get_at params = {0};
> > +
> > + if (copy_from_user(¶ms, arg, sizeof(params)))
> > + return -EFAULT;
> > +
> > + priv = (struct allocinfo_private *)m->private;
>
> Unneeded cast.
Ack.
>
> > + pos = params.pos;
> > +
> > + mutex_lock(&priv->ioctl_lock);
> > + codetag_lock_module_list(alloc_tag_cttype, true);
> > +
> > + /* Find the codetag */
> > + priv->ioctl_iter = codetag_get_ct_iter(alloc_tag_cttype);
> > + ct = codetag_next_ct(&priv->ioctl_iter);
> > + while (ct && pos--)
> > + ct = codetag_next_ct(&priv->ioctl_iter);
> > + if (ct) {
> > + allocinfo_to_params(ct, ¶ms.data);
> > + priv->positioned = true;
> > + }
> > +
> > + codetag_lock_module_list(alloc_tag_cttype, false);
> > + mutex_unlock(&priv->ioctl_lock);
> > +
> > + if (!ct)
> > + return -ENOENT;
> > +
> > + if (copy_to_user(arg, ¶ms, sizeof(params)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +static int allocinfo_ioctl_get_next(struct seq_file *m, void __user *arg)
> > +{
> > + struct allocinfo_private *priv;
> > + struct codetag *ct;
> > + struct allocinfo_tag_data params = {0};
> > + int ret = 0;
> > +
> > + priv = (struct allocinfo_private *)m->private;
>
> Ditto.
Ack.
>
> > + mutex_lock(&priv->ioctl_lock);
> > + codetag_lock_module_list(alloc_tag_cttype, true);
> > +
> > + if (!priv->positioned) {
> > + priv->ioctl_iter = codetag_get_ct_iter(alloc_tag_cttype);
> > + priv->positioned = true;
> > + }
> > +
> > + ct = codetag_next_ct(&priv->ioctl_iter);
> > + if (ct)
> > + allocinfo_to_params(ct, ¶ms);
> > +
> > + if (!ct) {
> > + priv->positioned = false;
> > + ret = -ENOENT;
> > + }
> > + codetag_lock_module_list(alloc_tag_cttype, false);
> > + mutex_unlock(&priv->ioctl_lock);
> > +
> > + if (ret == 0) {
> > + if (copy_to_user(arg, ¶ms, sizeof(params)))
> > + return -EFAULT;
> > + }
> > + return ret;
> > +}
> >
> > ...
> >