[PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Suman Anna]

The virtio_rpmsg_bus driver uses the "%p" format-specifier for
printing the vring buffer address. This prints only a hashed
pointer even for previliged users. Use "%pK" instead so that
the address can be printed during debug using kptr_restrict
sysctl.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index f29dee731026..1345f373a1a0 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		goto vqs_del;
 	}
 
-	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
+	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
 		bufs_va, &vrp->bufs_dma);
 
 	/* half of the buffers is dedicated for RX */
-- 
2.19.1

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Suman Anna]

Hi Bjorn,

On 10/23/18 8:19 PM, Suman Anna wrote:
> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
> printing the vring buffer address. This prints only a hashed
> pointer even for previliged users. Use "%pK" instead so that
> the address can be printed during debug using kptr_restrict
> sysctl.

Seems to have been lost among the patches, can you pick up this trivial
patch for 5.4? Should apply cleanly on the latest HEAD as well.

regards
Suman

> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index f29dee731026..1345f373a1a0 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  		goto vqs_del;
>  	}
>  
> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>  		bufs_va, &vrp->bufs_dma);
>  
>  	/* half of the buffers is dedicated for RX */
> 

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Andrew F. Davis]

On 10/23/18 9:19 PM, Suman Anna wrote:
> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
> printing the vring buffer address. This prints only a hashed
> pointer even for previliged users. Use "%pK" instead so that
> the address can be printed during debug using kptr_restrict
> sysctl.
> 


s/previliged/privileged

You describe what the code does, but not why you need this. %pK is used
for only about 1% of pointer printing, why do you want to leak this
address to userspace at all?

Andrew


> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index f29dee731026..1345f373a1a0 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  		goto vqs_del;
>  	}
>  
> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>  		bufs_va, &vrp->bufs_dma);
>  
>  	/* half of the buffers is dedicated for RX */
> 

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Suman Anna]

On 8/12/19 10:47 AM, Andrew F. Davis wrote:
> On 10/23/18 9:19 PM, Suman Anna wrote:
>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>> printing the vring buffer address. This prints only a hashed
>> pointer even for previliged users. Use "%pK" instead so that
>> the address can be printed during debug using kptr_restrict
>> sysctl.
>>
> 
> 
> s/previliged/privileged

Bjorn,
Can you fix this up when applying.

> 
> You describe what the code does, but not why you need this. %pK is used
> for only about 1% of pointer printing, why do you want to leak this
> address to userspace at all?

Andrew,
Default behavior of %pK is same as %p, but it does allow you to control
the print. The reason is clearly mentioned in the last sentence in the
patch description.

regards
Suman

> 
> Andrew
> 
> 
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index f29dee731026..1345f373a1a0 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  		goto vqs_del;
>>  	}
>>  
>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>  		bufs_va, &vrp->bufs_dma);
>>  
>>  	/* half of the buffers is dedicated for RX */
>>

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Andrew F. Davis]

On 8/12/19 12:28 PM, Suman Anna wrote:
> On 8/12/19 10:47 AM, Andrew F. Davis wrote:
>> On 10/23/18 9:19 PM, Suman Anna wrote:
>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>>> printing the vring buffer address. This prints only a hashed
>>> pointer even for previliged users. Use "%pK" instead so that
>>> the address can be printed during debug using kptr_restrict
>>> sysctl.
>>>
>>
>>
>> s/previliged/privileged
> 
> Bjorn,
> Can you fix this up when applying.
> 
>>
>> You describe what the code does, but not why you need this. %pK is used
>> for only about 1% of pointer printing, why do you want to leak this
>> address to userspace at all?
> 
> Andrew,
> Default behavior of %pK is same as %p, but it does allow you to control
> the print. The reason is clearly mentioned in the last sentence in the
> patch description.
> 


Let me rephrase then, why would you ever set 'kptr_restrict' to anything
other than 0, or why do you want to be able to leak this address to
userspace at all? If the answer is just because you can, then all 12,000
instances of %p in kernel could be converted for the same reason.

Andrew


> regards
> Suman
> 
>>
>> Andrew
>>
>>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>> ---
>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index f29dee731026..1345f373a1a0 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>  		goto vqs_del;
>>>  	}
>>>  
>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>>  		bufs_va, &vrp->bufs_dma);
>>>  
>>>  	/* half of the buffers is dedicated for RX */
>>>
> 

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Suman Anna]

