Re: [PATCH] HID: simplify code in fetch_item()

Re: [PATCH] HID: simplify code in fetch_item()

[Nathan Chancellor]

Hi Dmitry,

On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> We can easily calculate the size of the item using arithmetic (shifts).
> This allows to pull duplicated code out of the switch statement, making
> it cleaner.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/hid/hid-core.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 988d0acbdf04..00942d40fe08 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -754,35 +754,32 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
>  	}
>  
>  	item->format = HID_ITEM_FORMAT_SHORT;
> -	item->size = b & 3;
> +	item->size = BIT(b & 3) >> 1; /* 0, 1, 2, 3 -> 0, 1, 2, 4 */
> +
> +	if (end - start < item->size)
> +		return NULL;
>  
>  	switch (item->size) {
>  	case 0:
> -		return start;
> +		break;
>  
>  	case 1:
> -		if ((end - start) < 1)
> -			return NULL;
> -		item->data.u8 = *start++;
> -		return start;
> +		item->data.u8 = *start;
> +		break;
>  
>  	case 2:
> -		if ((end - start) < 2)
> -			return NULL;
>  		item->data.u16 = get_unaligned_le16(start);
> -		start = (__u8 *)((__le16 *)start + 1);
> -		return start;
> +		break;
>  
> -	case 3:
> -		item->size++;
> -		if ((end - start) < 4)
> -			return NULL;
> +	case 4:
>  		item->data.u32 = get_unaligned_le32(start);
> -		start = (__u8 *)((__le32 *)start + 1);
> -		return start;
> +		break;
> +
> +	default:
> +		unreachable();
>  	}
>  
> -	return NULL;
> +	return start + item->size;
>  }

