Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Wed, 2018-04-11 at 11:52 +0200, Petr Mladek wrote:
> On Tue 2018-04-10 14:41:55, Andy Shevchenko wrote:
> > On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> > > On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:

> > I think about 1-7 and 9 that can go as is before your changes.
> > And patch 8 postpone
> 
> Done. All the 8 patches are in printk.git, branch
> for-4.18-vsprintf-cleanup.

Thanks!

> I am sorry that we have missed 4.17. In theory, it still might be
> possible to push them there. But there does not seem to be a real
> reason to rush them.

Agree.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Wed, 2018-04-11 at 11:52 +0200, Petr Mladek wrote:
> On Tue 2018-04-10 14:41:55, Andy Shevchenko wrote:
> > On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> > > On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:

> > I think about 1-7 and 9 that can go as is before your changes.
> > And patch 8 postpone
> 
> Done. All the 8 patches are in printk.git, branch
> for-4.18-vsprintf-cleanup.

Thanks!

> I am sorry that we have missed 4.17. In theory, it still might be
> possible to push them there. But there does not seem to be a real
> reason to rush them.

Agree.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Tue 2018-04-10 14:41:55, Andy Shevchenko wrote:
> On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> > On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> > > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> 
> > > This change collides with my patch series. Can you elaborate what
> > > your
> > > thoughts are about my patches? Are you going incorporate them to
> > > your
> > > series? Should I send them independently?
> > 
> > Good question. I think that the best solution will be that I go
> > over your patchset and just add all valid ones into printk.git
> > for-4.18.
> 
> I think about 1-7 and 9 that can go as is before your changes.
> And patch 8 postpone

Done. All the 8 patches are in printk.git, branch
for-4.18-vsprintf-cleanup.

I am sorry that we have missed 4.17. In theory, it still might be
possible to push them there. But there does not seem to be a real
reason to rush them.

> >  Then I will base v5 of this patchset on top of it.
> 
> I'm going for vacation tomorrow. Can you just take them into your series
> or apply to your tree?

Good to know.

Enjoy vacation,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Tue 2018-04-10 14:41:55, Andy Shevchenko wrote:
> On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> > On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> > > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> 
> > > This change collides with my patch series. Can you elaborate what
> > > your
> > > thoughts are about my patches? Are you going incorporate them to
> > > your
> > > series? Should I send them independently?
> > 
> > Good question. I think that the best solution will be that I go
> > over your patchset and just add all valid ones into printk.git
> > for-4.18.
> 
> I think about 1-7 and 9 that can go as is before your changes.
> And patch 8 postpone

Done. All the 8 patches are in printk.git, branch
for-4.18-vsprintf-cleanup.

I am sorry that we have missed 4.17. In theory, it still might be
possible to push them there. But there does not seem to be a real
reason to rush them.

> >  Then I will base v5 of this patchset on top of it.
> 
> I'm going for vacation tomorrow. Can you just take them into your series
> or apply to your tree?

Good to know.

Enjoy vacation,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:

> > This change collides with my patch series. Can you elaborate what
> > your
> > thoughts are about my patches? Are you going incorporate them to
> > your
> > series? Should I send them independently?
> 
> Good question. I think that the best solution will be that I go
> over your patchset and just add all valid ones into printk.git
> for-4.18.

I think about 1-7 and 9 that can go as is before your changes.
And patch 8 postpone

>  Then I will base v5 of this patchset on top of it.

I'm going for vacation tomorrow. Can you just take them into your series
or apply to your tree?

> I should have done this earlier. But I did not expect that long
> way for the access-check stuff. We originally planned to
> do the access check first, see
> https://lkml.kernel.org/r/152254.10722.389.ca...@linux.intel.com

Yeah, I didn't consider that your one patch became a series...

> But the access check patchset still need some love, so it makes
> sense to switch the order.

I agree.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:

> > This change collides with my patch series. Can you elaborate what
> > your
> > thoughts are about my patches? Are you going incorporate them to
> > your
> > series? Should I send them independently?
> 
> Good question. I think that the best solution will be that I go
> over your patchset and just add all valid ones into printk.git
> for-4.18.

I think about 1-7 and 9 that can go as is before your changes.
And patch 8 postpone

>  Then I will base v5 of this patchset on top of it.

I'm going for vacation tomorrow. Can you just take them into your series
or apply to your tree?

> I should have done this earlier. But I did not expect that long
> way for the access-check stuff. We originally planned to
> do the access check first, see
> https://lkml.kernel.org/r/152254.10722.389.ca...@linux.intel.com

Yeah, I didn't consider that your one patch became a series...

> But the access check patchset still need some love, so it makes
> sense to switch the order.

I agree.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > There are few printk formats that make sense only with two or more
> > specifiers. Also some specifiers make sense only when a kernel feature
> > is enabled.
> > 
> > The handling of unknown specifiers is strange, inconsistent, and
> > even leaking the address. For example, netdev_bits() prints the
> > non-hashed pointer value or clock() prints "(null)".
> > 
> > The best solution seems to be in flags_string(). It does not print any
> > misleading value. Instead it calls WARN_ONCE() describing the unknown
> > specifier. Therefore it clearly shows the problem and helps to find
> > it.
> > 
> > Note that WARN_ONCE() used to cause recursive printk(). But it is safe
> > now because vscnprintf() is called in printk_safe context from
> > vprintk_emit().
> > 
> 
> > -   if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
> > +   if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
> > +   WARN_ONCE(1, "Unsupported pointer format specifier:
> > %%pC\n");
> > +   return buf;
> > +   }
> > +
> > +   if (!clk)
> > return string(buf, end, NULL, spec);
> 
> This change collides with my patch series. Can you elaborate what your
> thoughts are about my patches? Are you going incorporate them to your
> series? Should I send them independently?

Good question. I think that the best solution will be that I go
over your patchset and just add all valid ones into printk.git
for-4.18. Then I will base v5 of this patchset on top of it.