On 8/12/19 11:36 AM, Andrew F. Davis wrote:
> On 8/12/19 12:28 PM, Suman Anna wrote:
>> On 8/12/19 10:47 AM, Andrew F. Davis wrote:
>>> On 10/23/18 9:19 PM, Suman Anna wrote:
>>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>>>> printing the vring buffer address. This prints only a hashed
>>>> pointer even for previliged users. Use "%pK" instead so that
>>>> the address can be printed during debug using kptr_restrict
>>>> sysctl.
>>>>
>>>
>>>
>>> s/previliged/privileged
>>
>> Bjorn,
>> Can you fix this up when applying.
>>
>>>
>>> You describe what the code does, but not why you need this. %pK is used
>>> for only about 1% of pointer printing, why do you want to leak this
>>> address to userspace at all?
>>
>> Andrew,
>> Default behavior of %pK is same as %p, but it does allow you to control
>> the print. The reason is clearly mentioned in the last sentence in the
>> patch description.
>>
> 
> 
> Let me rephrase then, why would you ever set 'kptr_restrict' to anything
> other than 0, or why do you want to be able to leak this address to
> userspace at all? If the answer is just because you can, then all 12,000
> instances of %p in kernel could be converted for the same reason.

It is a dev_dbg statement, so it is already under dynamic debug control.
We would only ever use it during debug.

regards
Suman

> 
> Andrew
> 
> 
>> regards
>> Suman
>>
>>>
>>> Andrew
>>>
>>>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> ---
>>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> index f29dee731026..1345f373a1a0 100644
>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>  		goto vqs_del;
>>>>  	}
>>>>  
>>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>>>  		bufs_va, &vrp->bufs_dma);
>>>>  
>>>>  	/* half of the buffers is dedicated for RX */
>>>>
>>

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Andrew F. Davis]

On 8/12/19 12:39 PM, Suman Anna wrote:
> On 8/12/19 11:36 AM, Andrew F. Davis wrote:
>> On 8/12/19 12:28 PM, Suman Anna wrote:
>>> On 8/12/19 10:47 AM, Andrew F. Davis wrote:
>>>> On 10/23/18 9:19 PM, Suman Anna wrote:
>>>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>>>>> printing the vring buffer address. This prints only a hashed
>>>>> pointer even for previliged users. Use "%pK" instead so that
>>>>> the address can be printed during debug using kptr_restrict
>>>>> sysctl.
>>>>>
>>>>
>>>>
>>>> s/previliged/privileged
>>>
>>> Bjorn,
>>> Can you fix this up when applying.
>>>
>>>>
>>>> You describe what the code does, but not why you need this. %pK is used
>>>> for only about 1% of pointer printing, why do you want to leak this
>>>> address to userspace at all?
>>>
>>> Andrew,
>>> Default behavior of %pK is same as %p, but it does allow you to control
>>> the print. The reason is clearly mentioned in the last sentence in the
>>> patch description.
>>>
>>
>>
>> Let me rephrase then, why would you ever set 'kptr_restrict' to anything
>> other than 0, or why do you want to be able to leak this address to
>> userspace at all? If the answer is just because you can, then all 12,000
>> instances of %p in kernel could be converted for the same reason.
> 
> It is a dev_dbg statement, so it is already under dynamic debug control.
> We would only ever use it during debug.
> 


Most pointer printings are in debug statements..

I'm simply not seeing what this helps us do. The DMA address I can
understand, it may be given to a remote core so we may want to verify it
is the same on both sides, but the actual virtual kernel address is of
no value to us, a hash to track it across uses is just as good.

Andrew


> regards
> Suman
> 
>>
>> Andrew
>>
>>
>>> regards
>>> Suman
>>>
>>>>
>>>> Andrew
>>>>
>>>>
>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>>> ---
>>>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> index f29dee731026..1345f373a1a0 100644
>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>  		goto vqs_del;
>>>>>  	}
>>>>>  
>>>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>>>>  		bufs_va, &vrp->bufs_dma);
>>>>>  
>>>>>  	/* half of the buffers is dedicated for RX */
>>>>>
>>>
> 

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Bjorn Andersson]

On Fri 09 Aug 13:25 PDT 2019, Suman Anna wrote:

> Hi Bjorn,
> 

Hi Suman

