lib/bootconfig.c:322:25: warning: comparison of integer expressions
  of different signedness: 'int' and 'size_t' [-Wsign-compare]
  lib/bootconfig.c:325:30: warning: conversion to 'size_t' from 'int'
  may change the sign of the result [-Wsign-conversion]

snprintf() returns int but size is size_t, so comparing ret >= size
and subtracting size -= ret involve mixed-sign operations.  Cast ret
at the comparison and subtraction sites; ret is known non-negative at
this point because the ret < 0 early return has already been taken.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index e318b236e728..68a72dbc38fa 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
 			       depth ? "." : "");
 		if (ret < 0)
 			return ret;
-		if (ret >= size) {
+		if (ret >= (int)size) {
 			size = 0;
 		} else {
-			size -= ret;
+			size -= (size_t)ret;
 			buf += ret;
 		}
 		total += ret;
-- 
2.34.1

On Sun, 15 Mar 2026 12:20:14 +0000
Josh Law <objecting@objecting.org> wrote:

>   lib/bootconfig.c:322:25: warning: comparison of integer expressions
>   of different signedness: 'int' and 'size_t' [-Wsign-compare]
>   lib/bootconfig.c:325:30: warning: conversion to 'size_t' from 'int'
>   may change the sign of the result [-Wsign-conversion]
> 
> snprintf() returns int but size is size_t, so comparing ret >= size
> and subtracting size -= ret involve mixed-sign operations.  Cast ret
> at the comparison and subtraction sites; ret is known non-negative at
> this point because the ret < 0 early return has already been taken.
> 
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
>  lib/bootconfig.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index e318b236e728..68a72dbc38fa 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>  			       depth ? "." : "");
>  		if (ret < 0)
>  			return ret;
> -		if (ret >= size) {
> +		if (ret >= (int)size) {

nit:

	if ((size_t)ret >= size) {

because sizeof(size_t) > sizeof(int).

Thanks,

>  			size = 0;
>  		} else {
> -			size -= ret;
> +			size -= (size_t)ret;
>  			buf += ret;
>  		}
>  		total += ret;
> -- 
> 2.34.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Tue, 17 Mar 2026 16:55:49 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > --- a/lib/bootconfig.c
> > +++ b/lib/bootconfig.c
> > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
> >  			       depth ? "." : "");
> >  		if (ret < 0)
> >  			return ret;
> > -		if (ret >= size) {
> > +		if (ret >= (int)size) {  
> 
> nit:
> 
> 	if ((size_t)ret >= size) {
> 
> because sizeof(size_t) > sizeof(int).

I don't think we need to worry about this. But this does bring up an issue.
ret comes from:

		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
			       depth ? "." : "");

Where size is of type size_t

snprintf() takes size_t but returns int.

snprintf() calls vsnprintf() which has:

	size_t len, pos;

Where pos is incremented based on fmt, and vsnprintf() returns:

	return pos;

Which can overflow.

Now, honestly, we should never have a 2Gig string as that would likely
cause other horrible things. Does size really need to be size_t?

Perhaps we should have:

	if (WARN_ON_ONCE(size > MAX_INT))
		return -EINVAL;

?

-- Steve

On Tue, 17 Mar 2026 12:15:07 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 17 Mar 2026 16:55:49 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > --- a/lib/bootconfig.c
> > > +++ b/lib/bootconfig.c
> > > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
> > >  			       depth ? "." : "");
> > >  		if (ret < 0)
> > >  			return ret;
> > > -		if (ret >= size) {
> > > +		if (ret >= (int)size) {  
> > 
> > nit:
> > 
> > 	if ((size_t)ret >= size) {
> > 
> > because sizeof(size_t) > sizeof(int).
> 
> I don't think we need to worry about this. But this does bring up an issue.
> ret comes from:
> 
> 		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
> 			       depth ? "." : "");
> 
> Where size is of type size_t
> 
> snprintf() takes size_t but returns int.
> 
> snprintf() calls vsnprintf() which has:
> 
> 	size_t len, pos;
> 
> Where pos is incremented based on fmt, and vsnprintf() returns:
> 
> 	return pos;
> 
> Which can overflow.

I think that is vsnprintf() (maybe POSIX) design issue.
I believe we're simply using the size_t to represent size of memory
out of convention.

> 
> Now, honestly, we should never have a 2Gig string as that would likely
> cause other horrible things. Does size really need to be size_t?

Even if so, it should be done in vsnprintf() instead of this.
This function just believes that the caller gives collect size
and enough amount of memory. Or, we need to check "INT_MAX > size"
in everywhere.

> 
> Perhaps we should have:
> 
> 	if (WARN_ON_ONCE(size > MAX_INT))
> 		return -EINVAL;

I think this is an over engineering effort especially in
caller side. This overflow should be checked in vsnprintf() and
should return -EINVAL. (and the caller checks the return value.)

Thank you,

> 
> ?
> 
> -- Steve
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On 17 March 2026 23:15:40 GMT, Masami Hiramatsu <mhiramat@kernel.org> wrote:
>On Tue, 17 Mar 2026 12:15:07 -0400
>Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Tue, 17 Mar 2026 16:55:49 +0900
>> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>> 
>> > > --- a/lib/bootconfig.c
>> > > +++ b/lib/bootconfig.c
>> > > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>> > >  			       depth ? "." : "");
>> > >  		if (ret < 0)
>> > >  			return ret;
>> > > -		if (ret >= size) {
>> > > +		if (ret >= (int)size) {  
>> > 
>> > nit:
>> > 
>> > 	if ((size_t)ret >= size) {
>> > 
>> > because sizeof(size_t) > sizeof(int).
>> 
>> I don't think we need to worry about this. But this does bring up an issue.
>> ret comes from:
>> 
>> 		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
>> 			       depth ? "." : "");
>> 
>> Where size is of type size_t
>> 
>> snprintf() takes size_t but returns int.
>> 
>> snprintf() calls vsnprintf() which has:
>> 
>> 	size_t len, pos;
>> 
>> Where pos is incremented based on fmt, and vsnprintf() returns:
>> 
>> 	return pos;
>> 
>> Which can overflow.
>
>I think that is vsnprintf() (maybe POSIX) design issue.
>I believe we're simply using the size_t to represent size of memory
>out of convention.
>
>> 
>> Now, honestly, we should never have a 2Gig string as that would likely
>> cause other horrible things. Does size really need to be size_t?
>
>Even if so, it should be done in vsnprintf() instead of this.
>This function just believes that the caller gives collect size
>and enough amount of memory. Or, we need to check "INT_MAX > size"
>in everywhere.
>
>> 
>> Perhaps we should have:
>> 
>> 	if (WARN_ON_ONCE(size > MAX_INT))
>> 		return -EINVAL;
>
>I think this is an over engineering effort especially in
>caller side. This overflow should be checked in vsnprintf() and
>should return -EINVAL. (and the caller checks the return value.)
>
>Thank you,
>
>> 
>> ?
>> 
>> -- Steve
>> 
>
>


I submitted V7 dropping all them patches anyway, V7 should be perfect now.


V/R


Josh Law

On 17 March 2026 16:15:07 GMT, Steven Rostedt <rostedt@goodmis.org> wrote:
>On Tue, 17 Mar 2026 16:55:49 +0900
>Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>
>> > --- a/lib/bootconfig.c
>> > +++ b/lib/bootconfig.c
>> > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>> >  			       depth ? "." : "");
>> >  		if (ret < 0)
>> >  			return ret;
>> > -		if (ret >= size) {
>> > +		if (ret >= (int)size) {  
>> 
>> nit:
>> 
>> 	if ((size_t)ret >= size) {
>> 
>> because sizeof(size_t) > sizeof(int).
>
>I don't think we need to worry about this. But this does bring up an issue.
>ret comes from:
>
>		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
>			       depth ? "." : "");
>
>Where size is of type size_t
>
>snprintf() takes size_t but returns int.
>
>snprintf() calls vsnprintf() which has:
>
>	size_t len, pos;
>
>Where pos is incremented based on fmt, and vsnprintf() returns:
>
>	return pos;
>
>Which can overflow.
>
>Now, honestly, we should never have a 2Gig string as that would likely
>cause other horrible things. Does size really need to be size_t?
>
>Perhaps we should have:
>
>	if (WARN_ON_ONCE(size > MAX_INT))
>		return -EINVAL;
>
>?
>
>-- Steve



I'm making a separate patch based on this.

V/R

Josh Law

On 17 March 2026 16:15:07 GMT, Steven Rostedt <rostedt@goodmis.org> wrote:
>On Tue, 17 Mar 2026 16:55:49 +0900
>Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>
>> > --- a/lib/bootconfig.c
>> > +++ b/lib/bootconfig.c
>> > @@ -319,10 +319,10 @@ int __init xbc_node_compose_key_after(struct xbc_node *root,
>> >  			       depth ? "." : "");
>> >  		if (ret < 0)
>> >  			return ret;
>> > -		if (ret >= size) {
>> > +		if (ret >= (int)size) {  
>> 
>> nit:
>> 
>> 	if ((size_t)ret >= size) {
>> 
>> because sizeof(size_t) > sizeof(int).
>
>I don't think we need to worry about this. But this does bring up an issue.
>ret comes from:
>
>		ret = snprintf(buf, size, "%s%s", xbc_node_get_data(node),
>			       depth ? "." : "");
>
>Where size is of type size_t
>
>snprintf() takes size_t but returns int.
>
>snprintf() calls vsnprintf() which has:
>
>	size_t len, pos;
>
>Where pos is incremented based on fmt, and vsnprintf() returns:
>
>	return pos;
>
>Which can overflow.
>
>Now, honestly, we should never have a 2Gig string as that would likely
>cause other horrible things. Does size really need to be size_t?
>
>Perhaps we should have:
>
>	if (WARN_ON_ONCE(size > MAX_INT))
>		return -EINVAL;
>
>?
>
>-- Steve


Hello Steven! I made a V7 dropping that since masami nitted it anyway, and I was too busy to fix it.

V/R


If you would like to review V7, go right ahead!