| Message ID | 20190305095847.21428-5-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various core and virtual driver fixes | expand |
Hi Hans, Thank you for the patch. On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Ensure that this pointer is set to NULL after it is freed. > The vimc driver has a static media_entity and after > unbinding and rebinding the vimc device the media code will > try to free this pointer again since it wasn't set to NULL. I still think the problem lies in the vimc driver. Reusing static structures is really asking for trouble. I'm however not opposed to merging this patch, as you mentioned the problem will be fixed in vimc too. I still wonder, though, if merging this couldn't make it easier for drivers to do the wrong thing. > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/media-entity.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c > index 0b1cb3559140..7b2a2cc95530 100644 > --- a/drivers/media/media-entity.c > +++ b/drivers/media/media-entity.c > @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init); > void media_entity_enum_cleanup(struct media_entity_enum *ent_enum) > { > kfree(ent_enum->bmap); > + ent_enum->bmap = NULL; > } > EXPORT_SYMBOL_GPL(media_entity_enum_cleanup); >
On 3/5/19 8:39 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote: >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> Ensure that this pointer is set to NULL after it is freed. >> The vimc driver has a static media_entity and after >> unbinding and rebinding the vimc device the media code will >> try to free this pointer again since it wasn't set to NULL. > > I still think the problem lies in the vimc driver. Reusing static > structures is really asking for trouble. I'm however not opposed to > merging this patch, as you mentioned the problem will be fixed in vimc > too. I still wonder, though, if merging this couldn't make it easier for > drivers to do the wrong thing. I'm keeping this patch :-) I don't think that what vimc is doing is wrong in principle, just very unusual. Also I think it makes the mc framework more robust by properly zeroing this pointer. Regards, Hans > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/media-entity.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c >> index 0b1cb3559140..7b2a2cc95530 100644 >> --- a/drivers/media/media-entity.c >> +++ b/drivers/media/media-entity.c >> @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init); >> void media_entity_enum_cleanup(struct media_entity_enum *ent_enum) >> { >> kfree(ent_enum->bmap); >> + ent_enum->bmap = NULL; >> } >> EXPORT_SYMBOL_GPL(media_entity_enum_cleanup); >> >
Hi Hans, On Thu, Mar 07, 2019 at 10:23:03AM +0100, Hans Verkuil wrote: > On 3/5/19 8:39 PM, Laurent Pinchart wrote: > > On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote: > >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> > >> Ensure that this pointer is set to NULL after it is freed. > >> The vimc driver has a static media_entity and after > >> unbinding and rebinding the vimc device the media code will > >> try to free this pointer again since it wasn't set to NULL. > > > > I still think the problem lies in the vimc driver. Reusing static > > structures is really asking for trouble. I'm however not opposed to > > merging this patch, as you mentioned the problem will be fixed in vimc > > too. I still wonder, though, if merging this couldn't make it easier for > > drivers to do the wrong thing. > > I'm keeping this patch :-) > > I don't think that what vimc is doing is wrong in principle, just very unusual. I disagree here. We've developed the media controller (and V4L2) core code with many assumptions that structures are zeroed on allocation. For the structures that are meant to be registered once only, the code assumes, explicitly and implicitly, that some of the fields are zeroed. Removing that assumption for the odd case of vimc will require you to chase bugs for a long time. You've caught a few of the easier ones here, I'm sure other will linger for a much longer time before they get fixed. In the vimc case, the best option is to zero the structure manually if you don't want to allocate it dynamically (and I think it should be allocated dynamically). For the record, I ran into a similar problem before when trying to unregister and re-register a struct device. I reported what I considered to be a bug, and Greg very clearly told me it was plain wrong. You will run into similar issues due to the platform_device embedded in struct vimc_device. Let's just allocate it dynamically. > Also I think it makes the mc framework more robust by properly zeroing this > pointer. This patch is not wrong per-se, and I'm not opposed to it, but we should fix issues in drivers, which would render this patch unneeded.
On 3/8/19 12:26 PM, Laurent Pinchart wrote: > Hi Hans, > > On Thu, Mar 07, 2019 at 10:23:03AM +0100, Hans Verkuil wrote: >> On 3/5/19 8:39 PM, Laurent Pinchart wrote: >>> On Tue, Mar 05, 2019 at 10:58:42AM +0100, hverkuil-cisco@xs4all.nl wrote: >>>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>> >>>> Ensure that this pointer is set to NULL after it is freed. >>>> The vimc driver has a static media_entity and after >>>> unbinding and rebinding the vimc device the media code will >>>> try to free this pointer again since it wasn't set to NULL. >>> >>> I still think the problem lies in the vimc driver. Reusing static >>> structures is really asking for trouble. I'm however not opposed to >>> merging this patch, as you mentioned the problem will be fixed in vimc >>> too. I still wonder, though, if merging this couldn't make it easier for >>> drivers to do the wrong thing. >> >> I'm keeping this patch :-) >> >> I don't think that what vimc is doing is wrong in principle, just very unusual. > > I disagree here. We've developed the media controller (and V4L2) core > code with many assumptions that structures are zeroed on allocation. For > the structures that are meant to be registered once only, the code > assumes, explicitly and implicitly, that some of the fields are zeroed. > Removing that assumption for the odd case of vimc will require you to > chase bugs for a long time. You've caught a few of the easier ones here, > I'm sure other will linger for a much longer time before they get fixed. > In the vimc case, the best option is to zero the structure manually if > you don't want to allocate it dynamically (and I think it should be > allocated dynamically). > > For the record, I ran into a similar problem before when trying to > unregister and re-register a struct device. I reported what I considered > to be a bug, and Greg very clearly told me it was plain wrong. You will > run into similar issues due to the platform_device embedded in struct > vimc_device. Let's just allocate it dynamically. > >> Also I think it makes the mc framework more robust by properly zeroing this >> pointer. > > This patch is not wrong per-se, and I'm not opposed to it, but we should > fix issues in drivers, which would render this patch unneeded. > I posted a v4 where I drop this patch and instead zero the global media_device struct in the vimc probe() function. Regards, Hans
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index 0b1cb3559140..7b2a2cc95530 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(__media_entity_enum_init); void media_entity_enum_cleanup(struct media_entity_enum *ent_enum) { kfree(ent_enum->bmap); + ent_enum->bmap = NULL; } EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);