| Message ID | 20240906080047.21409-1-hailong.liu@oppo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | seq_file: replace kzalloc() with kvzalloc() | expand |
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 > >
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.
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
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; }
__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(-)