Re: [PATCH v2] powerpc/pseries/mobility: ignore ibm, platform-facilities updates

Daniel Axtens [off-list ref] writes:

quoted

On VMs with NX encryption, compression, and/or RNG offload, these
capabilities are described by nodes in the ibm,platform-facilities device
tree hierarchy:

  $ tree -d /sys/firmware/devicetree/base/ibm,platform-facilities/
  /sys/firmware/devicetree/base/ibm,platform-facilities/
  ├── ibm,compression-v1
  ├── ibm,random-v1
  └── ibm,sym-encryption-v1

  3 directories

The acceleration functions that these nodes describe are not disrupted by
live migration, not even temporarily.

But the post-migration ibm,update-nodes sequence firmware always sends
"delete" messages for this hierarchy, followed by an "add" directive to
reconstruct it via ibm,configure-connector (log with debugging statements
enabled in mobility.c):

  mobility: removing node /ibm,platform-facilities/ibm,random-v1:4294967285
  mobility: removing node /ibm,platform-facilities/ibm,compression-v1:4294967284
  mobility: removing node /ibm,platform-facilities/ibm,sym-encryption-v1:4294967283
  mobility: removing node /ibm,platform-facilities:4294967286
  ...
  mobility: added node /ibm,platform-facilities:4294967286

Note we receive a single "add" message for the entire hierarchy, and what
we receive from the ibm,configure-connector sequence is the top-level
platform-facilities node along with its three children. The debug message
simply reports the parent node and not the whole subtree.

If I understand correctly, (and again, this is not my area at all!) we
still have to go out to the firmware and call the
ibm,configure-connector sequence in order to figure out that the node
we're supposed to add is the ibm,platform-facilites node, right? We
can't save enough information at delete time to avoid the trip out to
firmware?

That is right... but maybe I don't understand your angle here. Unsure
what avoiding the configure-connector sequence for the nodes would buy
us.

quoted

Until that can be realized we have a confirmed use-after-free and the
possibility of memory corruption. So add a limited workaround that
discriminates on the node type, ignoring adds and removes. This should be
amenable to backporting in the meantime.

Yeah it's an unpleasant situation to find ourselves in. It's a bit icky
but as I think you said in a previous email, at least this isn't worse:
in the common case it should now succeed and and if properties change
significantly it will still fail.

My one question (from more of a security point of view) is:
 1) Say you start using the facilities with a particular set of
    parameters.

 2) Say you then get migrated and the parameters change.

 3) If you keep using the platform facilities as if the original
    properties are still valid, can this cause any Interesting,
    unexpected or otherwise Bad consequences? Are we going to end up
    (for example) scribbling over random memory somehow?

If drivers are safely handling errors from H_COP_OP etc, then no. (I
know, this looks like a Well That Would Be a Driver Bug dismissal, but
that's not my attitude.) And again this is a case where the change
cannot make things worse.

In the current design of the pseries LPM implementation, user space and
other normal system activity resume as soon as we return from the
stop_machine() call which suspends the partition, executing concurrently
with any device tree updates. So even if we had code in place to
correctly resolve the DT changes and the drivers were able to respond to
the changes, there would still be a window of exposure to the kind of
problem you describe: the changed characteristics, if any, of the
destination obtain as soon as execution resumes, regardless of when the
OS initiates the update-nodes sequence.

The way out of that mess is to use the Linux suspend framework, or
otherwise prevent user space from executing until the destination
system's characteristics have been appropriately propagated out to the
necessary drivers etc. I'm trying to get there.

Apart from that, the code seems to do what it says, it seems to solve a
real problem, the error and memory handling makes sense, you _put the DT
nodes that you _get, the comments are helpful and descriptive, and it
passes the automated tests on patchwork/snowpatch.

I appreciate your review!