Re: [Xen-devel] printf formatter for pci_sbdf_t

[George Dunlap <dunlapg@xxxxxxxxx>]

On Wed, Jul 17, 2019 at 3:08 PM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> Hello,
>
> As part of some PCI related cleanup I'm doing, which includes
> expanding the usage of pci_sbdf_t, I'm also planning to add a custom
> printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
> without having to specify four formatters (and pass four parameters)
> each time (%04x:%02x:%02x.%u).
>
> There's been some debate on the previous version about whether the
> formatter should be %pp or %op, and I would like to settle on one of
> them before sending a new version:
>
> Using %pp pros:
>  - Xen already overloads p with other custom implementations.
>
> Using %pp cons:
>  - Passes a pointer (which is always 64b on x86) to store a
>    32bit value (SBDF).
>  - Requires a dereference to access the value.
>
> Using %op pros:
>  - Can pass a 32bit integer naturally.
>
> Using %op cons:
>  - No other overloads of the o specifier exists so far, either in Xen
>    or in Linux AFAIK.
>
> My first implementation used %pp because it's inline with the current
> overloads already present, and printk not being performance critical I
> don't see much problem in using 64bit to pass a 32bit value, or in
> requiring a dereference to access it. We could keep using %pp and
> casting the sbdf value to 'void *' to avoid the dereference, but I
> don't think there's much value on doing that, the more that call sites
> would need to use a macro to hide the casting away.

This is missing a bit more background needed to understand what's going on.

In particular, we use gcc's -Wformat option to "check" the values for
printk and other printf-like functions.  gcc will only allow values
that it knows about.

This check is meant to increase security, but in fact, all of the
"extended" %p format are driving a big truck through this; to gcc,
"%pd" looks like it should print "<pointer value>d".  gcc can check
that *a pointer* was passed, but not *the right pointer*; so it's
entirely possible to make a mistake where you pass a complex pointer
and cause a buffer overflow or other issues.  And as the "extended"
pointer thing may print an arbitrary number of bytes, it completely
throws off any sprintf-style buffer size checking.

gcc already has lots of -Wformat* features; the very first time Linux
wanted to do this 15 years ago or whenever, they should have gone to
the gcc guys and asked for this type of formatting to be explicitly
supported and type checked.  But anyway, here we are.

Given how much what we do undermines the safety which -Wformat
provides, I think it is worth deciding whether it's something we
actually need.

Andy objects to the idea of using "%o" for anything other than
displaying octal.  I agree with that.  Furthermore, I think "%p"
should *only* be used for things which are actually pointers, for
basically the same reason.  If we use "%pp" or something like it, it
should pass a pointer, not a value cast to a (void *).

The idea behind passing-by-value rather than passing-by-reference
seems to be to be able to construct a value without having to create
one on the stack.  I agree this is a more satisfying thing to do, but
printk is already normally sending out bits over a serial line; I
don't think an extra word on the stack and an extra dereference of a
hot pointer is really that big of a win.

I would personally be OK with extending '%x', since 'x' to me
indicates direct visualization of a binary value.  But neither Jan nor
Andy seem to like that option, and I don't feel like pushing it.

Which leaves two options, AFAICT:

1. Use %pp (or something like it), and pass a pointer
2. Disable -Wformat, at least until such time as it supports extending
the format checking.

Andy seems keen on having "%p" be the only overloaded operator.  I
disagree; I don't mind overloading other operators, but I agree that
"%o" isn't a good fit. I think "%p" should be only for pointers.

Staying in sync with Linux isn't a big issue; if they end up using
"%pp", we can just switch to a different letter / identifier.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel