* [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
@ 2025-10-14 1:05 Nathan Chancellor
2025-10-14 20:33 ` Bjorn Helgaas
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Nathan Chancellor @ 2025-10-14 1:05 UTC (permalink / raw)
To: Mika Westerberg, Andy Shevchenko, Jan Dabros, Andi Shyti
Cc: Nick Desaulniers, Bill Wendling, Justin Stitt, Kees Cook,
Sami Tolvanen, linux-i2c, linux-kernel, llvm, Nathan Chancellor
When building certain configurations with CONFIG_FINEIBT=y after
commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
warning due to an indirect call in dw_i2c_plat_remove():
$ cat allno.config
CONFIG_ACPI=y
CONFIG_CFI=y
CONFIG_COMMON_CLK=y
CONFIG_CPU_MITIGATIONS=y
CONFIG_I2C=y
CONFIG_I2C_DESIGNWARE_BAYTRAIL=y
CONFIG_I2C_DESIGNWARE_CORE=y
CONFIG_I2C_DESIGNWARE_PLATFORM=y
CONFIG_IOSF_MBI=y
CONFIG_MITIGATION_RETPOLINE=y
CONFIG_MODULES=y
CONFIG_PCI=y
CONFIG_X86_KERNEL_IBT=y
$ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 clean allnoconfig vmlinux
vmlinux.o: warning: objtool: dw_i2c_plat_remove+0x3c: no-cfi indirect call!
With this configuration, i2c_dw_semaphore_cb_table has the BAYTRAIL
member and the sentinel (i.e., 2 members), both of which have an
implicit
.remove = NULL,
so Clang effectively turns i2c_dw_remove_lock_support(), which is later
inlined into dw_i2c_plat_remove(), into:
static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
{
if (dev->semaphore_idx > 2)
(*NULL)(dev):
}
which is not necessarily problematic from a logic perspective (as the
code was not bounds checking semaphore_idx so an out of bounds index
could already crash) but objtool's new __nocfi indirect call checking
trips over Clang dropping the kCFI setup from a known NULL indirect
call.
While it would be possible to fix this by transforming the initial check
into
if (dev->semaphore_idx < 0 || dev->semaphore_idx >= ARRAY_SIZE(i2c_dw_semaphore_cb_table))
the remove member is unused after commit 440da737cf8d ("i2c: designware:
Use PCI PSP driver for communication"), so i2c_dw_remove_lock_support()
can be removed altogether, as it will never actually do anything.
Closes: https://github.com/ClangBuiltLinux/linux/issues/2133
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
drivers/i2c/busses/i2c-designware-core.h | 1 -
drivers/i2c/busses/i2c-designware-platdrv.c | 11 -----------
2 files changed, 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 347843b4f5dd..d50664377c6b 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -330,7 +330,6 @@ struct dw_i2c_dev {
struct i2c_dw_semaphore_callbacks {
int (*probe)(struct dw_i2c_dev *dev);
- void (*remove)(struct dw_i2c_dev *dev);
};
int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 34d881572351..cff7e03dea7b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -197,15 +197,6 @@ static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
return 0;
}
-static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
-{
- if (dev->semaphore_idx < 0)
- return;
-
- if (i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove)
- i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove(dev);
-}
-
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
@@ -339,8 +330,6 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
i2c_dw_prepare_clk(dev, false);
- i2c_dw_remove_lock_support(dev);
-
reset_control_assert(dev->rst);
}
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-5f2040eaadc2
Best regards,
--
Nathan Chancellor <nathan@kernel.org>
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-14 1:05 [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support() Nathan Chancellor
@ 2025-10-14 20:33 ` Bjorn Helgaas
2025-10-14 21:58 ` Mario Limonciello (AMD) (kernel.org)
2025-10-14 21:10 ` Kees Cook
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2025-10-14 20:33 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Mika Westerberg, Andy Shevchenko, Jan Dabros, Andi Shyti,
Nick Desaulniers, Bill Wendling, Justin Stitt, Kees Cook,
Sami Tolvanen, Mario Limonciello, linux-i2c, linux-kernel, llvm
[+cc Mario, author of 440da737cf8d ("i2c: designware: Use PCI PSP
driver for communication")]
On Mon, Oct 13, 2025 at 06:05:03PM -0700, Nathan Chancellor wrote:
> When building certain configurations with CONFIG_FINEIBT=y after
> commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
> warning due to an indirect call in dw_i2c_plat_remove():
>
> $ cat allno.config
> CONFIG_ACPI=y
> CONFIG_CFI=y
> CONFIG_COMMON_CLK=y
> CONFIG_CPU_MITIGATIONS=y
> CONFIG_I2C=y
> CONFIG_I2C_DESIGNWARE_BAYTRAIL=y
> CONFIG_I2C_DESIGNWARE_CORE=y
> CONFIG_I2C_DESIGNWARE_PLATFORM=y
> CONFIG_IOSF_MBI=y
> CONFIG_MITIGATION_RETPOLINE=y
> CONFIG_MODULES=y
> CONFIG_PCI=y
> CONFIG_X86_KERNEL_IBT=y
>
> $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 clean allnoconfig vmlinux
> vmlinux.o: warning: objtool: dw_i2c_plat_remove+0x3c: no-cfi indirect call!
>
> With this configuration, i2c_dw_semaphore_cb_table has the BAYTRAIL
> member and the sentinel (i.e., 2 members), both of which have an
> implicit
>
> .remove = NULL,
>
> so Clang effectively turns i2c_dw_remove_lock_support(), which is later
> inlined into dw_i2c_plat_remove(), into:
>
> static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> {
> if (dev->semaphore_idx > 2)
> (*NULL)(dev):
> }
>
> which is not necessarily problematic from a logic perspective (as the
> code was not bounds checking semaphore_idx so an out of bounds index
> could already crash) but objtool's new __nocfi indirect call checking
> trips over Clang dropping the kCFI setup from a known NULL indirect
> call.
>
> While it would be possible to fix this by transforming the initial check
> into
>
> if (dev->semaphore_idx < 0 || dev->semaphore_idx >= ARRAY_SIZE(i2c_dw_semaphore_cb_table))
>
> the remove member is unused after commit 440da737cf8d ("i2c: designware:
> Use PCI PSP driver for communication"), so i2c_dw_remove_lock_support()
> can be removed altogether, as it will never actually do anything.
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2133
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
I'm totally fine with the patch itself, but I think the commit log
could be trimmed to something like the following with no loss:
Remove struct i2c_dw_semaphore_callbacks.remove() and
i2c_dw_remove_lock_support().
440da737cf8d ("i2c: designware: Use PCI PSP driver for
communication") removed the last place that set
i2c_dw_semaphore_callbacks.remove(), which made
i2c_dw_remove_lock_support() a no-op.
This has the side effect of avoiding this kCFI warning (see Link):
dw_i2c_plat_remove+0x3c: no-cfi indirect call!
Link: https://lore.kernel.org/r/20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-v1-1-8cc4842967bf@kernel.org
FWIW,
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/i2c/busses/i2c-designware-core.h | 1 -
> drivers/i2c/busses/i2c-designware-platdrv.c | 11 -----------
> 2 files changed, 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 347843b4f5dd..d50664377c6b 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -330,7 +330,6 @@ struct dw_i2c_dev {
>
> struct i2c_dw_semaphore_callbacks {
> int (*probe)(struct dw_i2c_dev *dev);
> - void (*remove)(struct dw_i2c_dev *dev);
> };
>
> int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 34d881572351..cff7e03dea7b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -197,15 +197,6 @@ static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
> return 0;
> }
>
> -static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> -{
> - if (dev->semaphore_idx < 0)
> - return;
> -
> - if (i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove)
> - i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove(dev);
> -}
> -
> static int dw_i2c_plat_probe(struct platform_device *pdev)
> {
> u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> @@ -339,8 +330,6 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
>
> i2c_dw_prepare_clk(dev, false);
>
> - i2c_dw_remove_lock_support(dev);
> -
> reset_control_assert(dev->rst);
> }
>
>
> ---
> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
> change-id: 20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-5f2040eaadc2
>
> Best regards,
> --
> Nathan Chancellor <nathan@kernel.org>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-14 1:05 [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support() Nathan Chancellor
2025-10-14 20:33 ` Bjorn Helgaas
@ 2025-10-14 21:10 ` Kees Cook
2025-10-15 20:47 ` Andi Shyti
2025-10-23 6:29 ` Andy Shevchenko
3 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2025-10-14 21:10 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Mika Westerberg, Andy Shevchenko, Jan Dabros, Andi Shyti,
Nick Desaulniers, Bill Wendling, Justin Stitt, Sami Tolvanen,
linux-i2c, linux-kernel, llvm
On Mon, Oct 13, 2025 at 06:05:03PM -0700, Nathan Chancellor wrote:
> When building certain configurations with CONFIG_FINEIBT=y after
> commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
> warning due to an indirect call in dw_i2c_plat_remove():
>
> $ cat allno.config
> CONFIG_ACPI=y
> CONFIG_CFI=y
> CONFIG_COMMON_CLK=y
> CONFIG_CPU_MITIGATIONS=y
> CONFIG_I2C=y
> CONFIG_I2C_DESIGNWARE_BAYTRAIL=y
> CONFIG_I2C_DESIGNWARE_CORE=y
> CONFIG_I2C_DESIGNWARE_PLATFORM=y
> CONFIG_IOSF_MBI=y
> CONFIG_MITIGATION_RETPOLINE=y
> CONFIG_MODULES=y
> CONFIG_PCI=y
> CONFIG_X86_KERNEL_IBT=y
>
> $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 clean allnoconfig vmlinux
> vmlinux.o: warning: objtool: dw_i2c_plat_remove+0x3c: no-cfi indirect call!
>
> With this configuration, i2c_dw_semaphore_cb_table has the BAYTRAIL
> member and the sentinel (i.e., 2 members), both of which have an
> implicit
>
> .remove = NULL,
>
> so Clang effectively turns i2c_dw_remove_lock_support(), which is later
> inlined into dw_i2c_plat_remove(), into:
>
> static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> {
> if (dev->semaphore_idx > 2)
> (*NULL)(dev):
> }
>
> which is not necessarily problematic from a logic perspective (as the
> code was not bounds checking semaphore_idx so an out of bounds index
> could already crash) but objtool's new __nocfi indirect call checking
> trips over Clang dropping the kCFI setup from a known NULL indirect
> call.
>
> While it would be possible to fix this by transforming the initial check
> into
>
> if (dev->semaphore_idx < 0 || dev->semaphore_idx >= ARRAY_SIZE(i2c_dw_semaphore_cb_table))
>
> the remove member is unused after commit 440da737cf8d ("i2c: designware:
> Use PCI PSP driver for communication"), so i2c_dw_remove_lock_support()
> can be removed altogether, as it will never actually do anything.
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2133
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Thanks for the analysis!
Reviewed-by: Kees Cook <kees@kernel.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-14 20:33 ` Bjorn Helgaas
@ 2025-10-14 21:58 ` Mario Limonciello (AMD) (kernel.org)
2025-10-14 22:39 ` Nathan Chancellor
0 siblings, 1 reply; 9+ messages in thread
From: Mario Limonciello (AMD) (kernel.org) @ 2025-10-14 21:58 UTC (permalink / raw)
To: Bjorn Helgaas, Nathan Chancellor
Cc: Mika Westerberg, Andy Shevchenko, Jan Dabros, Andi Shyti,
Nick Desaulniers, Bill Wendling, Justin Stitt, Kees Cook,
Sami Tolvanen, linux-i2c, linux-kernel, llvm
On 10/14/2025 3:33 PM, Bjorn Helgaas wrote:
> [+cc Mario, author of 440da737cf8d ("i2c: designware: Use PCI PSP
> driver for communication")]
>
> On Mon, Oct 13, 2025 at 06:05:03PM -0700, Nathan Chancellor wrote:
>> When building certain configurations with CONFIG_FINEIBT=y after
>> commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
>> warning due to an indirect call in dw_i2c_plat_remove():
>>
>> $ cat allno.config
>> CONFIG_ACPI=y
>> CONFIG_CFI=y
>> CONFIG_COMMON_CLK=y
>> CONFIG_CPU_MITIGATIONS=y
>> CONFIG_I2C=y
>> CONFIG_I2C_DESIGNWARE_BAYTRAIL=y
>> CONFIG_I2C_DESIGNWARE_CORE=y
>> CONFIG_I2C_DESIGNWARE_PLATFORM=y
>> CONFIG_IOSF_MBI=y
>> CONFIG_MITIGATION_RETPOLINE=y
>> CONFIG_MODULES=y
>> CONFIG_PCI=y
>> CONFIG_X86_KERNEL_IBT=y
>>
>> $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 clean allnoconfig vmlinux
>> vmlinux.o: warning: objtool: dw_i2c_plat_remove+0x3c: no-cfi indirect call!
>>
>> With this configuration, i2c_dw_semaphore_cb_table has the BAYTRAIL
>> member and the sentinel (i.e., 2 members), both of which have an
>> implicit
>>
>> .remove = NULL,
>>
>> so Clang effectively turns i2c_dw_remove_lock_support(), which is later
>> inlined into dw_i2c_plat_remove(), into:
>>
>> static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>> {
>> if (dev->semaphore_idx > 2)
>> (*NULL)(dev):
>> }
>>
>> which is not necessarily problematic from a logic perspective (as the
>> code was not bounds checking semaphore_idx so an out of bounds index
>> could already crash) but objtool's new __nocfi indirect call checking
>> trips over Clang dropping the kCFI setup from a known NULL indirect
>> call.
>>
>> While it would be possible to fix this by transforming the initial check
>> into
>>
>> if (dev->semaphore_idx < 0 || dev->semaphore_idx >= ARRAY_SIZE(i2c_dw_semaphore_cb_table))
>>
>> the remove member is unused after commit 440da737cf8d ("i2c: designware:
>> Use PCI PSP driver for communication"), so i2c_dw_remove_lock_support()
>> can be removed altogether, as it will never actually do anything.
>>
>> Closes: https://github.com/ClangBuiltLinux/linux/issues/2133
>> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
>
> I'm totally fine with the patch itself, but I think the commit log
> could be trimmed to something like the following with no loss:
>
> Remove struct i2c_dw_semaphore_callbacks.remove() and
> i2c_dw_remove_lock_support().
>
> 440da737cf8d ("i2c: designware: Use PCI PSP driver for
> communication") removed the last place that set
> i2c_dw_semaphore_callbacks.remove(), which made
> i2c_dw_remove_lock_support() a no-op.
>
> This has the side effect of avoiding this kCFI warning (see Link):
>
> dw_i2c_plat_remove+0x3c: no-cfi indirect call!
>
> Link: https://lore.kernel.org/r/20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-v1-1-8cc4842967bf@kernel.org
>
> FWIW,
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
I echo Bjorn's comments on the lengthy commit message.
Code change looks fine.
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>
>> ---
>> drivers/i2c/busses/i2c-designware-core.h | 1 -
>> drivers/i2c/busses/i2c-designware-platdrv.c | 11 -----------
>> 2 files changed, 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
>> index 347843b4f5dd..d50664377c6b 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -330,7 +330,6 @@ struct dw_i2c_dev {
>>
>> struct i2c_dw_semaphore_callbacks {
>> int (*probe)(struct dw_i2c_dev *dev);
>> - void (*remove)(struct dw_i2c_dev *dev);
>> };
>>
>> int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 34d881572351..cff7e03dea7b 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -197,15 +197,6 @@ static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
>> return 0;
>> }
>>
>> -static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>> -{
>> - if (dev->semaphore_idx < 0)
>> - return;
>> -
>> - if (i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove)
>> - i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove(dev);
>> -}
>> -
>> static int dw_i2c_plat_probe(struct platform_device *pdev)
>> {
>> u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
>> @@ -339,8 +330,6 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
>>
>> i2c_dw_prepare_clk(dev, false);
>>
>> - i2c_dw_remove_lock_support(dev);
>> -
>> reset_control_assert(dev->rst);
>> }
>>
>>
>> ---
>> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
>> change-id: 20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-5f2040eaadc2
>>
>> Best regards,
>> --
>> Nathan Chancellor <nathan@kernel.org>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-14 21:58 ` Mario Limonciello (AMD) (kernel.org)
@ 2025-10-14 22:39 ` Nathan Chancellor
2025-10-14 22:53 ` Bjorn Helgaas
0 siblings, 1 reply; 9+ messages in thread
From: Nathan Chancellor @ 2025-10-14 22:39 UTC (permalink / raw)
To: Mario Limonciello (AMD) (kernel.org)
Cc: Bjorn Helgaas, Mika Westerberg, Andy Shevchenko, Jan Dabros,
Andi Shyti, Nick Desaulniers, Bill Wendling, Justin Stitt,
Kees Cook, Sami Tolvanen, linux-i2c, linux-kernel, llvm
On Tue, Oct 14, 2025 at 04:58:56PM -0500, Mario Limonciello (AMD) (kernel.org) wrote:
> On 10/14/2025 3:33 PM, Bjorn Helgaas wrote:
> > I'm totally fine with the patch itself, but I think the commit log
> > could be trimmed to something like the following with no loss:
> >
> > Remove struct i2c_dw_semaphore_callbacks.remove() and
> > i2c_dw_remove_lock_support().
> >
> > 440da737cf8d ("i2c: designware: Use PCI PSP driver for
> > communication") removed the last place that set
> > i2c_dw_semaphore_callbacks.remove(), which made
> > i2c_dw_remove_lock_support() a no-op.
> >
> > This has the side effect of avoiding this kCFI warning (see Link):
> >
> > dw_i2c_plat_remove+0x3c: no-cfi indirect call!
> >
> > Link: https://lore.kernel.org/r/20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-v1-1-8cc4842967bf@kernel.org
> >
> > FWIW,
> > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I echo Bjorn's comments on the lengthy commit message.
> Code change looks fine.
>
> Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
I have no objections to trimming the commit message if so desired but I
think the solution (removing unused code) is more tangential to the
problem (potentially accessing an array out of bounds). I am sometimes
looking at changes from ten years ago where something was done to avoid
a problem but the problem was never mentioned in the message but may
have been elsewhere. Maybe nobody ever needs .remove() again but what if
new IP comes out that necessitates it and they go to revert this change
without avoiding this problem? I could try to make the analysis shorter
if that would help.
Cheers,
Nathan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-14 22:39 ` Nathan Chancellor
@ 2025-10-14 22:53 ` Bjorn Helgaas
0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2025-10-14 22:53 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Mario Limonciello (AMD) (kernel.org), Mika Westerberg,
Andy Shevchenko, Jan Dabros, Andi Shyti, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kees Cook, Sami Tolvanen, linux-i2c,
linux-kernel, llvm
On Tue, Oct 14, 2025 at 03:39:55PM -0700, Nathan Chancellor wrote:
> On Tue, Oct 14, 2025 at 04:58:56PM -0500, Mario Limonciello (AMD) (kernel.org) wrote:
> > On 10/14/2025 3:33 PM, Bjorn Helgaas wrote:
> > > I'm totally fine with the patch itself, but I think the commit log
> > > could be trimmed to something like the following with no loss:
> > >
> > > Remove struct i2c_dw_semaphore_callbacks.remove() and
> > > i2c_dw_remove_lock_support().
> > >
> > > 440da737cf8d ("i2c: designware: Use PCI PSP driver for
> > > communication") removed the last place that set
> > > i2c_dw_semaphore_callbacks.remove(), which made
> > > i2c_dw_remove_lock_support() a no-op.
> > >
> > > This has the side effect of avoiding this kCFI warning (see Link):
> > >
> > > dw_i2c_plat_remove+0x3c: no-cfi indirect call!
> > >
> > > Link: https://lore.kernel.org/r/20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-v1-1-8cc4842967bf@kernel.org
> > >
> > > FWIW,
> > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > I echo Bjorn's comments on the lengthy commit message.
> > Code change looks fine.
> >
> > Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
>
> I have no objections to trimming the commit message if so desired but I
> think the solution (removing unused code) is more tangential to the
> problem (potentially accessing an array out of bounds). I am sometimes
> looking at changes from ten years ago where something was done to avoid
> a problem but the problem was never mentioned in the message but may
> have been elsewhere. Maybe nobody ever needs .remove() again but what if
> new IP comes out that necessitates it and they go to revert this change
> without avoiding this problem? I could try to make the analysis shorter
> if that would help.
OK, I missed that there was an out-of-bounds array access involved.
Maybe that warrants more details.
Bjorn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-14 1:05 [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support() Nathan Chancellor
2025-10-14 20:33 ` Bjorn Helgaas
2025-10-14 21:10 ` Kees Cook
@ 2025-10-15 20:47 ` Andi Shyti
2025-10-23 6:29 ` Andy Shevchenko
3 siblings, 0 replies; 9+ messages in thread
From: Andi Shyti @ 2025-10-15 20:47 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Mika Westerberg, Andy Shevchenko, Jan Dabros, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kees Cook, Sami Tolvanen, linux-i2c,
linux-kernel, llvm
Hi Nathan,
On Mon, Oct 13, 2025 at 06:05:03PM -0700, Nathan Chancellor wrote:
> When building certain configurations with CONFIG_FINEIBT=y after
> commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
> warning due to an indirect call in dw_i2c_plat_remove():
...
> Closes: https://github.com/ClangBuiltLinux/linux/issues/2133
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
I'm OK with the "lengthy" (i.e. clear) message and I understood
that no r-b is binding. Applied to i2c/i2c-host.
Thanks,
Andi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-14 1:05 [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support() Nathan Chancellor
` (2 preceding siblings ...)
2025-10-15 20:47 ` Andi Shyti
@ 2025-10-23 6:29 ` Andy Shevchenko
2025-10-23 16:37 ` Nathan Chancellor
3 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2025-10-23 6:29 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Mika Westerberg, Jan Dabros, Andi Shyti, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kees Cook, Sami Tolvanen, linux-i2c,
linux-kernel, llvm
On Mon, Oct 13, 2025 at 06:05:03PM -0700, Nathan Chancellor wrote:
> When building certain configurations with CONFIG_FINEIBT=y after
> commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
> warning due to an indirect call in dw_i2c_plat_remove():
>
> $ cat allno.config
> CONFIG_ACPI=y
> CONFIG_CFI=y
> CONFIG_COMMON_CLK=y
> CONFIG_CPU_MITIGATIONS=y
> CONFIG_I2C=y
> CONFIG_I2C_DESIGNWARE_BAYTRAIL=y
> CONFIG_I2C_DESIGNWARE_CORE=y
> CONFIG_I2C_DESIGNWARE_PLATFORM=y
> CONFIG_IOSF_MBI=y
> CONFIG_MITIGATION_RETPOLINE=y
> CONFIG_MODULES=y
> CONFIG_PCI=y
> CONFIG_X86_KERNEL_IBT=y
>
> $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 clean allnoconfig vmlinux
> vmlinux.o: warning: objtool: dw_i2c_plat_remove+0x3c: no-cfi indirect call!
>
> With this configuration, i2c_dw_semaphore_cb_table has the BAYTRAIL
> member and the sentinel (i.e., 2 members), both of which have an
> implicit
>
> .remove = NULL,
>
> so Clang effectively turns i2c_dw_remove_lock_support(), which is later
> inlined into dw_i2c_plat_remove(), into:
>
> static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> {
> if (dev->semaphore_idx > 2)
> (*NULL)(dev):
> }
>
> which is not necessarily problematic from a logic perspective (as the
> code was not bounds checking semaphore_idx so an out of bounds index
> could already crash) but objtool's new __nocfi indirect call checking
> trips over Clang dropping the kCFI setup from a known NULL indirect
> call.
>
> While it would be possible to fix this by transforming the initial check
> into
>
> if (dev->semaphore_idx < 0 || dev->semaphore_idx >= ARRAY_SIZE(i2c_dw_semaphore_cb_table))
>
> the remove member is unused after commit 440da737cf8d ("i2c: designware:
> Use PCI PSP driver for communication"), so i2c_dw_remove_lock_support()
> can be removed altogether, as it will never actually do anything.
Have you seen this attempt to refactor a bit that code?
https://lore.kernel.org/linux-i2c/20231207141653.2785124-7-andriy.shevchenko@linux.intel.com/
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
2025-10-23 6:29 ` Andy Shevchenko
@ 2025-10-23 16:37 ` Nathan Chancellor
0 siblings, 0 replies; 9+ messages in thread
From: Nathan Chancellor @ 2025-10-23 16:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mika Westerberg, Jan Dabros, Andi Shyti, Nick Desaulniers,
Bill Wendling, Justin Stitt, Kees Cook, Sami Tolvanen, linux-i2c,
linux-kernel, llvm
On Thu, Oct 23, 2025 at 09:29:02AM +0300, Andy Shevchenko wrote:
> Have you seen this attempt to refactor a bit that code?
>
> https://lore.kernel.org/linux-i2c/20231207141653.2785124-7-andriy.shevchenko@linux.intel.com/
No, I did not. I did not look at historical attempts to fix this issue
since it was still present in -next.
Cheers,
Nathan
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-10-23 16:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-14 1:05 [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support() Nathan Chancellor
2025-10-14 20:33 ` Bjorn Helgaas
2025-10-14 21:58 ` Mario Limonciello (AMD) (kernel.org)
2025-10-14 22:39 ` Nathan Chancellor
2025-10-14 22:53 ` Bjorn Helgaas
2025-10-14 21:10 ` Kees Cook
2025-10-15 20:47 ` Andi Shyti
2025-10-23 6:29 ` Andy Shevchenko
2025-10-23 16:37 ` Nathan Chancellor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox