bcachefs: replace deprecated strncpy() with strscpy()

[PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Roxana Nicolescu 8 months, 3 weeks ago

strncpy() is deprecated for NUL-terminated destination buffers. Use
strscpy() instead.

Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Roxana Nicolescu <nicolescu.roxana@protonmail.com>
---
 fs/bcachefs/btree_trans_commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bcachefs/btree_trans_commit.c b/fs/bcachefs/btree_trans_commit.c
index c4f524b2ca9a..7ab25b425d11 100644
--- a/fs/bcachefs/btree_trans_commit.c
+++ b/fs/bcachefs/btree_trans_commit.c
@@ -364,7 +364,7 @@ static noinline void journal_transaction_name(struct btree_trans *trans)
 	struct jset_entry_log *l =
 		container_of(entry, struct jset_entry_log, entry);
 
-	strncpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
+	strscpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
 }
 
 static inline int btree_key_can_insert(struct btree_trans *trans,
-- 
2.34.1

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Alan Huang 8 months, 3 weeks ago

On Mar 26, 2025, at 17:44, Roxana Nicolescu <nicolescu.roxana@protonmail.com> wrote:
> 
> strncpy() is deprecated for NUL-terminated destination buffers. Use
> strscpy() instead.
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Roxana Nicolescu <nicolescu.roxana@protonmail.com>
> ---
> fs/bcachefs/btree_trans_commit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/btree_trans_commit.c b/fs/bcachefs/btree_trans_commit.c
> index c4f524b2ca9a..7ab25b425d11 100644
> --- a/fs/bcachefs/btree_trans_commit.c
> +++ b/fs/bcachefs/btree_trans_commit.c
> @@ -364,7 +364,7 @@ static noinline void journal_transaction_name(struct btree_trans *trans)
> struct jset_entry_log *l =
> container_of(entry, struct jset_entry_log, entry);
> 
> - strncpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
> + strscpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));

The last time I asked Kent about this line, he didn’t want this.

> }
> 
> static inline int btree_key_can_insert(struct btree_trans *trans,
> -- 
> 2.34.1
> 
> 
>

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Kent Overstreet 8 months, 3 weeks ago

On Wed, Mar 26, 2025 at 06:02:31PM +0800, Alan Huang wrote:
> On Mar 26, 2025, at 17:44, Roxana Nicolescu <nicolescu.roxana@protonmail.com> wrote:
> > 
> > strncpy() is deprecated for NUL-terminated destination buffers. Use
> > strscpy() instead.
> > 
> > Link: https://github.com/KSPP/linux/issues/90
> > Signed-off-by: Roxana Nicolescu <nicolescu.roxana@protonmail.com>
> > ---
> > fs/bcachefs/btree_trans_commit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/bcachefs/btree_trans_commit.c b/fs/bcachefs/btree_trans_commit.c
> > index c4f524b2ca9a..7ab25b425d11 100644
> > --- a/fs/bcachefs/btree_trans_commit.c
> > +++ b/fs/bcachefs/btree_trans_commit.c
> > @@ -364,7 +364,7 @@ static noinline void journal_transaction_name(struct btree_trans *trans)
> > struct jset_entry_log *l =
> > container_of(entry, struct jset_entry_log, entry);
> > 
> > - strncpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
> > + strscpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
> 
> The last time I asked Kent about this line, he didn’t want this.

Yes, the destination buffer isn't required to be nul terminated.

But it seems we should add a comment explaining that :)

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Roxana Nicolescu 8 months, 3 weeks ago

On Wednesday, March 26th, 2025 at 1:19 PM, Kent Overstreet <kent.overstreet@linux.dev> wrote:

> On Wed, Mar 26, 2025 at 06:02:31PM +0800, Alan Huang wrote:
> 
> > On Mar 26, 2025, at 17:44, Roxana Nicolescu nicolescu.roxana@protonmail.com wrote:
> > 
> > > strncpy() is deprecated for NUL-terminated destination buffers. Use
> > > strscpy() instead.
> > > 
> > > Link: https://github.com/KSPP/linux/issues/90
> > > Signed-off-by: Roxana Nicolescu nicolescu.roxana@protonmail.com
> > > ---
> > > fs/bcachefs/btree_trans_commit.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/bcachefs/btree_trans_commit.c b/fs/bcachefs/btree_trans_commit.c
> > > index c4f524b2ca9a..7ab25b425d11 100644
> > > --- a/fs/bcachefs/btree_trans_commit.c
> > > +++ b/fs/bcachefs/btree_trans_commit.c
> > > @@ -364,7 +364,7 @@ static noinline void journal_transaction_name(struct btree_trans *trans)
> > > struct jset_entry_log *l =
> > > container_of(entry, struct jset_entry_log, entry);
> > > 
> > > - strncpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
> > > + strscpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
> > 
> > The last time I asked Kent about this line, he didn’t want this.
> 
> 
> Yes, the destination buffer isn't required to be nul terminated.
> 
> But it seems we should add a comment explaining that :)

I should have checked the mailing list before, but I will keep this in mind for my next contributions.
I wonder if we should use memcpy in this case. This is also recommended by the security team here https://github.com/KSPP/linux/issues/90
This will also prevent other people from trying to send a similar patch in the future.

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Kent Overstreet 8 months, 3 weeks ago

On Wed, Mar 26, 2025 at 02:40:22PM +0000, Roxana Nicolescu wrote:
> On Wednesday, March 26th, 2025 at 1:19 PM, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> 
> > On Wed, Mar 26, 2025 at 06:02:31PM +0800, Alan Huang wrote:
> > 
> > > On Mar 26, 2025, at 17:44, Roxana Nicolescu nicolescu.roxana@protonmail.com wrote:
> > > 
> > > > strncpy() is deprecated for NUL-terminated destination buffers. Use
> > > > strscpy() instead.
> > > > 
> > > > Link: https://github.com/KSPP/linux/issues/90
> > > > Signed-off-by: Roxana Nicolescu nicolescu.roxana@protonmail.com
> > > > ---
> > > > fs/bcachefs/btree_trans_commit.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/bcachefs/btree_trans_commit.c b/fs/bcachefs/btree_trans_commit.c
> > > > index c4f524b2ca9a..7ab25b425d11 100644
> > > > --- a/fs/bcachefs/btree_trans_commit.c
> > > > +++ b/fs/bcachefs/btree_trans_commit.c
> > > > @@ -364,7 +364,7 @@ static noinline void journal_transaction_name(struct btree_trans *trans)
> > > > struct jset_entry_log *l =
> > > > container_of(entry, struct jset_entry_log, entry);
> > > > 
> > > > - strncpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
> > > > + strscpy(l->d, trans->fn, JSET_ENTRY_LOG_U64s * sizeof(u64));
> > > 
> > > The last time I asked Kent about this line, he didn’t want this.
> > 
> > 
> > Yes, the destination buffer isn't required to be nul terminated.
> > 
> > But it seems we should add a comment explaining that :)
> 
> I should have checked the mailing list before, but I will keep this in mind for my next contributions.
> I wonder if we should use memcpy in this case. This is also recommended by the security team here https://github.com/KSPP/linux/issues/90
> This will also prevent other people from trying to send a similar patch in the future.

Or better, a new helper: when we're copying to a fixed size buffer we
really want to zero out the rest of the buffer - we don't just want a
single terminating nul.

This has come up in other places in bcachefs, see __bch2_fs_log_msg() in
btree_update.c, and I would imagine the code for updating superblock
labels as well.

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Bharadwaj Raju 8 months, 3 weeks ago

On Wed, Mar 26, 2025 at 8:22 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
> Or better, a new helper: when we're copying to a fixed size buffer we
> really want to zero out the rest of the buffer - we don't just want a
> single terminating nul.

I believe strscpy_pad does this?

https://docs.kernel.org/core-api/kernel-api.html#c.strscpy_pad

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Kent Overstreet 8 months, 3 weeks ago

