lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Fri, 25 Sep 2009 06:16:33 +0200
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Zdenek Kabelac <zdenek.kabelac@...il.com>
Cc:	Li Zefan <lizf@...fujitsu.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	mbroz@...hat.com, tytso@....edu, mingo@...e.hu
Subject: Re: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with
	blk_trace_init_sysfs

On Thu, Sep 24 2009, Zdenek Kabelac wrote:
> 2009/9/24 Li Zefan <lizf@...fujitsu.com>:
> >>>> Subject: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with
> >>>> blk_trace_init_sysfs
> >>>> From: Zdenek Kabelac <zkabelac@...hat.com>
> >>>>
> >>>> Adds missing blk_trace_remove_sysfs() to be in pair with
> >>>> blk_trace_init_sysfs() introduced in commit
> >>>> 1d54ad6da9192fed5dd3b60224d9f2dfea0dcd82.
> >>>>
> >>>> Problem was noticed via kmemleak backtrace when some sysfs entries
> >>>> were note properly destroyed during  device removal:
> >>>>
> >>> Thanks for reporting and fixing this!
> >>>
> >>>> @@ -465,6 +466,7 @@ void blk_unregister_queue(struct gendisk *disk)
> >>>>
> >>>>               kobject_uevent(&q->kobj, KOBJ_REMOVE);
> >>>>               kobject_del(&q->kobj);
> >>>> +             blk_trace_remove_sysfs(disk_to_dev(disk));
> >>> This should be moved outside of 'if'.
> >>>
> >>
> >> I was not really sure about the proper place - if it could be placed
> >> before if() or after the if(){} - as I've not checked in depth
> >
> > Just use the reverse order against blk_register_queue() should be fine.
> 
> Yes - I think the order of release is correct.
> 
> >
> >> connection between kobj and sysfs. It's somewhat unclear why all the
> >> kobject operation are only within this if(){} block - so I've thought
> >> there is some reason...
> >> IMHO only elv_unregister_queue() should be probably in the if(){} block.
> >>
> >
> > Seems it's a bug to put kobject_put(dev->kobj) in the if block.
> >
> > I created a stacked device (mdadm) and kmemleak still reported leaks
> > even after I fixed the blktrace issue. And then I moved kobejct_put()
> > outside the if, no more leaks reports.
> >
> 
> Ok - I didn't have a testcase where the request_fn would be NULL.
> So in this case I propose this patch:
> 
> ---
> Add missing blk_trace_remove_sysfs to be in pair with blk_trace_init_sysfs
> introduced in commit 1d54ad6da9192fed5dd3b60224d9f2dfea0dcd82.
> Release kobject also in case the request_fn is NULL.
> 
> Problem was noticed via kmemleak backtrace when some sysfs entries were
> note properly destroyed during  device removal:

Thanks, this looks good now, applied.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