Re: [PATCH] seq_file: replace kzalloc() with kvzalloc()

[Hailong Liu]

On Fri, 06. Sep 21:25, Barry Song wrote:
> On Fri, Sep 6, 2024 at 8:06 PM Hailong Liu <hailong.liu@xxxxxxxx> wrote:
> >
> > __seq_open_private() uses kzalloc() to allocate a private buffer. However,
> > the size of the buffer might be greater than order-3, which may cause
> > allocation failure. To address this issue, use kvzalloc instead.
>
> In general, this patch seems sensible, but do we have a specific example
> of a driver that uses such a large amount of private data?
> Providing a real-world example of a driver with substantial private data could
> make this patch more convincing:-)
>
To be honest, it's a bit embarrassing, but the issue is that my own driver
allocates 256K of memory to store data. :)

Howeve I grep the __seq_open_private in drivers and found
https://elixir.bootlin.com/linux/v6.11-rc5/source/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c#L3765
static int ulprx_la_open(struct inode *inode, struct file *file)
{
        struct seq_tab *p;
        struct adapter *adap = inode->i_private;

        p = seq_open_tab(file, ULPRX_LA_SIZE, 8 * sizeof(u32), 1,
                         ulprx_la_show);

->                      seq_open_tab...
->                              p = __seq_open_private(f, &seq_tab_ops, sizeof(*p) + rows * width);
->                              ULPRX_LA_SIZE * 8 * sizeof(u32) = 32 * 512 = 16384 = order-2
->      if system is in highly fragmation, order-2 might allocation failure.
        if (!p)
                return -ENOMEM;

        t4_ulprx_read_la(adap, (u32 *)p->data);
        return 0;
}
So IMO this issue might also be encountered in other drivers.

I should also change the comment in Documentation/filesystems/seq_file.rst
```rst
There is also a wrapper function to seq_open() called seq_open_private(). It
kmallocs a zero filled block of memory and stores a pointer to it in the
private field of the seq_file structure, returning 0 on success. The
block size is specified in a third parameter to the function, e.g.::

        static int ct_open(struct inode *inode, struct file *file)
        {
                return seq_open_private(file, &ct_seq_ops,
                                        sizeof(struct mystruct));
        }
```

if the patch be ACKed. I will add this in next version.

> >
> > Signed-off-by: Hailong Liu <hailong.liu@xxxxxxxx>
> > ---
> >  fs/seq_file.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/seq_file.c b/fs/seq_file.c
> > index e676c8b0cf5d..cf23143bbb65 100644
> > --- a/fs/seq_file.c
> > +++ b/fs/seq_file.c
> > @@ -621,7 +621,7 @@ int seq_release_private(struct inode *inode, struct file *file)
> >  {
> >         struct seq_file *seq = file->private_data;
> >
> > -       kfree(seq->private);
> > +       kvfree(seq->private);
> >         seq->private = NULL;
> >         return seq_release(inode, file);
> >  }
> > @@ -634,7 +634,7 @@ void *__seq_open_private(struct file *f, const struct seq_operations *ops,
> >         void *private;
> >         struct seq_file *seq;
> >
> > -       private = kzalloc(psize, GFP_KERNEL_ACCOUNT);
> > +       private = kvzalloc(psize, GFP_KERNEL_ACCOUNT);
> >         if (private == NULL)
> >                 goto out;
> >
> > @@ -647,7 +647,7 @@ void *__seq_open_private(struct file *f, const struct seq_operations *ops,
> >         return private;
> >
> >  out_free:
> > -       kfree(private);
> > +       kvfree(private);
> >  out:
> >         return NULL;
> >  }
> > --
> > 2.30.0
> >
> >
>

--

Help you, Help me,
Hailong.