Re: [PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log

Matthew Rosato
2022-06-06T20:59:04Z

On 6/3/22 3:03 PM, Eric Farman wrote:
> On Thu, 2022-06-02 at 15:51 -0400, Matthew Rosato wrote:
>> On 6/2/22 1:19 PM, Eric Farman wrote:
>>> From: Michael Kawano <mkawano@linux.ibm.com>
>>>
>>> As vfio-ccw devices are created/destroyed, the uuid of the
>>> associated
>>> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost
>>> as
>>> they are created using pointers passed by reference.
>>>
>>> This is a deliberate design point of s390dbf, but it leaves the
>>> uuid
>>
>> This wording is confusing, maybe some re-wording would help here.
>>
>> Basically, s390dbf doesn't support values passed by reference today
>> (e.g. %pUl), it will just store that pointer (e.g. &mdev->uuid) and
>> not
>> its contents -- so a subsequent viewing of the s390dbf log at any
>> point
>> in the future will go peek at that referenced memory -- which might
>> have
>> been freed (e.g. mdev was removed). So this change will fix
>> potential
>> garbage data viewed from the log or worse an oops when viewing the
>> log
>> -- the latter of which should probably be mentioned in the commit
>> message.
>>
>> I'm not sure if it was a deliberate design decision of s390dbf or
>> just a
>> feature that was never implemented, so I'd omit that altogether --
>> but
>> it IS pointed out in the s390dbf documentation as a limitation
>> anyway.
>
> @Jason, @Matt... All fair, I obviously got too verbose in whatever I
> was writing at the time. I've changed this to:
>
> As vfio-ccw devices are created/destroyed, the uuid of the associated
> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost.
> This is because a pointer to the UUID is stored instead of the UUID
> itself, and that memory may have been repurposed if/when the logs are
> examined. The result is usually garbage UUID data in the logs, though
> there is an outside chance of an oops happening here.
>
> Simply remove the UUID from the traces, as the subchannel number will
> provide useful configuration information for problem determination,
> and is stored directly into the log instead of a pointer.
>
> As we were the only consumer of mdev_uuid(), remove that too.
>

Sounds good