I should have done this earlier. But I did not expect that long
way for the access-check stuff. We originally planned to
do the access check first, see
https://lkml.kernel.org/r/152254.10722.389.ca...@linux.intel.com
But the access check patchset still need some love, so it makes
sense to switch the order.

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > There are few printk formats that make sense only with two or more
> > specifiers. Also some specifiers make sense only when a kernel feature
> > is enabled.
> > 
> > The handling of unknown specifiers is strange, inconsistent, and
> > even leaking the address. For example, netdev_bits() prints the
> > non-hashed pointer value or clock() prints "(null)".
> > 
> > The best solution seems to be in flags_string(). It does not print any
> > misleading value. Instead it calls WARN_ONCE() describing the unknown
> > specifier. Therefore it clearly shows the problem and helps to find
> > it.
> > 
> > Note that WARN_ONCE() used to cause recursive printk(). But it is safe
> > now because vscnprintf() is called in printk_safe context from
> > vprintk_emit().
> > 
> 
> > -   if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
> > +   if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
> > +   WARN_ONCE(1, "Unsupported pointer format specifier:
> > %%pC\n");
> > +   return buf;
> > +   }
> > +
> > +   if (!clk)
> > return string(buf, end, NULL, spec);
> 
> This change collides with my patch series. Can you elaborate what your
> thoughts are about my patches? Are you going incorporate them to your
> series? Should I send them independently?

Good question. I think that the best solution will be that I go
over your patchset and just add all valid ones into printk.git
for-4.18. Then I will base v5 of this patchset on top of it.

I should have done this earlier. But I did not expect that long
way for the access-check stuff. We originally planned to
do the access check first, see
https://lkml.kernel.org/r/152254.10722.389.ca...@linux.intel.com
But the access check patchset still need some love, so it makes
sense to switch the order.

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Fri 2018-04-06 07:27:19, Joe Perches wrote:
> On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> > On 2018-04-06 13:43, Petr Mladek wrote:
> > > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > > which should get fixed before somebody claims those extensions.
> > > > > 
> > > > > Neither %pt nor %po is used in a vsprintf
> > > > > in the kernel.
> > > > 
> > > > Nope, you are right, both are defectively used in the
> > > > kernel via string concatenation.
> > > 
> > > Ah, I see.
> > 
> > Yes, that was the point of that particular grep pattern with context. I
> > sent patches three years ago and several pings, but despite an email
> > telling me it would be picked up "for the next release" nothing has
> > happened, so I've just been waiting for someone to implement a %po.
> > 
> > > Otherwise, the changes make perfect sense. Are you going to send
> > > them as proper patches, please?
> > 
> > Good luck. Unless Petr will be taking them through the printk tree?
> 
> As the trivial maintainer seems to use a leaky bucket algorithm
> for most trivial patches, I'll just retry and send actual
> patches through Andrew Morton as patches through his quilt tree
> seem to get upstream faster.

I could take them via printk.git. But Andrew is a good choice
as well.

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Fri 2018-04-06 07:27:19, Joe Perches wrote:
> On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> > On 2018-04-06 13:43, Petr Mladek wrote:
> > > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > > which should get fixed before somebody claims those extensions.
> > > > > 
> > > > > Neither %pt nor %po is used in a vsprintf
> > > > > in the kernel.
> > > > 
> > > > Nope, you are right, both are defectively used in the
> > > > kernel via string concatenation.
> > > 
> > > Ah, I see.
> > 
> > Yes, that was the point of that particular grep pattern with context. I
> > sent patches three years ago and several pings, but despite an email
> > telling me it would be picked up "for the next release" nothing has
> > happened, so I've just been waiting for someone to implement a %po.
> > 
> > > Otherwise, the changes make perfect sense. Are you going to send
> > > them as proper patches, please?
> > 
> > Good luck. Unless Petr will be taking them through the printk tree?
> 
> As the trivial maintainer seems to use a leaky bucket algorithm
> for most trivial patches, I'll just retry and send actual
> patches through Andrew Morton as patches through his quilt tree
> seem to get upstream faster.

I could take them via printk.git. But Andrew is a good choice
as well.

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> There are few printk formats that make sense only with two or more
> specifiers. Also some specifiers make sense only when a kernel feature
> is enabled.
> 
> The handling of unknown specifiers is strange, inconsistent, and
> even leaking the address. For example, netdev_bits() prints the
> non-hashed pointer value or clock() prints "(null)".
> 
> The best solution seems to be in flags_string(). It does not print any
> misleading value. Instead it calls WARN_ONCE() describing the unknown
> specifier. Therefore it clearly shows the problem and helps to find
> it.
> 
> Note that WARN_ONCE() used to cause recursive printk(). But it is safe
> now because vscnprintf() is called in printk_safe context from
> vprintk_emit().
> 

> - if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
> + if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
> + WARN_ONCE(1, "Unsupported pointer format specifier:
> %%pC\n");
> + return buf;
> + }
> +
> + if (!clk)
>   return string(buf, end, NULL, spec);

This change collides with my patch series. Can you elaborate what your
thoughts are about my patches? Are you going incorporate them to your
series? Should I send them independently?

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> There are few printk formats that make sense only with two or more
> specifiers. Also some specifiers make sense only when a kernel feature
> is enabled.
> 
> The handling of unknown specifiers is strange, inconsistent, and
> even leaking the address. For example, netdev_bits() prints the
> non-hashed pointer value or clock() prints "(null)".
> 
> The best solution seems to be in flags_string(). It does not print any
> misleading value. Instead it calls WARN_ONCE() describing the unknown
> specifier. Therefore it clearly shows the problem and helps to find
> it.
> 
> Note that WARN_ONCE() used to cause recursive printk(). But it is safe
> now because vscnprintf() is called in printk_safe context from
> vprintk_emit().
> 

> - if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
> + if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
> + WARN_ONCE(1, "Unsupported pointer format specifier:
> %%pC\n");
> + return buf;
> + }
> +
> + if (!clk)
>   return string(buf, end, NULL, spec);