I am noticing some interesting behavior when building with clang, namely
some objtool warnings and a failed boot when LTO is enabled, which I
bisected to this change as commit 61595012f280 ("HID: simplify code in
fetch_item()"), such as:

  $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig vmlinux
  vmlinux.o: warning: objtool: hid_open_report() falls through to next function hid_parser_main()
  vmlinux.o: warning: objtool: hid_scan_report() falls through to next function hid_allocate_device()

With LTO enabled, the warning becomes:

  vmlinux.o: warning: objtool: hid_open_report+0x21b: can't find jump dest instruction at .text.hid_open_report+0x40f

A bare unreachable(), especially in the default case of a switch
statement, is generally considered harmful in my experience, as it can
introduce undefined behavior, which can mess up how a compiler might
optimize a function. Commit d652d5f1eeeb ("drm/edid: fix objtool warning
in drm_cvt_modes()") and commit 3764647b255a ("bcachefs: Remove
undefined behavior in bch2_dev_buckets_reserved()") have some good
commit messages talking about it.

Getting rid of the unreachable() in some way resolves the issue. I
tested using BUG() in lieu of unreachable() like the second change I
mentioned above, which resolves the issue cleanly, as the default case
clearly cannot happen. Another option I tested was some sort of printk
statement and returning NULL, which some maintainers prefer, even in
spite of impossible conditions. I am happy to send a patch with one of
those changes or open to other suggestions.

Cheers,
Nathan

Re: [PATCH] HID: simplify code in fetch_item()

[Dmitry Torokhov]

Hi Nathan,

On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> Hi Dmitry,
> 
> On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> > We can easily calculate the size of the item using arithmetic (shifts).
> > This allows to pull duplicated code out of the switch statement, making
> > it cleaner.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/hid/hid-core.c | 31 ++++++++++++++-----------------
> >  1 file changed, 14 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 988d0acbdf04..00942d40fe08 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -754,35 +754,32 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
> >  	}
> >  
> >  	item->format = HID_ITEM_FORMAT_SHORT;
> > -	item->size = b & 3;
> > +	item->size = BIT(b & 3) >> 1; /* 0, 1, 2, 3 -> 0, 1, 2, 4 */
> > +
> > +	if (end - start < item->size)
> > +		return NULL;
> >  
> >  	switch (item->size) {
> >  	case 0:
> > -		return start;
> > +		break;
> >  
> >  	case 1:
> > -		if ((end - start) < 1)
> > -			return NULL;
> > -		item->data.u8 = *start++;
> > -		return start;
> > +		item->data.u8 = *start;
> > +		break;
> >  
> >  	case 2:
> > -		if ((end - start) < 2)
> > -			return NULL;
> >  		item->data.u16 = get_unaligned_le16(start);
> > -		start = (__u8 *)((__le16 *)start + 1);
> > -		return start;
> > +		break;
> >  
> > -	case 3:
> > -		item->size++;
> > -		if ((end - start) < 4)
> > -			return NULL;
> > +	case 4:
> >  		item->data.u32 = get_unaligned_le32(start);
> > -		start = (__u8 *)((__le32 *)start + 1);
> > -		return start;
> > +		break;
> > +
> > +	default:
> > +		unreachable();
> >  	}
> >  
> > -	return NULL;
> > +	return start + item->size;
> >  }
> 
> I am noticing some interesting behavior when building with clang, namely
> some objtool warnings and a failed boot when LTO is enabled, which I
> bisected to this change as commit 61595012f280 ("HID: simplify code in
> fetch_item()"), such as:
> 
>   $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig vmlinux
>   vmlinux.o: warning: objtool: hid_open_report() falls through to next function hid_parser_main()
>   vmlinux.o: warning: objtool: hid_scan_report() falls through to next function hid_allocate_device()
> 
> With LTO enabled, the warning becomes:
> 
>   vmlinux.o: warning: objtool: hid_open_report+0x21b: can't find jump dest instruction at .text.hid_open_report+0x40f
> 
> A bare unreachable(), especially in the default case of a switch
> statement, is generally considered harmful in my experience, as it can
> introduce undefined behavior, which can mess up how a compiler might
> optimize a function. Commit d652d5f1eeeb ("drm/edid: fix objtool warning
> in drm_cvt_modes()") and commit 3764647b255a ("bcachefs: Remove
> undefined behavior in bch2_dev_buckets_reserved()") have some good
> commit messages talking about it.
> 
> Getting rid of the unreachable() in some way resolves the issue. I
> tested using BUG() in lieu of unreachable() like the second change I
> mentioned above, which resolves the issue cleanly, as the default case
> clearly cannot happen. Another option I tested was some sort of printk
> statement and returning NULL, which some maintainers prefer, even in
> spite of impossible conditions. I am happy to send a patch with one of
> those changes or open to other suggestions.

Oh well, if our toolchain does not like "unreachable()" then we can
simply remove it - the switch does cover all possible values and the
"return" statement should be valid even if compiler somehow decides that
"switch" statement can be skipped.

If you can send a patch that would be great.

I'm adding Paul and a few others to CC who apparently seeing the same
issue.

Thanks.

-- 
Dmitry

Re: [PATCH] HID: simplify code in fetch_item()

[Paul E. McKenney]

On Tue, Oct 15, 2024 at 11:28:26AM -0700, Dmitry Torokhov wrote:
> Hi Nathan,
> 
> On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> > Hi Dmitry,
> > 
> > On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> > > We can easily calculate the size of the item using arithmetic (shifts).
> > > This allows to pull duplicated code out of the switch statement, making
> > > it cleaner.
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/hid/hid-core.c | 31 ++++++++++++++-----------------
> > >  1 file changed, 14 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 988d0acbdf04..00942d40fe08 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -754,35 +754,32 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
> > >  	}
> > >  
> > >  	item->format = HID_ITEM_FORMAT_SHORT;
> > > -	item->size = b & 3;
> > > +	item->size = BIT(b & 3) >> 1; /* 0, 1, 2, 3 -> 0, 1, 2, 4 */
> > > +
> > > +	if (end - start < item->size)
> > > +		return NULL;
> > >  
> > >  	switch (item->size) {
> > >  	case 0:
> > > -		return start;
> > > +		break;
> > >  
> > >  	case 1:
> > > -		if ((end - start) < 1)
> > > -			return NULL;
> > > -		item->data.u8 = *start++;
> > > -		return start;
> > > +		item->data.u8 = *start;
> > > +		break;
> > >  
> > >  	case 2:
> > > -		if ((end - start) < 2)
> > > -			return NULL;
> > >  		item->data.u16 = get_unaligned_le16(start);
> > > -		start = (__u8 *)((__le16 *)start + 1);
> > > -		return start;
> > > +		break;
> > >  
> > > -	case 3:
> > > -		item->size++;
> > > -		if ((end - start) < 4)
> > > -			return NULL;
> > > +	case 4:
> > >  		item->data.u32 = get_unaligned_le32(start);
> > > -		start = (__u8 *)((__le32 *)start + 1);
> > > -		return start;
> > > +		break;
> > > +
> > > +	default:
> > > +		unreachable();
> > >  	}
> > >  
> > > -	return NULL;
> > > +	return start + item->size;
> > >  }
> > 
> > I am noticing some interesting behavior when building with clang, namely
> > some objtool warnings and a failed boot when LTO is enabled, which I
> > bisected to this change as commit 61595012f280 ("HID: simplify code in
> > fetch_item()"), such as:
> > 
> >   $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig vmlinux
> >   vmlinux.o: warning: objtool: hid_open_report() falls through to next function hid_parser_main()
> >   vmlinux.o: warning: objtool: hid_scan_report() falls through to next function hid_allocate_device()
> > 
> > With LTO enabled, the warning becomes:
> > 
> >   vmlinux.o: warning: objtool: hid_open_report+0x21b: can't find jump dest instruction at .text.hid_open_report+0x40f
> > 
> > A bare unreachable(), especially in the default case of a switch
> > statement, is generally considered harmful in my experience, as it can
> > introduce undefined behavior, which can mess up how a compiler might
> > optimize a function. Commit d652d5f1eeeb ("drm/edid: fix objtool warning
> > in drm_cvt_modes()") and commit 3764647b255a ("bcachefs: Remove
> > undefined behavior in bch2_dev_buckets_reserved()") have some good
> > commit messages talking about it.
> > 
> > Getting rid of the unreachable() in some way resolves the issue. I
> > tested using BUG() in lieu of unreachable() like the second change I
> > mentioned above, which resolves the issue cleanly, as the default case
> > clearly cannot happen. Another option I tested was some sort of printk
> > statement and returning NULL, which some maintainers prefer, even in
> > spite of impossible conditions. I am happy to send a patch with one of
> > those changes or open to other suggestions.
> 
> Oh well, if our toolchain does not like "unreachable()" then we can
> simply remove it - the switch does cover all possible values and the
> "return" statement should be valid even if compiler somehow decides that
> "switch" statement can be skipped.
> 
> If you can send a patch that would be great.
> 
> I'm adding Paul and a few others to CC who apparently seeing the same
> issue.

Commenting out the unreachable() fixes things for me, as does
replacing the unreachable() with BUG().  So, for either solution:

Tested-by: Paul E. McKenney <paulmck@kernel.org>

							Thanx, Paul

Re: [PATCH] HID: simplify code in fetch_item()

[Nathan Chancellor]

On Tue, Oct 15, 2024 at 11:28:26AM -0700, Dmitry Torokhov wrote:
> Oh well, if our toolchain does not like "unreachable()" then we can
> simply remove it - the switch does cover all possible values and the
> "return" statement should be valid even if compiler somehow decides that
> "switch" statement can be skipped.
> 
> If you can send a patch that would be great.

Done, thanks a lot for the input!

https://lore.kernel.org/20241015-hid-fix-fetch_item-unreachable-v1-1-b131cd10dbd1@kernel.org/

Cheers,
Nathan

Re: [PATCH] HID: simplify code in fetch_item()

[Segher Boessenkool]

On Tue, Oct 15, 2024 at 12:26:04PM -0700, Nathan Chancellor wrote:
> On Tue, Oct 15, 2024 at 11:28:26AM -0700, Dmitry Torokhov wrote:
> > Oh well, if our toolchain does not like "unreachable()" then we can
> > simply remove it - the switch does cover all possible values and the
> > "return" statement should be valid even if compiler somehow decides that
> > "switch" statement can be skipped.
> > 
> > If you can send a patch that would be great.
> 
> Done, thanks a lot for the input!
> 
> https://lore.kernel.org/20241015-hid-fix-fetch_item-unreachable-v1-1-b131cd10dbd1@kernel.org/

There also is -funreachable-traps, which the kernel might want to use
(with that option every builtin_unreachablei() is compiled to a trap
instruction, instead of that the compiler just thinks "Aha!  This can
never happen!", and optimise based on that).


Segher

Re: [PATCH] HID: simplify code in fetch_item()

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:

...

> Getting rid of the unreachable() in some way resolves the issue. I
> tested using BUG() in lieu of unreachable() like the second change I
> mentioned above, which resolves the issue cleanly, as the default case
> clearly cannot happen. ...

As Dmitry pointed out to this old discussion, I have a question about the above
test. Have you tried to use BUG() while CONFIG_BUG=n? Does it _also_ solve the
issue?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] HID: simplify code in fetch_item()

[Nathan Chancellor]

On Mon, Apr 14, 2025 at 09:30:36AM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> > On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > Getting rid of the unreachable() in some way resolves the issue. I
> > tested using BUG() in lieu of unreachable() like the second change I
> > mentioned above, which resolves the issue cleanly, as the default case
> > clearly cannot happen. ...
> 
> As Dmitry pointed out to this old discussion, I have a question about the above
> test. Have you tried to use BUG() while CONFIG_BUG=n? Does it _also_ solve the
> issue?

Yes because x86 appears to always emit ud2 for BUG() regardless of
whether CONFIG_BUG is set or not since HAVE_ARCH_BUG is always
respected.

Cheers,
Nathan

Re: [PATCH] HID: simplify code in fetch_item()

[Andy Shevchenko]

On Mon, Apr 14, 2025 at 05:33:26PM -0700, Nathan Chancellor wrote:
> On Mon, Apr 14, 2025 at 09:30:36AM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> > > On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:

...

> > > Getting rid of the unreachable() in some way resolves the issue. I
> > > tested using BUG() in lieu of unreachable() like the second change I
> > > mentioned above, which resolves the issue cleanly, as the default case
> > > clearly cannot happen. ...
> > 
> > As Dmitry pointed out to this old discussion, I have a question about the above
> > test. Have you tried to use BUG() while CONFIG_BUG=n? Does it _also_ solve the
> > issue?
> 
> Yes because x86 appears to always emit ud2 for BUG() regardless of
> whether CONFIG_BUG is set or not since HAVE_ARCH_BUG is always
> respected.

Thank you for the reply. But do you know if this is guaranteed on the rest of
supported architectures? I.o.w. may we assume that BUG() in lieu of unreachable()
will always fix the issue?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] HID: simplify code in fetch_item()

[Nathan Chancellor]

On Tue, Apr 15, 2025 at 09:45:58AM +0300, Andy Shevchenko wrote:
> On Mon, Apr 14, 2025 at 05:33:26PM -0700, Nathan Chancellor wrote:
> > On Mon, Apr 14, 2025 at 09:30:36AM +0300, Andy Shevchenko wrote:
> > > On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> > > > On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > Getting rid of the unreachable() in some way resolves the issue. I
> > > > tested using BUG() in lieu of unreachable() like the second change I
> > > > mentioned above, which resolves the issue cleanly, as the default case
> > > > clearly cannot happen. ...
> > > 
> > > As Dmitry pointed out to this old discussion, I have a question about the above
> > > test. Have you tried to use BUG() while CONFIG_BUG=n? Does it _also_ solve the
> > > issue?
> > 
> > Yes because x86 appears to always emit ud2 for BUG() regardless of
> > whether CONFIG_BUG is set or not since HAVE_ARCH_BUG is always
> > respected.
> 
> Thank you for the reply. But do you know if this is guaranteed on the rest of
> supported architectures? I.o.w. may we assume that BUG() in lieu of unreachable()
> will always fix the issue?

I don't know. As far as I can tell, BUG() is always better than a bare
unreachable() because it is either the same as unreachable() if the
architecture does not define HAVE_ARCH_BUG and CONFIG_BUG=n (and in the
case of CONFIG_BUG=n, I think the user should get to pick up the pieces)
or when CONFIG_BUG=y and/or HAVE_ARCH_BUG is defined, the unreachable()
will truly be unreachable in the control flow graph because of the trap
or __noreturn from BUG(), so no undefined behavior. I think you would
only be able to find cases where BUG() was not sufficient to avoid
undefined behavior at runtime instead of compile time, as objtool only
supports loongarch and x86 right now and both ensure BUG() always traps.
I might be missing something though.

Cheers,
Nathan

Re: [PATCH] HID: simplify code in fetch_item()

[Andy Shevchenko]

On Tue, Apr 15, 2025 at 08:21:49AM -0700, Nathan Chancellor wrote:
> On Tue, Apr 15, 2025 at 09:45:58AM +0300, Andy Shevchenko wrote:
> > On Mon, Apr 14, 2025 at 05:33:26PM -0700, Nathan Chancellor wrote:
> > > On Mon, Apr 14, 2025 at 09:30:36AM +0300, Andy Shevchenko wrote:
> > > > On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> > > > > On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:

...

> > > > > Getting rid of the unreachable() in some way resolves the issue. I
> > > > > tested using BUG() in lieu of unreachable() like the second change I
> > > > > mentioned above, which resolves the issue cleanly, as the default case
> > > > > clearly cannot happen. ...
> > > > 
> > > > As Dmitry pointed out to this old discussion, I have a question about the above
> > > > test. Have you tried to use BUG() while CONFIG_BUG=n? Does it _also_ solve the
> > > > issue?
> > > 
> > > Yes because x86 appears to always emit ud2 for BUG() regardless of
> > > whether CONFIG_BUG is set or not since HAVE_ARCH_BUG is always
> > > respected.
> > 
> > Thank you for the reply. But do you know if this is guaranteed on the rest of
> > supported architectures? I.o.w. may we assume that BUG() in lieu of unreachable()
> > will always fix the issue?
> 
> I don't know. As far as I can tell, BUG() is always better than a bare
> unreachable() because it is either the same as unreachable() if the
> architecture does not define HAVE_ARCH_BUG and CONFIG_BUG=n (and in the
> case of CONFIG_BUG=n, I think the user should get to pick up the pieces)
> or when CONFIG_BUG=y and/or HAVE_ARCH_BUG is defined, the unreachable()
> will truly be unreachable in the control flow graph because of the trap
> or __noreturn from BUG(), so no undefined behavior. I think you would
> only be able to find cases where BUG() was not sufficient to avoid
> undefined behavior at runtime instead of compile time, as objtool only
> supports loongarch and x86 right now and both ensure BUG() always traps.
> I might be missing something though.

Thank you for this information!

-- 
With Best Regards,
Andy Shevchenko