Message ID | 20200325123843.47452-1-yuyufen@huawei.com (mailing list archive) |
---|---|
Headers | show |
Series | bdi: fix use-after-free for bdi device | expand |
ping On 2020/3/25 20:38, Yufen Yu wrote: > Hi, all > > We have reported a use-after-free crash for bdi device in __blkg_prfill_rwstat(). > The bug is caused by printing device kobj->name while the device and kobj->name > has been freed by bdi_unregister(). > > In fact, commit 68f23b8906 "memcg: fix a crash in wb_workfn when a device disappears" > has tried to address the issue, but the code is till somewhat racy after that commit. > > In this patchset, we try to protect bdi->dev with spinlock and copy device name > into caller buffer, avoiding use-after-free. > > V4: > * Fix coding error in bdi_get_dev_name() > * Write one patch for each broken caller > > V3: > https://www.spinics.net/lists/linux-block/msg51111.html > Use spinlock to protect bdi->dev and copy device name into caller buffer > > V2: > https://www.spinics.net/lists/linux-fsdevel/msg163206.html > Try to protect device lifetime with RCU. > > V1: > https://www.spinics.net/lists/linux-block/msg49693.html > Add a new spinlock and copy kobj->name into caller buffer. > Or using synchronize_rcu() to wait until reader complete. > > Yufen Yu (6): > bdi: use bdi_dev_name() to get device name > bdi: protect bdi->dev with spinlock > bfq: fix potential kernel crash when print error info > memcg: fix crash in wb_workfn when bdi unregister > blk-wbt: replace bdi_dev_name() with bdi_get_dev_name() > blkcg: fix use-after-free for bdi->dev > > block/bfq-iosched.c | 6 +++-- > block/blk-cgroup-rwstat.c | 6 +++-- > block/blk-cgroup.c | 19 +++++----------- > block/blk-iocost.c | 14 +++++++----- > block/blk-iolatency.c | 5 +++-- > block/blk-throttle.c | 6 +++-- > fs/ceph/debugfs.c | 2 +- > fs/fs-writeback.c | 4 +++- > include/linux/backing-dev-defs.h | 1 + > include/linux/backing-dev.h | 26 ++++++++++++++++++++++ > include/linux/blk-cgroup.h | 1 - > include/trace/events/wbt.h | 8 +++---- > include/trace/events/writeback.h | 38 ++++++++++++++------------------ > mm/backing-dev.c | 9 ++++++-- > 14 files changed, 88 insertions(+), 57 deletions(-) >
On Thu, Apr 09, 2020 at 09:28:07PM +0800, Yufen Yu wrote: > ping ping**2 Can this go in as a bugfix during this cycle? - Ted > > On 2020/3/25 20:38, Yufen Yu wrote: > > Hi, all > > > > We have reported a use-after-free crash for bdi device in __blkg_prfill_rwstat(). > > The bug is caused by printing device kobj->name while the device and kobj->name > > has been freed by bdi_unregister(). > > > > In fact, commit 68f23b8906 "memcg: fix a crash in wb_workfn when a device disappears" > > has tried to address the issue, but the code is till somewhat racy after that commit. > > > > In this patchset, we try to protect bdi->dev with spinlock and copy device name > > into caller buffer, avoiding use-after-free. > > > > V4: > > * Fix coding error in bdi_get_dev_name() > > * Write one patch for each broken caller > > > > V3: > > https://www.spinics.net/lists/linux-block/msg51111.html > > Use spinlock to protect bdi->dev and copy device name into caller buffer > > > > V2: > > https://www.spinics.net/lists/linux-fsdevel/msg163206.html > > Try to protect device lifetime with RCU. > > > > V1: > > https://www.spinics.net/lists/linux-block/msg49693.html > > Add a new spinlock and copy kobj->name into caller buffer. > > Or using synchronize_rcu() to wait until reader complete. > > > > Yufen Yu (6): > > bdi: use bdi_dev_name() to get device name > > bdi: protect bdi->dev with spinlock > > bfq: fix potential kernel crash when print error info > > memcg: fix crash in wb_workfn when bdi unregister > > blk-wbt: replace bdi_dev_name() with bdi_get_dev_name() > > blkcg: fix use-after-free for bdi->dev > > > > block/bfq-iosched.c | 6 +++-- > > block/blk-cgroup-rwstat.c | 6 +++-- > > block/blk-cgroup.c | 19 +++++----------- > > block/blk-iocost.c | 14 +++++++----- > > block/blk-iolatency.c | 5 +++-- > > block/blk-throttle.c | 6 +++-- > > fs/ceph/debugfs.c | 2 +- > > fs/fs-writeback.c | 4 +++- > > include/linux/backing-dev-defs.h | 1 + > > include/linux/backing-dev.h | 26 ++++++++++++++++++++++ > > include/linux/blk-cgroup.h | 1 - > > include/trace/events/wbt.h | 8 +++---- > > include/trace/events/writeback.h | 38 ++++++++++++++------------------ > > mm/backing-dev.c | 9 ++++++-- > > 14 files changed, 88 insertions(+), 57 deletions(-) > >
Looking through this series the whoe approach of using a lock to clear the ->dev pointer looks rather odd to me. What is the reason for now simply adding a separately allocated name field to struct backing_dev_info that the name is copied to on allocation, and then the ->dev field is not relevant for name printing and we don't need to copy out the name in the potentionally more fast path callers that want to print it?
On Tue 14-04-20 08:52:28, Christoph Hellwig wrote: > Looking through this series the whoe approach of using a lock to clear > the ->dev pointer looks rather odd to me. What is the reason for now > simply adding a separately allocated name field to struct > backing_dev_info that the name is copied to on allocation, and then > the ->dev field is not relevant for name printing and we don't need > to copy out the name in the potentionally more fast path callers that > want to print it? Yeah, that's what I was suggesting as well [1] - especially since we already have bdi->name with a dubious value (but looking into it now, we would need a separate dev_name field since bdi->name is visible in sysfs so we cannot change that). But Yufen explained to me that this could result in bogus name being reported when bdi gets re-registered. Not sure if that's serious enough but it could happen... Honza [1] https://lore.kernel.org/linux-block/20200219125505.GP16121@quack2.suse.cz
On Wed, Apr 15, 2020 at 11:34:59AM +0200, Jan Kara wrote: > Yeah, that's what I was suggesting as well [1] - especially since we > already have bdi->name with a dubious value (but looking into it now, we > would need a separate dev_name field since bdi->name is visible in sysfs so > we cannot change that). That is a little anoying, but not the end of the world. > But Yufen explained to me that this could result in > bogus name being reported when bdi gets re-registered. Not sure if that's > serious enough but it could happen... I don't think that is a problem at all. If it is a problem we can just replace the ->dev_name pointer with one that says "(unregistered)" at unregister time, but to me that seems worse than just keeping the name around.