On Fri, Sep 6, 2024 at 9:56 PM Hailong Liu <hailong.liu@xxxxxxxx> wrote: > > 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.:: 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@xxxxxxxxxx> If you plan to send a v2 to correct the changelog and documentation. > > > > > > 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. Thanks Barry
