| Message ID | ZvwYbESMZ667QZqY@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| 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
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(-)