| Message ID | 98a3d1794bc001f312a7db31ad03465ba697bb36.1481829722.git.shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shuah, Thank you for the patch. Sakari has submitted a similar patch as part of his kref series. Please use it as a base point and rework it if you want to get it merged separately. I've reviewed the patch and left quite a few comments that need to be addressed. On Thursday 15 Dec 2016 12:40:08 Shuah Khan wrote: > Using devm resources that have external dependencies such as a dev > for a file handler could result in devm resources getting released > durin unbind while an application has the file open holding pointer > to the devm resource. This results in use-after-free errors when the > application exits. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/media/platform/omap3isp/isp.c | 71 ++++++++++++++++-------- > drivers/media/platform/omap3isp/ispccp2.c | 10 +++- > drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++--- > drivers/media/platform/omap3isp/isph3a_af.c | 21 +++++--- > drivers/media/platform/omap3isp/isphist.c | 5 +- > 5 files changed, 92 insertions(+), 36 deletions(-)
On 12/15/2016 03:33 PM, Laurent Pinchart wrote: > Hi Shuah, > > Thank you for the patch. > > Sakari has submitted a similar patch as part of his kref series. Please use it > as a base point and rework it if you want to get it merged separately. I've > reviewed the patch and left quite a few comments that need to be addressed. > I really don't mind if Sakari uses this patch as is and makes the changes you requested and submits devm removal as an independent patch. My intent behind sending this one is to help him out since I already did this patch that is on top of 4.9-rc8 without any dependencies on Sakari's RFC patch. thanks, -- Shuah > On Thursday 15 Dec 2016 12:40:08 Shuah Khan wrote: >> Using devm resources that have external dependencies such as a dev >> for a file handler could result in devm resources getting released >> durin unbind while an application has the file open holding pointer >> to the devm resource. This results in use-after-free errors when the >> application exits. >> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >> --- >> drivers/media/platform/omap3isp/isp.c | 71 ++++++++++++++++-------- >> drivers/media/platform/omap3isp/ispccp2.c | 10 +++- >> drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++--- >> drivers/media/platform/omap3isp/isph3a_af.c | 21 +++++--- >> drivers/media/platform/omap3isp/isphist.c | 5 +- >> 5 files changed, 92 insertions(+), 36 deletions(-) > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Shuah, On Thursday 15 Dec 2016 15:51:41 Shuah Khan wrote: > On 12/15/2016 03:33 PM, Laurent Pinchart wrote: > > Hi Shuah, > > > > Thank you for the patch. > > > > Sakari has submitted a similar patch as part of his kref series. Please > > use it as a base point and rework it if you want to get it merged > > separately. I've reviewed the patch and left quite a few comments that > > need to be addressed. > > I really don't mind if Sakari uses this patch as is and makes the changes > you requested and submits devm removal as an independent patch. > > My intent behind sending this one is to help him out since I already did > this patch that is on top of 4.9-rc8 without any dependencies on Sakari's > RFC patch. I've only seen your reply to Sakari's patch after replying to this one. Thank you for providing your version, I'll let Sakari merge both and resubmit. > > On Thursday 15 Dec 2016 12:40:08 Shuah Khan wrote: > >> Using devm resources that have external dependencies such as a dev > >> for a file handler could result in devm resources getting released > >> durin unbind while an application has the file open holding pointer > >> to the devm resource. This results in use-after-free errors when the > >> application exits. > >> > >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > >> --- > >> > >> drivers/media/platform/omap3isp/isp.c | 71 +++++++++++++-------- > >> drivers/media/platform/omap3isp/ispccp2.c | 10 +++- > >> drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++--- > >> drivers/media/platform/omap3isp/isph3a_af.c | 21 +++++--- > >> drivers/media/platform/omap3isp/isphist.c | 5 +- > >> 5 files changed, 92 insertions(+), 36 deletions(-)
Hi Shuah,
[auto build test WARNING on v4.9-rc8]
[cannot apply to linuxtv-media/master next-20161215]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Shuah-Khan/omap3-devm-usage-removal/20161216-111439
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All warnings (new ones prefixed by >>):
drivers/media/platform/omap3isp/isp.c: In function 'isp_probe':
>> drivers/media/platform/omap3isp/isp.c:2289:3: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (IS_ERR(isp->mmio_base[map_idx]))
^~
drivers/media/platform/omap3isp/isp.c:2291:4: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto error_put_vdd_csiphy2;
^~~~
vim +/if +2289 drivers/media/platform/omap3isp/isp.c
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27 2273 /* Clocks
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27 2274 *
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27 2275 * The ISP clock tree is revision-dependent. We thus need to enable ICLK
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27 2276 * manually to read the revision before calling __omap3isp_get().
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2277 *
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2278 * Start by mapping the ISP MMIO area, which is in two pieces.
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2279 * The ISP IOMMU is in between. Map both now, and fill in the
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2280 * ISP revision specific portions a little later in the
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2281 * function.
d8658bca drivers/media/platform/omap3isp/isp.c Laurent Pinchart 2012-09-27 2282 */
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2283 for (i = 0; i < 2; i++) {
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2284 unsigned int map_idx = i ? OMAP3_ISP_IOMEM_CSI2A_REGS1 : 0;
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2285
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2286 mem = platform_get_resource(pdev, IORESOURCE_MEM, i);
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2287 isp->mmio_base[map_idx] =
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2288 devm_ioremap_resource(isp->dev, mem);
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 @2289 if (IS_ERR(isp->mmio_base[map_idx]))
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan 2016-12-15 2290 ret = PTR_ERR(isp->mmio_base[map_idx]);
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan 2016-12-15 2291 goto error_put_vdd_csiphy2;
8644cdf9 drivers/media/platform/omap3isp/isp.c Sakari Ailus 2015-03-25 2292 }
448de7e7 drivers/media/video/omap3isp/isp.c Sakari Ailus 2011-02-12 2293
448de7e7 drivers/media/video/omap3isp/isp.c Sakari Ailus 2011-02-12 2294 ret = isp_get_clocks(isp);
448de7e7 drivers/media/video/omap3isp/isp.c Sakari Ailus 2011-02-12 2295 if (ret < 0)
27e86e5f drivers/media/platform/omap3isp/isp.c Shuah Khan 2016-12-15 2296 goto error_put_vdd_csiphy2;
448de7e7 drivers/media/video/omap3isp/isp.c Sakari Ailus 2011-02-12 2297
:::::: The code at line 2289 was first introduced by commit
:::::: 8644cdf972dd6dfebf98161025900f6a9d1ad58a [media] omap3isp: Replace many MMIO regions by two
:::::: TO: Sakari Ailus <sakari.ailus@iki.fi>
:::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 15/12/16 20:40, Shuah Khan wrote: > Using devm resources that have external dependencies such as a dev > for a file handler could result in devm resources getting released > durin unbind while an application has the file open holding pointer > to the devm resource. This results in use-after-free errors when the > application exits. That's solving the wrong problem. The real problem is that when registering a video_device it should do this: devnode->cdev.kobj.parent = &devnode->dev.kobj; (taken from cec-core.c) This will prevent isp->dev from being released as long as there is a filehandle still open. After that change I believe that this will work correctly, but this has to be tested first! Regards, Hans > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/media/platform/omap3isp/isp.c | 71 +++++++++++++++++++-------- > drivers/media/platform/omap3isp/ispccp2.c | 10 +++- > drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++--- > drivers/media/platform/omap3isp/isph3a_af.c | 21 +++++--- > drivers/media/platform/omap3isp/isphist.c | 5 +- > 5 files changed, 92 insertions(+), 36 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c > index 0321d84..a11c509 100644 > --- a/drivers/media/platform/omap3isp/isp.c > +++ b/drivers/media/platform/omap3isp/isp.c > @@ -1374,7 +1374,7 @@ static int isp_get_clocks(struct isp_device *isp) > unsigned int i; > > for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) { > - clk = devm_clk_get(isp->dev, isp_clocks[i]); > + clk = clk_get(isp->dev, isp_clocks[i]); > if (IS_ERR(clk)) { > dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]); > return PTR_ERR(clk); > @@ -1386,6 +1386,14 @@ static int isp_get_clocks(struct isp_device *isp) > return 0; > } > > +static void isp_put_clocks(struct isp_device *isp) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) > + clk_put(isp->clock[i]); > +} > + > /* > * omap3isp_get - Acquire the ISP resource. > * > @@ -2015,6 +2023,11 @@ static int isp_remove(struct platform_device *pdev) > > media_entity_enum_cleanup(&isp->crashed); > > + regulator_put(isp->isp_csiphy2.vdd); > + regulator_put(isp->isp_csiphy1.vdd); > + > + isp_put_clocks(isp); > + kfree(isp); > return 0; > } > > @@ -2107,8 +2120,8 @@ static int isp_of_parse_nodes(struct device *dev, > { > struct device_node *node = NULL; > > - notifier->subdevs = devm_kcalloc( > - dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL); > + notifier->subdevs = kcalloc( > + ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL); > if (!notifier->subdevs) > return -ENOMEM; > > @@ -2116,11 +2129,9 @@ static int isp_of_parse_nodes(struct device *dev, > (node = of_graph_get_next_endpoint(dev->of_node, node))) { > struct isp_async_subdev *isd; > > - isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL); > - if (!isd) { > - of_node_put(node); > + isd = kzalloc(sizeof(*isd), GFP_KERNEL); > + if (!isd) > return -ENOMEM; > - } > > notifier->subdevs[notifier->num_subdevs] = &isd->asd; > > @@ -2204,7 +2215,7 @@ static int isp_probe(struct platform_device *pdev) > int ret; > int i, m; > > - isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL); > + isp = kzalloc(sizeof(*isp), GFP_KERNEL); > if (!isp) { > dev_err(&pdev->dev, "could not allocate memory\n"); > return -ENOMEM; > @@ -2213,21 +2224,23 @@ static int isp_probe(struct platform_device *pdev) > ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type", > &isp->phy_type); > if (ret) > - return ret; > + goto error_release_isp; > > isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, > "syscon"); > - if (IS_ERR(isp->syscon)) > - return PTR_ERR(isp->syscon); > + if (IS_ERR(isp->syscon)) { > + ret = PTR_ERR(isp->syscon); > + goto error_release_isp; > + } > > ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, > &isp->syscon_offset); > if (ret) > - return ret; > + goto error_release_isp; > > ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier); > if (ret < 0) > - return ret; > + goto error_release_isp; > > isp->autoidle = autoidle; > > @@ -2244,8 +2257,18 @@ static int isp_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, isp); > > /* Regulators */ > - isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1"); > - isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2"); > + isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "vdd-csiphy1"); > + if (IS_ERR(isp->isp_csiphy1.vdd)) { > + ret = PTR_ERR(isp->isp_csiphy1.vdd); > + isp->isp_csiphy1.vdd = NULL; > + goto error; > + } > + isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "vdd-csiphy2"); > + if (IS_ERR(isp->isp_csiphy2.vdd)) { > + ret = PTR_ERR(isp->isp_csiphy2.vdd); > + isp->isp_csiphy2.vdd = NULL; > + goto error_put_vdd_csiphy1; > + } > > /* Clocks > * > @@ -2264,16 +2287,17 @@ static int isp_probe(struct platform_device *pdev) > isp->mmio_base[map_idx] = > devm_ioremap_resource(isp->dev, mem); > if (IS_ERR(isp->mmio_base[map_idx])) > - return PTR_ERR(isp->mmio_base[map_idx]); > + ret = PTR_ERR(isp->mmio_base[map_idx]); > + goto error_put_vdd_csiphy2; > } > > ret = isp_get_clocks(isp); > if (ret < 0) > - goto error; > + goto error_put_vdd_csiphy2; > > ret = clk_enable(isp->clock[ISP_CLK_CAM_ICK]); > if (ret < 0) > - goto error; > + goto error_put_vdd_csiphy2; > > isp->revision = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_REVISION); > dev_info(isp->dev, "Revision %d.%d found\n", > @@ -2283,7 +2307,7 @@ static int isp_probe(struct platform_device *pdev) > > if (__omap3isp_get(isp, false) == NULL) { > ret = -ENODEV; > - goto error; > + goto error_put_vdd_csiphy2; > } > > ret = isp_reset(isp); > @@ -2334,7 +2358,7 @@ static int isp_probe(struct platform_device *pdev) > } > isp->irq_num = ret; > > - if (devm_request_irq(isp->dev, isp->irq_num, isp_isr, IRQF_SHARED, > + if (request_irq(isp->irq_num, isp_isr, IRQF_SHARED, > "OMAP3 ISP", isp)) { > dev_err(isp->dev, "Unable to request IRQ\n"); > ret = -EINVAL; > @@ -2375,8 +2399,15 @@ static int isp_probe(struct platform_device *pdev) > error_isp: > isp_xclk_cleanup(isp); > __omap3isp_put(isp, false); > +error_put_vdd_csiphy2: > + regulator_put(isp->isp_csiphy2.vdd); > +error_put_vdd_csiphy1: > + regulator_put(isp->isp_csiphy1.vdd); > error: > mutex_destroy(&isp->isp_mutex); > + isp_put_clocks(isp); > +error_release_isp: > + kfree(isp); > > return ret; > } > diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c > index 4c1e7f0..adf4191 100644 > --- a/drivers/media/platform/omap3isp/ispccp2.c > +++ b/drivers/media/platform/omap3isp/ispccp2.c > @@ -1135,7 +1135,7 @@ int omap3isp_ccp2_init(struct isp_device *isp) > * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c). > */ > if (isp->revision == ISP_REVISION_2_0) { > - ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); > + ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib"); > if (IS_ERR(ccp2->vdds_csib)) { > dev_dbg(isp->dev, > "Could not get regulator vdds_csib\n"); > @@ -1147,10 +1147,15 @@ int omap3isp_ccp2_init(struct isp_device *isp) > > ret = ccp2_init_entities(ccp2); > if (ret < 0) > - return ret; > + goto error_put_vdds_csib; > > ccp2_reset(ccp2); > return 0; > + > +error_put_vdds_csib: > + regulator_put(ccp2->vdds_csib); > + > + return ret; > } > > /* > @@ -1162,4 +1167,5 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp) > struct isp_ccp2_device *ccp2 = &isp->isp_ccp2; > > omap3isp_video_cleanup(&ccp2->video_in); > + regulator_put(ccp2->vdds_csib); > } > diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c > index ccaf92f..042de3e 100644 > --- a/drivers/media/platform/omap3isp/isph3a_aewb.c > +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c > @@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp) > { > struct ispstat *aewb = &isp->isp_aewb; > struct omap3isp_h3a_aewb_config *aewb_cfg; > - struct omap3isp_h3a_aewb_config *aewb_recover_cfg; > + struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL; > + int ret; > > - aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL); > + aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL); > if (!aewb_cfg) > return -ENOMEM; > > @@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp) > aewb->isp = isp; > > /* Set recover state configuration */ > - aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg), > - GFP_KERNEL); > + aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL); > if (!aewb_recover_cfg) { > dev_err(aewb->isp->dev, "AEWB: cannot allocate memory for " > "recover configuration.\n"); > - return -ENOMEM; > + ret = -ENOMEM; > + goto err_release_aewb_cfg; > } > > aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM; > @@ -323,13 +324,21 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp) > if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) { > dev_err(aewb->isp->dev, "AEWB: recover configuration is " > "invalid.\n"); > - return -EINVAL; > + ret = -EINVAL; > + goto err_release_aewb_recover_cfg; > } > > aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg); > aewb->recover_priv = aewb_recover_cfg; > > return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops); > + > +err_release_aewb_recover_cfg: > + kfree(aewb_recover_cfg); > +err_release_aewb_cfg: > + kfree(aewb_cfg); > + > + return ret; > } > > /* > diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c > index 92937f7..1919cb2 100644 > --- a/drivers/media/platform/omap3isp/isph3a_af.c > +++ b/drivers/media/platform/omap3isp/isph3a_af.c > @@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp) > { > struct ispstat *af = &isp->isp_af; > struct omap3isp_h3a_af_config *af_cfg; > - struct omap3isp_h3a_af_config *af_recover_cfg; > + struct omap3isp_h3a_af_config *af_recover_cfg = NULL; > + int ret; > > - af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL); > + af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL); > if (af_cfg == NULL) > return -ENOMEM; > > @@ -364,12 +365,12 @@ int omap3isp_h3a_af_init(struct isp_device *isp) > af->isp = isp; > > /* Set recover state configuration */ > - af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg), > - GFP_KERNEL); > + af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL); > if (!af_recover_cfg) { > dev_err(af->isp->dev, "AF: cannot allocate memory for recover " > "configuration.\n"); > - return -ENOMEM; > + ret = -ENOMEM; > + goto err_release_af_cfg; > } > > af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN; > @@ -381,13 +382,21 @@ int omap3isp_h3a_af_init(struct isp_device *isp) > if (h3a_af_validate_params(af, af_recover_cfg)) { > dev_err(af->isp->dev, "AF: recover configuration is " > "invalid.\n"); > - return -EINVAL; > + ret = -EINVAL; > + goto err_release_af_recover_cfg; > } > > af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg); > af->recover_priv = af_recover_cfg; > > return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops); > + > +err_release_af_recover_cfg: > + kfree(af_recover_cfg); > +err_release_af_cfg: > + kfree(af_cfg); > + > + return ret; > } > > void omap3isp_h3a_af_cleanup(struct isp_device *isp) > diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c > index 7138b04..5d8f04b 100644 > --- a/drivers/media/platform/omap3isp/isphist.c > +++ b/drivers/media/platform/omap3isp/isphist.c > @@ -477,9 +477,9 @@ int omap3isp_hist_init(struct isp_device *isp) > { > struct ispstat *hist = &isp->isp_hist; > struct omap3isp_hist_config *hist_cfg; > - int ret = -1; > + int ret = 0; > > - hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL); > + hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL); > if (hist_cfg == NULL) > return -ENOMEM; > > @@ -517,6 +517,7 @@ int omap3isp_hist_init(struct isp_device *isp) > if (ret) { > if (hist->dma_ch) > dma_release_channel(hist->dma_ch); > + kfree(hist_cfg); > } > > return ret; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Friday 16 Dec 2016 12:39:49 Hans Verkuil wrote: > On 15/12/16 20:40, Shuah Khan wrote: > > Using devm resources that have external dependencies such as a dev > > for a file handler could result in devm resources getting released > > durin unbind while an application has the file open holding pointer > > to the devm resource. This results in use-after-free errors when the > > application exits. > > That's solving the wrong problem. > > The real problem is that when registering a video_device it should do > this: > > devnode->cdev.kobj.parent = &devnode->dev.kobj; > > (taken from cec-core.c) > > This will prevent isp->dev from being released as long as there is a > filehandle still open. But it won't be enough, devm_* resources are released at unbind time, not at device release time. Right after the unbind (.remove() for platform devices) handler returns, devm_kzalloc allocated memory goes away. > After that change I believe that this will work correctly, but this > has to be tested first!
On 16/12/16 13:19, Laurent Pinchart wrote: > Hi Hans, > > On Friday 16 Dec 2016 12:39:49 Hans Verkuil wrote: >> On 15/12/16 20:40, Shuah Khan wrote: >>> Using devm resources that have external dependencies such as a dev >>> for a file handler could result in devm resources getting released >>> durin unbind while an application has the file open holding pointer >>> to the devm resource. This results in use-after-free errors when the >>> application exits. >> >> That's solving the wrong problem. >> >> The real problem is that when registering a video_device it should do >> this: >> >> devnode->cdev.kobj.parent = &devnode->dev.kobj; >> >> (taken from cec-core.c) >> >> This will prevent isp->dev from being released as long as there is a >> filehandle still open. > > But it won't be enough, devm_* resources are released at unbind time, not at > device release time. Right after the unbind (.remove() for platform devices) > handler returns, devm_kzalloc allocated memory goes away. You're completely right, I keep forgetting about that. Sorry for the noise. Hans > >> After that change I believe that this will work correctly, but this >> has to be tested first! > -- To unsubscribe from this list: send the line "unsubscribe linux-media" 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/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index 0321d84..a11c509 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -1374,7 +1374,7 @@ static int isp_get_clocks(struct isp_device *isp) unsigned int i; for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) { - clk = devm_clk_get(isp->dev, isp_clocks[i]); + clk = clk_get(isp->dev, isp_clocks[i]); if (IS_ERR(clk)) { dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]); return PTR_ERR(clk); @@ -1386,6 +1386,14 @@ static int isp_get_clocks(struct isp_device *isp) return 0; } +static void isp_put_clocks(struct isp_device *isp) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) + clk_put(isp->clock[i]); +} + /* * omap3isp_get - Acquire the ISP resource. * @@ -2015,6 +2023,11 @@ static int isp_remove(struct platform_device *pdev) media_entity_enum_cleanup(&isp->crashed); + regulator_put(isp->isp_csiphy2.vdd); + regulator_put(isp->isp_csiphy1.vdd); + + isp_put_clocks(isp); + kfree(isp); return 0; } @@ -2107,8 +2120,8 @@ static int isp_of_parse_nodes(struct device *dev, { struct device_node *node = NULL; - notifier->subdevs = devm_kcalloc( - dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL); + notifier->subdevs = kcalloc( + ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL); if (!notifier->subdevs) return -ENOMEM; @@ -2116,11 +2129,9 @@ static int isp_of_parse_nodes(struct device *dev, (node = of_graph_get_next_endpoint(dev->of_node, node))) { struct isp_async_subdev *isd; - isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL); - if (!isd) { - of_node_put(node); + isd = kzalloc(sizeof(*isd), GFP_KERNEL); + if (!isd) return -ENOMEM; - } notifier->subdevs[notifier->num_subdevs] = &isd->asd; @@ -2204,7 +2215,7 @@ static int isp_probe(struct platform_device *pdev) int ret; int i, m; - isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL); + isp = kzalloc(sizeof(*isp), GFP_KERNEL); if (!isp) { dev_err(&pdev->dev, "could not allocate memory\n"); return -ENOMEM; @@ -2213,21 +2224,23 @@ static int isp_probe(struct platform_device *pdev) ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type", &isp->phy_type); if (ret) - return ret; + goto error_release_isp; isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "syscon"); - if (IS_ERR(isp->syscon)) - return PTR_ERR(isp->syscon); + if (IS_ERR(isp->syscon)) { + ret = PTR_ERR(isp->syscon); + goto error_release_isp; + } ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, &isp->syscon_offset); if (ret) - return ret; + goto error_release_isp; ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier); if (ret < 0) - return ret; + goto error_release_isp; isp->autoidle = autoidle; @@ -2244,8 +2257,18 @@ static int isp_probe(struct platform_device *pdev) platform_set_drvdata(pdev, isp); /* Regulators */ - isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1"); - isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2"); + isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "vdd-csiphy1"); + if (IS_ERR(isp->isp_csiphy1.vdd)) { + ret = PTR_ERR(isp->isp_csiphy1.vdd); + isp->isp_csiphy1.vdd = NULL; + goto error; + } + isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "vdd-csiphy2"); + if (IS_ERR(isp->isp_csiphy2.vdd)) { + ret = PTR_ERR(isp->isp_csiphy2.vdd); + isp->isp_csiphy2.vdd = NULL; + goto error_put_vdd_csiphy1; + } /* Clocks * @@ -2264,16 +2287,17 @@ static int isp_probe(struct platform_device *pdev) isp->mmio_base[map_idx] = devm_ioremap_resource(isp->dev, mem); if (IS_ERR(isp->mmio_base[map_idx])) - return PTR_ERR(isp->mmio_base[map_idx]); + ret = PTR_ERR(isp->mmio_base[map_idx]); + goto error_put_vdd_csiphy2; } ret = isp_get_clocks(isp); if (ret < 0) - goto error; + goto error_put_vdd_csiphy2; ret = clk_enable(isp->clock[ISP_CLK_CAM_ICK]); if (ret < 0) - goto error; + goto error_put_vdd_csiphy2; isp->revision = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_REVISION); dev_info(isp->dev, "Revision %d.%d found\n", @@ -2283,7 +2307,7 @@ static int isp_probe(struct platform_device *pdev) if (__omap3isp_get(isp, false) == NULL) { ret = -ENODEV; - goto error; + goto error_put_vdd_csiphy2; } ret = isp_reset(isp); @@ -2334,7 +2358,7 @@ static int isp_probe(struct platform_device *pdev) } isp->irq_num = ret; - if (devm_request_irq(isp->dev, isp->irq_num, isp_isr, IRQF_SHARED, + if (request_irq(isp->irq_num, isp_isr, IRQF_SHARED, "OMAP3 ISP", isp)) { dev_err(isp->dev, "Unable to request IRQ\n"); ret = -EINVAL; @@ -2375,8 +2399,15 @@ static int isp_probe(struct platform_device *pdev) error_isp: isp_xclk_cleanup(isp); __omap3isp_put(isp, false); +error_put_vdd_csiphy2: + regulator_put(isp->isp_csiphy2.vdd); +error_put_vdd_csiphy1: + regulator_put(isp->isp_csiphy1.vdd); error: mutex_destroy(&isp->isp_mutex); + isp_put_clocks(isp); +error_release_isp: + kfree(isp); return ret; } diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c index 4c1e7f0..adf4191 100644 --- a/drivers/media/platform/omap3isp/ispccp2.c +++ b/drivers/media/platform/omap3isp/ispccp2.c @@ -1135,7 +1135,7 @@ int omap3isp_ccp2_init(struct isp_device *isp) * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c). */ if (isp->revision == ISP_REVISION_2_0) { - ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib"); + ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib"); if (IS_ERR(ccp2->vdds_csib)) { dev_dbg(isp->dev, "Could not get regulator vdds_csib\n"); @@ -1147,10 +1147,15 @@ int omap3isp_ccp2_init(struct isp_device *isp) ret = ccp2_init_entities(ccp2); if (ret < 0) - return ret; + goto error_put_vdds_csib; ccp2_reset(ccp2); return 0; + +error_put_vdds_csib: + regulator_put(ccp2->vdds_csib); + + return ret; } /* @@ -1162,4 +1167,5 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp) struct isp_ccp2_device *ccp2 = &isp->isp_ccp2; omap3isp_video_cleanup(&ccp2->video_in); + regulator_put(ccp2->vdds_csib); } diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c index ccaf92f..042de3e 100644 --- a/drivers/media/platform/omap3isp/isph3a_aewb.c +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c @@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp) { struct ispstat *aewb = &isp->isp_aewb; struct omap3isp_h3a_aewb_config *aewb_cfg; - struct omap3isp_h3a_aewb_config *aewb_recover_cfg; + struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL; + int ret; - aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL); + aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL); if (!aewb_cfg) return -ENOMEM; @@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp) aewb->isp = isp; /* Set recover state configuration */ - aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg), - GFP_KERNEL); + aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL); if (!aewb_recover_cfg) { dev_err(aewb->isp->dev, "AEWB: cannot allocate memory for " "recover configuration.\n"); - return -ENOMEM; + ret = -ENOMEM; + goto err_release_aewb_cfg; } aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM; @@ -323,13 +324,21 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp) if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) { dev_err(aewb->isp->dev, "AEWB: recover configuration is " "invalid.\n"); - return -EINVAL; + ret = -EINVAL; + goto err_release_aewb_recover_cfg; } aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg); aewb->recover_priv = aewb_recover_cfg; return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops); + +err_release_aewb_recover_cfg: + kfree(aewb_recover_cfg); +err_release_aewb_cfg: + kfree(aewb_cfg); + + return ret; } /* diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c index 92937f7..1919cb2 100644 --- a/drivers/media/platform/omap3isp/isph3a_af.c +++ b/drivers/media/platform/omap3isp/isph3a_af.c @@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp) { struct ispstat *af = &isp->isp_af; struct omap3isp_h3a_af_config *af_cfg; - struct omap3isp_h3a_af_config *af_recover_cfg; + struct omap3isp_h3a_af_config *af_recover_cfg = NULL; + int ret; - af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL); + af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL); if (af_cfg == NULL) return -ENOMEM; @@ -364,12 +365,12 @@ int omap3isp_h3a_af_init(struct isp_device *isp) af->isp = isp; /* Set recover state configuration */ - af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg), - GFP_KERNEL); + af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL); if (!af_recover_cfg) { dev_err(af->isp->dev, "AF: cannot allocate memory for recover " "configuration.\n"); - return -ENOMEM; + ret = -ENOMEM; + goto err_release_af_cfg; } af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN; @@ -381,13 +382,21 @@ int omap3isp_h3a_af_init(struct isp_device *isp) if (h3a_af_validate_params(af, af_recover_cfg)) { dev_err(af->isp->dev, "AF: recover configuration is " "invalid.\n"); - return -EINVAL; + ret = -EINVAL; + goto err_release_af_recover_cfg; } af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg); af->recover_priv = af_recover_cfg; return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops); + +err_release_af_recover_cfg: + kfree(af_recover_cfg); +err_release_af_cfg: + kfree(af_cfg); + + return ret; } void omap3isp_h3a_af_cleanup(struct isp_device *isp) diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c index 7138b04..5d8f04b 100644 --- a/drivers/media/platform/omap3isp/isphist.c +++ b/drivers/media/platform/omap3isp/isphist.c @@ -477,9 +477,9 @@ int omap3isp_hist_init(struct isp_device *isp) { struct ispstat *hist = &isp->isp_hist; struct omap3isp_hist_config *hist_cfg; - int ret = -1; + int ret = 0; - hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL); + hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL); if (hist_cfg == NULL) return -ENOMEM; @@ -517,6 +517,7 @@ int omap3isp_hist_init(struct isp_device *isp) if (ret) { if (hist->dma_ch) dma_release_channel(hist->dma_ch); + kfree(hist_cfg); } return ret;
Using devm resources that have external dependencies such as a dev for a file handler could result in devm resources getting released durin unbind while an application has the file open holding pointer to the devm resource. This results in use-after-free errors when the application exits. Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/media/platform/omap3isp/isp.c | 71 +++++++++++++++++++-------- drivers/media/platform/omap3isp/ispccp2.c | 10 +++- drivers/media/platform/omap3isp/isph3a_aewb.c | 21 +++++--- drivers/media/platform/omap3isp/isph3a_af.c | 21 +++++--- drivers/media/platform/omap3isp/isphist.c | 5 +- 5 files changed, 92 insertions(+), 36 deletions(-)