[PATCH] bcachefs: avoid copy from NULL printbuf

[PATCH] bcachefs: avoid copy from NULL printbuf

[David Disseldorp]

printbuf.buf may be NULL if allocation_failure is set. Check for this
prior to using it as a seq_puts() source.

Fixes: fa8e94faeece1 ("bcachefs: Heap allocate printbufs")
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
test test

 fs/bcachefs/fs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 1aee5bafaae54..6e270bf24591d 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -1931,13 +1931,16 @@ static int bch2_show_options(struct seq_file *seq, struct dentry *root)
 {
 	struct bch_fs *c = root->d_sb->s_fs_info;
 	struct printbuf buf = PRINTBUF;
+	int ret = 0;
 
 	bch2_opts_to_text(&buf, c->opts, c, c->disk_sb.sb,
 			  OPT_MOUNT, OPT_HIDDEN, OPT_SHOW_MOUNT_STYLE);
 	printbuf_nul_terminate(&buf);
-	seq_puts(seq, buf.buf);
+	if (buf.allocation_failure)
+		ret = -ENOMEM;
+	else
+		seq_puts(seq, buf.buf);
 
-	int ret = buf.allocation_failure ? -ENOMEM : 0;
 	printbuf_exit(&buf);
 	return ret;
 }
-- 
2.46.1

Re: [PATCH] bcachefs: avoid copy from NULL printbuf

[Hongbo Li]

On 2024/9/24 10:52, David Disseldorp wrote:
> printbuf.buf may be NULL if allocation_failure is set. Check for this
> prior to using it as a seq_puts() source.
> 
> Fixes: fa8e94faeece1 ("bcachefs: Heap allocate printbufs")
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
> test test
> 
>   fs/bcachefs/fs.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> index 1aee5bafaae54..6e270bf24591d 100644
> --- a/fs/bcachefs/fs.c
> +++ b/fs/bcachefs/fs.c
> @@ -1931,13 +1931,16 @@ static int bch2_show_options(struct seq_file *seq, struct dentry *root)
>   {
>   	struct bch_fs *c = root->d_sb->s_fs_info;
>   	struct printbuf buf = PRINTBUF;
> +	int ret = 0;
>   
>   	bch2_opts_to_text(&buf, c->opts, c, c->disk_sb.sb,
>   			  OPT_MOUNT, OPT_HIDDEN, OPT_SHOW_MOUNT_STYLE);
>   	printbuf_nul_terminate(&buf);
> -	seq_puts(seq, buf.buf);
> +	if (buf.allocation_failure)
> +		ret = -ENOMEM;
> +	else
> +		seq_puts(seq, buf.buf);
>   
May I fix here in the last patch "bcachefs: fix incorrect show_options 
results" in v2, and add you as co-author?

Thanks,
Hongbo

> -	int ret = buf.allocation_failure ? -ENOMEM : 0;
>   	printbuf_exit(&buf);
>   	return ret;
>   }

Re: [PATCH] bcachefs: avoid copy from NULL printbuf

[David Disseldorp]

On Wed, 25 Sep 2024 10:21:58 +0800, Hongbo Li wrote:

> On 2024/9/24 10:52, David Disseldorp wrote:
> > printbuf.buf may be NULL if allocation_failure is set. Check for this
> > prior to using it as a seq_puts() source.
> > 
> > Fixes: fa8e94faeece1 ("bcachefs: Heap allocate printbufs")
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> > test test
> > 
> >   fs/bcachefs/fs.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > index 1aee5bafaae54..6e270bf24591d 100644
> > --- a/fs/bcachefs/fs.c
> > +++ b/fs/bcachefs/fs.c
> > @@ -1931,13 +1931,16 @@ static int bch2_show_options(struct seq_file *seq, struct dentry *root)
> >   {
> >   	struct bch_fs *c = root->d_sb->s_fs_info;
> >   	struct printbuf buf = PRINTBUF;
> > +	int ret = 0;
> >   
> >   	bch2_opts_to_text(&buf, c->opts, c, c->disk_sb.sb,
> >   			  OPT_MOUNT, OPT_HIDDEN, OPT_SHOW_MOUNT_STYLE);
> >   	printbuf_nul_terminate(&buf);
> > -	seq_puts(seq, buf.buf);
> > +	if (buf.allocation_failure)
> > +		ret = -ENOMEM;
> > +	else
> > +		seq_puts(seq, buf.buf);
> >     
> May I fix here in the last patch "bcachefs: fix incorrect show_options 
> results" in v2, and add you as co-author?

I'm fine with this being squashed in with or without a co-author tag.
However, I think they deserve separate commits given that they address
separate regressions, and fixing show_options is more critical.

Thanks, David

Re: [PATCH] bcachefs: avoid copy from NULL printbuf

[Hongbo Li]

On 2024/9/25 12:24, David Disseldorp wrote:
> On Wed, 25 Sep 2024 10:21:58 +0800, Hongbo Li wrote:
> 
>> On 2024/9/24 10:52, David Disseldorp wrote:
>>> printbuf.buf may be NULL if allocation_failure is set. Check for this
>>> prior to using it as a seq_puts() source.
>>>
>>> Fixes: fa8e94faeece1 ("bcachefs: Heap allocate printbufs")
>>> Signed-off-by: David Disseldorp <ddiss@suse.de>
>>> ---
>>> test test
>>>
>>>    fs/bcachefs/fs.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
>>> index 1aee5bafaae54..6e270bf24591d 100644
>>> --- a/fs/bcachefs/fs.c
>>> +++ b/fs/bcachefs/fs.c
>>> @@ -1931,13 +1931,16 @@ static int bch2_show_options(struct seq_file *seq, struct dentry *root)
>>>    {
>>>    	struct bch_fs *c = root->d_sb->s_fs_info;
>>>    	struct printbuf buf = PRINTBUF;
>>> +	int ret = 0;
>>>    
>>>    	bch2_opts_to_text(&buf, c->opts, c, c->disk_sb.sb,
>>>    			  OPT_MOUNT, OPT_HIDDEN, OPT_SHOW_MOUNT_STYLE);
>>>    	printbuf_nul_terminate(&buf);
>>> -	seq_puts(seq, buf.buf);
>>> +	if (buf.allocation_failure)
>>> +		ret = -ENOMEM;
>>> +	else
>>> +		seq_puts(seq, buf.buf);
>>>      
>> May I fix here in the last patch "bcachefs: fix incorrect show_options
>> results" in v2, and add you as co-author?
> 
> I'm fine with this being squashed in with or without a co-author tag.
> However, I think they deserve separate commits given that they address
> separate regressions, and fixing show_options is more critical.
> 

I collect these two patch in [1] with separated commits. :)

[1] 
https://lore.kernel.org/linux-bcachefs/20240926020002.1746333-1-lihongbo22@huawei.com/T/#t

Thanks,
Hongbo

> Thanks, David