be->mode is obtained from xenbus_read, which does a kmalloc for the message body. The short string is never released, so do it on blkbk remove. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- !! Not compile tested !! drivers/block/xen-blkback/xenbus.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c index f58434c..a6585a4 100644 --- a/drivers/block/xen-blkback/xenbus.c +++ b/drivers/block/xen-blkback/xenbus.c @@ -366,6 +366,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev) be->blkif = NULL; } + kfree(be->mode); kfree(be); dev_set_drvdata(&dev->dev, NULL); return 0; -- 1.8.0.1
>>> On 03.12.12 at 20:32, Olaf Hering <olaf@aepfle.de> wrote: > be->mode is obtained from xenbus_read, which does a kmalloc for the > message body. The short string is never released, so do it on blkbk > remove. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > --- > > !! Not compile tested !! > > drivers/block/xen-blkback/xenbus.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/block/xen-blkback/xenbus.c > b/drivers/block/xen-blkback/xenbus.c > index f58434c..a6585a4 100644 > --- a/drivers/block/xen-blkback/xenbus.c > +++ b/drivers/block/xen-blkback/xenbus.c > @@ -366,6 +366,7 @@ static int xen_blkbk_remove(struct xenbus_device *dev) > be->blkif = NULL; > } > > + kfree(be->mode);This looks necessary but insufficient - there''s nothing really preventing backend_changed() from being called more than once for a given device (is simply the handler of xenbus watch). Hence I think either that function needs to be guarded against multiple execution (e.g. by removing the watch from that function itself, if that''s permitted by xenbus), or to properly deal with the effects this has (including but probably not limited to the leaking of be->mode). Jan> kfree(be); > dev_set_drvdata(&dev->dev, NULL); > return 0; > -- > 1.8.0.1
On Tue, Dec 04, Jan Beulich wrote:> This looks necessary but insufficient - there''s nothing really > preventing backend_changed() from being called more than once > for a given device (is simply the handler of xenbus watch). Hence > I think either that function needs to be guarded against multiple > execution (e.g. by removing the watch from that function itself, > if that''s permitted by xenbus), or to properly deal with the > effects this has (including but probably not limited to the leaking > of be->mode).If another watch does really trigger after the kfree(be) in xen_blkbk_remove(), wouldnt backend_changed access stale memory? So if that can really happen in practice, shouldnt the backend_watch be a separate allocation instead being contained within backend_info? Looking at unregister_xenbus_watch, it clears removes the watch from the list, so that process_msg will not see it anymore. Olaf
>>> On 04.12.12 at 19:21, Olaf Hering <olaf@aepfle.de> wrote: > On Tue, Dec 04, Jan Beulich wrote: > >> This looks necessary but insufficient - there''s nothing really >> preventing backend_changed() from being called more than once >> for a given device (is simply the handler of xenbus watch). Hence >> I think either that function needs to be guarded against multiple >> execution (e.g. by removing the watch from that function itself, >> if that''s permitted by xenbus), or to properly deal with the >> effects this has (including but probably not limited to the leaking >> of be->mode). > > If another watch does really trigger after the kfree(be) in > xen_blkbk_remove(), wouldnt backend_changed access stale memory? > So if that can really happen in practice, shouldnt the backend_watch be > a separate allocation instead being contained within backend_info? > > Looking at unregister_xenbus_watch, it clears removes the watch from the > list, so that process_msg will not see it anymore.That''s not the scenario I was talking about: I''m concerned about multiple calls to backend_changed() to similarly leak "mode" (and possibly cause other bad stuff to happen) while the device is still alive - after all it overwrites "mode" without checking what''s in there. Jan