On Wed, Mar 26, 2025 at 09:19:06PM +0530, Bharadwaj Raju wrote:
> On Wed, Mar 26, 2025 at 8:22 PM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> > Or better, a new helper: when we're copying to a fixed size buffer we
> > really want to zero out the rest of the buffer - we don't just want a
> > single terminating nul.
> 
> I believe strscpy_pad does this?
> 
> https://docs.kernel.org/core-api/kernel-api.html#c.strscpy_pad

almost, we don't want the 'required nul termination'; that's a
requirement at least for disk labels where we need to preserve existing
behaviour

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Alan Huang 8 months, 3 weeks ago

On Mar 27, 2025, at 00:17, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> 
> On Wed, Mar 26, 2025 at 09:19:06PM +0530, Bharadwaj Raju wrote:
>> On Wed, Mar 26, 2025 at 8:22 PM Kent Overstreet
>> <kent.overstreet@linux.dev> wrote:
>>> Or better, a new helper: when we're copying to a fixed size buffer we
>>> really want to zero out the rest of the buffer - we don't just want a
>>> single terminating nul.
>> 
>> I believe strscpy_pad does this?
>> 
>> https://docs.kernel.org/core-api/kernel-api.html#c.strscpy_pad
> 
> almost, we don't want the 'required nul termination'; that's a
> requirement at least for disk labels where we need to preserve existing
> behaviour

memcpy_and_pad :)

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Kent Overstreet 8 months, 3 weeks ago

On Thu, Mar 27, 2025 at 12:36:25AM +0800, Alan Huang wrote:
> On Mar 27, 2025, at 00:17, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > 
> > On Wed, Mar 26, 2025 at 09:19:06PM +0530, Bharadwaj Raju wrote:
> >> On Wed, Mar 26, 2025 at 8:22 PM Kent Overstreet
> >> <kent.overstreet@linux.dev> wrote:
> >>> Or better, a new helper: when we're copying to a fixed size buffer we
> >>> really want to zero out the rest of the buffer - we don't just want a
> >>> single terminating nul.
> >> 
> >> I believe strscpy_pad does this?
> >> 
> >> https://docs.kernel.org/core-api/kernel-api.html#c.strscpy_pad
> > 
> > almost, we don't want the 'required nul termination'; that's a
> > requirement at least for disk labels where we need to preserve existing
> > behaviour
> 
> memcpy_and_pad :)

I'll take that patch :)

Re: [PATCH] bcachefs: replace deprecated strncpy() with strscpy()

Posted by Roxana Nicolescu 8 months, 3 weeks ago

On Wednesday, March 26th, 2025 at 5:41 PM, Kent Overstreet <kent.overstreet@linux.dev> wrote:

> On Thu, Mar 27, 2025 at 12:36:25AM +0800, Alan Huang wrote:
> 
> > On Mar 27, 2025, at 00:17, Kent Overstreet kent.overstreet@linux.dev wrote:
> > 
> > > On Wed, Mar 26, 2025 at 09:19:06PM +0530, Bharadwaj Raju wrote:
> > > 
> > > > On Wed, Mar 26, 2025 at 8:22 PM Kent Overstreet
> > > > kent.overstreet@linux.dev wrote:
> > > > 
> > > > > Or better, a new helper: when we're copying to a fixed size buffer we
> > > > > really want to zero out the rest of the buffer - we don't just want a
> > > > > single terminating nul.
> > > > 
> > > > I believe strscpy_pad does this?
> > > > 
> > > > https://docs.kernel.org/core-api/kernel-api.html#c.strscpy_pad
> > > 
> > > almost, we don't want the 'required nul termination'; that's a
> > > requirement at least for disk labels where we need to preserve existing
> > > behaviour
> > 
> > memcpy_and_pad :)
> 
> 
> I'll take that patch :)

I'll send a follow-up patch with memcpy_and_pad with the pad_char=0. Even though strscpy_pad does not require it, it does make the assumption that the destination buffer should be NUL-terminated, and it does some extra checks we don't need, so memcpy_and_pad is the way to go.

I'll also replace the memcpy calls in bch2_trans_log_msg() and __bch2_fs_log_ms().