Re: [PATCH 3/3] drivers/media/media-device: fix double free bug in _unregister()
On 06/16/2016 03:29 AM, Max Kellermann wrote: > (Shuah, I did not receive your second reply; I only found it in an > email archive.) > >> Yes media_devnode_create() creates the interfaces links and these >> links are deleted by media_devnode_remove(). >> media_device_unregister() still needs to delete the interfaces >> links. The reason for that is the API dynalic use-case. >> >> Drivers (other than dvb-core and v4l2-core) can create and delete >> media devnode interfaces during run-time > > My point was that they do not. There are no other > media_devnode_create() callers. Correct. There are none in the base now. However as I explained the dynamic use-case. There is work in progress that uses this feature in the API. > >> So removing kfree() from media_device_unregister() isn't the correct >> fix. > > Then what is? I don't know anything other than the (mostly > undocumented) code I read, and my patch implements the design that I > interpreted from the code. Apparently my interpretation of the design > is wrong after all. > >> I don't see the stack trace for the double free error you are >> seeing? > > Actually, it didn't crash at the double free; it hung forever because > it tried to lock a mutex which was already stale. I don't have a > stack trace of that; would it help to produce one? I think you are running into another set of problems related to media devnode, cdev, and race between media ioctl/syscall and media unregister sequence. These patches are in git://linuxtv.org/media_tree.git master branch > >> Could it be that there is a driver problem in the order in which it >> is calling media_device_unregister()? > > Maybe it's due to my patch 1/3 which adds a kref, and it only occurs > if one process still has a file handle. So you are adding another refcounted object to the mix, in addition to media_device, media_devnode, and cdev. Now you have three or more objects with varying lifetimes. Not a good situation to be in. > > In any case, the kernel must decide who's responsible for freeing the > object, and how the dvbdev.c library gets to know that its pointer has > been invalidated. Yes it does that. intf links need to be free'd in both cases, one when driver does a devnode_remove() and then when unregister is done. There could be two drivers that are bound to the media hardware and both might own their own sections of the media graph. Media Controller core has to allow the possibility of one driver unbind/rmmod and be able to delete the devnode it created. I don't think the problem you are running into is due to this code path. Without seeing the stack trace, it is hard to debug as you really don't know what the problem is, leave alone being able to fix it. thanks, -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] drivers/media/media-device: fix double free bug in _unregister()
(Shuah, I did not receive your second reply; I only found it in an email archive.) > Yes media_devnode_create() creates the interfaces links and these > links are deleted by media_devnode_remove(). > media_device_unregister() still needs to delete the interfaces > links. The reason for that is the API dynalic use-case. > > Drivers (other than dvb-core and v4l2-core) can create and delete > media devnode interfaces during run-time My point was that they do not. There are no other media_devnode_create() callers. > So removing kfree() from media_device_unregister() isn't the correct > fix. Then what is? I don't know anything other than the (mostly undocumented) code I read, and my patch implements the design that I interpreted from the code. Apparently my interpretation of the design is wrong after all. > I don't see the stack trace for the double free error you are > seeing? Actually, it didn't crash at the double free; it hung forever because it tried to lock a mutex which was already stale. I don't have a stack trace of that; would it help to produce one? > Could it be that there is a driver problem in the order in which it > is calling media_device_unregister()? Maybe it's due to my patch 1/3 which adds a kref, and it only occurs if one process still has a file handle. In any case, the kernel must decide who's responsible for freeing the object, and how the dvbdev.c library gets to know that its pointer has been invalidated. Please explain how it should be done, and I'll try to adapt my patches to the "grand design". -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] drivers/media/media-device: fix double free bug in _unregister()
On 06/15/2016 02:37 PM, Max Kellermann wrote: > On 2016/06/15 22:32, Shuah Khanwrote: >> This change introduces memory leaks, since drivers are relying on >> media_device_unregister() to free interfaces. > > This is what I thought, too, until I checked the code paths. Who adds > entries to that list? Only media_gobj_create() does, and only when > type==MEDIA_GRAPH_INTF_DEVNODE. That is called via > media_interface_init(), via media_devnode_create(). > > In the whole kernel, there are two calls to media_devnode_create(): > one in dvbdev.c and another one in v4l2-dev.c. Both callers take care > for freeing their interface. Both would crash if somebody else would > free it for them before they get a chance to do it. Which is the very > thing my patch addresses. > > Did I miss something? > Yes media_devnode_create() creates the interfaces links and these links are deleted by media_devnode_remove(). media_device_unregister() still needs to delete the interfaces links. The reason for that is the API dynalic use-case. Drivers (other than dvb-core and v4l2-core) can create and delete media devnode interfaces during run-time, hence media_devnode_remove() has to call media_remove_intf_links(). However, driver isn't required to call media_devnode_remove() and media-core can't enforce that. So it is safe for media_device_unregister() to remove interface links if the list isn't empty. If driver does delete them, media_device_unregister() has nothing to do since the list is going to be empty. So removing kfree() from media_device_unregister() isn't the correct fix. I don't see the stack trace for the double free error you are seeing? Could it be that there is a driver problem in the order in which it is calling media_device_unregister()? thanks, -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] drivers/media/media-device: fix double free bug in _unregister()
On 2016/06/15 22:32, Shuah Khanwrote: > This change introduces memory leaks, since drivers are relying on > media_device_unregister() to free interfaces. This is what I thought, too, until I checked the code paths. Who adds entries to that list? Only media_gobj_create() does, and only when type==MEDIA_GRAPH_INTF_DEVNODE. That is called via media_interface_init(), via media_devnode_create(). In the whole kernel, there are two calls to media_devnode_create(): one in dvbdev.c and another one in v4l2-dev.c. Both callers take care for freeing their interface. Both would crash if somebody else would free it for them before they get a chance to do it. Which is the very thing my patch addresses. Did I miss something? Max -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] drivers/media/media-device: fix double free bug in _unregister()
On 06/15/2016 02:15 PM, Max Kellermann wrote: > While removing all interfaces in media_device_unregister(), all > media_interface pointers are freed. This is illegal and results in > double kfree() if any media_interface is still linked at this point; > maybe because a userspace process still has a file handle. Once the > process closes the file handle, dvb_media_device_free() gets called, > which frees the dvb_device.intf_devnode again. > > This patch removes the unnecessary kfree() call, and documents who's > responsible for really freeing it. > > Signed-off-by: Max Kellermann> --- > drivers/media/media-device.c |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index 33a9952..1db4707 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -799,9 +799,11 @@ void media_device_unregister(struct media_device *mdev) > /* Remove all interfaces from the media device */ > list_for_each_entry_safe(intf, tmp_intf, >interfaces, >graph_obj.list) { > + /* unlink the interface, but don't free it here; the > +module which created it is responsible for freeing > +it */ > __media_remove_intf_links(intf); > media_gobj_destroy(>graph_obj); > - kfree(intf); This change introduces memory leaks, since drivers are relying on media_device_unregister() to free interfaces. thanks, -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] drivers/media/media-device: fix double free bug in _unregister()
While removing all interfaces in media_device_unregister(), all media_interface pointers are freed. This is illegal and results in double kfree() if any media_interface is still linked at this point; maybe because a userspace process still has a file handle. Once the process closes the file handle, dvb_media_device_free() gets called, which frees the dvb_device.intf_devnode again. This patch removes the unnecessary kfree() call, and documents who's responsible for really freeing it. Signed-off-by: Max Kellermann--- drivers/media/media-device.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 33a9952..1db4707 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -799,9 +799,11 @@ void media_device_unregister(struct media_device *mdev) /* Remove all interfaces from the media device */ list_for_each_entry_safe(intf, tmp_intf, >interfaces, graph_obj.list) { + /* unlink the interface, but don't free it here; the + module which created it is responsible for freeing + it */ __media_remove_intf_links(intf); media_gobj_destroy(>graph_obj); - kfree(intf); } mutex_unlock(>graph_mutex); -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html