> On 10/23/18 8:19 PM, Suman Anna wrote:
> > The virtio_rpmsg_bus driver uses the "%p" format-specifier for
> > printing the vring buffer address. This prints only a hashed
> > pointer even for previliged users. Use "%pK" instead so that
> > the address can be printed during debug using kptr_restrict
> > sysctl.
> 
> Seems to have been lost among the patches, can you pick up this trivial
> patch for 5.4? Should apply cleanly on the latest HEAD as well.
> 

I share Andrew's question regarding what benefit you have from knowing
this value. Should we not just remove the va from the print? Or do you
actually have a use case for it?

Regards,
Bjorn

> regards
> Suman
> 
> > 
> > Signed-off-by: Suman Anna <s-anna@ti.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index f29dee731026..1345f373a1a0 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  		goto vqs_del;
> >  	}
> >  
> > -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> > +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
> >  		bufs_va, &vrp->bufs_dma);
> >  
> >  	/* half of the buffers is dedicated for RX */
> > 
> 

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Suman Anna]

Hi Bjorn,

On 8/27/19 12:10 AM, Bjorn Andersson wrote:
> On Fri 09 Aug 13:25 PDT 2019, Suman Anna wrote:
> 
>> Hi Bjorn,
>>
> 
> Hi Suman
> 
>> On 10/23/18 8:19 PM, Suman Anna wrote:
>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>>> printing the vring buffer address. This prints only a hashed
>>> pointer even for previliged users. Use "%pK" instead so that
>>> the address can be printed during debug using kptr_restrict
>>> sysctl.
>>
>> Seems to have been lost among the patches, can you pick up this trivial
>> patch for 5.4? Should apply cleanly on the latest HEAD as well.
>>
> 
> I share Andrew's question regarding what benefit you have from knowing
> this value. Should we not just remove the va from the print? Or do you
> actually have a use case for it?.

I mainly use it during debug when comparing against kernel_page_tables
and vmallocinfo. The pools that we use are not always guaranteed to be
from linear memory, and behavior changes when using with CMA or DMA pools.

Note that usage of %pK does not leak the addresses automatically, but
atleast enables me to get the values when needed. The changes also bring
the usage in rpmsg core in sync with the remoteproc core.

regards
Suman

> 
> Regards,
> Bjorn
> 
>> regards
>> Suman
>>
>>>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>> ---
>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index f29dee731026..1345f373a1a0 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>  		goto vqs_del;
>>>  	}
>>>  
>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>>  		bufs_va, &vrp->bufs_dma);
>>>  
>>>  	/* half of the buffers is dedicated for RX */
>>>
>>

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Bjorn Andersson]

On Tue 27 Aug 13:25 PDT 2019, Suman Anna wrote:

> Hi Bjorn,
> 
> On 8/27/19 12:10 AM, Bjorn Andersson wrote:
> > On Fri 09 Aug 13:25 PDT 2019, Suman Anna wrote:
> > 
> >> Hi Bjorn,
> >>
> > 
> > Hi Suman
> > 
> >> On 10/23/18 8:19 PM, Suman Anna wrote:
> >>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
> >>> printing the vring buffer address. This prints only a hashed
> >>> pointer even for previliged users. Use "%pK" instead so that
> >>> the address can be printed during debug using kptr_restrict
> >>> sysctl.
> >>
> >> Seems to have been lost among the patches, can you pick up this trivial
> >> patch for 5.4? Should apply cleanly on the latest HEAD as well.
> >>
> > 
> > I share Andrew's question regarding what benefit you have from knowing
> > this value. Should we not just remove the va from the print? Or do you
> > actually have a use case for it?.
> 
> I mainly use it during debug when comparing against kernel_page_tables
> and vmallocinfo. The pools that we use are not always guaranteed to be
> from linear memory, and behavior changes when using with CMA or DMA pools.
> 

Thanks Suman. It seems to me that there's room for improvement to aid
this kind of debugging. But your usecase seems reasonable, so I'm
merging the patch.

> Note that usage of %pK does not leak the addresses automatically, but
> atleast enables me to get the values when needed. The changes also bring
> the usage in rpmsg core in sync with the remoteproc core.
> 

Sounds like shouldn't have merged them in remoteproc then ;P

Thanks,
Bjorn

> regards
> Suman
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> regards
> >> Suman
> >>
> >>>
> >>> Signed-off-by: Suman Anna <s-anna@ti.com>
> >>> ---
> >>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> index f29dee731026..1345f373a1a0 100644
> >>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>  		goto vqs_del;
> >>>  	}
> >>>  
> >>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> >>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
> >>>  		bufs_va, &vrp->bufs_dma);
> >>>  
> >>>  	/* half of the buffers is dedicated for RX */
> >>>
> >>
> 

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Suman Anna]

