linux-kernel - Re: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with blk_trace_init

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]

Message-ID: <20090925041633.GQ23126@kernel.dk>
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/