| Message ID | 20200303172629.21339-1-bparrot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti-vpe: cal: fix a kernel oops when unloading module | expand |
On 03/03/2020 19:26, Benoit Parrot wrote: > After the switch to use v4l2_async_notifier_add_subdev() and > v4l2_async_notifier_cleanup(), unloading the ti_cal module would casue a > kernel oops. > > This was root cause to the fact that v4l2_async_notifier_cleanup() tries > to kfree the asd pointer passed into v4l2_async_notifier_add_subdev(). > > In our case the asd reference was from a statically allocated struct. > So in effect v4l2_async_notifier_cleanup() was trying to free a pointer > that was not kalloc. > > So here we switch to using a kzalloc struct instead of a static one. > > Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev") > > Cc: stable@vger.kernel.org > Signed-off-by: Benoit Parrot <bparrot@ti.com> > --- > drivers/media/platform/ti-vpe/cal.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 6d4cbb8782ed..18fe2cb9dd17 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -372,8 +372,6 @@ struct cal_ctx { > struct v4l2_subdev *sensor; > struct v4l2_fwnode_endpoint endpoint; > > - struct v4l2_async_subdev asd; > - > struct v4l2_fh fh; > struct cal_dev *dev; > struct cc_data *cc; > @@ -2032,7 +2030,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) > > parent = pdev->dev.of_node; > > - asd = &ctx->asd; > endpoint = &ctx->endpoint; > > ep_node = NULL; > @@ -2040,6 +2037,10 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) > sensor_node = NULL; > ret = -EINVAL; > > + asd = kzalloc(sizeof(*asd), GFP_KERNEL); > + if (!asd) > + goto cleanup_exit; > + > ctx_dbg(3, ctx, "Scanning Port node for csi2 port: %d\n", inst); > for (index = 0; index < CAL_NUM_CSI2_PORTS; index++) { > port = of_get_next_port(parent, port); > Thanks, this fixes the crash for me. It does look a bit odd that something is allocated with kzalloc, and then it's freed somewhere inside v4l2_async_notifier_cleanup, though. But if that's how it supposed to be used, looks fine to me. Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Tomi
On 04/03/2020 10:40, Tomi Valkeinen wrote: > On 03/03/2020 19:26, Benoit Parrot wrote: >> After the switch to use v4l2_async_notifier_add_subdev() and >> v4l2_async_notifier_cleanup(), unloading the ti_cal module would casue a >> kernel oops. >> >> This was root cause to the fact that v4l2_async_notifier_cleanup() tries >> to kfree the asd pointer passed into v4l2_async_notifier_add_subdev(). >> >> In our case the asd reference was from a statically allocated struct. >> So in effect v4l2_async_notifier_cleanup() was trying to free a pointer >> that was not kalloc. >> >> So here we switch to using a kzalloc struct instead of a static one. >> >> Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev") >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Benoit Parrot <bparrot@ti.com> >> --- >> drivers/media/platform/ti-vpe/cal.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c >> index 6d4cbb8782ed..18fe2cb9dd17 100644 >> --- a/drivers/media/platform/ti-vpe/cal.c >> +++ b/drivers/media/platform/ti-vpe/cal.c >> @@ -372,8 +372,6 @@ struct cal_ctx { >> struct v4l2_subdev *sensor; >> struct v4l2_fwnode_endpoint endpoint; >> - struct v4l2_async_subdev asd; >> - >> struct v4l2_fh fh; >> struct cal_dev *dev; >> struct cc_data *cc; >> @@ -2032,7 +2030,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) >> parent = pdev->dev.of_node; >> - asd = &ctx->asd; >> endpoint = &ctx->endpoint; >> ep_node = NULL; >> @@ -2040,6 +2037,10 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) >> sensor_node = NULL; >> ret = -EINVAL; >> + asd = kzalloc(sizeof(*asd), GFP_KERNEL); >> + if (!asd) >> + goto cleanup_exit; >> + >> ctx_dbg(3, ctx, "Scanning Port node for csi2 port: %d\n", inst); >> for (index = 0; index < CAL_NUM_CSI2_PORTS; index++) { >> port = of_get_next_port(parent, port); >> > > Thanks, this fixes the crash for me. > > It does look a bit odd that something is allocated with kzalloc, and then it's freed somewhere > inside v4l2_async_notifier_cleanup, though. But if that's how it supposed to be used, looks fine to me. Well, sent that a few seconds too early... With this patch, I see kmemleaks. Tomi
On 04/03/2020 10:41, Tomi Valkeinen wrote: >> Thanks, this fixes the crash for me. >> >> It does look a bit odd that something is allocated with kzalloc, and then it's freed somewhere >> inside v4l2_async_notifier_cleanup, though. But if that's how it supposed to be used, looks fine >> to me. > > Well, sent that a few seconds too early... With this patch, I see kmemleaks. This is caused by allocating asd for all ports, even if the port is not used, causing the allocated asd to be forgotten. Also, any error there would cause leak too. I think something like this fixes both the unused port case and error paths: diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index a928c9d66d5d..4b89dd53d2b4 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -372,8 +372,6 @@ struct cal_ctx { struct v4l2_subdev *sensor; struct v4l2_fwnode_endpoint endpoint; - struct v4l2_async_subdev asd; - struct v4l2_fh fh; struct cal_dev *dev; struct cc_data *cc; @@ -2020,7 +2018,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) parent = pdev->dev.of_node; - asd = &ctx->asd; endpoint = &ctx->endpoint; ep_node = NULL; @@ -2067,8 +2064,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) ctx_dbg(3, ctx, "can't get remote parent\n"); goto cleanup_exit; } - asd->match_type = V4L2_ASYNC_MATCH_FWNODE; - asd->match.fwnode = of_fwnode_handle(sensor_node); v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_node), endpoint); @@ -2098,9 +2093,17 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) v4l2_async_notifier_init(&ctx->notifier); + asd = kzalloc(sizeof(*asd), GFP_KERNEL); + if (!asd) + goto cleanup_exit; + + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; + asd->match.fwnode = of_fwnode_handle(sensor_node); + ret = v4l2_async_notifier_add_subdev(&ctx->notifier, asd); if (ret) { ctx_err(ctx, "Error adding asd\n"); + kfree(asd); goto cleanup_exit; } Tomi
Tomi, Thanks for the review, and fix! Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2020-Mar-04 11:22:26 +0200]: > On 04/03/2020 10:41, Tomi Valkeinen wrote: > > >> Thanks, this fixes the crash for me. > >> > >> It does look a bit odd that something is allocated with kzalloc, and then it's freed somewhere > >> inside v4l2_async_notifier_cleanup, though. But if that's how it supposed to be used, looks fine > >> to me. > > > > Well, sent that a few seconds too early... With this patch, I see kmemleaks. > > This is caused by allocating asd for all ports, even if the port is not used, causing the allocated asd to be forgotten. Also, any error there would cause leak too. > > I think something like this fixes both the unused port case and error paths: > Yes I see that now. Good catch. I'll fix it in v2. Benoit > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index a928c9d66d5d..4b89dd53d2b4 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -372,8 +372,6 @@ struct cal_ctx { > struct v4l2_subdev *sensor; > struct v4l2_fwnode_endpoint endpoint; > > - struct v4l2_async_subdev asd; > - > struct v4l2_fh fh; > struct cal_dev *dev; > struct cc_data *cc; > @@ -2020,7 +2018,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) > > parent = pdev->dev.of_node; > > - asd = &ctx->asd; > endpoint = &ctx->endpoint; > > ep_node = NULL; > @@ -2067,8 +2064,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) > ctx_dbg(3, ctx, "can't get remote parent\n"); > goto cleanup_exit; > } > - asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > - asd->match.fwnode = of_fwnode_handle(sensor_node); > > v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_node), endpoint); > > @@ -2098,9 +2093,17 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) > > v4l2_async_notifier_init(&ctx->notifier); > > + asd = kzalloc(sizeof(*asd), GFP_KERNEL); > + if (!asd) > + goto cleanup_exit; > + > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > + asd->match.fwnode = of_fwnode_handle(sensor_node); > + > ret = v4l2_async_notifier_add_subdev(&ctx->notifier, asd); > if (ret) { > ctx_err(ctx, "Error adding asd\n"); > + kfree(asd); > goto cleanup_exit; > } > > Tomi > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index 6d4cbb8782ed..18fe2cb9dd17 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -372,8 +372,6 @@ struct cal_ctx { struct v4l2_subdev *sensor; struct v4l2_fwnode_endpoint endpoint; - struct v4l2_async_subdev asd; - struct v4l2_fh fh; struct cal_dev *dev; struct cc_data *cc; @@ -2032,7 +2030,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) parent = pdev->dev.of_node; - asd = &ctx->asd; endpoint = &ctx->endpoint; ep_node = NULL; @@ -2040,6 +2037,10 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst) sensor_node = NULL; ret = -EINVAL; + asd = kzalloc(sizeof(*asd), GFP_KERNEL); + if (!asd) + goto cleanup_exit; + ctx_dbg(3, ctx, "Scanning Port node for csi2 port: %d\n", inst); for (index = 0; index < CAL_NUM_CSI2_PORTS; index++) { port = of_get_next_port(parent, port);
After the switch to use v4l2_async_notifier_add_subdev() and v4l2_async_notifier_cleanup(), unloading the ti_cal module would casue a kernel oops. This was root cause to the fact that v4l2_async_notifier_cleanup() tries to kfree the asd pointer passed into v4l2_async_notifier_add_subdev(). In our case the asd reference was from a statically allocated struct. So in effect v4l2_async_notifier_cleanup() was trying to free a pointer that was not kalloc. So here we switch to using a kzalloc struct instead of a static one. Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev") Cc: stable@vger.kernel.org Signed-off-by: Benoit Parrot <bparrot@ti.com> --- drivers/media/platform/ti-vpe/cal.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)