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

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

[Hailong Liu]

__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.

Signed-off-by: Hailong Liu <hailong.liu@oppo.com>
---
 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

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

[Barry Song]

On Fri, Sep 6, 2024 at 8:06 PM Hailong Liu <hailong.liu@oppo.com> 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:-)

>
> Signed-off-by: Hailong Liu <hailong.liu@oppo.com>
> ---
>  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
>
>

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@oppo.com> 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@oppo.com>
> > ---
> >  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.

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

[Barry Song]

On Fri, Sep 6, 2024 at 9:56 PM Hailong Liu <hailong.liu@oppo.com> wrote:
>
> On Fri, 06. Sep 21:25, Barry Song wrote:
> > On Fri, Sep 6, 2024 at 8:06 PM Hailong Liu <hailong.liu@oppo.com> 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.::

That's correct. Additionally, we should update the changelog to specify
that 'the buffer size might exceed order-3, potentially causing allocation
failures,' as order-2 could also be a concern, using
drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c as an instance.

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

not the authority to acknowledge this, but personally, it makes a lot of
sense from the memory management perspective. Please feel free to add
Reviewed-by: Barry Song <baohua@kernel.org>

If you plan to send a v2 to correct the changelog and documentation.

> > >
> > > Signed-off-by: Hailong Liu <hailong.liu@oppo.com>
> > > ---
> > >  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.

Thanks
Barry