Smatch reports:
ath_pci_probe() warn: argument 4 to %lx specifier is cast from pointer
ath_ahb_probe() warn: argument 4 to %lx specifier is cast from pointer
Fix it by modifying %lx to %p in printk.
Signed-off-by: Dongliang Mu <[email protected]>
---
v2->v3: modify %px to %p in the patch
v1->v2: modify %px to %p in the commit message
drivers/net/wireless/ath/ath9k/ahb.c | 4 ++--
drivers/net/wireless/ath/ath9k/pci.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
index 9cd12b20b18d..a683757eecd4 100644
--- a/drivers/net/wireless/ath/ath9k/ahb.c
+++ b/drivers/net/wireless/ath/ath9k/ahb.c
@@ -132,8 +132,8 @@ static int ath_ahb_probe(struct platform_device *pdev)
ah = sc->sc_ah;
ath9k_hw_name(ah, hw_name, sizeof(hw_name));
- wiphy_info(hw->wiphy, "%s mem=0x%lx, irq=%d\n",
- hw_name, (unsigned long)mem, irq);
+ wiphy_info(hw->wiphy, "%s mem=0x%p, irq=%d\n",
+ hw_name, mem, irq);
return 0;
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index a09f9d223f3d..12c00c7c1408 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -988,8 +988,8 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
sc->sc_ah->msi_reg = 0;
ath9k_hw_name(sc->sc_ah, hw_name, sizeof(hw_name));
- wiphy_info(hw->wiphy, "%s mem=0x%lx, irq=%d\n",
- hw_name, (unsigned long)sc->mem, pdev->irq);
+ wiphy_info(hw->wiphy, "%s mem=0x%p, irq=%d\n",
+ hw_name, sc->mem, pdev->irq);
return 0;
--
2.34.1
Dongliang Mu <[email protected]> writes:
> Smatch reports:
>
> ath_pci_probe() warn: argument 4 to %lx specifier is cast from pointer
> ath_ahb_probe() warn: argument 4 to %lx specifier is cast from pointer
>
> Fix it by modifying %lx to %p in printk.
>
> Signed-off-by: Dongliang Mu <[email protected]>
Third time is the charm! :)
Acked-by: Toke Høiland-Jørgensen <[email protected]>
On Thu, 6 Jul 2023 at 13:39, Toke Høiland-Jørgensen <[email protected]> wrote:
>
> Dongliang Mu <[email protected]> writes:
>
> > Smatch reports:
> >
> > ath_pci_probe() warn: argument 4 to %lx specifier is cast from pointer
> > ath_ahb_probe() warn: argument 4 to %lx specifier is cast from pointer
> >
> > Fix it by modifying %lx to %p in printk.
> >
> > Signed-off-by: Dongliang Mu <[email protected]>
>
> Third time is the charm! :)
>
> Acked-by: Toke Høiland-Jørgensen <[email protected]>
But do we *want* to replace %lx with %p? Because this now causes a
change in behavior, as the kernel will omit the pointer's address,
while before it printed the actual memory address. Using %px would
have kept the original behavior.
So if the change is desired, it should be noted in the commit message.
best Regards,
Jonas
On 7/7/23 00:04, Jonas Gorski wrote:
> On Thu, 6 Jul 2023 at 13:39, Toke Høiland-Jørgensen <[email protected]> wrote:
>> Dongliang Mu <[email protected]> writes:
>>
>>> Smatch reports:
>>>
>>> ath_pci_probe() warn: argument 4 to %lx specifier is cast from pointer
>>> ath_ahb_probe() warn: argument 4 to %lx specifier is cast from pointer
>>>
>>> Fix it by modifying %lx to %p in printk.
>>>
>>> Signed-off-by: Dongliang Mu <[email protected]>
>> Third time is the charm! :)
>>
>> Acked-by: Toke Høiland-Jørgensen <[email protected]>
> But do we *want* to replace %lx with %p? Because this now causes a
> change in behavior, as the kernel will omit the pointer's address,
> while before it printed the actual memory address. Using %px would
> have kept the original behavior.
This is why I made mistake in the patch submission. I think %px is more
suitable and generate a patch, but then checkpatch.pl thinks I should
use %p instead.
I change the code change, but forget the commit message.
>
> So if the change is desired, it should be noted in the commit message.
>
> best Regards,
> Jonas
Jonas Gorski <[email protected]> writes:
> On Thu, 6 Jul 2023 at 13:39, Toke Høiland-Jørgensen <[email protected]> wrote:
>>
>> Dongliang Mu <[email protected]> writes:
>>
>> > Smatch reports:
>> >
>> > ath_pci_probe() warn: argument 4 to %lx specifier is cast from pointer
>> > ath_ahb_probe() warn: argument 4 to %lx specifier is cast from pointer
>> >
>> > Fix it by modifying %lx to %p in printk.
>> >
>> > Signed-off-by: Dongliang Mu <[email protected]>
>>
>> Third time is the charm! :)
>>
>> Acked-by: Toke Høiland-Jørgensen <[email protected]>
>
> But do we *want* to replace %lx with %p? Because this now causes a
> change in behavior, as the kernel will omit the pointer's address,
> while before it printed the actual memory address. Using %px would
> have kept the original behavior.
So this is basically an informational log message, which really
shouldn't be leaking kernel pointer addresses. So I think %p is the
right choice here, and if someone wants to see the real address for
debugging they should be booting with no_hash_pointers.
> So if the change is desired, it should be noted in the commit message.
That is a fair point, documenting this in the commit message would be
good...
-Toke
On 2023/7/7 21:44, Toke Høiland-Jørgensen wrote:
> Jonas Gorski <[email protected]> writes:
>
>> On Thu, 6 Jul 2023 at 13:39, Toke Høiland-Jørgensen <[email protected]> wrote:
>>> Dongliang Mu <[email protected]> writes:
>>>
>>>> Smatch reports:
>>>>
>>>> ath_pci_probe() warn: argument 4 to %lx specifier is cast from pointer
>>>> ath_ahb_probe() warn: argument 4 to %lx specifier is cast from pointer
>>>>
>>>> Fix it by modifying %lx to %p in printk.
>>>>
>>>> Signed-off-by: Dongliang Mu <[email protected]>
>>> Third time is the charm! :)
>>>
>>> Acked-by: Toke Høiland-Jørgensen <[email protected]>
>> But do we *want* to replace %lx with %p? Because this now causes a
>> change in behavior, as the kernel will omit the pointer's address,
>> while before it printed the actual memory address. Using %px would
>> have kept the original behavior.
> So this is basically an informational log message, which really
> shouldn't be leaking kernel pointer addresses. So I think %p is the
> right choice here, and if someone wants to see the real address for
> debugging they should be booting with no_hash_pointers.
>
>> So if the change is desired, it should be noted in the commit message.
> That is a fair point, documenting this in the commit message would be
> good...
Hi Toke,
What's the next? Patch v4 with a note at the end, like the following,
"Note that %p would by default print a hashed value without leaking the
kernel pointer to user space."
> -Toke
Dongliang Mu <[email protected]> writes:
> On 2023/7/7 21:44, Toke Høiland-Jørgensen wrote:
>> Jonas Gorski <[email protected]> writes:
>>
>>> On Thu, 6 Jul 2023 at 13:39, Toke Høiland-Jørgensen <[email protected]> wrote:
>>>> Dongliang Mu <[email protected]> writes:
>>>>
>>>>> Smatch reports:
>>>>>
>>>>> ath_pci_probe() warn: argument 4 to %lx specifier is cast from pointer
>>>>> ath_ahb_probe() warn: argument 4 to %lx specifier is cast from pointer
>>>>>
>>>>> Fix it by modifying %lx to %p in printk.
>>>>>
>>>>> Signed-off-by: Dongliang Mu <[email protected]>
>>>> Third time is the charm! :)
>>>>
>>>> Acked-by: Toke Høiland-Jørgensen <[email protected]>
>>> But do we *want* to replace %lx with %p? Because this now causes a
>>> change in behavior, as the kernel will omit the pointer's address,
>>> while before it printed the actual memory address. Using %px would
>>> have kept the original behavior.
>> So this is basically an informational log message, which really
>> shouldn't be leaking kernel pointer addresses. So I think %p is the
>> right choice here, and if someone wants to see the real address for
>> debugging they should be booting with no_hash_pointers.
>>
>>> So if the change is desired, it should be noted in the commit message.
>> That is a fair point, documenting this in the commit message would be
>> good...
>
> Hi Toke,
>
> What's the next? Patch v4 with a note at the end, like the following,
>
> "Note that %p would by default print a hashed value without leaking the
> kernel pointer to user space."
Yes, please send a new version with the explanation added. However, this
should include not just *what* is changed, but *why* as well. So maybe
something like:
"Note that with this change, the pointer address will be printed as a
hashed value by default. This is appropriate because the kernel
shouldn't leak kernel pointers to user space in an informational
message. If someone wants to see the real address for debugging
purposes, this can be achieved using the no_hash_pointers kernel option"
-Toke