This change collides with my patch series. Can you elaborate what your
thoughts are about my patches? Are you going incorporate them to your
series? Should I send them independently?

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> On 2018-04-06 13:43, Petr Mladek wrote:
> > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > which should get fixed before somebody claims those
> > > > > extensions.
> > > > 
> > > > Neither %pt nor %po is used in a vsprintf
> > > > in the kernel.
> > > 
> > > Nope, you are right, both are defectively used in the
> > > kernel via string concatenation.
> > 
> > Ah, I see.
> 
> Yes, that was the point of that particular grep pattern with context.
> I
> sent patches three years ago and several pings, but despite an email
> telling me it would be picked up "for the next release" nothing has
> happened, so I've just been waiting for someone to implement a %po.

It used to be for SCSI subsystem until Martin gets involved.
My record is 6 years for the patch made the upstream.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> On 2018-04-06 13:43, Petr Mladek wrote:
> > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > which should get fixed before somebody claims those
> > > > > extensions.
> > > > 
> > > > Neither %pt nor %po is used in a vsprintf
> > > > in the kernel.
> > > 
> > > Nope, you are right, both are defectively used in the
> > > kernel via string concatenation.
> > 
> > Ah, I see.
> 
> Yes, that was the point of that particular grep pattern with context.
> I
> sent patches three years ago and several pings, but despite an email
> telling me it would be picked up "for the next release" nothing has
> happened, so I've just been waiting for someone to implement a %po.

It used to be for SCSI subsystem until Martin gets involved.
My record is 6 years for the patch made the upstream.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

