| Message ID | 20211018163424.2491472-1-nathanl@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/pseries/mobility: ignore ibm, platform-facilities updates | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
On 10/18/21 9:34 AM, Nathan Lynch wrote: > 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 It always bothered me that the update from firmware here was an delete/add at the entire '/ibm,platform-facilities' node level instead of update properties calls. When I asked about it years ago the claim was it was just easier to pass an entire sub-tree as a single node add. > > 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. > > Also, significantly, the nodes added are almost completely equivalent to > the ones removed; even phandles are unchanged. ibm,shared-interrupt-pool in > the leaf nodes is the only property I've observed to differ, and Linux does > not use that. So in practice, the sum of update messages Linux receives for > this hierarchy is equivalent to minor property updates. The two props I would think maybe we would need to be most be concerned about ensuring don't change are "ibm,resource-id" which gets used in the vio bus code when configuring platform facilities nodes, and 'ibm,max-sync-cop' used by the pseries-nx driver. > > We succeed in removing the original hierarchy from the device tree. But the > drivers bound to the leaf nodes are ignorant of this, and do not relinquish > their references. The leaf nodes, still reachable through sysfs, of course > still refer to the now-freed ibm,platform-facilities parent node, which > makes use-after-free possible: It is actually more subtle then the drivers themselves being ignorant. They could register node update notifiers, but the real problem here is that the vio bus device itself takes a reference to each child node registered to the bus. I'm not sure we really want to unbind/rebind drivers as a result of LPM, but it would be generic to the vio bus instead of updating each driver to ensure its handling it device node references properly. > > refcount_t: addition on 0; use-after-free. > WARNING: CPU: 3 PID: 1706 at lib/refcount.c:25 refcount_warn_saturate+0x164/0x1f0 > refcount_warn_saturate+0x160/0x1f0 (unreliable) > kobject_get+0xf0/0x100 > of_node_get+0x30/0x50 > of_get_parent+0x50/0xb0 > of_fwnode_get_parent+0x54/0x90 > fwnode_count_parents+0x50/0x150 > fwnode_full_name_string+0x30/0x110 > device_node_string+0x49c/0x790 > vsnprintf+0x1c0/0x4c0 > sprintf+0x44/0x60 > devspec_show+0x34/0x50 > dev_attr_show+0x40/0xa0 > sysfs_kf_seq_show+0xbc/0x200 > kernfs_seq_show+0x44/0x60 > seq_read_iter+0x2a4/0x740 > kernfs_fop_read_iter+0x254/0x2e0 > new_sync_read+0x120/0x190 > vfs_read+0x1d0/0x240 > > Moreover, the "new" replacement subtree is not correctly added to the > device tree, resulting in ibm,platform-facilities parent node without the > appropriate leaf nodes, and broken symlinks in the sysfs device hierarchy: > > $ tree -d /sys/firmware/devicetree/base/ibm,platform-facilities/ > /sys/firmware/devicetree/base/ibm,platform-facilities/ > > 0 directories > > $ cd /sys/devices/vio ; find . -xtype l -exec file {} + > ./ibm,sym-encryption-v1/of_node: broken symbolic link to > ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,sym-encryption-v1 > ./ibm,random-v1/of_node: broken symbolic link to > ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,random-v1 > ./ibm,compression-v1/of_node: broken symbolic link to > ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,compression-v1 > > This is because add_dt_node() -> dlpar_attach_node() attaches only the > parent node returned from configure-connector, ignoring any children. This > should be corrected for the general case, but fixing that won't help with > the stale OF node references, which is the more urgent problem. I don't follow. If the code path you mention is truly broken in the way you say then DLPAR operations involving nodes with child nodes should also be broken. Last I had seen was that sysfs adds of the new nodes got renamed because the old nodes still existed as a result of there reference count not going to zero. Has this behavior changed, or am I misremembering the observed behavior? > > One way to address that would be to make the drivers respond to node > removal notifications, so that node references can be dropped > appropriately. But this would likely force the drivers to disrupt active > clients for no useful purpose: equivalent nodes are immediately re-added. > And recall that the acceleration capabilities described by the nodes remain > available throughout the whole process. See my comments above about its the vio bus more at fault here then the drivers themselves. I'm inclined to agree though that disrupting active operations with a driver unbind/rebind is a little extreme. This also brings me back to firmware removing and re-adding the whole '/ibm,platform-facilities' node instead of simply updating changed properties could avoid this whole fiasco. > > The solution I believe to be robust for this situation is to convert > remove+add of a node with an unchanged phandle to an update of the node's > properties in the Linux device tree structure. That would involve changing > and adding a fair amount of code, and may take several iterations to land. > > 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. The reality is that '/ibm,platform-facilities' and 'cache' nodes are the only LPM scoped device tree nodes that allow node delete/add. So, as a one-off workaround to deal with what I consider a bad firmware approach I think this is probably the best approach barring getting firmware to move to an update properties approach. An audit of the drivers is probably still a valid exercise to ensure any device tree props they care about they pick up a new value should it change. -Tyrel > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> > Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel") > Cc: stable@vger.kernel.org > --- > arch/powerpc/platforms/pseries/mobility.c | 34 +++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c > index e83e0891272d..210a37a065fb 100644 > --- a/arch/powerpc/platforms/pseries/mobility.c > +++ b/arch/powerpc/platforms/pseries/mobility.c > @@ -63,6 +63,27 @@ static int mobility_rtas_call(int token, char *buf, s32 scope) > > static int delete_dt_node(struct device_node *dn) > { > + struct device_node *pdn; > + bool is_platfac; > + > + pdn = of_get_parent(dn); > + is_platfac = of_node_is_type(dn, "ibm,platform-facilities") || > + of_node_is_type(pdn, "ibm,platform-facilities"); > + of_node_put(pdn); > + > + /* > + * The drivers that bind to nodes in the platform-facilities > + * hierarchy don't support node removal, and the removal directive > + * from firmware is always followed by an add of an equivalent > + * node. The capability (e.g. RNG, encryption, compression) > + * represented by the node is never interrupted by the migration. > + * So ignore changes to this part of the tree. > + */ > + if (is_platfac) { > + pr_notice("ignoring remove operation for %pOFfp\n", dn); > + return 0; > + } > + > pr_debug("removing node %pOFfp\n", dn); > dlpar_detach_node(dn); > return 0; > @@ -222,6 +243,19 @@ static int add_dt_node(struct device_node *parent_dn, __be32 drc_index) > if (!dn) > return -ENOENT; > > + /* > + * Since delete_dt_node() ignores this node type, this is the > + * necessary counterpart. We also know that a platform-facilities > + * node returned from dlpar_configure_connector() has children > + * attached, and dlpar_attach_node() only adds the parent, leaking > + * the children. So ignore these on the add side for now. > + */ > + if (of_node_is_type(dn, "ibm,platform-facilities")) { > + pr_notice("ignoring add operation for %pOF\n", dn); > + dlpar_free_cc_nodes(dn); > + return 0; > + } > + > rc = dlpar_attach_node(dn, parent_dn); > if (rc) > dlpar_free_cc_nodes(dn); >
On 10/18/21 3:37 PM, Tyrel Datwyler wrote: > On 10/18/21 9:34 AM, Nathan Lynch wrote: <<snip>> >> >> One way to address that would be to make the drivers respond to node >> removal notifications, so that node references can be dropped >> appropriately. But this would likely force the drivers to disrupt active >> clients for no useful purpose: equivalent nodes are immediately re-added. >> And recall that the acceleration capabilities described by the nodes remain >> available throughout the whole process. > > See my comments above about its the vio bus more at fault here then the drivers > themselves. I'm inclined to agree though that disrupting active operations with > a driver unbind/rebind is a little extreme. > > This also brings me back to firmware removing and re-adding the whole > '/ibm,platform-facilities' node instead of simply updating changed properties > could avoid this whole fiasco. > Thinking more on this and trying to recall my discussion so very long ago with firmware I now recall that I had complained that the idea of a node remove/add is akin to a DLPAR operation which we have no notion of for platform facilities. They know better than to do this with other virtual devices so I'm still not sure why they insist on doing it here. -Tyrel
Le 19/10/2021 à 00:37, Tyrel Datwyler a écrit : > On 10/18/21 9:34 AM, Nathan Lynch wrote: >> 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 > > It always bothered me that the update from firmware here was an delete/add at > the entire '/ibm,platform-facilities' node level instead of update properties > calls. When I asked about it years ago the claim was it was just easier to pass > an entire sub-tree as a single node add. > >> >> 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. >> >> Also, significantly, the nodes added are almost completely equivalent to >> the ones removed; even phandles are unchanged. ibm,shared-interrupt-pool in >> the leaf nodes is the only property I've observed to differ, and Linux does >> not use that. So in practice, the sum of update messages Linux receives for >> this hierarchy is equivalent to minor property updates. > > The two props I would think maybe we would need to be most be concerned about > ensuring don't change are "ibm,resource-id" which gets used in the vio bus code > when configuring platform facilities nodes, and 'ibm,max-sync-cop' used by the > pseries-nx driver. > >> >> We succeed in removing the original hierarchy from the device tree. But the >> drivers bound to the leaf nodes are ignorant of this, and do not relinquish >> their references. The leaf nodes, still reachable through sysfs, of course >> still refer to the now-freed ibm,platform-facilities parent node, which >> makes use-after-free possible: > > It is actually more subtle then the drivers themselves being ignorant. They > could register node update notifiers, but the real problem here is that the vio > bus device itself takes a reference to each child node registered to the bus. > I'm not sure we really want to unbind/rebind drivers as a result of LPM, but it > would be generic to the vio bus instead of updating each driver to ensure its > handling it device node references properly. > >> >> refcount_t: addition on 0; use-after-free. >> WARNING: CPU: 3 PID: 1706 at lib/refcount.c:25 refcount_warn_saturate+0x164/0x1f0 >> refcount_warn_saturate+0x160/0x1f0 (unreliable) >> kobject_get+0xf0/0x100 >> of_node_get+0x30/0x50 >> of_get_parent+0x50/0xb0 >> of_fwnode_get_parent+0x54/0x90 >> fwnode_count_parents+0x50/0x150 >> fwnode_full_name_string+0x30/0x110 >> device_node_string+0x49c/0x790 >> vsnprintf+0x1c0/0x4c0 >> sprintf+0x44/0x60 >> devspec_show+0x34/0x50 >> dev_attr_show+0x40/0xa0 >> sysfs_kf_seq_show+0xbc/0x200 >> kernfs_seq_show+0x44/0x60 >> seq_read_iter+0x2a4/0x740 >> kernfs_fop_read_iter+0x254/0x2e0 >> new_sync_read+0x120/0x190 >> vfs_read+0x1d0/0x240 >> >> Moreover, the "new" replacement subtree is not correctly added to the >> device tree, resulting in ibm,platform-facilities parent node without the >> appropriate leaf nodes, and broken symlinks in the sysfs device hierarchy: >> >> $ tree -d /sys/firmware/devicetree/base/ibm,platform-facilities/ >> /sys/firmware/devicetree/base/ibm,platform-facilities/ >> >> 0 directories >> >> $ cd /sys/devices/vio ; find . -xtype l -exec file {} + >> ./ibm,sym-encryption-v1/of_node: broken symbolic link to >> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,sym-encryption-v1 >> ./ibm,random-v1/of_node: broken symbolic link to >> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,random-v1 >> ./ibm,compression-v1/of_node: broken symbolic link to >> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,compression-v1 >> >> This is because add_dt_node() -> dlpar_attach_node() attaches only the >> parent node returned from configure-connector, ignoring any children. This >> should be corrected for the general case, but fixing that won't help with >> the stale OF node references, which is the more urgent problem. > > I don't follow. If the code path you mention is truly broken in the way you say > then DLPAR operations involving nodes with child nodes should also be broken. > Last I had seen was that sysfs adds of the new nodes got renamed because the old > nodes still existed as a result of there reference count not going to zero. Has > this behavior changed, or am I misremembering the observed behavior? > >> >> One way to address that would be to make the drivers respond to node >> removal notifications, so that node references can be dropped >> appropriately. But this would likely force the drivers to disrupt active >> clients for no useful purpose: equivalent nodes are immediately re-added. >> And recall that the acceleration capabilities described by the nodes remain >> available throughout the whole process. > > See my comments above about its the vio bus more at fault here then the drivers > themselves. I'm inclined to agree though that disrupting active operations with > a driver unbind/rebind is a little extreme. > > This also brings me back to firmware removing and re-adding the whole > '/ibm,platform-facilities' node instead of simply updating changed properties > could avoid this whole fiasco. > >> >> The solution I believe to be robust for this situation is to convert >> remove+add of a node with an unchanged phandle to an update of the node's >> properties in the Linux device tree structure. That would involve changing >> and adding a fair amount of code, and may take several iterations to land. >> >> 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. > > The reality is that '/ibm,platform-facilities' and 'cache' nodes are the only > LPM scoped device tree nodes that allow node delete/add. So, as a one-off > workaround to deal with what I consider a bad firmware approach I think this is > probably the best approach barring getting firmware to move to an update > properties approach. I do agree, this is probably the best option until the firmware is moving to an update notification. > > An audit of the drivers is probably still a valid exercise to ensure any device > tree props they care about they pick up a new value should it change. > > -Tyrel > >> >> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> >> Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel") >> Cc: stable@vger.kernel.org >> --- >> arch/powerpc/platforms/pseries/mobility.c | 34 +++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c >> index e83e0891272d..210a37a065fb 100644 >> --- a/arch/powerpc/platforms/pseries/mobility.c >> +++ b/arch/powerpc/platforms/pseries/mobility.c >> @@ -63,6 +63,27 @@ static int mobility_rtas_call(int token, char *buf, s32 scope) >> >> static int delete_dt_node(struct device_node *dn) >> { >> + struct device_node *pdn; >> + bool is_platfac; >> + >> + pdn = of_get_parent(dn); >> + is_platfac = of_node_is_type(dn, "ibm,platform-facilities") || >> + of_node_is_type(pdn, "ibm,platform-facilities"); >> + of_node_put(pdn); >> + >> + /* >> + * The drivers that bind to nodes in the platform-facilities >> + * hierarchy don't support node removal, and the removal directive >> + * from firmware is always followed by an add of an equivalent >> + * node. The capability (e.g. RNG, encryption, compression) >> + * represented by the node is never interrupted by the migration. >> + * So ignore changes to this part of the tree. >> + */ >> + if (is_platfac) { >> + pr_notice("ignoring remove operation for %pOFfp\n", dn); >> + return 0; >> + } >> + >> pr_debug("removing node %pOFfp\n", dn); >> dlpar_detach_node(dn); >> return 0; >> @@ -222,6 +243,19 @@ static int add_dt_node(struct device_node *parent_dn, __be32 drc_index) >> if (!dn) >> return -ENOENT; >> >> + /* >> + * Since delete_dt_node() ignores this node type, this is the >> + * necessary counterpart. We also know that a platform-facilities >> + * node returned from dlpar_configure_connector() has children >> + * attached, and dlpar_attach_node() only adds the parent, leaking >> + * the children. So ignore these on the add side for now. >> + */ >> + if (of_node_is_type(dn, "ibm,platform-facilities")) { >> + pr_notice("ignoring add operation for %pOF\n", dn); >> + dlpar_free_cc_nodes(dn); >> + return 0; >> + } >> + >> rc = dlpar_attach_node(dn, parent_dn); >> if (rc) >> dlpar_free_cc_nodes(dn); >> >
Hi Tyrel, thanks for the detailed review. Tyrel Datwyler <tyreld@linux.ibm.com> writes: > On 10/18/21 9:34 AM, Nathan Lynch wrote: >> 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 > > It always bothered me that the update from firmware here was an delete/add at > the entire '/ibm,platform-facilities' node level instead of update properties > calls. When I asked about it years ago the claim was it was just easier to pass > an entire sub-tree as a single node add. Yes, I believe this is for ease of implementation on their part. >> 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. >> >> Also, significantly, the nodes added are almost completely equivalent to >> the ones removed; even phandles are unchanged. ibm,shared-interrupt-pool in >> the leaf nodes is the only property I've observed to differ, and Linux does >> not use that. So in practice, the sum of update messages Linux receives for >> this hierarchy is equivalent to minor property updates. > > The two props I would think maybe we would need to be most be concerned about > ensuring don't change are "ibm,resource-id" which gets used in the vio bus code > when configuring platform facilities nodes, and 'ibm,max-sync-cop' used by the > pseries-nx driver. The proposed change does not introduce any regressions with respect to those properties. At least, that's the intention. ibm,resource-id: the vio code lacks any way of reacting to this changing at runtime. (I could be wrong, but I suspect it does not change in practice even though it is technically allowed by the spec.) ibm,max-sync-cop: the nx code has a notifier callback to handle changes to this and a couple other properties, but only if they are propagated through an "update property" event. Node remove+add doesn't trigger the callback. I think this one is more likely than resource-id to change between different models or configurations, but I haven't observed this to happen in my test environment. Without this change, the child nodes of the ibm,platform-facilities container in Linux's device tree are completely discarded (see more below) after a partition migration. This makes driver re-initialization impossible without a reboot. With the change, the nodes are retained, albeit with properties that may not correctly reflect the destination system. To be clear, I think this is bad, and I am working on a better solution. But I consider the change here a net improvement for the common case (no meaningful property changes), and it's a wash for migrations where the properties could change (we weren't handling that anyway). >> We succeed in removing the original hierarchy from the device tree. But the >> drivers bound to the leaf nodes are ignorant of this, and do not relinquish >> their references. The leaf nodes, still reachable through sysfs, of course >> still refer to the now-freed ibm,platform-facilities parent node, which >> makes use-after-free possible: > > It is actually more subtle then the drivers themselves being ignorant. They > could register node update notifiers, but the real problem here is that the vio > bus device itself takes a reference to each child node registered to the bus. > I'm not sure we really want to unbind/rebind drivers as a result of LPM, but it > would be generic to the vio bus instead of updating each driver to ensure its > handling it device node references properly. You're right. I will modify the commit message. >> refcount_t: addition on 0; use-after-free. >> WARNING: CPU: 3 PID: 1706 at lib/refcount.c:25 refcount_warn_saturate+0x164/0x1f0 >> refcount_warn_saturate+0x160/0x1f0 (unreliable) >> kobject_get+0xf0/0x100 >> of_node_get+0x30/0x50 >> of_get_parent+0x50/0xb0 >> of_fwnode_get_parent+0x54/0x90 >> fwnode_count_parents+0x50/0x150 >> fwnode_full_name_string+0x30/0x110 >> device_node_string+0x49c/0x790 >> vsnprintf+0x1c0/0x4c0 >> sprintf+0x44/0x60 >> devspec_show+0x34/0x50 >> dev_attr_show+0x40/0xa0 >> sysfs_kf_seq_show+0xbc/0x200 >> kernfs_seq_show+0x44/0x60 >> seq_read_iter+0x2a4/0x740 >> kernfs_fop_read_iter+0x254/0x2e0 >> new_sync_read+0x120/0x190 >> vfs_read+0x1d0/0x240 >> >> Moreover, the "new" replacement subtree is not correctly added to the >> device tree, resulting in ibm,platform-facilities parent node without the >> appropriate leaf nodes, and broken symlinks in the sysfs device hierarchy: >> >> $ tree -d /sys/firmware/devicetree/base/ibm,platform-facilities/ >> /sys/firmware/devicetree/base/ibm,platform-facilities/ >> >> 0 directories >> >> $ cd /sys/devices/vio ; find . -xtype l -exec file {} + >> ./ibm,sym-encryption-v1/of_node: broken symbolic link to >> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,sym-encryption-v1 >> ./ibm,random-v1/of_node: broken symbolic link to >> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,random-v1 >> ./ibm,compression-v1/of_node: broken symbolic link to >> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,compression-v1 >> >> This is because add_dt_node() -> dlpar_attach_node() attaches only the >> parent node returned from configure-connector, ignoring any children. This >> should be corrected for the general case, but fixing that won't help with >> the stale OF node references, which is the more urgent problem. > > I don't follow. If the code path you mention is truly broken in the way you say > then DLPAR operations involving nodes with child nodes should also be > broken. Hmm I don't know of any kernel-based DLPAR workflow that attaches sub-trees i.e. parent + children. Processor addition attaches CPU nodes and possibly cache nodes, but they have sibling relationships. If you're thinking of I/O adapters, the DT updates are (still!) driven from user space. As undesirable as that is, that use case actually works correctly AFAIK. What happens for the platfac LPM scenario is that dlpar_configure_connector() returns the node pointer for the root ibm,platform-facilities node, with children attached. That node pointer is passed from add_dt_node() -> dlpar_attach_node() -> of_attach_node() -> __of_attach_node(), where its child and sibling fields are overwritten in the process of attaching it to the tree. > Last I had seen was that sysfs adds of the new nodes got renamed because the old > nodes still existed as a result of there reference count not going to zero. Has > this behavior changed, or am I misremembering the observed behavior? Maybe you're thinking of the CPU cache node issue recently fixed by 7edd5c9a8820 ("powerpc/pseries/cpuhp: cache node corrections")? >> One way to address that would be to make the drivers respond to node >> removal notifications, so that node references can be dropped >> appropriately. But this would likely force the drivers to disrupt active >> clients for no useful purpose: equivalent nodes are immediately re-added. >> And recall that the acceleration capabilities described by the nodes remain >> available throughout the whole process. > > See my comments above about its the vio bus more at fault here then the drivers > themselves. I'm inclined to agree though that disrupting active operations with > a driver unbind/rebind is a little extreme. > > This also brings me back to firmware removing and re-adding the whole > '/ibm,platform-facilities' node instead of simply updating changed properties > could avoid this whole fiasco. It's a little unfortunate for us, but it's longstanding behavior that is presumably tolerated fine by other OSes, and it's allowed by the spec. So we just need to deal with it. >> The solution I believe to be robust for this situation is to convert >> remove+add of a node with an unchanged phandle to an update of the node's >> properties in the Linux device tree structure. That would involve changing >> and adding a fair amount of code, and may take several iterations to land. >> >> 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. > > The reality is that '/ibm,platform-facilities' and 'cache' nodes are the only > LPM scoped device tree nodes that allow node delete/add. So, as a one-off > workaround to deal with what I consider a bad firmware approach I think this is > probably the best approach barring getting firmware to move to an update > properties approach. Thanks. I think there's no reason to expect firmware's behavior to change, and there wouldn't be much payoff for us at this point -- we would still have to support the current behavior.
Laurent Dufour <ldufour@linux.ibm.com> writes: > Le 19/10/2021 à 00:37, Tyrel Datwyler a écrit : >> On 10/18/21 9:34 AM, Nathan Lynch wrote: >> The reality is that '/ibm,platform-facilities' and 'cache' nodes are the only >> LPM scoped device tree nodes that allow node delete/add. So, as a one-off >> workaround to deal with what I consider a bad firmware approach I think this is >> probably the best approach barring getting firmware to move to an update >> properties approach. > > I do agree, this is probably the best option until the firmware is moving to an > update notification. Just to be clear, my proposal is to carry this hack until *Linux* can be changed to better support the current firmware behavior. There's little point in trying to get firmware to change now.
On 10/19/21 2:36 PM, Nathan Lynch wrote: > Hi Tyrel, thanks for the detailed review. > > Tyrel Datwyler <tyreld@linux.ibm.com> writes: >> On 10/18/21 9:34 AM, Nathan Lynch wrote: >>> 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 >> >> It always bothered me that the update from firmware here was an delete/add at >> the entire '/ibm,platform-facilities' node level instead of update properties >> calls. When I asked about it years ago the claim was it was just easier to pass >> an entire sub-tree as a single node add. > > Yes, I believe this is for ease of implementation on their part. I'd still argue that a full node removal or addition is a platform reconfiguration on par with a hotplug operation. > >>> 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. >>> >>> Also, significantly, the nodes added are almost completely equivalent to >>> the ones removed; even phandles are unchanged. ibm,shared-interrupt-pool in >>> the leaf nodes is the only property I've observed to differ, and Linux does >>> not use that. So in practice, the sum of update messages Linux receives for >>> this hierarchy is equivalent to minor property updates. >> >> The two props I would think maybe we would need to be most be concerned about >> ensuring don't change are "ibm,resource-id" which gets used in the vio bus code >> when configuring platform facilities nodes, and 'ibm,max-sync-cop' used by the >> pseries-nx driver. > > The proposed change does not introduce any regressions with respect to > those properties. At least, that's the intention. > > ibm,resource-id: the vio code lacks any way of reacting to this changing > at runtime. (I could be wrong, but I suspect it does not change in > practice even though it is technically allowed by the spec.) > > ibm,max-sync-cop: the nx code has a notifier callback to handle changes > to this and a couple other properties, but only if they are propagated > through an "update property" event. Node remove+add doesn't trigger the > callback. I think this one is more likely than resource-id to change > between different models or configurations, but I haven't observed this > to happen in my test environment. > > Without this change, the child nodes of the ibm,platform-facilities > container in Linux's device tree are completely discarded (see more > below) after a partition migration. This makes driver re-initialization > impossible without a reboot. With the change, the nodes are retained, > albeit with properties that may not correctly reflect the destination > system. To be clear, I think this is bad, and I am working on a better > solution. But I consider the change here a net improvement for the > common case (no meaningful property changes), and it's a wash for > migrations where the properties could change (we weren't handling that > anyway). This didn't use to be the case. After some digging I see the problem, and it is clearly a long standing regression (see comment further down). Part of that is my fault for putting the platform-facilities errors on the back burner when I was working this area. > >>> We succeed in removing the original hierarchy from the device tree. But the >>> drivers bound to the leaf nodes are ignorant of this, and do not relinquish >>> their references. The leaf nodes, still reachable through sysfs, of course >>> still refer to the now-freed ibm,platform-facilities parent node, which >>> makes use-after-free possible: >> >> It is actually more subtle then the drivers themselves being ignorant. They >> could register node update notifiers, but the real problem here is that the vio >> bus device itself takes a reference to each child node registered to the bus. >> I'm not sure we really want to unbind/rebind drivers as a result of LPM, but it >> would be generic to the vio bus instead of updating each driver to ensure its >> handling it device node references properly. > > You're right. I will modify the commit message. > >>> refcount_t: addition on 0; use-after-free. >>> WARNING: CPU: 3 PID: 1706 at lib/refcount.c:25 refcount_warn_saturate+0x164/0x1f0 >>> refcount_warn_saturate+0x160/0x1f0 (unreliable) >>> kobject_get+0xf0/0x100 >>> of_node_get+0x30/0x50 >>> of_get_parent+0x50/0xb0 >>> of_fwnode_get_parent+0x54/0x90 >>> fwnode_count_parents+0x50/0x150 >>> fwnode_full_name_string+0x30/0x110 >>> device_node_string+0x49c/0x790 >>> vsnprintf+0x1c0/0x4c0 >>> sprintf+0x44/0x60 >>> devspec_show+0x34/0x50 >>> dev_attr_show+0x40/0xa0 >>> sysfs_kf_seq_show+0xbc/0x200 >>> kernfs_seq_show+0x44/0x60 >>> seq_read_iter+0x2a4/0x740 >>> kernfs_fop_read_iter+0x254/0x2e0 >>> new_sync_read+0x120/0x190 >>> vfs_read+0x1d0/0x240 >>> >>> Moreover, the "new" replacement subtree is not correctly added to the >>> device tree, resulting in ibm,platform-facilities parent node without the >>> appropriate leaf nodes, and broken symlinks in the sysfs device hierarchy: >>> >>> $ tree -d /sys/firmware/devicetree/base/ibm,platform-facilities/ >>> /sys/firmware/devicetree/base/ibm,platform-facilities/ >>> >>> 0 directories >>> >>> $ cd /sys/devices/vio ; find . -xtype l -exec file {} + >>> ./ibm,sym-encryption-v1/of_node: broken symbolic link to >>> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,sym-encryption-v1 >>> ./ibm,random-v1/of_node: broken symbolic link to >>> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,random-v1 >>> ./ibm,compression-v1/of_node: broken symbolic link to >>> ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,compression-v1 >>> >>> This is because add_dt_node() -> dlpar_attach_node() attaches only the >>> parent node returned from configure-connector, ignoring any children. This >>> should be corrected for the general case, but fixing that won't help with >>> the stale OF node references, which is the more urgent problem. >> >> I don't follow. If the code path you mention is truly broken in the way you say >> then DLPAR operations involving nodes with child nodes should also be >> broken. > > Hmm I don't know of any kernel-based DLPAR workflow that attaches > sub-trees i.e. parent + children. Processor addition attaches CPU nodes > and possibly cache nodes, but they have sibling relationships. If you're > thinking of I/O adapters, the DT updates are (still!) driven from user > space. As undesirable as that is, that use case actually works correctly > AFAIK. > > What happens for the platfac LPM scenario is that > dlpar_configure_connector() returns the node pointer for the root > ibm,platform-facilities node, with children attached. That node pointer > is passed from add_dt_node() -> dlpar_attach_node() -> of_attach_node() > -> __of_attach_node(), where its child and sibling fields are > overwritten in the process of attaching it to the tree. Well it worked back in 2013 when I got all the in kernel device tree update stuff working. Looks like I missed this one which now explains one area where platform-facilities update originally went to shit. commit 6162dbe49a451f96431a23b4821f05e3bd925bc1 Author: Grant Likely <grant.likely@linaro.org> Date: Wed Jul 16 08:48:46 2014 -0600 of: Make sure attached nodes don't carry along extra children The child pointer does not get cleared when attaching new nodes which could cause the tree to be inconsistent. Clear the child pointer in __of_attach_node() to be absolutely sure that the structure remains in a consistent layout. Signed-off-by: Grant Likely <grant.likely@linaro.org> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c index c875787fa394..b96d83100987 100644 --- a/drivers/of/dynamic.c +++ b/drivers/of/dynamic.c @@ -98,6 +98,7 @@ int of_property_notify(int action, struct device_node *np, void __of_attach_node(struct device_node *np) { + np->child = NULL; np->sibling = np->parent->child; np->allnext = np->parent->allnext; np->parent->allnext = np; Not sure what the reasoning was here since this prevents attaching a node that is a sub tree. Either need to revert that, rewrite dlpar_attach_node to iterate over the sub-tree, or probably best rewrite dlpar_configure_connector to use a of_changeset instead. > >> Last I had seen was that sysfs adds of the new nodes got renamed because the old >> nodes still existed as a result of there reference count not going to zero. Has >> this behavior changed, or am I misremembering the observed behavior? > > Maybe you're thinking of the CPU cache node issue recently fixed by > 7edd5c9a8820 ("powerpc/pseries/cpuhp: cache node corrections")? > >>> One way to address that would be to make the drivers respond to node >>> removal notifications, so that node references can be dropped >>> appropriately. But this would likely force the drivers to disrupt active >>> clients for no useful purpose: equivalent nodes are immediately re-added. >>> And recall that the acceleration capabilities described by the nodes remain >>> available throughout the whole process. >> >> See my comments above about its the vio bus more at fault here then the drivers >> themselves. I'm inclined to agree though that disrupting active operations with >> a driver unbind/rebind is a little extreme. >> >> This also brings me back to firmware removing and re-adding the whole >> '/ibm,platform-facilities' node instead of simply updating changed properties >> could avoid this whole fiasco. > > It's a little unfortunate for us, but it's longstanding behavior that is > presumably tolerated fine by other OSes, and it's allowed by the spec. > So we just need to deal with it. > >>> The solution I believe to be robust for this situation is to convert >>> remove+add of a node with an unchanged phandle to an update of the node's >>> properties in the Linux device tree structure. That would involve changing >>> and adding a fair amount of code, and may take several iterations to land. >>> >>> 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. >> >> The reality is that '/ibm,platform-facilities' and 'cache' nodes are the only >> LPM scoped device tree nodes that allow node delete/add. So, as a one-off >> workaround to deal with what I consider a bad firmware approach I think this is >> probably the best approach barring getting firmware to move to an update >> properties approach. > > Thanks. I think there's no reason to expect firmware's behavior to > change, and there wouldn't be much payoff for us at this point -- we > would still have to support the current behavior. >
Tyrel Datwyler <tyreld@linux.ibm.com> writes: > On 10/19/21 2:36 PM, Nathan Lynch wrote: >> Tyrel Datwyler <tyreld@linux.ibm.com> writes: >>> On 10/18/21 9:34 AM, Nathan Lynch wrote: >>>> 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 >>> >>> It always bothered me that the update from firmware here was an delete/add at >>> the entire '/ibm,platform-facilities' node level instead of update properties >>> calls. When I asked about it years ago the claim was it was just easier to pass >>> an entire sub-tree as a single node add. >> >> Yes, I believe this is for ease of implementation on their part. > > I'd still argue that a full node removal or addition is a platform > reconfiguration on par with a hotplug operation. Well... I think we might be better served by a slightly more flexible view of things :-) These are just a series of messages from firmware that should be considered as a whole, and they don't dictate exactly what the OS must do to its own data structures. Nothing mandates that the OS immediately and literally apply the changes as they are individually reported by the update-nodes sequence. Anyway, whether the firmware behavior is reasonable is sort of beside the point since we're stuck with it. >>>> This is because add_dt_node() -> dlpar_attach_node() attaches only the >>>> parent node returned from configure-connector, ignoring any children. This >>>> should be corrected for the general case, but fixing that won't help with >>>> the stale OF node references, which is the more urgent problem. >>> >>> I don't follow. If the code path you mention is truly broken in the way you say >>> then DLPAR operations involving nodes with child nodes should also be >>> broken. >> >> Hmm I don't know of any kernel-based DLPAR workflow that attaches >> sub-trees i.e. parent + children. Processor addition attaches CPU nodes >> and possibly cache nodes, but they have sibling relationships. If you're >> thinking of I/O adapters, the DT updates are (still!) driven from user >> space. As undesirable as that is, that use case actually works correctly >> AFAIK. >> >> What happens for the platfac LPM scenario is that >> dlpar_configure_connector() returns the node pointer for the root >> ibm,platform-facilities node, with children attached. That node pointer >> is passed from add_dt_node() -> dlpar_attach_node() -> of_attach_node() >> -> __of_attach_node(), where its child and sibling fields are >> overwritten in the process of attaching it to the tree. > > Well it worked back in 2013 when I got all the in kernel device tree update > stuff working. Looks like I missed this one which now explains one area where > platform-facilities update originally went to shit. > > commit 6162dbe49a451f96431a23b4821f05e3bd925bc1 > Author: Grant Likely <grant.likely@linaro.org> > Date: Wed Jul 16 08:48:46 2014 -0600 > > of: Make sure attached nodes don't carry along extra children > > The child pointer does not get cleared when attaching new nodes which > could cause the tree to be inconsistent. Clear the child pointer in > __of_attach_node() to be absolutely sure that the structure remains in a > consistent layout. > > Signed-off-by: Grant Likely <grant.likely@linaro.org> > > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c > index c875787fa394..b96d83100987 100644 > --- a/drivers/of/dynamic.c > +++ b/drivers/of/dynamic.c > @@ -98,6 +98,7 @@ int of_property_notify(int action, struct device_node *np, > > void __of_attach_node(struct device_node *np) > { > + np->child = NULL; > np->sibling = np->parent->child; > np->allnext = np->parent->allnext; > np->parent->allnext = np; > > Not sure what the reasoning was here since this prevents attaching a node that > is a sub tree. Either need to revert that, rewrite dlpar_attach_node to iterate > over the sub-tree, or probably best rewrite dlpar_configure_connector to use a > of_changeset instead. Good find. I'd guess that adding a subtree used to sort of work, except that children of np were not added to the allnext list, which would effectively hide them from some lookups. Regardless, yes, the pseries code needs to change to attach and detach nodes individually. I don't think the OF core supports more complex changes.
On 10/20/21 8:54 AM, Nathan Lynch wrote: > Tyrel Datwyler <tyreld@linux.ibm.com> writes: >> On 10/19/21 2:36 PM, Nathan Lynch wrote: >>> Tyrel Datwyler <tyreld@linux.ibm.com> writes: >>>> On 10/18/21 9:34 AM, Nathan Lynch wrote: >>>>> 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 >>>> >>>> It always bothered me that the update from firmware here was an delete/add at >>>> the entire '/ibm,platform-facilities' node level instead of update properties >>>> calls. When I asked about it years ago the claim was it was just easier to pass >>>> an entire sub-tree as a single node add. >>> >>> Yes, I believe this is for ease of implementation on their part. >> >> I'd still argue that a full node removal or addition is a platform >> reconfiguration on par with a hotplug operation. > > Well... I think we might be better served by a slightly more flexible > view of things :-) These are just a series of messages from firmware > that should be considered as a whole, and they don't dictate exactly > what the OS must do to its own data structures. Nothing mandates that > the OS immediately and literally apply the changes as they are > individually reported by the update-nodes sequence. Agree to disagree. Not saying we can do much about it here, but I think being flexible leads to scope creep down the road that may eventually complicate things worse. A node removal doesn't guarantee an immediate node addition. So we are just papering over the issue with a plan to paper over it some more in a more complicated fashion. > > Anyway, whether the firmware behavior is reasonable is sort of beside > the point since we're stuck with it. I'm aware. Moaning for the sake of moaning about it. > > >>>>> This is because add_dt_node() -> dlpar_attach_node() attaches only the >>>>> parent node returned from configure-connector, ignoring any children. This >>>>> should be corrected for the general case, but fixing that won't help with >>>>> the stale OF node references, which is the more urgent problem. >>>> >>>> I don't follow. If the code path you mention is truly broken in the way you say >>>> then DLPAR operations involving nodes with child nodes should also be >>>> broken. >>> >>> Hmm I don't know of any kernel-based DLPAR workflow that attaches >>> sub-trees i.e. parent + children. Processor addition attaches CPU nodes >>> and possibly cache nodes, but they have sibling relationships. If you're >>> thinking of I/O adapters, the DT updates are (still!) driven from user >>> space. As undesirable as that is, that use case actually works correctly >>> AFAIK. >>> >>> What happens for the platfac LPM scenario is that >>> dlpar_configure_connector() returns the node pointer for the root >>> ibm,platform-facilities node, with children attached. That node pointer >>> is passed from add_dt_node() -> dlpar_attach_node() -> of_attach_node() >>> -> __of_attach_node(), where its child and sibling fields are >>> overwritten in the process of attaching it to the tree. >> >> Well it worked back in 2013 when I got all the in kernel device tree update >> stuff working. Looks like I missed this one which now explains one area where >> platform-facilities update originally went to shit. >> >> commit 6162dbe49a451f96431a23b4821f05e3bd925bc1 >> Author: Grant Likely <grant.likely@linaro.org> >> Date: Wed Jul 16 08:48:46 2014 -0600 >> >> of: Make sure attached nodes don't carry along extra children >> >> The child pointer does not get cleared when attaching new nodes which >> could cause the tree to be inconsistent. Clear the child pointer in >> __of_attach_node() to be absolutely sure that the structure remains in a >> consistent layout. >> >> Signed-off-by: Grant Likely <grant.likely@linaro.org> >> >> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c >> index c875787fa394..b96d83100987 100644 >> --- a/drivers/of/dynamic.c >> +++ b/drivers/of/dynamic.c >> @@ -98,6 +98,7 @@ int of_property_notify(int action, struct device_node *np, >> >> void __of_attach_node(struct device_node *np) >> { >> + np->child = NULL; >> np->sibling = np->parent->child; >> np->allnext = np->parent->allnext; >> np->parent->allnext = np; >> >> Not sure what the reasoning was here since this prevents attaching a node that >> is a sub tree. Either need to revert that, rewrite dlpar_attach_node to iterate >> over the sub-tree, or probably best rewrite dlpar_configure_connector to use a >> of_changeset instead. > > Good find. I'd guess that adding a subtree used to sort of work, except > that children of np were not added to the allnext list, which would > effectively hide them from some lookups. Pretty sure allnodes/allnext list is a relic of the past. It guaranteed depth first traversal after boot, but dynamic nodes broke that guarantee. It was removed because you can make the same traversal via the parent<->child lists. > > Regardless, yes, the pseries code needs to change to attach and detach > nodes individually. I don't think the OF core supports more complex > changes. > Indeed. I clearly capitalized on the behavior at the time while missing the intended usage. -Tyrel
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c index e83e0891272d..210a37a065fb 100644 --- a/arch/powerpc/platforms/pseries/mobility.c +++ b/arch/powerpc/platforms/pseries/mobility.c @@ -63,6 +63,27 @@ static int mobility_rtas_call(int token, char *buf, s32 scope) static int delete_dt_node(struct device_node *dn) { + struct device_node *pdn; + bool is_platfac; + + pdn = of_get_parent(dn); + is_platfac = of_node_is_type(dn, "ibm,platform-facilities") || + of_node_is_type(pdn, "ibm,platform-facilities"); + of_node_put(pdn); + + /* + * The drivers that bind to nodes in the platform-facilities + * hierarchy don't support node removal, and the removal directive + * from firmware is always followed by an add of an equivalent + * node. The capability (e.g. RNG, encryption, compression) + * represented by the node is never interrupted by the migration. + * So ignore changes to this part of the tree. + */ + if (is_platfac) { + pr_notice("ignoring remove operation for %pOFfp\n", dn); + return 0; + } + pr_debug("removing node %pOFfp\n", dn); dlpar_detach_node(dn); return 0; @@ -222,6 +243,19 @@ static int add_dt_node(struct device_node *parent_dn, __be32 drc_index) if (!dn) return -ENOENT; + /* + * Since delete_dt_node() ignores this node type, this is the + * necessary counterpart. We also know that a platform-facilities + * node returned from dlpar_configure_connector() has children + * attached, and dlpar_attach_node() only adds the parent, leaking + * the children. So ignore these on the add side for now. + */ + if (of_node_is_type(dn, "ibm,platform-facilities")) { + pr_notice("ignoring add operation for %pOF\n", dn); + dlpar_free_cc_nodes(dn); + return 0; + } + rc = dlpar_attach_node(dn, parent_dn); if (rc) dlpar_free_cc_nodes(dn);
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. Also, significantly, the nodes added are almost completely equivalent to the ones removed; even phandles are unchanged. ibm,shared-interrupt-pool in the leaf nodes is the only property I've observed to differ, and Linux does not use that. So in practice, the sum of update messages Linux receives for this hierarchy is equivalent to minor property updates. We succeed in removing the original hierarchy from the device tree. But the drivers bound to the leaf nodes are ignorant of this, and do not relinquish their references. The leaf nodes, still reachable through sysfs, of course still refer to the now-freed ibm,platform-facilities parent node, which makes use-after-free possible: refcount_t: addition on 0; use-after-free. WARNING: CPU: 3 PID: 1706 at lib/refcount.c:25 refcount_warn_saturate+0x164/0x1f0 refcount_warn_saturate+0x160/0x1f0 (unreliable) kobject_get+0xf0/0x100 of_node_get+0x30/0x50 of_get_parent+0x50/0xb0 of_fwnode_get_parent+0x54/0x90 fwnode_count_parents+0x50/0x150 fwnode_full_name_string+0x30/0x110 device_node_string+0x49c/0x790 vsnprintf+0x1c0/0x4c0 sprintf+0x44/0x60 devspec_show+0x34/0x50 dev_attr_show+0x40/0xa0 sysfs_kf_seq_show+0xbc/0x200 kernfs_seq_show+0x44/0x60 seq_read_iter+0x2a4/0x740 kernfs_fop_read_iter+0x254/0x2e0 new_sync_read+0x120/0x190 vfs_read+0x1d0/0x240 Moreover, the "new" replacement subtree is not correctly added to the device tree, resulting in ibm,platform-facilities parent node without the appropriate leaf nodes, and broken symlinks in the sysfs device hierarchy: $ tree -d /sys/firmware/devicetree/base/ibm,platform-facilities/ /sys/firmware/devicetree/base/ibm,platform-facilities/ 0 directories $ cd /sys/devices/vio ; find . -xtype l -exec file {} + ./ibm,sym-encryption-v1/of_node: broken symbolic link to ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,sym-encryption-v1 ./ibm,random-v1/of_node: broken symbolic link to ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,random-v1 ./ibm,compression-v1/of_node: broken symbolic link to ../../../firmware/devicetree/base/ibm,platform-facilities/ibm,compression-v1 This is because add_dt_node() -> dlpar_attach_node() attaches only the parent node returned from configure-connector, ignoring any children. This should be corrected for the general case, but fixing that won't help with the stale OF node references, which is the more urgent problem. One way to address that would be to make the drivers respond to node removal notifications, so that node references can be dropped appropriately. But this would likely force the drivers to disrupt active clients for no useful purpose: equivalent nodes are immediately re-added. And recall that the acceleration capabilities described by the nodes remain available throughout the whole process. The solution I believe to be robust for this situation is to convert remove+add of a node with an unchanged phandle to an update of the node's properties in the Linux device tree structure. That would involve changing and adding a fair amount of code, and may take several iterations to land. 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. Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel") Cc: stable@vger.kernel.org --- arch/powerpc/platforms/pseries/mobility.c | 34 +++++++++++++++++++++++ 1 file changed, 34 insertions(+)