| Message ID | 296998949.155413.1438375938630.JavaMail.zimbra@xes-inc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On Fri, Jul 31, 2015 at 03:52:18PM -0500, Aaron Sierra wrote: > > @@ -2905,8 +2919,7 @@ static int talitos_probe(struct platform_device *ofdev) > priv->reg = of_iomap(np, 0); > if (!priv->reg) { > dev_err(dev, "failed to of_iomap\n"); > - err = -ENOMEM; > - goto err_out; > + return -ENOMEM; > } This is the only bit that seems remotely related to your change description. Please ensure that your patch actually corresponds to your changelog and do not include unrelated changes without documenting them. And even this bit is wrong because you're leaking priv. Cheers,
----- Original Message ----- > From: "Herbert Xu" <herbert@gondor.apana.org.au> > Sent: Tuesday, August 4, 2015 2:18:05 AM > > On Fri, Jul 31, 2015 at 03:52:18PM -0500, Aaron Sierra wrote: > > > > @@ -2905,8 +2919,7 @@ static int talitos_probe(struct platform_device > > *ofdev) > > priv->reg = of_iomap(np, 0); > > if (!priv->reg) { > > dev_err(dev, "failed to of_iomap\n"); > > - err = -ENOMEM; > > - goto err_out; > > + return -ENOMEM; > > } > > This is the only bit that seems remotely related to your change > description. Please ensure that your patch actually corresponds > to your changelog and do not include unrelated changes without > documenting them. > > And even this bit is wrong because you're leaking priv. Herbert, You are correct about the leak and I regret introducing that (I am also leaking priv->rng), but I disagree with your dismissal of the rest of the changes as unrelated to the changelog. The probe error path for this driver, for all intents and purposes, is the talitos_remove() function due to the common "goto err_out". Without my patch applied, talitos_remove() will panic under the two conditions that I outlined in the changelog: 1. If the RNG device hasn't been registered via talitos_register_rng() prior to entry into talitos_remove(), then the attempt to unregister the RNG "device" will cause a panic. 2. If the priv->chan array has not been allocated prior to entry into talitos_remove(), then the per-channel FIFO cleanup will panic because of the dereference of that NULL "array". Both of the above scenarios occur if talitos_probe_irq() fails. The patch that I submitted resolves issue #1 by changing priv->rng to a struct hwrng pointer, which allows talitos_unregister_rng() to know whether an RNG device has been registered (or at least prepared for registration). As an alternative, I considered introducing a boolean to serve the same purpose. It resolves issue #2 by checking that priv->chan is not NULL in the per-channel FIFO cleanup for loop. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 04, 2015 at 09:43:50AM -0500, Aaron Sierra wrote: > > You are correct about the leak and I regret introducing that (I am > also leaking priv->rng), but I disagree with your dismissal of the > rest of the changes as unrelated to the changelog. I understand the problem, but your change description is way too vague and does not correspond to the change, especially the bit where you're replacing the static variable with kzalloc. You should have included the following text in your description. > The probe error path for this driver, for all intents and purposes, > is the talitos_remove() function due to the common "goto err_out". > > Without my patch applied, talitos_remove() will panic under the > two conditions that I outlined in the changelog: > > 1. If the RNG device hasn't been registered via > talitos_register_rng() prior to entry into talitos_remove(), > then the attempt to unregister the RNG "device" will cause a panic. > > 2. If the priv->chan array has not been allocated prior to entry > into talitos_remove(), then the per-channel FIFO cleanup will panic > because of the dereference of that NULL "array". > > Both of the above scenarios occur if talitos_probe_irq() fails. > > The patch that I submitted resolves issue #1 by changing priv->rng > to a struct hwrng pointer, which allows talitos_unregister_rng() to > know whether an RNG device has been registered (or at least prepared > for registration). As an alternative, I considered introducing a > boolean to serve the same purpose. I would prefer a boolean as that means less churn. Thanks,
----- Original Message ----- > From: "Herbert Xu" <herbert@gondor.apana.org.au> > Sent: Tuesday, August 4, 2015 7:08:13 PM > > On Tue, Aug 04, 2015 at 09:43:50AM -0500, Aaron Sierra wrote: > > > > You are correct about the leak and I regret introducing that (I am > > also leaking priv->rng), but I disagree with your dismissal of the > > rest of the changes as unrelated to the changelog. > > I understand the problem, but your change description is way > too vague and does not correspond to the change, especially the > bit where you're replacing the static variable with kzalloc. You > should have included the following text in your description. > > > The probe error path for this driver, for all intents and purposes, > > is the talitos_remove() function due to the common "goto err_out". > > > > Without my patch applied, talitos_remove() will panic under the > > two conditions that I outlined in the changelog: > > > > 1. If the RNG device hasn't been registered via > > talitos_register_rng() prior to entry into talitos_remove(), > > then the attempt to unregister the RNG "device" will cause a panic. > > > > 2. If the priv->chan array has not been allocated prior to entry > > into talitos_remove(), then the per-channel FIFO cleanup will panic > > because of the dereference of that NULL "array". > > > > Both of the above scenarios occur if talitos_probe_irq() fails. > > > > The patch that I submitted resolves issue #1 by changing priv->rng > > to a struct hwrng pointer, which allows talitos_unregister_rng() to > > know whether an RNG device has been registered (or at least prepared > > for registration). As an alternative, I considered introducing a > > boolean to serve the same purpose. > > I would prefer a boolean as that means less churn. > Herbert, Thanks for the feedback. I'll address your concerns in the next version. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c index 83aca95..684fe89 100644 --- a/drivers/crypto/talitos.c +++ b/drivers/crypto/talitos.c @@ -766,21 +766,33 @@ static int talitos_rng_init(struct hwrng *rng) static int talitos_register_rng(struct device *dev) { struct talitos_private *priv = dev_get_drvdata(dev); + int err = 0; - priv->rng.name = dev_driver_string(dev), - priv->rng.init = talitos_rng_init, - priv->rng.data_present = talitos_rng_data_present, - priv->rng.data_read = talitos_rng_data_read, - priv->rng.priv = (unsigned long)dev; + priv->rng = kzalloc(sizeof(struct hwrng), GFP_KERNEL); + if (!priv->rng) + return -ENOMEM; + + priv->rng->name = dev_driver_string(dev), + priv->rng->init = talitos_rng_init, + priv->rng->data_present = talitos_rng_data_present, + priv->rng->data_read = talitos_rng_data_read, + priv->rng->priv = (unsigned long)dev; + + err = hwrng_register(priv->rng); + if (err) { + kfree(priv->rng); + priv->rng = NULL; + } - return hwrng_register(&priv->rng); + return err; } static void talitos_unregister_rng(struct device *dev) { struct talitos_private *priv = dev_get_drvdata(dev); - hwrng_unregister(&priv->rng); + if (priv->rng) + hwrng_unregister(priv->rng); } /* @@ -2727,7 +2739,7 @@ static int talitos_remove(struct platform_device *ofdev) if (hw_supports(dev, DESC_HDR_SEL0_RNG)) talitos_unregister_rng(dev); - for (i = 0; i < priv->num_channels; i++) + for (i = 0; priv->chan && i < priv->num_channels; i++) kfree(priv->chan[i].fifo); kfree(priv->chan); @@ -2746,6 +2758,8 @@ static int talitos_remove(struct platform_device *ofdev) kfree(priv); + dev_set_drvdata(dev, NULL); + return 0; } @@ -2905,8 +2919,7 @@ static int talitos_probe(struct platform_device *ofdev) priv->reg = of_iomap(np, 0); if (!priv->reg) { dev_err(dev, "failed to of_iomap\n"); - err = -ENOMEM; - goto err_out; + return -ENOMEM; } /* get SEC version capabilities from device tree */ diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h index bf88fe7..e86f67c 100644 --- a/drivers/crypto/talitos.h +++ b/drivers/crypto/talitos.h @@ -148,7 +148,7 @@ struct talitos_private { struct list_head alg_list; /* hwrng device */ - struct hwrng rng; + struct hwrng *rng; }; extern int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
RNG unregistration and per-channel FIFO cleanup can cause a kernel panic, depending on how early in talitos_probe() an error occurs. This patch prevents these panics from happening. For completeness, this patch also sets device drvdata to NULL after the private structure is freed in talitos_remove(). Signed-off-by: Aaron Sierra <asierra@xes-inc.com> --- drivers/crypto/talitos.c | 33 +++++++++++++++++++++++---------- drivers/crypto/talitos.h | 2 +- 2 files changed, 24 insertions(+), 11 deletions(-)