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

From: Matthew Rosato <mjrosato@linux.ibm.com>
To: Eric Farman <farman@linux.ibm.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Liu Yi L <yi.l.liu@intel.com>, Halil Pasic <pasic@linux.ibm.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	Michael Kawano <mkawano@linux.ibm.com>,
	Kirti Wankhede <kwankhede@nvidia.com>
Subject: Re: [PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log
Date: Mon, 6 Jun 2022 16:45:20 -0400	[thread overview]
Message-ID: <54ae7d76-cf67-c77b-a7e9-608fb09674d9@linux.ibm.com> (raw)
In-Reply-To: <a9ff5850a4964fe0238261c35591c3c18a4a8df3.camel@linux.ibm.com>

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