| Message ID | 5432348a53c7ec3fb97d4a21121d435fd3a1be74.1599234146.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups | expand |
On 04/09/2020 19:37, Anand Jain wrote: > The following test case leads to null kobject-being-freed error. > > mount seed /mnt > add sprout to /mnt > umount /mnt > mount sprout to /mnt > delete seed > > kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called. > WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350 > RIP: 0010:kobject_put+0x80/0x350 > :: > Call Trace: > btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs] > btrfs_rm_device.cold+0xa8/0x298 [btrfs] > btrfs_ioctl+0x206c/0x22a0 [btrfs] > ksys_ioctl+0xe2/0x140 > __x64_sys_ioctl+0x1e/0x29 > do_syscall_64+0x96/0x150 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > RIP: 0033:0x7f4047c6288b > :: > > This is because, at the end of the seed device-delete, we try to remove > the seed's devid sysfs entry. But for the seed devices under the sprout > fs, we don't initialize the devid kobject yet. So add a kobject state > check, which takes care of the Warning. > > Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/sysfs.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > index 190e59152be5..438a367371c4 100644 > --- a/fs/btrfs/sysfs.c > +++ b/fs/btrfs/sysfs.c > @@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, > disk_kobj->name); > } > > - kobject_del(&one_device->devid_kobj); > - kobject_put(&one_device->devid_kobj); > + if (one_device->devid_kobj.state_initialized) { > + kobject_del(&one_device->devid_kobj); > + kobject_put(&one_device->devid_kobj); > > - wait_for_completion(&one_device->kobj_unregister); > + wait_for_completion(&one_device->kobj_unregister); > + } > > return 0; > } > @@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, > sysfs_remove_link(fs_devices->devices_kobj, > disk_kobj->name); > } > - kobject_del(&one_device->devid_kobj); > - kobject_put(&one_device->devid_kobj); > + if (one_device->devid_kobj.state_initialized) { > + kobject_del(&one_device->devid_kobj); > + kobject_put(&one_device->devid_kobj); > > - wait_for_completion(&one_device->kobj_unregister); > + wait_for_completion(&one_device->kobj_unregister); > + } > } > > return 0; > Hmm this is a lot of duplicated code. It was before as well, this patch only adds to the code duplication. Can't we do sth like this:
On 07/09/2020 13:29, Johannes Thumshirn wrote: > On 04/09/2020 19:37, Anand Jain wrote: >> The following test case leads to null kobject-being-freed error. >> >> mount seed /mnt >> add sprout to /mnt >> umount /mnt >> mount sprout to /mnt >> delete seed >> >> kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called. >> WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350 >> RIP: 0010:kobject_put+0x80/0x350 >> :: >> Call Trace: >> btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs] >> btrfs_rm_device.cold+0xa8/0x298 [btrfs] >> btrfs_ioctl+0x206c/0x22a0 [btrfs] >> ksys_ioctl+0xe2/0x140 >> __x64_sys_ioctl+0x1e/0x29 >> do_syscall_64+0x96/0x150 >> entry_SYSCALL_64_after_hwframe+0x44/0xa9 >> RIP: 0033:0x7f4047c6288b >> :: >> >> This is because, at the end of the seed device-delete, we try to remove >> the seed's devid sysfs entry. But for the seed devices under the sprout >> fs, we don't initialize the devid kobject yet. So add a kobject state >> check, which takes care of the Warning. >> >> Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/sysfs.c | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c >> index 190e59152be5..438a367371c4 100644 >> --- a/fs/btrfs/sysfs.c >> +++ b/fs/btrfs/sysfs.c >> @@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, >> disk_kobj->name); >> } >> >> - kobject_del(&one_device->devid_kobj); >> - kobject_put(&one_device->devid_kobj); >> + if (one_device->devid_kobj.state_initialized) { >> + kobject_del(&one_device->devid_kobj); >> + kobject_put(&one_device->devid_kobj); >> >> - wait_for_completion(&one_device->kobj_unregister); >> + wait_for_completion(&one_device->kobj_unregister); >> + } >> >> return 0; >> } >> @@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, >> sysfs_remove_link(fs_devices->devices_kobj, >> disk_kobj->name); >> } >> - kobject_del(&one_device->devid_kobj); >> - kobject_put(&one_device->devid_kobj); >> + if (one_device->devid_kobj.state_initialized) { >> + kobject_del(&one_device->devid_kobj); >> + kobject_put(&one_device->devid_kobj); >> >> - wait_for_completion(&one_device->kobj_unregister); >> + wait_for_completion(&one_device->kobj_unregister); >> + } >> } >> >> return 0; >> > > Hmm this is a lot of duplicated code. It was before as well, this patch only adds > to the code duplication. > > Can't we do sth like this: > Hit sent too early I'm sorry. I meant add this (untested) as a prep patch: diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 190e59152be5..84114a1ec502 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -1151,44 +1151,40 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info, /* when one_device is NULL, it removes all device links */ + +static void btrfs_sysfs_remove_one_devices_dir(struct btrfs_device *dev) +{ + if (one_device->bdev) { + struct hd_struct *disk; + struct kobject *disk_kobj; + + disk = one_device->bdev->bd_part; + disk_kobj = &part_to_dev(disk)->kobj; + sysfs_remove_link(fs_devices->devices_kobj, + disk_kobj->name); + } + + kobject_del(&one_device->devid_kobj); + kobject_put(&one_device->devid_kobj); + + wait_for_completion(&one_device->kobj_unregister); + + return; +} + int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, struct btrfs_device *one_device) { - struct hd_struct *disk; - struct kobject *disk_kobj; - if (!fs_devices->devices_kobj) return -EINVAL; if (one_device) { - if (one_device->bdev) { - disk = one_device->bdev->bd_part; - disk_kobj = &part_to_dev(disk)->kobj; - sysfs_remove_link(fs_devices->devices_kobj, - disk_kobj->name); - } - - kobject_del(&one_device->devid_kobj); - kobject_put(&one_device->devid_kobj); - - wait_for_completion(&one_device->kobj_unregister); - + btrfs_sysfs_remove_one_devices_dir(one_device); return 0; } - list_for_each_entry(one_device, &fs_devices->devices, dev_list) { - - if (one_device->bdev) { - disk = one_device->bdev->bd_part; - disk_kobj = &part_to_dev(disk)->kobj; - sysfs_remove_link(fs_devices->devices_kobj, - disk_kobj->name); - } - kobject_del(&one_device->devid_kobj); - kobject_put(&one_device->devid_kobj); - - wait_for_completion(&one_device->kobj_unregister); - } + list_for_each_entry(one_device, &fs_devices->devices, dev_list) + btrfs_sysfs_remove_one_devices_dir(one_device); return 0; }
On 7/9/20 7:30 pm, Johannes Thumshirn wrote: > On 07/09/2020 13:29, Johannes Thumshirn wrote: >> On 04/09/2020 19:37, Anand Jain wrote: >>> The following test case leads to null kobject-being-freed error. >>> >>> mount seed /mnt >>> add sprout to /mnt >>> umount /mnt >>> mount sprout to /mnt >>> delete seed >>> >>> kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called. >>> WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350 >>> RIP: 0010:kobject_put+0x80/0x350 >>> :: >>> Call Trace: >>> btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs] >>> btrfs_rm_device.cold+0xa8/0x298 [btrfs] >>> btrfs_ioctl+0x206c/0x22a0 [btrfs] >>> ksys_ioctl+0xe2/0x140 >>> __x64_sys_ioctl+0x1e/0x29 >>> do_syscall_64+0x96/0x150 >>> entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> RIP: 0033:0x7f4047c6288b >>> :: >>> >>> This is because, at the end of the seed device-delete, we try to remove >>> the seed's devid sysfs entry. But for the seed devices under the sprout >>> fs, we don't initialize the devid kobject yet. So add a kobject state >>> check, which takes care of the Warning. >>> >>> Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> fs/btrfs/sysfs.c | 16 ++++++++++------ >>> 1 file changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c >>> index 190e59152be5..438a367371c4 100644 >>> --- a/fs/btrfs/sysfs.c >>> +++ b/fs/btrfs/sysfs.c >>> @@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, >>> disk_kobj->name); >>> } >>> >>> - kobject_del(&one_device->devid_kobj); >>> - kobject_put(&one_device->devid_kobj); >>> + if (one_device->devid_kobj.state_initialized) { >>> + kobject_del(&one_device->devid_kobj); >>> + kobject_put(&one_device->devid_kobj); >>> >>> - wait_for_completion(&one_device->kobj_unregister); >>> + wait_for_completion(&one_device->kobj_unregister); >>> + } >>> >>> return 0; >>> } >>> @@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, >>> sysfs_remove_link(fs_devices->devices_kobj, >>> disk_kobj->name); >>> } >>> - kobject_del(&one_device->devid_kobj); >>> - kobject_put(&one_device->devid_kobj); >>> + if (one_device->devid_kobj.state_initialized) { >>> + kobject_del(&one_device->devid_kobj); >>> + kobject_put(&one_device->devid_kobj); >>> >>> - wait_for_completion(&one_device->kobj_unregister); >>> + wait_for_completion(&one_device->kobj_unregister); >>> + } >>> } >>> >>> return 0; >>> >> >> Hmm this is a lot of duplicated code. It was before as well, this patch only adds >> to the code duplication. >> >> Can't we do sth like this: >> > > > Hit sent too early I'm sorry. I meant add this (untested) as a prep patch: David and I decided to avoid cleanups in the patch 1/16, and are pushed into the patch 3-7/16, mainly to make the bug fix patch easy to backport. Thanks, Anand > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > index 190e59152be5..84114a1ec502 100644 > --- a/fs/btrfs/sysfs.c > +++ b/fs/btrfs/sysfs.c > @@ -1151,44 +1151,40 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info, > > /* when one_device is NULL, it removes all device links */ > > + > +static void btrfs_sysfs_remove_one_devices_dir(struct btrfs_device *dev) > +{ > + if (one_device->bdev) { > + struct hd_struct *disk; > + struct kobject *disk_kobj; > + > + disk = one_device->bdev->bd_part; > + disk_kobj = &part_to_dev(disk)->kobj; > + sysfs_remove_link(fs_devices->devices_kobj, > + disk_kobj->name); > + } > + > + kobject_del(&one_device->devid_kobj); > + kobject_put(&one_device->devid_kobj); > + > + wait_for_completion(&one_device->kobj_unregister); > + > + return; > +} > + > int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, > struct btrfs_device *one_device) > { > - struct hd_struct *disk; > - struct kobject *disk_kobj; > - > if (!fs_devices->devices_kobj) > return -EINVAL; > > if (one_device) { > - if (one_device->bdev) { > - disk = one_device->bdev->bd_part; > - disk_kobj = &part_to_dev(disk)->kobj; > - sysfs_remove_link(fs_devices->devices_kobj, > - disk_kobj->name); > - } > - > - kobject_del(&one_device->devid_kobj); > - kobject_put(&one_device->devid_kobj); > - > - wait_for_completion(&one_device->kobj_unregister); > - > + btrfs_sysfs_remove_one_devices_dir(one_device); > return 0; > } > > - list_for_each_entry(one_device, &fs_devices->devices, dev_list) { > - > - if (one_device->bdev) { > - disk = one_device->bdev->bd_part; > - disk_kobj = &part_to_dev(disk)->kobj; > - sysfs_remove_link(fs_devices->devices_kobj, > - disk_kobj->name); > - } > - kobject_del(&one_device->devid_kobj); > - kobject_put(&one_device->devid_kobj); > - > - wait_for_completion(&one_device->kobj_unregister); > - } > + list_for_each_entry(one_device, &fs_devices->devices, dev_list) > + btrfs_sysfs_remove_one_devices_dir(one_device); > > return 0; > } >
On 07/09/2020 14:00, Anand Jain wrote: > David and I decided to avoid cleanups in the patch 1/16, and are > pushed into the patch 3-7/16, mainly to make the bug fix patch easy > to backport. Yep I know, but one prep patch in front shoukld be ok even, when you need to backport something to stable. I think 4/16 can go in front of the series and be backported as well. Sorry that I didn't scan the whole series before replying to the 1st patch. Byte, Johannes
On Mon, Sep 07, 2020 at 03:09:20PM +0000, Johannes Thumshirn wrote: > On 07/09/2020 14:00, Anand Jain wrote: > > David and I decided to avoid cleanups in the patch 1/16, and are > > pushed into the patch 3-7/16, mainly to make the bug fix patch easy > > to backport. > > Yep I know, but one prep patch in front shoukld be ok even, when you > need to backport something to stable. I think 4/16 can go in front of > the series and be backported as well. Sorry that I didn't scan the whole > series before replying to the 1st patch. There is some duplication but the bugfix is clear and minimal, just adding the condition around some code which I'd prefer for a stable patch. The prep or cleanup creates a dependency and this needs to be tracked in the real fix either by subject or in Fixes: and commit id. When the fix can be in one patch it's all easier.
On Sat, Sep 05, 2020 at 01:34:21AM +0800, Anand Jain wrote: > The following test case leads to null kobject-being-freed error. > > mount seed /mnt > add sprout to /mnt > umount /mnt > mount sprout to /mnt > delete seed > > kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called. > WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350 > RIP: 0010:kobject_put+0x80/0x350 > :: > Call Trace: > btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs] > btrfs_rm_device.cold+0xa8/0x298 [btrfs] > btrfs_ioctl+0x206c/0x22a0 [btrfs] > ksys_ioctl+0xe2/0x140 > __x64_sys_ioctl+0x1e/0x29 > do_syscall_64+0x96/0x150 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > RIP: 0033:0x7f4047c6288b > :: > > This is because, at the end of the seed device-delete, we try to remove > the seed's devid sysfs entry. But for the seed devices under the sprout > fs, we don't initialize the devid kobject yet. So add a kobject state > check, which takes care of the Warning. > > Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes Please note that the correct format of the Fixes: tag is Fixes: 668e48af7a94 ("btrfs: sysfs, add devid/dev_state kobject and device attributes") > Signed-off-by: Anand Jain <anand.jain@oracle.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 190e59152be5..438a367371c4 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -1168,10 +1168,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, disk_kobj->name); } - kobject_del(&one_device->devid_kobj); - kobject_put(&one_device->devid_kobj); + if (one_device->devid_kobj.state_initialized) { + kobject_del(&one_device->devid_kobj); + kobject_put(&one_device->devid_kobj); - wait_for_completion(&one_device->kobj_unregister); + wait_for_completion(&one_device->kobj_unregister); + } return 0; } @@ -1184,10 +1186,12 @@ int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices, sysfs_remove_link(fs_devices->devices_kobj, disk_kobj->name); } - kobject_del(&one_device->devid_kobj); - kobject_put(&one_device->devid_kobj); + if (one_device->devid_kobj.state_initialized) { + kobject_del(&one_device->devid_kobj); + kobject_put(&one_device->devid_kobj); - wait_for_completion(&one_device->kobj_unregister); + wait_for_completion(&one_device->kobj_unregister); + } } return 0;
The following test case leads to null kobject-being-freed error. mount seed /mnt add sprout to /mnt umount /mnt mount sprout to /mnt delete seed kobject: '(null)' (00000000dd2b87e4): is not initialized, yet kobject_put() is being called. WARNING: CPU: 1 PID: 15784 at lib/kobject.c:736 kobject_put+0x80/0x350 RIP: 0010:kobject_put+0x80/0x350 :: Call Trace: btrfs_sysfs_remove_devices_dir+0x6e/0x160 [btrfs] btrfs_rm_device.cold+0xa8/0x298 [btrfs] btrfs_ioctl+0x206c/0x22a0 [btrfs] ksys_ioctl+0xe2/0x140 __x64_sys_ioctl+0x1e/0x29 do_syscall_64+0x96/0x150 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f4047c6288b :: This is because, at the end of the seed device-delete, we try to remove the seed's devid sysfs entry. But for the seed devices under the sprout fs, we don't initialize the devid kobject yet. So add a kobject state check, which takes care of the Warning. Fixes: 668e48af btrfs: sysfs, add devid/dev_state kobject and device attributes Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/sysfs.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)