Re: [PATCH net-next v3 1/2] keys, dns: drop unused upayload->data NUL terminator

Re: [PATCH net-next v3 1/2] keys, dns: drop unused upayload->data NUL terminator

[Jarkko Sakkinen]

On Mon, Apr 27, 2026 at 01:44:23PM +0200, Thorsten Blum wrote:
> ->data includes an extra NUL terminator despite never being used as a C
> string and only accessing ->datalen bytes. Remove the redundant NUL
> terminator and allocate one byte less in dns_resolver_preparse().
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

Never being used where?

Let me go this through.

I read this from the documentation:

  /*
   * Preparse instantiation data for a dns_resolver key.
   *
   * For normal hostname lookups, the data must be a NUL-terminated string, with
   * the NUL char accounted in datalen.

So what is confusing here for me is that should upayload, which is
original data with options and '\0'.

So my question is which is the regression here:

1. Incorrect length. Then the fix would be simply setting length as
   'result_len + 1', which aligns also with the snippet of documentation
   I pasted.
2. Unnecessary '\0'.

If there is an issue, your commit is lacking fixes tag and cc tag to the
author of potentially failing commit.

> ---
> Changes in v3:
> - Update commit message
> - v2: https://lore.kernel.org/lkml/20260409225703.158552-4-thorsten.blum@linux.dev/
> 
> Changes in v2:
> - No changes in patch 1/2
> - v1: https://lore.kernel.org/lkml/20260406175810.1018681-3-thorsten.blum@linux.dev/
> ---
>  net/dns_resolver/dns_key.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
> index c3c8c3240ef9..451247864a63 100644
> --- a/net/dns_resolver/dns_key.c
> +++ b/net/dns_resolver/dns_key.c
> @@ -203,7 +203,7 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
>  	kdebug("store result");
>  	prep->quotalen = result_len;
>  
> -	upayload = kmalloc_flex(*upayload, data, result_len + 1);
> +	upayload = kmalloc_flex(*upayload, data, result_len);
>  	if (!upayload) {
>  		kleave(" = -ENOMEM");
>  		return -ENOMEM;
> @@ -211,7 +211,6 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
>  
>  	upayload->datalen = result_len;
>  	memcpy(upayload->data, data, result_len);
> -	upayload->data[result_len] = '\0';
>  
>  	prep->payload.data[dns_key_data] = upayload;
>  	kleave(" = 0");

BR, Jarkko

Re: [PATCH net-next v3 1/2] keys, dns: drop unused upayload->data NUL terminator

[Thorsten Blum]

On Sat, May 09, 2026 at 12:21:01AM +0300, Jarkko Sakkinen wrote:
> On Mon, Apr 27, 2026 at 01:44:23PM +0200, Thorsten Blum wrote:
> > ->data includes an extra NUL terminator despite never being used as a C
> > string and only accessing ->datalen bytes. Remove the redundant NUL
> > terminator and allocate one byte less in dns_resolver_preparse().
> > 
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> 
> Never being used where?

Does this conversation [1] with Jakub answer your question?

[1] https://lore.kernel.org/lkml/adw5cvtPfx1SWQq9@linux.dev/

> Let me go this through.
> 
> I read this from the documentation:
> 
>   /*
>    * Preparse instantiation data for a dns_resolver key.
>    *
>    * For normal hostname lookups, the data must be a NUL-terminated string, with
>    * the NUL char accounted in datalen.

This documents prep->data and prep->datalen, which must be
NUL-terminated and is checked here in dns_resolver_preparse():

	if (!data || data[datalen - 1] != '\0')
		return -EINVAL;

According to this, datalen (prep->datalen really) must include the NUL
terminator, otherwise -EINVAL is returned.

However, my patch is about upayload->data and upayload->datalen, and
upayload->data currently has a trailing '\0', which upayload->datalen
doesn't account for (a mismatch that prevents adding __counted_by).

> So what is confusing here for me is that should upayload, which is
> original data with options and '\0'.
> 
> So my question is which is the regression here:
> 
> 1. Incorrect length. Then the fix would be simply setting length as
>    'result_len + 1', which aligns also with the snippet of documentation
>    I pasted.

This would be an option, but there is currently no consumer of
upayload->data in the kernel that uses the trailing '\0'.

> 2. Unnecessary '\0'.

Yes, '\0' is an unnecessary extra byte that no consumer of
upayload->data uses, and my patch removes it.

> If there is an issue, your commit is lacking fixes tag and cc tag to the
> author of potentially failing commit.

It's just an unused extra byte that prevents adding __counted_by(),
which is otherwise harmless, hence no fixes tag.

Thanks,
Thorsten

Re: [PATCH net-next v3 2/2] KEYS: annotate struct user_key_payload with __counted_by

[Jarkko Sakkinen]

On Mon, Apr 27, 2026 at 01:44:24PM +0200, Thorsten Blum wrote:
> Add the __counted_by() compiler attribute to the flexible array member
> 'data' to improve bounds checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

This is again a crippled commit message.

I.e. this requires either knowing, remembering and/or cross-referencing
"bunch of" unrelated to keyring.

I gave up already.

BR, Jarkko

> ---
> Changes in v3:
> - Formatting only, do not split the declaration into two lines (Jarkko)
> - v2: https://lore.kernel.org/lkml/20260409225703.158552-7-thorsten.blum@linux.dev/
> 
> Changes in v2:
> - Use __aligned(8) as suggested by David
> - v1: https://lore.kernel.org/lkml/20260409073711.57020-6-thorsten.blum@linux.dev/
> ---
>  include/keys/user-type.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/keys/user-type.h b/include/keys/user-type.h
> index 386c31432789..30de4f92a721 100644
> --- a/include/keys/user-type.h
> +++ b/include/keys/user-type.h
> @@ -27,7 +27,7 @@
>  struct user_key_payload {
>  	struct rcu_head	rcu;		/* RCU destructor */
>  	unsigned short	datalen;	/* length of this data */
> -	char		data[] __aligned(__alignof__(u64)); /* actual data */
> +	char		data[] __aligned(8) __counted_by(datalen); /* actual data */
>  };
>  
>  extern struct key_type key_type_user;

