[PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[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

RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Marciniszyn, Mike]

> 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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Bart Van Assche]

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.

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Leon Romanovsky]

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

RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Marciniszyn, 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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Leon Romanovsky]

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

RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Marciniszyn, 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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Bart Van Assche]

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

RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Marciniszyn, Mike]

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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Guo Zhi]

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

RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Marciniszyn, Mike]

> 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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Jason Gunthorpe]

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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[郭志]

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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[郭志]

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

RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Marciniszyn, Mike]

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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Jason Gunthorpe]

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

[PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[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

Re: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Dennis Dalessandro]

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

RE: [PATCH] infiniband hfi1: fix misuse of %x in ipoib_tx.c

[Marciniszyn, Mike]

> 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