linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
@ 2021-09-22 12:33 Guo Zhi
  2021-09-22 12:37 ` Dennis Dalessandro
  2021-09-24 14:46 ` Marciniszyn, Mike
  0 siblings, 2 replies; 19+ messages in thread
From: Guo Zhi @ 2021-09-22 12:33 UTC (permalink / raw)
  To: mike.marciniszyn, dennis.dalessandro, dledford
  Cc: linux-rdma, linux-kernel, Guo Zhi

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 <qtxuning1999@sjtu.edu.cn>
---
 drivers/infiniband/hw/hfi1/ipoib_tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/ipoib_tx.c b/drivers/infiniband/hw/hfi1/ipoib_tx.c
index e74ddbe4658..7381f352311 100644
--- a/drivers/infiniband/hw/hfi1/ipoib_tx.c
+++ b/drivers/infiniband/hw/hfi1/ipoib_tx.c
@@ -876,13 +876,13 @@ 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",
+	dd_dev_info(priv->dd, "timeout txq %p q %u stopped %u stops %d no_desc %d ring_full %d\n",
 		    (unsigned long long)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",
+	dd_dev_info(priv->dd, "sde %p engine %u\n",
 		    (unsigned long long)txq->sde,
 		    txq->sde ? txq->sde->this_idx : 0);
 	dd_dev_info(priv->dd, "flow %x\n", txq->flow.as_int);
-- 
2.33.0


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 12:33 [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c Guo Zhi
@ 2021-09-22 12:37 ` Dennis Dalessandro
  2021-09-24 14:46 ` Marciniszyn, Mike
  1 sibling, 0 replies; 19+ messages in thread
From: Dennis Dalessandro @ 2021-09-22 12:37 UTC (permalink / raw)
  To: Guo Zhi, mike.marciniszyn, dledford; +Cc: linux-rdma, linux-kernel


On 9/22/21 8:33 AM, 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.

You are still casting to (unsigned long long) though.

> 
> Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn>
> ---
>  drivers/infiniband/hw/hfi1/ipoib_tx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/ipoib_tx.c b/drivers/infiniband/hw/hfi1/ipoib_tx.c
> index e74ddbe4658..7381f352311 100644
> --- a/drivers/infiniband/hw/hfi1/ipoib_tx.c
> +++ b/drivers/infiniband/hw/hfi1/ipoib_tx.c
> @@ -876,13 +876,13 @@ 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",
> +	dd_dev_info(priv->dd, "timeout txq %p q %u stopped %u stops %d no_desc %d ring_full %d\n",
>  		    (unsigned long long)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",
> +	dd_dev_info(priv->dd, "sde %p engine %u\n",
>  		    (unsigned long long)txq->sde,
>  		    txq->sde ? txq->sde->this_idx : 0);
>  	dd_dev_info(priv->dd, "flow %x\n", txq->flow.as_int);
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 12:33 [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c Guo Zhi
  2021-09-22 12:37 ` Dennis Dalessandro
@ 2021-09-24 14:46 ` Marciniszyn, Mike
  1 sibling, 0 replies; 19+ messages in thread
From: Marciniszyn, Mike @ 2021-09-24 14:46 UTC (permalink / raw)
  To: Guo Zhi, Dalessandro, Dennis, dledford; +Cc: linux-rdma, linux-kernel

> 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 <qtxuning1999@sjtu.edu.cn>
> ---
>  drivers/infiniband/hw/hfi1/ipoib_tx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/ipoib_tx.c
> b/drivers/infiniband/hw/hfi1/ipoib_tx.c
> index e74ddbe4658..7381f352311 100644
> --- a/drivers/infiniband/hw/hfi1/ipoib_tx.c
> +++ b/drivers/infiniband/hw/hfi1/ipoib_tx.c
> @@ -876,13 +876,13 @@ 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",
> +	dd_dev_info(priv->dd, "timeout txq %p q %u stopped %u stops %d
> no_desc
> +%d ring_full %d\n",
>  		    (unsigned long long)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",
> +	dd_dev_info(priv->dd, "sde %p engine %u\n",
>  		    (unsigned long long)txq->sde,

Gho,

As Denny mentioned, I'm assuming that the case is no longer required?

Submit a v2 with that change and I will Ack.

Mike

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 13:48 Guo Zhi
  2021-09-22 17:51 ` Marciniszyn, Mike
  2021-09-27 13:05 ` Marciniszyn, Mike
@ 2021-09-27 17:42 ` Jason Gunthorpe
  2 siblings, 0 replies; 19+ messages in thread
From: Jason Gunthorpe @ 2021-09-27 17:42 UTC (permalink / raw)
  To: Guo Zhi
  Cc: mike.marciniszyn, dennis.dalessandro, dledford, linux-rdma, linux-kernel

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 <qtxuning1999@sjtu.edu.cn>
> Acked-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> ---
>  drivers/infiniband/hw/hfi1/ipoib_tx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied to for-rc with a fixes line

Jason

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 13:48 Guo Zhi
  2021-09-22 17:51 ` Marciniszyn, Mike
@ 2021-09-27 13:05 ` Marciniszyn, Mike
  2021-09-27 17:42 ` Jason Gunthorpe
  2 siblings, 0 replies; 19+ messages in thread
From: Marciniszyn, Mike @ 2021-09-27 13:05 UTC (permalink / raw)
  To: Guo Zhi, Dalessandro, Dennis, dledford; +Cc: linux-rdma, linux-kernel

> 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 <qtxuning1999@sjtu.edu.cn>
> ---
>  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 <mike.marciniszyn@cornelisnetworks.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-24 14:43         ` Marciniszyn, Mike
@ 2021-09-25  0:20           ` Guo Zhi
  0 siblings, 0 replies; 19+ messages in thread
From: Guo Zhi @ 2021-09-25  0:20 UTC (permalink / raw)
  To: Marciniszyn, Mike, Bart Van Assche
  Cc: Dalessandro, Dennis, dledford, linux-rdma, linux-kernel,
	Leon Romanovsky, Jason Gunthorpe

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-24  2:46       ` Bart Van Assche
@ 2021-09-24 14:43         ` Marciniszyn, Mike
  2021-09-25  0:20           ` Guo Zhi
  0 siblings, 1 reply; 19+ messages in thread
From: Marciniszyn, Mike @ 2021-09-24 14:43 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Guo Zhi, Dalessandro, Dennis, dledford, linux-rdma, linux-kernel,
	Leon Romanovsky, Jason Gunthorpe

> 
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-23  6:45     ` Leon Romanovsky
  2021-09-23 11:04       ` Marciniszyn, Mike
@ 2021-09-24  2:46       ` Bart Van Assche
  2021-09-24 14:43         ` Marciniszyn, Mike
  1 sibling, 1 reply; 19+ messages in thread
From: Bart Van Assche @ 2021-09-24  2:46 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Marciniszyn, Mike, Guo Zhi, Dalessandro, Dennis, dledford,
	linux-rdma, linux-kernel

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 <timur@kernel.org>
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 <timur@kernel.org>
     Acked-by: Petr Mladek <pmladek@suse.com>
     Acked-by: Randy Dunlap <rdunlap@infradead.org>
     Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
     Acked-by: Vlastimil Babka <vbabka@suse.cz>
     Acked-by: Marco Elver <elver@google.com>
     Signed-off-by: Petr Mladek <pmladek@suse.com>
     Link: https://lore.kernel.org/r/20210214161348.369023-4-timur@kernel.org

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-23 11:03     ` Marciniszyn, Mike
@ 2021-09-23 13:15       ` Jason Gunthorpe
  0 siblings, 0 replies; 19+ messages in thread
From: Jason Gunthorpe @ 2021-09-23 13:15 UTC (permalink / raw)
  To: Marciniszyn, Mike
  Cc: Bart Van Assche, Guo Zhi, Dalessandro, Dennis, dledford,
	linux-rdma, linux-kernel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 17:51 ` Marciniszyn, Mike
  2021-09-22 18:05   ` Bart Van Assche
  2021-09-23  2:03   ` 郭志
@ 2021-09-23 12:51   ` 郭志
  2 siblings, 0 replies; 19+ messages in thread
From: 郭志 @ 2021-09-23 12:51 UTC (permalink / raw)
  To: Mike Marciniszyn; +Cc: Dennis Dalessandro, dledford, linux-rdma, linux-kernel

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" <mike.marciniszyn@cornelisnetworks.com>
To: "Guo Zhi" <qtxuning1999@sjtu.edu.cn>, "Dennis Dalessandro" <dennis.dalessandro@cornelisnetworks.com>, "dledford" <dledford@redhat.com>
Cc: "linux-rdma" <linux-rdma@vger.kernel.org>, "linux-kernel" <linux-kernel@vger.kernel.org>
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 <qtxuning1999@sjtu.edu.cn>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-23 11:44         ` Leon Romanovsky
@ 2021-09-23 12:18           ` Marciniszyn, Mike
  0 siblings, 0 replies; 19+ messages in thread
From: Marciniszyn, Mike @ 2021-09-23 12:18 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Bart Van Assche, Guo Zhi, Dalessandro, Dennis, dledford,
	linux-rdma, linux-kernel

> > >
> >
> > 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-23 11:04       ` Marciniszyn, Mike
@ 2021-09-23 11:44         ` Leon Romanovsky
  2021-09-23 12:18           ` Marciniszyn, Mike
  0 siblings, 1 reply; 19+ messages in thread
From: Leon Romanovsky @ 2021-09-23 11:44 UTC (permalink / raw)
  To: Marciniszyn, Mike
  Cc: Bart Van Assche, Guo Zhi, Dalessandro, Dennis, dledford,
	linux-rdma, linux-kernel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-23  6:45     ` Leon Romanovsky
@ 2021-09-23 11:04       ` Marciniszyn, Mike
  2021-09-23 11:44         ` Leon Romanovsky
  2021-09-24  2:46       ` Bart Van Assche
  1 sibling, 1 reply; 19+ messages in thread
From: Marciniszyn, Mike @ 2021-09-23 11:04 UTC (permalink / raw)
  To: Leon Romanovsky, Bart Van Assche
  Cc: Guo Zhi, Dalessandro, Dennis, dledford, linux-rdma, linux-kernel

> 
> Isn't kptr_restrict sysctl is for that?
> 

It doesn't look like that works in irqs, softirqs.

We have plenty of those.

Mike

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 18:05   ` Bart Van Assche
  2021-09-23  6:45     ` Leon Romanovsky
@ 2021-09-23 11:03     ` Marciniszyn, Mike
  2021-09-23 13:15       ` Jason Gunthorpe
  1 sibling, 1 reply; 19+ messages in thread
From: Marciniszyn, Mike @ 2021-09-23 11:03 UTC (permalink / raw)
  To: Bart Van Assche, Guo Zhi, Dalessandro, Dennis, dledford
  Cc: linux-rdma, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 18:05   ` Bart Van Assche
@ 2021-09-23  6:45     ` Leon Romanovsky
  2021-09-23 11:04       ` Marciniszyn, Mike
  2021-09-24  2:46       ` Bart Van Assche
  2021-09-23 11:03     ` Marciniszyn, Mike
  1 sibling, 2 replies; 19+ messages in thread
From: Leon Romanovsky @ 2021-09-23  6:45 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Marciniszyn, Mike, Guo Zhi, Dalessandro, Dennis, dledford,
	linux-rdma, linux-kernel

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 <qtxuning1999@sjtu.edu.cn>
> > 
> > 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.
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 17:51 ` Marciniszyn, Mike
  2021-09-22 18:05   ` Bart Van Assche
@ 2021-09-23  2:03   ` 郭志
  2021-09-23 12:51   ` 郭志
  2 siblings, 0 replies; 19+ messages in thread
From: 郭志 @ 2021-09-23  2:03 UTC (permalink / raw)
  To: Marciniszyn, Mike, Dalessandro, Dennis, dledford; +Cc: linux-rdma, linux-kernel

I will change %p to %px and submit a new patch.

Thanks.

Guo

----- 原始邮件 -----
发件人: "Marciniszyn, Mike" <mike.marciniszyn@cornelisnetworks.com>
收件人: "Guo Zhi" <qtxuning1999@sjtu.edu.cn>, "Dalessandro, Dennis" <dennis.dalessandro@cornelisnetworks.com>, dledford@redhat.com
抄送: linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
发送时间: 星期四, 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 <qtxuning1999@sjtu.edu.cn>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 17:51 ` Marciniszyn, Mike
@ 2021-09-22 18:05   ` Bart Van Assche
  2021-09-23  6:45     ` Leon Romanovsky
  2021-09-23 11:03     ` Marciniszyn, Mike
  2021-09-23  2:03   ` 郭志
  2021-09-23 12:51   ` 郭志
  2 siblings, 2 replies; 19+ messages in thread
From: Bart Van Assche @ 2021-09-22 18:05 UTC (permalink / raw)
  To: Marciniszyn, Mike, Guo Zhi, Dalessandro, Dennis, dledford
  Cc: linux-rdma, linux-kernel

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 <qtxuning1999@sjtu.edu.cn>
> 
> 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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
  2021-09-22 13:48 Guo Zhi
@ 2021-09-22 17:51 ` Marciniszyn, Mike
  2021-09-22 18:05   ` Bart Van Assche
                     ` (2 more replies)
  2021-09-27 13:05 ` Marciniszyn, Mike
  2021-09-27 17:42 ` Jason Gunthorpe
  2 siblings, 3 replies; 19+ messages in thread
From: Marciniszyn, Mike @ 2021-09-22 17:51 UTC (permalink / raw)
  To: Guo Zhi, Dalessandro, Dennis, dledford; +Cc: linux-rdma, linux-kernel

> 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 <qtxuning1999@sjtu.edu.cn>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c
@ 2021-09-22 13:48 Guo Zhi
  2021-09-22 17:51 ` Marciniszyn, Mike
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Guo Zhi @ 2021-09-22 13:48 UTC (permalink / raw)
  To: mike.marciniszyn, dennis.dalessandro, dledford
  Cc: linux-rdma, linux-kernel, Guo Zhi

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 <qtxuning1999@sjtu.edu.cn>
---
 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-09-27 17:42 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 12:33 [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c Guo Zhi
2021-09-22 12:37 ` Dennis Dalessandro
2021-09-24 14:46 ` Marciniszyn, Mike
2021-09-22 13:48 Guo Zhi
2021-09-22 17:51 ` Marciniszyn, Mike
2021-09-22 18:05   ` Bart Van Assche
2021-09-23  6:45     ` Leon Romanovsky
2021-09-23 11:04       ` Marciniszyn, Mike
2021-09-23 11:44         ` Leon Romanovsky
2021-09-23 12:18           ` Marciniszyn, Mike
2021-09-24  2:46       ` Bart Van Assche
2021-09-24 14:43         ` Marciniszyn, Mike
2021-09-25  0:20           ` Guo Zhi
2021-09-23 11:03     ` Marciniszyn, Mike
2021-09-23 13:15       ` Jason Gunthorpe
2021-09-23  2:03   ` 郭志
2021-09-23 12:51   ` 郭志
2021-09-27 13:05 ` Marciniszyn, Mike
2021-09-27 17:42 ` Jason Gunthorpe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).