| Message ID | ZvwYbESMZ667QZqY@google.com |
|---|---|
| State | Accepted |
| Commit | 61595012f28036a58293df5a2ab75f80ca15c327 |
| Headers | show |
| Series | HID: simplify code in fetch_item() | expand |
On Tue, 01 Oct 2024 08:42:36 -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. > > Applied to hid/hid.git (for-6.13/core), thanks! [1/1] HID: simplify code in fetch_item() https://git.kernel.org/hid/hid/c/61595012f280 Cheers,
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
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.
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
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
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
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?
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
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?
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
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!
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; } static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
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(-)