On 8/27/19 5:07 PM, Bjorn Andersson wrote:
> On Tue 27 Aug 13:25 PDT 2019, Suman Anna wrote:
> 
>> Hi Bjorn,
>>
>> On 8/27/19 12:10 AM, Bjorn Andersson wrote:
>>> On Fri 09 Aug 13:25 PDT 2019, Suman Anna wrote:
>>>
>>>> Hi Bjorn,
>>>>
>>>
>>> Hi Suman
>>>
>>>> On 10/23/18 8:19 PM, Suman Anna wrote:
>>>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>>>>> printing the vring buffer address. This prints only a hashed
>>>>> pointer even for previliged users. Use "%pK" instead so that
>>>>> the address can be printed during debug using kptr_restrict
>>>>> sysctl.
>>>>
>>>> Seems to have been lost among the patches, can you pick up this trivial
>>>> patch for 5.4? Should apply cleanly on the latest HEAD as well.
>>>>
>>>
>>> I share Andrew's question regarding what benefit you have from knowing
>>> this value. Should we not just remove the va from the print? Or do you
>>> actually have a use case for it?.
>>
>> I mainly use it during debug when comparing against kernel_page_tables
>> and vmallocinfo. The pools that we use are not always guaranteed to be
>> from linear memory, and behavior changes when using with CMA or DMA pools.
>>
> 
> Thanks Suman. It seems to me that there's room for improvement to aid
> this kind of debugging. But your usecase seems reasonable, so I'm
> merging the patch.

Thanks Bjorn.

> 
>> Note that usage of %pK does not leak the addresses automatically, but
>> atleast enables me to get the values when needed. The changes also bring
>> the usage in rpmsg core in sync with the remoteproc core.
>>
> 
> Sounds like shouldn't have merged them in remoteproc then ;P

Slightly different reasoning looking at the commit, it was probably when
%p was leaking the addresses.

regards
Suman

> 
> Thanks,
> Bjorn
> 
>> regards
>> Suman
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> regards
>>>> Suman
>>>>
>>>>>
>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>>> ---
>>>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> index f29dee731026..1345f373a1a0 100644
>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>  		goto vqs_del;
>>>>>  	}
>>>>>  
>>>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>>>>  		bufs_va, &vrp->bufs_dma);
>>>>>  
>>>>>  	/* half of the buffers is dedicated for RX */
>>>>>
>>>>
>>

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Suman Anna]

On 8/27/19 5:15 PM, Suman Anna wrote:
> On 8/27/19 5:07 PM, Bjorn Andersson wrote:
>> On Tue 27 Aug 13:25 PDT 2019, Suman Anna wrote:
>>
>>> Hi Bjorn,
>>>
>>> On 8/27/19 12:10 AM, Bjorn Andersson wrote:
>>>> On Fri 09 Aug 13:25 PDT 2019, Suman Anna wrote:
>>>>
>>>>> Hi Bjorn,
>>>>>
>>>>
>>>> Hi Suman
>>>>
>>>>> On 10/23/18 8:19 PM, Suman Anna wrote:
>>>>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
>>>>>> printing the vring buffer address. This prints only a hashed
>>>>>> pointer even for previliged users. Use "%pK" instead so that
>>>>>> the address can be printed during debug using kptr_restrict
>>>>>> sysctl.
>>>>>
>>>>> Seems to have been lost among the patches, can you pick up this trivial
>>>>> patch for 5.4? Should apply cleanly on the latest HEAD as well.
>>>>>
>>>>
>>>> I share Andrew's question regarding what benefit you have from knowing
>>>> this value. Should we not just remove the va from the print? Or do you
>>>> actually have a use case for it?.
>>>
>>> I mainly use it during debug when comparing against kernel_page_tables
>>> and vmallocinfo. The pools that we use are not always guaranteed to be
>>> from linear memory, and behavior changes when using with CMA or DMA pools.
>>>
>>
>> Thanks Suman. It seems to me that there's room for improvement to aid
>> this kind of debugging. But your usecase seems reasonable, so I'm
>> merging the patch.
> 
> Thanks Bjorn.

Btw, looks like you applied the patch against rproc-next instead of
rpmsg-next.

regards
Suman