On (04/06/18 18:00), Joe Perches wrote:
[..]
> This finds the current two bad uses in addition to
> the existing similar message for string concatenation
> without a space char between concatenated fragments.
> 
> For example:
> 
> WARNING: break quoted strings at a space character
> #3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
> + dev_notice(>pdev->dev, "moving 
> cmd[%d]:%p:%d:%p"
> + "on the defer queue as internal\n",
> 
> WARNING: vsprintf %p string concatenation
> #3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
> + dev_notice(>pdev->dev, "moving 
> cmd[%d]:%p:%d:%p"
> + "on the defer queue as internal\n",
> 
> I think the new message is not that useful really as the
> existing warning is probably enough.

Oh, so we already have it... Didn't know that. Yes, I think the existing
one is good enough. Thanks for the pointers.

-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

On (04/06/18 18:00), Joe Perches wrote:
[..]
> This finds the current two bad uses in addition to
> the existing similar message for string concatenation
> without a space char between concatenated fragments.
> 
> For example:
> 
> WARNING: break quoted strings at a space character
> #3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
> + dev_notice(>pdev->dev, "moving 
> cmd[%d]:%p:%d:%p"
> + "on the defer queue as internal\n",
> 
> WARNING: vsprintf %p string concatenation
> #3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
> + dev_notice(>pdev->dev, "moving 
> cmd[%d]:%p:%d:%p"
> + "on the defer queue as internal\n",
> 
> I think the new message is not that useful really as the
> existing warning is probably enough.

Oh, so we already have it... Didn't know that. Yes, I think the existing
one is good enough. Thanks for the pointers.

-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Sat, 2018-04-07 at 09:33 +0900, Sergey Senozhatsky wrote:
> Hi Joe,
> 
> On (04/06/18 16:59), Joe Perches wrote:
> > > 
> > > Can we tweak checkpatch to catch such things?
> > 
> > Not really, no.
> > 
> > Adding regex logic for this is tricky at best
> > and probably not worth the effort because of
> > the various bits of patch contexts aren't
> > necessarily visible.
> 
> Agreed. I was more thinking about catching "... %p" and saying
> that we'd rather prefer either "... %p," or "... %p " or "... %p\n".
> Doesn't sound so complex, can probably catch something fishy one day
> (or may be not), and more or less is visible to checkpatch. Well,
> more or less...

This finds the current two bad uses in addition to
the existing similar message for string concatenation
without a space char between concatenated fragments.

For example:

WARNING: break quoted strings at a space character
#3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
+   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p"
+   "on the defer queue as internal\n",

WARNING: vsprintf %p string concatenation
#3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
+   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p"
+   "on the defer queue as internal\n",

I think the new message is not that useful really as the
existing warning is probably enough.

---
 scripts/checkpatch.pl | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eb534d48140e..a0e43232431e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5313,6 +5313,12 @@ sub process {
 "break quoted strings at a space character\n" . 
$hereprev);
}
 
+# check for vsprintf pointer extension concatenation
+   if ($prevrawline =~ /\%p"\s*$/ && $rawline =~ /^\+\s*"\w/) {
+   WARN('POINTER_CONCATENATION',
+"vsprintf %p string concatenation\n" . 
$hereprev);
+   }
+
 # check for an embedded function name in a string when the function is known
 # This does not work very well for -f --file checking as it depends on patch
 # context providing the function name or a single line form for in-file

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Sat, 2018-04-07 at 09:33 +0900, Sergey Senozhatsky wrote:
> Hi Joe,
> 
> On (04/06/18 16:59), Joe Perches wrote:
> > > 
> > > Can we tweak checkpatch to catch such things?
> > 
> > Not really, no.
> > 
> > Adding regex logic for this is tricky at best
> > and probably not worth the effort because of
> > the various bits of patch contexts aren't
> > necessarily visible.
> 
> Agreed. I was more thinking about catching "... %p" and saying
> that we'd rather prefer either "... %p," or "... %p " or "... %p\n".
> Doesn't sound so complex, can probably catch something fishy one day
> (or may be not), and more or less is visible to checkpatch. Well,
> more or less...

This finds the current two bad uses in addition to
the existing similar message for string concatenation
without a space char between concatenated fragments.

For example:

WARNING: break quoted strings at a space character
#3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
+   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p"
+   "on the defer queue as internal\n",

WARNING: vsprintf %p string concatenation
#3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
+   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p"
+   "on the defer queue as internal\n",

I think the new message is not that useful really as the
existing warning is probably enough.

---
 scripts/checkpatch.pl | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eb534d48140e..a0e43232431e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5313,6 +5313,12 @@ sub process {
 "break quoted strings at a space character\n" . 
$hereprev);
}
 
+# check for vsprintf pointer extension concatenation
+   if ($prevrawline =~ /\%p"\s*$/ && $rawline =~ /^\+\s*"\w/) {
+   WARN('POINTER_CONCATENATION',
+"vsprintf %p string concatenation\n" . 
$hereprev);
+   }
+
 # check for an embedded function name in a string when the function is known
 # This does not work very well for -f --file checking as it depends on patch
 # context providing the function name or a single line form for in-file

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

Hi Joe,

On (04/06/18 16:59), Joe Perches wrote:
> > 
> > Can we tweak checkpatch to catch such things?
> 
> Not really, no.
> 
> Adding regex logic for this is tricky at best
> and probably not worth the effort because of
> the various bits of patch contexts aren't
> necessarily visible.

Agreed. I was more thinking about catching "... %p" and saying
that we'd rather prefer either "... %p," or "... %p " or "... %p\n".
Doesn't sound so complex, can probably catch something fishy one day
(or may be not), and more or less is visible to checkpatch. Well,
more or less...

> There are also concatenations like
>   "foo" DEFINE "bar"
> where DEFINE may not be visible in the patch
> context and checkpatch is and likely will
> remain just a limited regex checker.

Right. One example might be XFS

alert("%s: Bad regular inode %Lu, ptr "PTR_FMT,
__func__, ip->i_ino, ip);

where PTR_FMT is

#ifdef DEBUG
# define PTR_FMT "%px"
#else
# define PTR_FMT "%p"
#endif

-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

Hi Joe,

On (04/06/18 16:59), Joe Perches wrote:
> > 
> > Can we tweak checkpatch to catch such things?
> 
> Not really, no.
> 
> Adding regex logic for this is tricky at best
> and probably not worth the effort because of
> the various bits of patch contexts aren't
> necessarily visible.

Agreed. I was more thinking about catching "... %p" and saying
that we'd rather prefer either "... %p," or "... %p " or "... %p\n".
Doesn't sound so complex, can probably catch something fishy one day
(or may be not), and more or less is visible to checkpatch. Well,
more or less...

> There are also concatenations like
>   "foo" DEFINE "bar"
> where DEFINE may not be visible in the patch
> context and checkpatch is and likely will
> remain just a limited regex checker.

Right. One example might be XFS

alert("%s: Bad regular inode %Lu, ptr "PTR_FMT,
__func__, ip->i_ino, ip);

where PTR_FMT is

#ifdef DEBUG
# define PTR_FMT "%px"
#else
# define PTR_FMT "%p"
#endif

-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Sat, 2018-04-07 at 08:52 +0900, Sergey Senozhatsky wrote:
> On (04/05/18 16:55), Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > which should get fixed before somebody claims those extensions.
> > > 
> > > Neither %pt nor %po is used in a vsprintf
> > > in the kernel.
> > 
> > Nope, you are right, both are defectively used in the
> > kernel via string concatenation.
> > 
> > Also there's a missing space in a concatenation adjacent.
> 
> Can we tweak checkpatch to catch such things?

Not really, no.

Adding regex logic for this is tricky at best
and probably not worth the effort because of
the various bits of patch contexts aren't
necessarily visible.

There are also concatenations like
"foo" DEFINE "bar"
where DEFINE may not be visible in the patch
context and checkpatch is and likely will
remain just a limited regex checker.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Sat, 2018-04-07 at 08:52 +0900, Sergey Senozhatsky wrote:
> On (04/05/18 16:55), Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > which should get fixed before somebody claims those extensions.
> > > 
> > > Neither %pt nor %po is used in a vsprintf
> > > in the kernel.
> > 
> > Nope, you are right, both are defectively used in the
> > kernel via string concatenation.
> > 
> > Also there's a missing space in a concatenation adjacent.
> 
> Can we tweak checkpatch to catch such things?

Not really, no.

Adding regex logic for this is tricky at best
and probably not worth the effort because of
the various bits of patch contexts aren't
necessarily visible.

There are also concatenations like
"foo" DEFINE "bar"
where DEFINE may not be visible in the patch
context and checkpatch is and likely will
remain just a limited regex checker.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

On (04/05/18 16:55), Joe Perches wrote:
> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > which should get fixed before somebody claims those extensions.
> > 
> > Neither %pt nor %po is used in a vsprintf
> > in the kernel.
> 
> Nope, you are right, both are defectively used in the
> kernel via string concatenation.
> 
> Also there's a missing space in a concatenation adjacent.

Can we tweak checkpatch to catch such things?


Hm... *Probably* also wouldn't hurt if checkpatch can require at least
one character after pointer format specifiers:

printk("string %p"  vs   printk("string %p "
   " Object\n", ptr);   "Object\n", ptr);

Especially if we can have a "potential" %px, like here

   dev_vdbg(>input->dev,
   "%s: *axis=%02X(%d) size=%d max=%08X xy_data=%p"
   " xy_data[%d]=%02X(%d) bofs=%d\n",

or here

   dev_vdbg(>input->dev,
   "%s: *axis=%02X(%d) size=%d max=%08X xy_data=%p"
   " xy_data[%d]=%02X(%d)\n",

Opinions?

-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

On (04/05/18 16:55), Joe Perches wrote:
> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > which should get fixed before somebody claims those extensions.
> > 
> > Neither %pt nor %po is used in a vsprintf
> > in the kernel.
> 
> Nope, you are right, both are defectively used in the
> kernel via string concatenation.
> 
> Also there's a missing space in a concatenation adjacent.

Can we tweak checkpatch to catch such things?


Hm... *Probably* also wouldn't hurt if checkpatch can require at least
one character after pointer format specifiers:

printk("string %p"  vs   printk("string %p "
   " Object\n", ptr);   "Object\n", ptr);

Especially if we can have a "potential" %px, like here

   dev_vdbg(>input->dev,
   "%s: *axis=%02X(%d) size=%d max=%08X xy_data=%p"
   " xy_data[%d]=%02X(%d) bofs=%d\n",

or here

   dev_vdbg(>input->dev,
   "%s: *axis=%02X(%d) size=%d max=%08X xy_data=%p"
   " xy_data[%d]=%02X(%d)\n",

Opinions?

-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> On 2018-04-06 13:43, Petr Mladek wrote:
> > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > which should get fixed before somebody claims those extensions.
> > > > 
> > > > Neither %pt nor %po is used in a vsprintf
> > > > in the kernel.
> > > 
> > > Nope, you are right, both are defectively used in the
> > > kernel via string concatenation.
> > 
> > Ah, I see.
> 
> Yes, that was the point of that particular grep pattern with context. I
> sent patches three years ago and several pings, but despite an email
> telling me it would be picked up "for the next release" nothing has
> happened, so I've just been waiting for someone to implement a %po.
> 
> > Otherwise, the changes make perfect sense. Are you going to send
> > them as proper patches, please?
> 
> Good luck. Unless Petr will be taking them through the printk tree?

As the trivial maintainer seems to use a leaky bucket algorithm
for most trivial patches, I'll just retry and send actual
patches through Andrew Morton as patches through his quilt tree
seem to get upstream faster.

And thanks Petr for noticing the missing __func__, I was just
mindlessly editing those two files as a placeholder for where
the missing spaces in the %p concatenations exist.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> On 2018-04-06 13:43, Petr Mladek wrote:
> > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > which should get fixed before somebody claims those extensions.
> > > > 
> > > > Neither %pt nor %po is used in a vsprintf
> > > > in the kernel.
> > > 
> > > Nope, you are right, both are defectively used in the
> > > kernel via string concatenation.
> > 
> > Ah, I see.
> 
> Yes, that was the point of that particular grep pattern with context. I
> sent patches three years ago and several pings, but despite an email
> telling me it would be picked up "for the next release" nothing has
> happened, so I've just been waiting for someone to implement a %po.
> 
> > Otherwise, the changes make perfect sense. Are you going to send
> > them as proper patches, please?
> 
> Good luck. Unless Petr will be taking them through the printk tree?

As the trivial maintainer seems to use a leaky bucket algorithm
for most trivial patches, I'll just retry and send actual
patches through Andrew Morton as patches through his quilt tree
seem to get upstream faster.

And thanks Petr for noticing the missing __func__, I was just
mindlessly editing those two files as a placeholder for where
the missing spaces in the %p concatenations exist.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Rasmus Villemoes]

On 2018-04-06 13:43, Petr Mladek wrote:
> On Thu 2018-04-05 16:55:35, Joe Perches wrote:
>> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
>>> On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
 Even just git grep -1 -E '%p"$' finds %pt and %po
 which should get fixed before somebody claims those extensions.
>>>
>>> Neither %pt nor %po is used in a vsprintf
>>> in the kernel.
>>
>> Nope, you are right, both are defectively used in the
>> kernel via string concatenation.
> 
> Ah, I see.

Yes, that was the point of that particular grep pattern with context. I
sent patches three years ago and several pings, but despite an email
telling me it would be picked up "for the next release" nothing has
happened, so I've just been waiting for someone to implement a %po.

> Otherwise, the changes make perfect sense. Are you going to send
> them as proper patches, please?

Good luck. Unless Petr will be taking them through the printk tree?

Rasmus

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Rasmus Villemoes]

On 2018-04-06 13:43, Petr Mladek wrote:
> On Thu 2018-04-05 16:55:35, Joe Perches wrote:
>> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
>>> On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
 Even just git grep -1 -E '%p"$' finds %pt and %po
 which should get fixed before somebody claims those extensions.
>>>
>>> Neither %pt nor %po is used in a vsprintf
>>> in the kernel.
>>
>> Nope, you are right, both are defectively used in the
>> kernel via string concatenation.
> 
> Ah, I see.

Yes, that was the point of that particular grep pattern with context. I
sent patches three years ago and several pings, but despite an email
telling me it would be picked up "for the next release" nothing has
happened, so I've just been waiting for someone to implement a %po.

> Otherwise, the changes make perfect sense. Are you going to send
> them as proper patches, please?

Good luck. Unless Petr will be taking them through the printk tree?

Rasmus

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > which should get fixed before somebody claims those extensions.
> > 
> > Neither %pt nor %po is used in a vsprintf
> > in the kernel.
> 
> Nope, you are right, both are defectively used in the
> kernel via string concatenation.

Ah, I see.

> Also there's a missing space in a concatenation adjacent.


> ---
>  arch/s390/kernel/perf_cpum_sf.c   | 4 +---
>  drivers/scsi/megaraid/megaraid_sas_base.c | 7 ++-
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
> index 1c9ddd7aa5ec..458b8f321043 100644
> --- a/arch/s390/kernel/perf_cpum_sf.c
> +++ b/arch/s390/kernel/perf_cpum_sf.c
> @@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
>* the sampling buffer origin.
>*/
>   if (sfb->sdbt != get_next_sdbt(tail)) {
> - debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
> - "sampling buffer is not linked: origin=%p"
> - "tail=%p\n",
> + debug_sprintf_event(sfdbg, 3, "%s: sampling buffer is not 
> linked: origin=%p tail=%p\n",
   ^^
I guess that you wanted to add __func__ as the next parameter?

Otherwise, the changes make perfect sense. Are you going to send
them as proper patches, please?

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > which should get fixed before somebody claims those extensions.
> > 
> > Neither %pt nor %po is used in a vsprintf
> > in the kernel.
> 
> Nope, you are right, both are defectively used in the
> kernel via string concatenation.

Ah, I see.

> Also there's a missing space in a concatenation adjacent.


> ---
>  arch/s390/kernel/perf_cpum_sf.c   | 4 +---
>  drivers/scsi/megaraid/megaraid_sas_base.c | 7 ++-
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
> index 1c9ddd7aa5ec..458b8f321043 100644
> --- a/arch/s390/kernel/perf_cpum_sf.c
> +++ b/arch/s390/kernel/perf_cpum_sf.c
> @@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
>* the sampling buffer origin.
>*/
>   if (sfb->sdbt != get_next_sdbt(tail)) {
> - debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
> - "sampling buffer is not linked: origin=%p"
> - "tail=%p\n",
> + debug_sprintf_event(sfdbg, 3, "%s: sampling buffer is not 
> linked: origin=%p tail=%p\n",
   ^^
I guess that you wanted to add __func__ as the next parameter?

Otherwise, the changes make perfect sense. Are you going to send
them as proper patches, please?

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Thu 2018-04-05 16:25:41, Rasmus Villemoes wrote:
> On 2018-04-04 10:58, Petr Mladek wrote:
> > There are few printk formats that make sense only with two or more
> > specifiers. Also some specifiers make sense only when a kernel feature
> > is enabled.
> > 
> > The handling of unknown specifiers is strange, inconsistent, and
> > even leaking the address. For example, netdev_bits() prints the
> > non-hashed pointer value or clock() prints "(null)".
> > 
> > The best solution seems to be in flags_string(). It does not print any
> > misleading value. Instead it calls WARN_ONCE() describing the unknown
> > specifier. Therefore it clearly shows the problem and helps to find it.
> >
> I'm not sure it's actually worth WARNing about the unknown variants
> since

I agree that WARN() looks like an overkill for this type of error.
On the other hand, I am not sure how to make it lightweight and useful
at the same time.

We could not be much talkative in the passed buffer because we do not
know how big it is. We could call printk but it might be hard to match
the line with the caller without the backtrace.

The extra printk() message would end up in the per-CPU printk_safe
buffer when the broken vsprintf() comes from printk(). It might be
pushed to the main log buffer "much" later. On the other hand,
the original string would not end in the log buffer at all
when vsprintf() is not called from printk().


> we have static analysis (at least checkpatch and smatch) that
> should catch that.

Yes but not everybody uses it. Also there are things that depends
on CONFIG setting.

Anyway, I think that triggering the WARN() is rather a corner case.
So I would not be much concerned about using WARN().


> Even just git grep -1 -E '%p"$' finds %pt and %po
> which should get fixed before somebody claims those extensions.

I do not understand this much. git grep -1 -E '%p"$' does not find
lines with %pt and %po. Also it seems that %pt and %po are not
part of any format passed to vsprintf.



> But, I don't disagree with trying to fix up the inconsistency, and
> certainly not with fixing netdev_bits(), but it seems you've missed that
> e.g. the "case: 'g'" is completely compiled out for !CONFIG_BLOCK.
> There's also %pOF which is effectively disabled for !CONFIG_OF (which
> obviously makes sense), but with yet a different fallback behaviour.

You are accurate ;-) I was a bit lazy to change them. %pg was "nicely"
compiled out. %pOF was nicely minimalist solution but not usable in
general. I could do it in v5 or in a separate patchset.


> Hm. I think we should somehow distinguish between the cases of "%po" and
> "%pNX", i.e. specifiers/variants that are always bogus, and the cases of
> a %pOF or %pC that somehow happens even though nobody should have a
> struct device_node* or struct clk* to pass.

I think that the existing formulation is fine for the always bogus
specifiers, for example:

  "Unsupported pointer format specifier: %pGa"

What about the following for the other case:

  "Format specifier %%pC supported only with CONFIG_HAVE_CLK\n"

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Thu 2018-04-05 16:25:41, Rasmus Villemoes wrote:
> On 2018-04-04 10:58, Petr Mladek wrote:
> > There are few printk formats that make sense only with two or more
> > specifiers. Also some specifiers make sense only when a kernel feature
> > is enabled.
> > 
> > The handling of unknown specifiers is strange, inconsistent, and
> > even leaking the address. For example, netdev_bits() prints the
> > non-hashed pointer value or clock() prints "(null)".
> > 
> > The best solution seems to be in flags_string(). It does not print any
> > misleading value. Instead it calls WARN_ONCE() describing the unknown
> > specifier. Therefore it clearly shows the problem and helps to find it.
> >
> I'm not sure it's actually worth WARNing about the unknown variants
> since

I agree that WARN() looks like an overkill for this type of error.
On the other hand, I am not sure how to make it lightweight and useful
at the same time.

We could not be much talkative in the passed buffer because we do not
know how big it is. We could call printk but it might be hard to match
the line with the caller without the backtrace.

The extra printk() message would end up in the per-CPU printk_safe
buffer when the broken vsprintf() comes from printk(). It might be
pushed to the main log buffer "much" later. On the other hand,
the original string would not end in the log buffer at all
when vsprintf() is not called from printk().


> we have static analysis (at least checkpatch and smatch) that
> should catch that.

Yes but not everybody uses it. Also there are things that depends
on CONFIG setting.

Anyway, I think that triggering the WARN() is rather a corner case.
So I would not be much concerned about using WARN().


> Even just git grep -1 -E '%p"$' finds %pt and %po
> which should get fixed before somebody claims those extensions.

I do not understand this much. git grep -1 -E '%p"$' does not find
lines with %pt and %po. Also it seems that %pt and %po are not
part of any format passed to vsprintf.



> But, I don't disagree with trying to fix up the inconsistency, and
> certainly not with fixing netdev_bits(), but it seems you've missed that
> e.g. the "case: 'g'" is completely compiled out for !CONFIG_BLOCK.
> There's also %pOF which is effectively disabled for !CONFIG_OF (which
> obviously makes sense), but with yet a different fallback behaviour.

You are accurate ;-) I was a bit lazy to change them. %pg was "nicely"
compiled out. %pOF was nicely minimalist solution but not usable in
general. I could do it in v5 or in a separate patchset.


> Hm. I think we should somehow distinguish between the cases of "%po" and
> "%pNX", i.e. specifiers/variants that are always bogus, and the cases of
> a %pOF or %pC that somehow happens even though nobody should have a
> struct device_node* or struct clk* to pass.

I think that the existing formulation is fine for the always bogus
specifiers, for example:

  "Unsupported pointer format specifier: %pGa"

What about the following for the other case:

  "Format specifier %%pC supported only with CONFIG_HAVE_CLK\n"

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > Even just git grep -1 -E '%p"$' finds %pt and %po
> > which should get fixed before somebody claims those extensions.
> 
> Neither %pt nor %po is used in a vsprintf
> in the kernel.

Nope, you are right, both are defectively used in the
kernel via string concatenation.

Also there's a missing space in a concatenation adjacent.

---
 arch/s390/kernel/perf_cpum_sf.c   | 4 +---
 drivers/scsi/megaraid/megaraid_sas_base.c | 7 ++-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index 1c9ddd7aa5ec..458b8f321043 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
 * the sampling buffer origin.
 */
if (sfb->sdbt != get_next_sdbt(tail)) {
-   debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
-   "sampling buffer is not linked: origin=%p"
-   "tail=%p\n",
+   debug_sprintf_event(sfdbg, 3, "%s: sampling buffer is not 
linked: origin=%p tail=%p\n",
(void *) sfb->sdbt, (void *) tail);
return -EINVAL;
}
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c 
b/drivers/scsi/megaraid/megaraid_sas_base.c
index b89c6e6c0589..56f1c8132664 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -3546,14 +3546,11 @@ megasas_internal_reset_defer_cmds(struct 
megasas_instance *instance)
for (i = 0; i < max_cmd; i++) {
cmd = instance->cmd_list[i];
if (cmd->sync_cmd == 1 || cmd->scmd) {
-   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p"
-   "on the defer queue as internal\n",
+   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p on the defer queue as internal\n",
defer_index, cmd, cmd->sync_cmd, cmd->scmd);
 
if (!list_empty(>list)) {
-   dev_notice(>pdev->dev, "ERROR while"
-   " moving this cmd:%p, %d %p, it was"
-   "discovered on some list?\n",
+   dev_notice(>pdev->dev, "ERROR while 
moving this cmd:%p, %d %p, it was discovered on some list?\n",
cmd, cmd->sync_cmd, cmd->scmd);
 
list_del_init(>list);

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > Even just git grep -1 -E '%p"$' finds %pt and %po
> > which should get fixed before somebody claims those extensions.
> 
> Neither %pt nor %po is used in a vsprintf
> in the kernel.

Nope, you are right, both are defectively used in the
kernel via string concatenation.

Also there's a missing space in a concatenation adjacent.

---
 arch/s390/kernel/perf_cpum_sf.c   | 4 +---
 drivers/scsi/megaraid/megaraid_sas_base.c | 7 ++-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index 1c9ddd7aa5ec..458b8f321043 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
 * the sampling buffer origin.
 */
if (sfb->sdbt != get_next_sdbt(tail)) {
-   debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
-   "sampling buffer is not linked: origin=%p"
-   "tail=%p\n",
+   debug_sprintf_event(sfdbg, 3, "%s: sampling buffer is not 
linked: origin=%p tail=%p\n",
(void *) sfb->sdbt, (void *) tail);
return -EINVAL;
}
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c 
b/drivers/scsi/megaraid/megaraid_sas_base.c
index b89c6e6c0589..56f1c8132664 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -3546,14 +3546,11 @@ megasas_internal_reset_defer_cmds(struct 
megasas_instance *instance)
for (i = 0; i < max_cmd; i++) {
cmd = instance->cmd_list[i];
if (cmd->sync_cmd == 1 || cmd->scmd) {
-   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p"
-   "on the defer queue as internal\n",
+   dev_notice(>pdev->dev, "moving 
cmd[%d]:%p:%d:%p on the defer queue as internal\n",
defer_index, cmd, cmd->sync_cmd, cmd->scmd);
 
if (!list_empty(>list)) {
-   dev_notice(>pdev->dev, "ERROR while"
-   " moving this cmd:%p, %d %p, it was"
-   "discovered on some list?\n",
+   dev_notice(>pdev->dev, "ERROR while 
moving this cmd:%p, %d %p, it was discovered on some list?\n",
cmd, cmd->sync_cmd, cmd->scmd);
 
list_del_init(>list);

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> Even just git grep -1 -E '%p"$' finds %pt and %po
> which should get fixed before somebody claims those extensions.

Neither %pt nor %po is used in a vsprintf
in the kernel.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> Even just git grep -1 -E '%p"$' finds %pt and %po
> which should get fixed before somebody claims those extensions.

Neither %pt nor %po is used in a vsprintf
in the kernel.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Rasmus Villemoes]

On 2018-04-04 10:58, Petr Mladek wrote:
> There are few printk formats that make sense only with two or more
> specifiers. Also some specifiers make sense only when a kernel feature
> is enabled.
> 
> The handling of unknown specifiers is strange, inconsistent, and
> even leaking the address. For example, netdev_bits() prints the
> non-hashed pointer value or clock() prints "(null)".
> 
> The best solution seems to be in flags_string(). It does not print any
> misleading value. Instead it calls WARN_ONCE() describing the unknown
> specifier. Therefore it clearly shows the problem and helps to find it.
>

I'm not sure it's actually worth WARNing about the unknown variants
since we have static analysis (at least checkpatch and smatch) that
should catch that. Even just git grep -1 -E '%p"$' finds %pt and %po
which should get fixed before somebody claims those extensions.

But, I don't disagree with trying to fix up the inconsistency, and
certainly not with fixing netdev_bits(), but it seems you've missed that
e.g. the "case: 'g'" is completely compiled out for !CONFIG_BLOCK.
There's also %pOF which is effectively disabled for !CONFIG_OF (which
obviously makes sense), but with yet a different fallback behaviour.

Hm. I think we should somehow distinguish between the cases of "%po" and
"%pNX", i.e. specifiers/variants that are always bogus, and the cases of
a %pOF or %pC that somehow happens even though nobody should have a
struct device_node* or struct clk* to pass.

Rasmus

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Rasmus Villemoes]

On 2018-04-04 10:58, Petr Mladek wrote:
> There are few printk formats that make sense only with two or more
> specifiers. Also some specifiers make sense only when a kernel feature
> is enabled.
> 
> The handling of unknown specifiers is strange, inconsistent, and
> even leaking the address. For example, netdev_bits() prints the
> non-hashed pointer value or clock() prints "(null)".
> 
> The best solution seems to be in flags_string(). It does not print any
> misleading value. Instead it calls WARN_ONCE() describing the unknown
> specifier. Therefore it clearly shows the problem and helps to find it.
>

I'm not sure it's actually worth WARNing about the unknown variants
since we have static analysis (at least checkpatch and smatch) that
should catch that. Even just git grep -1 -E '%p"$' finds %pt and %po
which should get fixed before somebody claims those extensions.

But, I don't disagree with trying to fix up the inconsistency, and
certainly not with fixing netdev_bits(), but it seems you've missed that
e.g. the "case: 'g'" is completely compiled out for !CONFIG_BLOCK.
There's also %pOF which is effectively disabled for !CONFIG_OF (which
obviously makes sense), but with yet a different fallback behaviour.

Hm. I think we should somehow distinguish between the cases of "%po" and
"%pNX", i.e. specifiers/variants that are always bogus, and the cases of
a %pOF or %pC that somehow happens even though nobody should have a
struct device_node* or struct clk* to pass.

Rasmus

[PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

There are few printk formats that make sense only with two or more
specifiers. Also some specifiers make sense only when a kernel feature
is enabled.

The handling of unknown specifiers is strange, inconsistent, and
even leaking the address. For example, netdev_bits() prints the
non-hashed pointer value or clock() prints "(null)".

The best solution seems to be in flags_string(). It does not print any
misleading value. Instead it calls WARN_ONCE() describing the unknown
specifier. Therefore it clearly shows the problem and helps to find it.

Note that WARN_ONCE() used to cause recursive printk(). But it is safe
now because vscnprintf() is called in printk_safe context from
vprintk_emit().

Signed-off-by: Petr Mladek 
---
 lib/vsprintf.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dd71738d7a09..5a0d01846a11 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1484,9 +1484,9 @@ char *netdev_bits(char *buf, char *end, const void *addr, 
const char *fmt)
size = sizeof(netdev_features_t);
break;
default:
-   num = (unsigned long)addr;
-   size = sizeof(unsigned long);
-   break;
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pN%c\n",
+ fmt[1]);
+   return buf;
}
 
return special_hex_number(buf, end, num, size);
@@ -1517,7 +1517,12 @@ static noinline_for_stack
 char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
const char *fmt)
 {
-   if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
+   if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pC\n");
+   return buf;
+   }
+
+   if (!clk)
return string(buf, end, NULL, spec);
 
switch (fmt[1]) {
@@ -1529,7 +1534,8 @@ char *clock(char *buf, char *end, struct clk *clk, struct 
printf_spec spec,
 #ifdef CONFIG_COMMON_CLK
return string(buf, end, __clk_get_name(clk), spec);
 #else
-   return special_hex_number(buf, end, (unsigned long)clk, 
sizeof(unsigned long));
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pC\n");
+   return buf;
 #endif
}
 }
@@ -1593,7 +1599,8 @@ char *flags_string(char *buf, char *end, void *flags_ptr, 
const char *fmt)
names = gfpflag_names;
break;
default:
-   WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pG%c\n",
+ fmt[1]);
return buf;
}
 
-- 
2.13.6

[PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

There are few printk formats that make sense only with two or more
specifiers. Also some specifiers make sense only when a kernel feature
is enabled.

The handling of unknown specifiers is strange, inconsistent, and
even leaking the address. For example, netdev_bits() prints the
non-hashed pointer value or clock() prints "(null)".

The best solution seems to be in flags_string(). It does not print any
misleading value. Instead it calls WARN_ONCE() describing the unknown
specifier. Therefore it clearly shows the problem and helps to find it.

Note that WARN_ONCE() used to cause recursive printk(). But it is safe
now because vscnprintf() is called in printk_safe context from
vprintk_emit().

Signed-off-by: Petr Mladek 
---
 lib/vsprintf.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dd71738d7a09..5a0d01846a11 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1484,9 +1484,9 @@ char *netdev_bits(char *buf, char *end, const void *addr, 
const char *fmt)
size = sizeof(netdev_features_t);
break;
default:
-   num = (unsigned long)addr;
-   size = sizeof(unsigned long);
-   break;
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pN%c\n",
+ fmt[1]);
+   return buf;
}
 
return special_hex_number(buf, end, num, size);
@@ -1517,7 +1517,12 @@ static noinline_for_stack
 char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
const char *fmt)
 {
-   if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
+   if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pC\n");
+   return buf;
+   }
+
+   if (!clk)
return string(buf, end, NULL, spec);
 
switch (fmt[1]) {
@@ -1529,7 +1534,8 @@ char *clock(char *buf, char *end, struct clk *clk, struct 
printf_spec spec,
 #ifdef CONFIG_COMMON_CLK
return string(buf, end, __clk_get_name(clk), spec);
 #else
-   return special_hex_number(buf, end, (unsigned long)clk, 
sizeof(unsigned long));
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pC\n");
+   return buf;
 #endif
}
 }
@@ -1593,7 +1599,8 @@ char *flags_string(char *buf, char *end, void *flags_ptr, 
const char *fmt)
names = gfpflag_names;
break;
default:
-   WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
+   WARN_ONCE(1, "Unsupported pointer format specifier: %%pG%c\n",
+ fmt[1]);
return buf;
}
 
-- 
2.13.6