| Message ID | 20240415223612.738535-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/hwmon: Get rid of devm | expand |
Am 16.04.24 um 00:36 schrieb Ashutosh Dixit: > When both hwmon and hwmon drvdata (on which hwmon depends) are device > managed resources, the expectation, on device unbind, is that hwmon will be > released before drvdata. However, in i915 there are two separate code > paths, which both release either drvdata or hwmon and either can be > released before the other. These code paths (for device unbind) are as > follows (see also the bug referenced below): > > Call Trace: > release_nodes+0x11/0x70 > devres_release_group+0xb2/0x110 > component_unbind_all+0x8d/0xa0 > component_del+0xa5/0x140 > intel_pxp_tee_component_fini+0x29/0x40 [i915] > intel_pxp_fini+0x33/0x80 [i915] > i915_driver_remove+0x4c/0x120 [i915] > i915_pci_remove+0x19/0x30 [i915] > pci_device_remove+0x32/0xa0 > device_release_driver_internal+0x19c/0x200 > unbind_store+0x9c/0xb0 > > and > > Call Trace: > release_nodes+0x11/0x70 > devres_release_all+0x8a/0xc0 > device_unbind_cleanup+0x9/0x70 > device_release_driver_internal+0x1c1/0x200 > unbind_store+0x9c/0xb0 > > This means that in i915, if use devm, we cannot gurantee that hwmon will > always be released before drvdata. Which means that we have a uaf if hwmon > sysfs is accessed when drvdata has been released but hwmon hasn't. > > The only way out of this seems to be do get rid of devm_ and release/free > everything explicitly during device unbind. > > v2: Change commit message and other minor code changes > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 8c3f443c8347..46c24b1ee6df 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > if (!IS_DGFX(i915)) > return; > > - hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL); > + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL); > if (!hwmon) > return; > > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > hwm_get_preregistration_info(i915); > > /* hwmon_dev points to device hwmon<i> */ > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name, > - ddat, > - &hwm_chip_info, > - hwm_groups); > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, > + ddat, > + &hwm_chip_info, > + hwm_groups); > if (IS_ERR(hwmon_dev)) { > i915->hwmon = NULL; Hi, you need to free hwmon here, since it is not managed by devres anymore. > return; > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0)) > continue; > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name, > - ddat_gt, > - &hwm_gt_chip_info, > - NULL); > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name, > + ddat_gt, > + &hwm_gt_chip_info, > + NULL); > if (!IS_ERR(hwmon_dev)) > ddat_gt->hwmon_dev = hwmon_dev; > } > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > void i915_hwmon_unregister(struct drm_i915_private *i915) > { > - fetch_and_zero(&i915->hwmon); > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); Why is fetch_and_zero() necessary here? Thanks, Armin Wolf > + struct hwm_drvdata *ddat = &hwmon->ddat; > + struct intel_gt *gt; > + int i; > + > + if (!hwmon) > + return; > + > + for_each_gt(gt, i915, i) { > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; > + > + if (ddat_gt->hwmon_dev) { > + hwmon_device_unregister(ddat_gt->hwmon_dev); > + ddat_gt->hwmon_dev = NULL; > + } > + } > + > + if (ddat->hwmon_dev) > + hwmon_device_unregister(ddat->hwmon_dev); > + > + mutex_destroy(&hwmon->hwmon_lock); > + kfree(hwmon); > }
On Mon, 15 Apr 2024 16:35:02 -0700, Armin Wolf wrote: > Hi Armin, > Am 16.04.24 um 00:36 schrieb Ashutosh Dixit: > > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > hwm_get_preregistration_info(i915); > > > > /* hwmon_dev points to device hwmon<i> */ > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name, > > - ddat, > > - &hwm_chip_info, > > - hwm_groups); > > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, > > + ddat, > > + &hwm_chip_info, > > + hwm_groups); > > if (IS_ERR(hwmon_dev)) { > > i915->hwmon = NULL; > > you need to free hwmon here, since it is not managed by devres anymore. Thanks a lot for catching this, I had missed it in v2, it's fixed in v3. I am actually reusing i915_hwmon_unregister() for error unwinding in v3. > > > return; > > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0)) > > continue; > > > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name, > > - ddat_gt, > > - &hwm_gt_chip_info, > > - NULL); > > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name, > > + ddat_gt, > > + &hwm_gt_chip_info, > > + NULL); > > if (!IS_ERR(hwmon_dev)) > > ddat_gt->hwmon_dev = hwmon_dev; > > } > > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > > > void i915_hwmon_unregister(struct drm_i915_private *i915) > > { > > - fetch_and_zero(&i915->hwmon); > > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); > > Why is fetch_and_zero() necessary here? As mentioned, in v3 i915_hwmon_unregister() itself is used for error unwinding so we need to prevent multiple device_unregister's etc. That is the purpose of setting i915->hwmon to NULL. But even earlier, though it is not obvious, i915_hwmon_unregister() is called multiple times. So e.g. it will be called at device unbind as well as module unload. So once again we prevent multiple device_unregister's by setting and checking for NULL i915->hwmon. > > > + struct hwm_drvdata *ddat = &hwmon->ddat; > > + struct intel_gt *gt; > > + int i; > > + > > + if (!hwmon) > > + return; > > + > > + for_each_gt(gt, i915, i) { > > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; > > + > > + if (ddat_gt->hwmon_dev) { > > + hwmon_device_unregister(ddat_gt->hwmon_dev); > > + ddat_gt->hwmon_dev = NULL; > > + } > > + } > > + > > + if (ddat->hwmon_dev) > > + hwmon_device_unregister(ddat->hwmon_dev); > > + > > + mutex_destroy(&hwmon->hwmon_lock); > > + kfree(hwmon); > > } Thanks. -- Ashutosh
On Mon, 15 Apr 2024, "Dixit, Ashutosh" <ashutosh.dixit@intel.com> wrote: > On Mon, 15 Apr 2024 16:35:02 -0700, Armin Wolf wrote: >> > > Hi Armin, > >> Am 16.04.24 um 00:36 schrieb Ashutosh Dixit: >> > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) >> > hwm_get_preregistration_info(i915); >> > >> > /* hwmon_dev points to device hwmon<i> */ >> > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name, >> > - ddat, >> > - &hwm_chip_info, >> > - hwm_groups); >> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, >> > + ddat, >> > + &hwm_chip_info, >> > + hwm_groups); >> > if (IS_ERR(hwmon_dev)) { >> > i915->hwmon = NULL; >> >> you need to free hwmon here, since it is not managed by devres anymore. > > Thanks a lot for catching this, I had missed it in v2, it's fixed in v3. I > am actually reusing i915_hwmon_unregister() for error unwinding in v3. > >> >> > return; >> > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) >> > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0)) >> > continue; >> > >> > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name, >> > - ddat_gt, >> > - &hwm_gt_chip_info, >> > - NULL); >> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name, >> > + ddat_gt, >> > + &hwm_gt_chip_info, >> > + NULL); >> > if (!IS_ERR(hwmon_dev)) >> > ddat_gt->hwmon_dev = hwmon_dev; >> > } >> > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) >> > >> > void i915_hwmon_unregister(struct drm_i915_private *i915) >> > { >> > - fetch_and_zero(&i915->hwmon); >> > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); >> >> Why is fetch_and_zero() necessary here? > > As mentioned, in v3 i915_hwmon_unregister() itself is used for error > unwinding so we need to prevent multiple device_unregister's etc. That is > the purpose of setting i915->hwmon to NULL. But even earlier, though it is > not obvious, i915_hwmon_unregister() is called multiple times. So e.g. it > will be called at device unbind as well as module unload. So once again we > prevent multiple device_unregister's by setting and checking for NULL > i915->hwmon. IMO it's more obvious to set i915->hwmon to NULL separately. BR, Jani. > >> >> > + struct hwm_drvdata *ddat = &hwmon->ddat; >> > + struct intel_gt *gt; >> > + int i; >> > + >> > + if (!hwmon) >> > + return; >> > + >> > + for_each_gt(gt, i915, i) { >> > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; >> > + >> > + if (ddat_gt->hwmon_dev) { >> > + hwmon_device_unregister(ddat_gt->hwmon_dev); >> > + ddat_gt->hwmon_dev = NULL; >> > + } >> > + } >> > + >> > + if (ddat->hwmon_dev) >> > + hwmon_device_unregister(ddat->hwmon_dev); >> > + >> > + mutex_destroy(&hwmon->hwmon_lock); >> > + kfree(hwmon); >> > } > > Thanks. > -- > Ashutosh
On Mon, Apr 15, 2024 at 03:36:12PM -0700, Ashutosh Dixit wrote: > When both hwmon and hwmon drvdata (on which hwmon depends) are device > managed resources, the expectation, on device unbind, is that hwmon will be > released before drvdata. However, in i915 there are two separate code > paths, which both release either drvdata or hwmon and either can be > released before the other. These code paths (for device unbind) are as > follows (see also the bug referenced below): > > Call Trace: > release_nodes+0x11/0x70 > devres_release_group+0xb2/0x110 > component_unbind_all+0x8d/0xa0 > component_del+0xa5/0x140 > intel_pxp_tee_component_fini+0x29/0x40 [i915] > intel_pxp_fini+0x33/0x80 [i915] > i915_driver_remove+0x4c/0x120 [i915] > i915_pci_remove+0x19/0x30 [i915] > pci_device_remove+0x32/0xa0 > device_release_driver_internal+0x19c/0x200 > unbind_store+0x9c/0xb0 > > and > > Call Trace: > release_nodes+0x11/0x70 > devres_release_all+0x8a/0xc0 > device_unbind_cleanup+0x9/0x70 > device_release_driver_internal+0x1c1/0x200 > unbind_store+0x9c/0xb0 > > This means that in i915, if use devm, we cannot gurantee that hwmon will > always be released before drvdata. Which means that we have a uaf if hwmon > sysfs is accessed when drvdata has been released but hwmon hasn't. > > The only way out of this seems to be do get rid of devm_ and release/free > everything explicitly during device unbind. > > v2: Change commit message and other minor code changes > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 8c3f443c8347..46c24b1ee6df 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > if (!IS_DGFX(i915)) > return; > > - hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL); > + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL); > if (!hwmon) > return; > > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > hwm_get_preregistration_info(i915); > > /* hwmon_dev points to device hwmon<i> */ > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name, > - ddat, > - &hwm_chip_info, > - hwm_groups); > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, > + ddat, > + &hwm_chip_info, > + hwm_groups); > if (IS_ERR(hwmon_dev)) { > i915->hwmon = NULL; > return; > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0)) > continue; > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name, > - ddat_gt, > - &hwm_gt_chip_info, > - NULL); > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name, > + ddat_gt, > + &hwm_gt_chip_info, > + NULL); > if (!IS_ERR(hwmon_dev)) > ddat_gt->hwmon_dev = hwmon_dev; > } > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > void i915_hwmon_unregister(struct drm_i915_private *i915) > { > - fetch_and_zero(&i915->hwmon); > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); > + struct hwm_drvdata *ddat = &hwmon->ddat; > + struct intel_gt *gt; > + int i; > + > + if (!hwmon) > + return; "that's too late", we are going to hear from static analyzer tools. beter to move ddat = &hwmon->ddat; after this return. with that, Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > + > + for_each_gt(gt, i915, i) { > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; > + > + if (ddat_gt->hwmon_dev) { > + hwmon_device_unregister(ddat_gt->hwmon_dev); > + ddat_gt->hwmon_dev = NULL; > + } > + } > + > + if (ddat->hwmon_dev) > + hwmon_device_unregister(ddat->hwmon_dev); > + > + mutex_destroy(&hwmon->hwmon_lock); > + kfree(hwmon); > } > -- > 2.41.0 >
On Tue, 16 Apr 2024 11:55:20 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > > > void i915_hwmon_unregister(struct drm_i915_private *i915) > > { > > - fetch_and_zero(&i915->hwmon); > > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); > > + struct hwm_drvdata *ddat = &hwmon->ddat; > > + struct intel_gt *gt; > > + int i; > > + > > + if (!hwmon) > > + return; > > "that's too late", we are going to hear from static analyzer tools. > > beter to move ddat = &hwmon->ddat; after this return. Yeah, I worried a lot about it :/ But then finally decided (and verified) that we are never actually dereferencing the (possibly NULL) pointer. But not sure about static analyzer tools, maybe you are right, I'll move it. > with that, > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Thanks a lot :) Ashutosh > > > + > > + for_each_gt(gt, i915, i) { > > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; > > + > > + if (ddat_gt->hwmon_dev) { > > + hwmon_device_unregister(ddat_gt->hwmon_dev); > > + ddat_gt->hwmon_dev = NULL; > > + } > > + } > > + > > + if (ddat->hwmon_dev) > > + hwmon_device_unregister(ddat->hwmon_dev); > > + > > + mutex_destroy(&hwmon->hwmon_lock); > > + kfree(hwmon); > > } > > -- > > 2.41.0 > >
On Tue, Apr 16, 2024 at 12:02:10PM -0700, Dixit, Ashutosh wrote: > On Tue, 16 Apr 2024 11:55:20 -0700, Rodrigo Vivi wrote: > > > > Hi Rodrigo, > > > > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > > > > > void i915_hwmon_unregister(struct drm_i915_private *i915) > > > { > > > - fetch_and_zero(&i915->hwmon); > > > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); > > > + struct hwm_drvdata *ddat = &hwmon->ddat; > > > + struct intel_gt *gt; > > > + int i; > > > + > > > + if (!hwmon) > > > + return; > > > > "that's too late", we are going to hear from static analyzer tools. > > > > beter to move ddat = &hwmon->ddat; after this return. > > Yeah, I worried a lot about it :/ But then finally decided (and verified) > that we are never actually dereferencing the (possibly NULL) pointer. yeap, another acceptable approach is to simply remove this check entirely. > > But not sure about static analyzer tools, maybe you are right, I'll move > it. > > > with that, > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Thanks a lot :) > > Ashutosh > > > > > > + > > > + for_each_gt(gt, i915, i) { > > > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; > > > + > > > + if (ddat_gt->hwmon_dev) { > > > + hwmon_device_unregister(ddat_gt->hwmon_dev); > > > + ddat_gt->hwmon_dev = NULL; > > > + } > > > + } > > > + > > > + if (ddat->hwmon_dev) > > > + hwmon_device_unregister(ddat->hwmon_dev); > > > + > > > + mutex_destroy(&hwmon->hwmon_lock); > > > + kfree(hwmon); > > > } > > > -- > > > 2.41.0 > > >
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 8c3f443c8347..46c24b1ee6df 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) if (!IS_DGFX(i915)) return; - hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL); + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL); if (!hwmon) return; @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) hwm_get_preregistration_info(i915); /* hwmon_dev points to device hwmon<i> */ - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name, - ddat, - &hwm_chip_info, - hwm_groups); + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, + ddat, + &hwm_chip_info, + hwm_groups); if (IS_ERR(hwmon_dev)) { i915->hwmon = NULL; return; @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915) if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0)) continue; - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name, - ddat_gt, - &hwm_gt_chip_info, - NULL); + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name, + ddat_gt, + &hwm_gt_chip_info, + NULL); if (!IS_ERR(hwmon_dev)) ddat_gt->hwmon_dev = hwmon_dev; } @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915) void i915_hwmon_unregister(struct drm_i915_private *i915) { - fetch_and_zero(&i915->hwmon); + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon); + struct hwm_drvdata *ddat = &hwmon->ddat; + struct intel_gt *gt; + int i; + + if (!hwmon) + return; + + for_each_gt(gt, i915, i) { + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i; + + if (ddat_gt->hwmon_dev) { + hwmon_device_unregister(ddat_gt->hwmon_dev); + ddat_gt->hwmon_dev = NULL; + } + } + + if (ddat->hwmon_dev) + hwmon_device_unregister(ddat->hwmon_dev); + + mutex_destroy(&hwmon->hwmon_lock); + kfree(hwmon); }
When both hwmon and hwmon drvdata (on which hwmon depends) are device managed resources, the expectation, on device unbind, is that hwmon will be released before drvdata. However, in i915 there are two separate code paths, which both release either drvdata or hwmon and either can be released before the other. These code paths (for device unbind) are as follows (see also the bug referenced below): Call Trace: release_nodes+0x11/0x70 devres_release_group+0xb2/0x110 component_unbind_all+0x8d/0xa0 component_del+0xa5/0x140 intel_pxp_tee_component_fini+0x29/0x40 [i915] intel_pxp_fini+0x33/0x80 [i915] i915_driver_remove+0x4c/0x120 [i915] i915_pci_remove+0x19/0x30 [i915] pci_device_remove+0x32/0xa0 device_release_driver_internal+0x19c/0x200 unbind_store+0x9c/0xb0 and Call Trace: release_nodes+0x11/0x70 devres_release_all+0x8a/0xc0 device_unbind_cleanup+0x9/0x70 device_release_driver_internal+0x1c1/0x200 unbind_store+0x9c/0xb0 This means that in i915, if use devm, we cannot gurantee that hwmon will always be released before drvdata. Which means that we have a uaf if hwmon sysfs is accessed when drvdata has been released but hwmon hasn't. The only way out of this seems to be do get rid of devm_ and release/free everything explicitly during device unbind. v2: Change commit message and other minor code changes Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 10 deletions(-)