> 
>>
>>> Note that usage of %pK does not leak the addresses automatically, but
>>> atleast enables me to get the values when needed. The changes also bring
>>> the usage in rpmsg core in sync with the remoteproc core.
>>>
>>
>> Sounds like shouldn't have merged them in remoteproc then ;P
> 
> Slightly different reasoning looking at the commit, it was probably when
> %p was leaking the addresses.
> 
> regards
> Suman
> 
>>
>> Thanks,
>> Bjorn
>>
>>> regards
>>> Suman
>>>
>>>>
>>>> Regards,
>>>> Bjorn
>>>>
>>>>> regards
>>>>> Suman
>>>>>
>>>>>>
>>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>>>> ---
>>>>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>> index f29dee731026..1345f373a1a0 100644
>>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>>  		goto vqs_del;
>>>>>>  	}
>>>>>>  
>>>>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>>>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
>>>>>>  		bufs_va, &vrp->bufs_dma);
>>>>>>  
>>>>>>  	/* half of the buffers is dedicated for RX */
>>>>>>
>>>>>
>>>
> 

Re: [PATCH] rpmsg: virtio_rpmsg_bus: replace "%p" with "%pK"

[Bjorn Andersson]

On Tue 27 Aug 15:17 PDT 2019, Suman Anna wrote:

> On 8/27/19 5:15 PM, Suman Anna wrote:
> > On 8/27/19 5:07 PM, Bjorn Andersson wrote:
> >> On Tue 27 Aug 13:25 PDT 2019, Suman Anna wrote:
> >>
> >>> Hi Bjorn,
> >>>
> >>> On 8/27/19 12:10 AM, Bjorn Andersson wrote:
> >>>> On Fri 09 Aug 13:25 PDT 2019, Suman Anna wrote:
> >>>>
> >>>>> Hi Bjorn,
> >>>>>
> >>>>
> >>>> Hi Suman
> >>>>
> >>>>> On 10/23/18 8:19 PM, Suman Anna wrote:
> >>>>>> The virtio_rpmsg_bus driver uses the "%p" format-specifier for
> >>>>>> printing the vring buffer address. This prints only a hashed
> >>>>>> pointer even for previliged users. Use "%pK" instead so that
> >>>>>> the address can be printed during debug using kptr_restrict
> >>>>>> sysctl.
> >>>>>
> >>>>> Seems to have been lost among the patches, can you pick up this trivial
> >>>>> patch for 5.4? Should apply cleanly on the latest HEAD as well.
> >>>>>
> >>>>
> >>>> I share Andrew's question regarding what benefit you have from knowing
> >>>> this value. Should we not just remove the va from the print? Or do you
> >>>> actually have a use case for it?.
> >>>
> >>> I mainly use it during debug when comparing against kernel_page_tables
> >>> and vmallocinfo. The pools that we use are not always guaranteed to be
> >>> from linear memory, and behavior changes when using with CMA or DMA pools.
> >>>
> >>
> >> Thanks Suman. It seems to me that there's room for improvement to aid
> >> this kind of debugging. But your usecase seems reasonable, so I'm
> >> merging the patch.
> > 
> > Thanks Bjorn.
> 
> Btw, looks like you applied the patch against rproc-next instead of
> rpmsg-next.
> 

Thanks for noticing so quick; I moved the change to the correct
branch.

Regards,
Bjorn

> regards
> Suman
> 
> > 
> >>
> >>> Note that usage of %pK does not leak the addresses automatically, but
> >>> atleast enables me to get the values when needed. The changes also bring
> >>> the usage in rpmsg core in sync with the remoteproc core.
> >>>
> >>
> >> Sounds like shouldn't have merged them in remoteproc then ;P
> > 
> > Slightly different reasoning looking at the commit, it was probably when
> > %p was leaking the addresses.
> > 
> > regards
> > Suman
> > 
> >>
> >> Thanks,
> >> Bjorn
> >>
> >>> regards
> >>> Suman
> >>>
> >>>>
> >>>> Regards,
> >>>> Bjorn
> >>>>
> >>>>> regards
> >>>>> Suman
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
> >>>>>> ---
> >>>>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>> index f29dee731026..1345f373a1a0 100644
> >>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>>> @@ -950,7 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>>  		goto vqs_del;
> >>>>>>  	}
> >>>>>>  
> >>>>>> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> >>>>>> +	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
> >>>>>>  		bufs_va, &vrp->bufs_dma);
> >>>>>>  
> >>>>>>  	/* half of the buffers is dedicated for RX */
> >>>>>>
> >>>>>
> >>>
> > 
>