Re: [PATCH net-next v3 2/2] KEYS: annotate struct user_key_payload with __counted_by

[Thorsten Blum]

On Sat, May 09, 2026 at 02:24:01PM +0300, Jarkko Sakkinen wrote:
> On Mon, Apr 27, 2026 at 01:44:24PM +0200, Thorsten Blum wrote:
> > Add the __counted_by() compiler attribute to the flexible array member
> > 'data' to improve bounds checking via CONFIG_UBSAN_BOUNDS and
> > CONFIG_FORTIFY_SOURCE.
> > 
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> 
> This is again a crippled commit message.
> 
> I.e. this requires either knowing, remembering and/or cross-referencing
> "bunch of" unrelated to keyring.

I kept the commit message short because it follows the same pattern as
many other __counted_by() patches that have been accepted across several
subsystems.

I'm not sure which part of the message you want expanded. Could you be
more specific?

Thanks,
Thorsten

Re: [PATCH net-next v3 2/2] KEYS: annotate struct user_key_payload with __counted_by

[Jarkko Sakkinen]

On Sun, May 10, 2026 at 12:47:24PM +0200, Thorsten Blum wrote:
> On Sat, May 09, 2026 at 02:24:01PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Apr 27, 2026 at 01:44:24PM +0200, Thorsten Blum wrote:
> > > Add the __counted_by() compiler attribute to the flexible array member
> > > 'data' to improve bounds checking via CONFIG_UBSAN_BOUNDS and
> > > CONFIG_FORTIFY_SOURCE.
> > > 
> > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > 
> > This is again a crippled commit message.
> > 
> > I.e. this requires either knowing, remembering and/or cross-referencing
> > "bunch of" unrelated to keyring.
> 
> I kept the commit message short because it follows the same pattern as
> many other __counted_by() patches that have been accepted across several
> subsystems.
> 
> I'm not sure which part of the message you want expanded. Could you be
> more specific?

"Describe your problem. Whether your patch is a one-line bug fix or 5000
lines of a new feature, there must be an underlying problem that
motivated you to do this work. Convince the reviewer that there is a
problem worth fixing and that it makes sense for them to read past the
first paragraph."

This is a direct quote from https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> 
> Thanks,
> Thorsten

BR, Jarkko

Re: [PATCH net-next v3 2/2] KEYS: annotate struct user_key_payload with __counted_by

[Jarkko Sakkinen]

On Sun, May 10, 2026 at 05:12:13PM +0300, Jarkko Sakkinen wrote:
> On Sun, May 10, 2026 at 12:47:24PM +0200, Thorsten Blum wrote:
> > On Sat, May 09, 2026 at 02:24:01PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Apr 27, 2026 at 01:44:24PM +0200, Thorsten Blum wrote:
> > > > Add the __counted_by() compiler attribute to the flexible array member
> > > > 'data' to improve bounds checking via CONFIG_UBSAN_BOUNDS and
> > > > CONFIG_FORTIFY_SOURCE.
> > > > 
> > > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > > 
> > > This is again a crippled commit message.
> > > 
> > > I.e. this requires either knowing, remembering and/or cross-referencing
> > > "bunch of" unrelated to keyring.
> > 
> > I kept the commit message short because it follows the same pattern as
> > many other __counted_by() patches that have been accepted across several
> > subsystems.
> > 
> > I'm not sure which part of the message you want expanded. Could you be
> > more specific?
> 
> "Describe your problem. Whether your patch is a one-line bug fix or 5000
> lines of a new feature, there must be an underlying problem that
> motivated you to do this work. Convince the reviewer that there is a
> problem worth fixing and that it makes sense for them to read past the
> first paragraph."
> 
> This is a direct quote from https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

And there's pretty good reasons to do this too as commit message is not
just a cover leter for a patch. It's also reminder in the commit log why
something was done.

BR, Jarkko