| Message ID | 20221206075512.812389-1-linmq006@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: Fix memory leak in msm_mdss_parse_data_bus_icc_path | expand |
Hi, On Mon, Dec 5, 2022 at 11:55 PM Miaoqian Lin <linmq006@gmail.com> wrote: > > of_icc_get() alloc resources for path1, we should release it when not > need anymore. Early return when IS_ERR_OR_NULL(path0) may leak path1. > Add icc_put(path1) in the error path to fix this. > > Fixes: b9364eed9232 ("drm/msm/dpu: Move min BW request and full BW disable back to mdss") > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > --- > drivers/gpu/drm/msm/msm_mdss.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > index e13c5c12b775..a38fa9a9a3d6 100644 > --- a/drivers/gpu/drm/msm/msm_mdss.c > +++ b/drivers/gpu/drm/msm/msm_mdss.c > @@ -49,8 +49,10 @@ static int msm_mdss_parse_data_bus_icc_path(struct device *dev, > struct icc_path *path0 = of_icc_get(dev, "mdp0-mem"); > struct icc_path *path1 = of_icc_get(dev, "mdp1-mem"); > > - if (IS_ERR_OR_NULL(path0)) > + if (IS_ERR_OR_NULL(path0)) { > + icc_put(path1); > return PTR_ERR_OR_ZERO(path0); > + } > > msm_mdss->path[0] = path0; > msm_mdss->num_paths = 1; Hmmm. I guess the original author of the code (which wasn't me--I just restored the code that was deleted by a previous change) was assuming that if mdp0-mem had a problem that mdp1-mem would also have a problem. That would mean that you wouldn't need to call icc_put() on it. ...and, in fact, your patch doesn't handle that case, does it? If _both_ of the two are error or NULL then you'll be calling icc_put() on something invalid. I guess icc_put() handles those cases without crashing but it will give a WARN_ON() splat if it happens to be an error... Really, there's a better solution anyway. Instead, you should do: path0 = of_icc_get(dev, "mdp0-mem"); if (IS_ERR_OR_NULL(path0)) return PTR_ERR_OR_ZERO(path0); msm_mdss->path[0] = path0; msm_mdss->num_paths = 1; path1 = of_icc_get(dev, "mdp1-mem"); if (!IS_ERR_OR_NULL(path1)) { ... } In other words just defer getting path1 until after you've checked path0 for an error. -Doug
diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c index e13c5c12b775..a38fa9a9a3d6 100644 --- a/drivers/gpu/drm/msm/msm_mdss.c +++ b/drivers/gpu/drm/msm/msm_mdss.c @@ -49,8 +49,10 @@ static int msm_mdss_parse_data_bus_icc_path(struct device *dev, struct icc_path *path0 = of_icc_get(dev, "mdp0-mem"); struct icc_path *path1 = of_icc_get(dev, "mdp1-mem"); - if (IS_ERR_OR_NULL(path0)) + if (IS_ERR_OR_NULL(path0)) { + icc_put(path1); return PTR_ERR_OR_ZERO(path0); + } msm_mdss->path[0] = path0; msm_mdss->num_paths = 1;
of_icc_get() alloc resources for path1, we should release it when not need anymore. Early return when IS_ERR_OR_NULL(path0) may leak path1. Add icc_put(path1) in the error path to fix this. Fixes: b9364eed9232 ("drm/msm/dpu: Move min BW request and full BW disable back to mdss") Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- drivers/gpu/drm/msm/msm_mdss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)