| Message ID | 20210922134857.619602-1-qtxuning1999@sjtu.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | infiniband hfi1: fix misuse of %x in ipoib_tx.c | expand |
> 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
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.
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
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. >
> 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" <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
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 <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
> > 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
> 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>
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
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",
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(-)