Pointers should be printed with %p or %px rather than
cast to (unsigned long long) and printed with %llx.
Change %llx to %p to print the pointer.
Signed-off-by: Guo Zhi <[email protected]>
---
drivers/infiniband/hw/hfi1/ipoib_tx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/ipoib_tx.c b/drivers/infiniband/hw/hfi1/ipoib_tx.c
index e74ddbe4658..15b0cb0f363 100644
--- a/drivers/infiniband/hw/hfi1/ipoib_tx.c
+++ b/drivers/infiniband/hw/hfi1/ipoib_tx.c
@@ -876,14 +876,14 @@ void hfi1_ipoib_tx_timeout(struct net_device *dev, unsigned int q)
struct hfi1_ipoib_txq *txq = &priv->txqs[q];
u64 completed = atomic64_read(&txq->complete_txreqs);
- dd_dev_info(priv->dd, "timeout txq %llx q %u stopped %u stops %d no_desc %d ring_full %d\n",
- (unsigned long long)txq, q,
+ dd_dev_info(priv->dd, "timeout txq %p q %u stopped %u stops %d no_desc %d ring_full %d\n",
+ txq, q,
__netif_subqueue_stopped(dev, txq->q_idx),
atomic_read(&txq->stops),
atomic_read(&txq->no_desc),
atomic_read(&txq->ring_full));
- dd_dev_info(priv->dd, "sde %llx engine %u\n",
- (unsigned long long)txq->sde,
+ dd_dev_info(priv->dd, "sde %p engine %u\n",
+ txq->sde,
txq->sde ? txq->sde->this_idx : 0);
dd_dev_info(priv->dd, "flow %x\n", txq->flow.as_int);
dd_dev_info(priv->dd, "sent %llu completed %llu used %llu\n",
--
2.33.0
> Subject: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
>
> Pointers should be printed with %p or %px rather than cast to (unsigned long
> long) and printed with %llx.
> Change %llx to %p to print the pointer.
>
> Signed-off-by: Guo Zhi <[email protected]>
The unsigned long long was originally used to insure the entire accurate pointer as emitted.
This is to ensure the pointers in prints and event traces match values in stacks and register dumps.
I think the %p will obfuscate the pointer so %px is correct for our use case.
Mike
On 9/22/21 10:51 AM, Marciniszyn, Mike wrote:
>> Subject: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
>>
>> Pointers should be printed with %p or %px rather than cast to (unsigned long
>> long) and printed with %llx.
>> Change %llx to %p to print the pointer.
>>
>> Signed-off-by: Guo Zhi <[email protected]>
>
> The unsigned long long was originally used to insure the entire accurate pointer as emitted.
>
> This is to ensure the pointers in prints and event traces match values in stacks and register dumps.
>
> I think the %p will obfuscate the pointer so %px is correct for our use case.
How about applying Guo's patch and adding a configuration option to the
kernel for disabling pointer hashing for %p and related format specifiers?
Pointer hashing is useful on production systems but not on development
systems.
Thanks,
Bart.
I will change %p to %px and submit a new patch.
Thanks.
Guo
----- ԭʼ?ʼ? -----
??????: "Marciniszyn, Mike" <[email protected]>
?ռ???: "Guo Zhi" <[email protected]>, "Dalessandro, Dennis" <[email protected]>, [email protected]
????: [email protected], [email protected]
????ʱ??: ??????, 2021?? 9 ?? 23?? ???? 1:51:08
????: RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
> Subject: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
>
> Pointers should be printed with %p or %px rather than cast to (unsigned long
> long) and printed with %llx.
> Change %llx to %p to print the pointer.
>
> Signed-off-by: Guo Zhi <[email protected]>
The unsigned long long was originally used to insure the entire accurate pointer as emitted.
This is to ensure the pointers in prints and event traces match values in stacks and register dumps.
I think the %p will obfuscate the pointer so %px is correct for our use case.
Mike
On Wed, Sep 22, 2021 at 11:05:42AM -0700, Bart Van Assche wrote:
> On 9/22/21 10:51 AM, Marciniszyn, Mike wrote:
> > > Subject: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
> > >
> > > Pointers should be printed with %p or %px rather than cast to (unsigned long
> > > long) and printed with %llx.
> > > Change %llx to %p to print the pointer.
> > >
> > > Signed-off-by: Guo Zhi <[email protected]>
> >
> > The unsigned long long was originally used to insure the entire accurate pointer as emitted.
> >
> > This is to ensure the pointers in prints and event traces match values in stacks and register dumps.
> >
> > I think the %p will obfuscate the pointer so %px is correct for our use case.
>
> How about applying Guo's patch and adding a configuration option to the
> kernel for disabling pointer hashing for %p and related format specifiers?
Isn't kptr_restrict sysctl is for that?
> Pointer hashing is useful on production systems but not on development
> systems.
>
> Thanks,
>
> Bart.
>
> How about applying Guo's patch and adding a configuration option to the
> kernel for disabling pointer hashing for %p and related format specifiers?
> Pointer hashing is useful on production systems but not on development
> systems.
>
The prints and traces are leave-behind and intended once in a distro for field support.
Re-generating a distro kernel is not really an option.
Mike
>
> Isn't kptr_restrict sysctl is for that?
>
It doesn't look like that works in irqs, softirqs.
We have plenty of those.
Mike
On Thu, Sep 23, 2021 at 11:04:02AM +0000, Marciniszyn, Mike wrote:
> >
> > Isn't kptr_restrict sysctl is for that?
> >
>
> It doesn't look like that works in irqs, softirqs.
Are you certain about it?
That sysctl is supposed to control the output of %p, nothing more.
>
> We have plenty of those.
>
> Mike
> > >
> >
> > It doesn't look like that works in irqs, softirqs.
>
> Are you certain about it?
>
> That sysctl is supposed to control the output of %p, nothing more.
>
Actually I think is controls %pK.
The code here is what I was referring to.
/*
* kptr_restrict==1 cannot be used in IRQ context
* because its test for CAP_SYSLOG would be meaningless.
*/
if (in_irq() || in_serving_softirq() || in_nmi()) {
if (spec.field_width == -1)
spec.field_width = 2 * sizeof(ptr);
return error_string(buf, end, "pK-error", spec);
}
Mike
I have tried using %px rather than %p. However when checking the new patch through scripts/checkpatch.pl, there is a warning: Using vsprintf specifier '%px' potentially exposes the kernel memory layout.
Maybe %pK is the right one?
Thanks.
Guo
----- Original Message -----
From: "Mike Marciniszyn" <[email protected]>
To: "Guo Zhi" <[email protected]>, "Dennis Dalessandro" <[email protected]>, "dledford" <[email protected]>
Cc: "linux-rdma" <[email protected]>, "linux-kernel" <[email protected]>
Sent: Thursday, September 23, 2021 1:51:08 AM
Subject: RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
> Subject: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
>
> Pointers should be printed with %p or %px rather than cast to (unsigned long
> long) and printed with %llx.
> Change %llx to %p to print the pointer.
>
> Signed-off-by: Guo Zhi <[email protected]>
The unsigned long long was originally used to insure the entire accurate pointer as emitted.
This is to ensure the pointers in prints and event traces match values in stacks and register dumps.
I think the %p will obfuscate the pointer so %px is correct for our use case.
Mike
On Thu, Sep 23, 2021 at 11:03:06AM +0000, Marciniszyn, Mike wrote:
> > How about applying Guo's patch and adding a configuration option to the
> > kernel for disabling pointer hashing for %p and related format specifiers?
> > Pointer hashing is useful on production systems but not on development
> > systems.
>
> The prints and traces are leave-behind and intended once in a distro
> for field support.
It doesn't matter, our security model is that drivers do not get to
subvert the kASLR by unilaterally leaking memory layout information,
so you have to get this fixed.
Do not defeat the mechanisms to obscure kernel pointers in trace or
print.
Jason
On 9/22/21 23:45, Leon Romanovsky wrote:
> Isn't kptr_restrict sysctl is for that?
Hi Leon,
After I sent my email I discovered the following commit: 5ead723a20e0
("lib/vsprintf: no_hash_pointers prints all addresses as unhashed"; v5.12).
I think that commit does what we need?
Thanks,
Bart.
commit 5ead723a20e0447bc7db33dc3070b420e5f80aa6
Author: Timur Tabi <[email protected]>
Date: Sun Feb 14 10:13:48 2021 -0600
lib/vsprintf: no_hash_pointers prints all addresses as unhashed
If the no_hash_pointers command line parameter is set, then
printk("%p") will print pointers as unhashed, which is useful for
debugging purposes. This change applies to any function that uses
vsprintf, such as print_hex_dump() and seq_buf_printf().
A large warning message is displayed if this option is enabled.
Unhashed pointers expose kernel addresses, which can be a security
risk.
Also update test_printf to skip the hashed pointer tests if the
command-line option is set.
Signed-off-by: Timur Tabi <[email protected]>
Acked-by: Petr Mladek <[email protected]>
Acked-by: Randy Dunlap <[email protected]>
Acked-by: Sergey Senozhatsky <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
Acked-by: Marco Elver <[email protected]>
Signed-off-by: Petr Mladek <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
>
> On 9/22/21 23:45, Leon Romanovsky wrote:
> > Isn't kptr_restrict sysctl is for that?
>
> Hi Leon,
>
> After I sent my email I discovered the following commit: 5ead723a20e0
> ("lib/vsprintf: no_hash_pointers prints all addresses as unhashed"; v5.12).
> I think that commit does what we need?
>
Thanks Bart,
I agree.
Jason, as to traces, I suspect we need to do the same %p thing there for existing code and any new work.
For situations for debugging in the wild, a command line arg can show the actual value. I'm ok with that.
Mike
On 2021/9/24 22:43, Marciniszyn, Mike wrote:
>> On 9/22/21 23:45, Leon Romanovsky wrote:
>>> Isn't kptr_restrict sysctl is for that?
>> Hi Leon,
>>
>> After I sent my email I discovered the following commit: 5ead723a20e0
>> ("lib/vsprintf: no_hash_pointers prints all addresses as unhashed"; v5.12).
>> I think that commit does what we need?
>>
> Thanks Bart,
>
> I agree.
>
> Jason, as to traces, I suspect we need to do the same %p thing there for existing code and any new work.
>
> For situations for debugging in the wild, a command line arg can show the actual value. I'm ok with that.
>
> Mike
Can this patch which changes %llx to %p in infiniband hfi1 to avoid
kernel pointer release be applied?
Thanks.
Guo
On Wed, Sep 22, 2021 at 09:48:57PM +0800, Guo Zhi wrote:
> Pointers should be printed with %p or %px rather than
> cast to (unsigned long long) and printed with %llx.
> Change %llx to %p to print the pointer.
>
> Signed-off-by: Guo Zhi <[email protected]>
> Acked-by: Mike Marciniszyn <[email protected]>
> ---
> drivers/infiniband/hw/hfi1/ipoib_tx.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Applied to for-rc with a fixes line
Jason
> Subject: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
>
> Pointers should be printed with %p or %px rather than cast to (unsigned long
> long) and printed with %llx.
> Change %llx to %p to print the pointer.
>
> Signed-off-by: Guo Zhi <[email protected]>
> ---
> drivers/infiniband/hw/hfi1/ipoib_tx.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hfi1/ipoib_tx.c
> b/drivers/infiniband/hw/hfi1/ipoib_tx.c
> index e74ddbe4658..15b0cb0f363 100644
> --- a/drivers/infiniband/hw/hfi1/ipoib_tx.c
> +++ b/drivers/infiniband/hw/hfi1/ipoib_tx.c
> @@ -876,14 +876,14 @@ void hfi1_ipoib_tx_timeout(struct net_device
> *dev, unsigned int q)
> struct hfi1_ipoib_txq *txq = &priv->txqs[q];
> u64 completed = atomic64_read(&txq->complete_txreqs);
>
> - dd_dev_info(priv->dd, "timeout txq %llx q %u stopped %u stops %d
> no_desc %d ring_full %d\n",
> - (unsigned long long)txq, q,
> + dd_dev_info(priv->dd, "timeout txq %p q %u stopped %u stops %d
> no_desc %d ring_full %d\n",
> + txq, q,
> __netif_subqueue_stopped(dev, txq->q_idx),
> atomic_read(&txq->stops),
> atomic_read(&txq->no_desc),
> atomic_read(&txq->ring_full));
> - dd_dev_info(priv->dd, "sde %llx engine %u\n",
> - (unsigned long long)txq->sde,
> + dd_dev_info(priv->dd, "sde %p engine %u\n",
> + txq->sde,
> txq->sde ? txq->sde->this_idx : 0);
> dd_dev_info(priv->dd, "flow %x\n", txq->flow.as_int);
> dd_dev_info(priv->dd, "sent %llu completed %llu used %llu\n",
> --
> 2.33.0
This patch has the correct case, but is not noted as a V2.
Jason, this one is ok.
Acked-by: Mike Marciniszyn <[email protected]>