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.
