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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGsJ_4xd=O8pDz4_hWZ+k8Je7=y9BwnZMw=tfcprN5DaTArtjw@mail.gmail.com>
Date: Fri, 6 Sep 2024 22:34:00 +1200
From: Barry Song <21cnbao@...il.com>
To: Hailong Liu <hailong.liu@...o.com>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org
Subject: Re: [PATCH] seq_file: replace kzalloc() with kvzalloc()

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

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

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