Re: [PATCH 01/11] btrfs: initialize sysfs devid and device link for seed device

Anand Jain
2020-08-31T12:00:53Z

On 31/8/20 5:07 pm, Nikolay Borisov wrote:
>
>
> On 30.08.20 г. 17:40 ч., 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 this patch initializes
>> the seed device devid kobject and the device link in the sysfs. This takes
>> care of the Warning.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>

> This patch is doing too many things at once. Split it so that:
> 1. You first add the new functions add_device/remove_device in 2
> separate patches.
> 2. Switch btrfs_sysfs_add_devices_dir/btrfs_sysfs_remove_devices_dir to
> ousing the newly added helpers, again in 2 separate patches

yeah and

3. Initialize seed devices devid kobject to fix the bug.

fixing.

-Anand

>
>> ---
>> fs/btrfs/sysfs.c | 146 ++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 93 insertions(+), 53 deletions(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 190e59152be5..9b5e58091fae 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -1149,45 +1149,48 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info,
>> return 0;
>> }
>>
>> -/* when one_device is NULL, it removes all device links */
>> -
>> -int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>> - struct btrfs_device *one_device)
>> +static void btrfs_sysfs_remove_device(struct btrfs_device *device)
>> {
>> struct hd_struct *disk;
>> struct kobject *disk_kobj;
>> + struct kobject *devices_kobj;
>>
>> - if (!fs_devices->devices_kobj)
>> - return -EINVAL;
>> + /*
>> + * Seed fs_devices devices_kobj aren't used, fetch kobject from the
>> + * fs_info::fs_devices.
>> + */
>> + devices_kobj = device->fs_info->fs_devices->devices_kobj;
>> + ASSERT(devices_kobj);
>>
>> - 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);
>> - }
>> + if (device->bdev) {
>> + disk = device->bdev->bd_part;
>> + disk_kobj = &part_to_dev(disk)->kobj;
>> + sysfs_remove_link(devices_kobj, disk_kobj->name);
>> + }
>>
>> - kobject_del(&one_device->devid_kobj);
>> - kobject_put(&one_device->devid_kobj);
>> + kobject_del(&device->devid_kobj);
>> + kobject_put(&device->devid_kobj);
>>
>> - wait_for_completion(&one_device->kobj_unregister);
>> + wait_for_completion(&device->kobj_unregister);
>> +}
>>
>> +/* when 2nd argument device is NULL, it removes all devices link */
>> +int btrfs_sysfs_remove_devices_dir(struct btrfs_fs_devices *fs_devices,
>> + struct btrfs_device *device)
>> +{
>> + struct btrfs_fs_devices *seed_fs_devices;
>> +
>> + if (device) {
>> + btrfs_sysfs_remove_device(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);
>> + list_for_each_entry(device, &fs_devices->devices, dev_list)
>> + btrfs_sysfs_remove_device(device);
>>
>> - wait_for_completion(&one_device->kobj_unregister);
>> + list_for_each_entry(seed_fs_devices, &fs_devices->seed_list, seed_list) {
>> + list_for_each_entry(device, &seed_fs_devices->devices, dev_list)
>> + btrfs_sysfs_remove_device(device);
>> }
>>
>> return 0;
>> @@ -1271,44 +1274,81 @@ static struct kobj_type devid_ktype = {
>> .release = btrfs_release_devid_kobj,
>> };
>>
>> +static int btrfs_sysfs_add_device(struct btrfs_device *device)
>> +{
>> + int ret;
>> + struct kobject *devices_kobj;
>> + struct kobject *devinfo_kobj;
>> +
>> + /*
>> + * make sure we use the fs_info::fs_devices to fetch the kobjects
>> + * even for the seed fs_devices
>> + */
>> + devices_kobj = device->fs_devices->fs_info->fs_devices->devices_kobj;
>> + devinfo_kobj = device->fs_devices->fs_info->fs_devices->devinfo_kobj;
>> + ASSERT(devices_kobj);
>> + ASSERT(devinfo_kobj);
>> +
>> + if (device->bdev) {
>> + struct hd_struct *disk;
>> + struct kobject *disk_kobj;
>> +
>> + disk = device->bdev->bd_part;
>> + disk_kobj = &part_to_dev(disk)->kobj;
>> +
>> + ret = sysfs_create_link(devices_kobj, disk_kobj,
>> + disk_kobj->name);
>> + if (ret) {
>> + btrfs_warn(device->fs_info,
>> + "sysfs create device link failed %d devid %llu",
>> + ret, device->devid);
>> + return ret;
>> + }
>> + }
>> +
>> + init_completion(&device->kobj_unregister);
>> + ret = kobject_init_and_add(&device->devid_kobj, &devid_ktype,
>> + devinfo_kobj, "%llu", device->devid);
>> + if (ret) {
>> + kobject_put(&device->devid_kobj);
>> + btrfs_warn(device->fs_info,
>> + "sysfs devinfo init failed %d devid %llu",
>> + ret, device->devid);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> int btrfs_sysfs_add_devices_dir(struct btrfs_fs_devices *fs_devices,
>> - struct btrfs_device *one_device)
>> + struct btrfs_device *device)
>> {
>> - int error = 0;
>> - struct btrfs_device *dev;
>> + int ret = 0;
>> unsigned int nofs_flag;
>> + struct btrfs_fs_devices *seed_fs_devices;
>>
>> nofs_flag = memalloc_nofs_save();
>> - list_for_each_entry(dev, &fs_devices->devices, dev_list) {
>>
>> - if (one_device && one_device != dev)
>> - continue;
>> + if (device)
>> + return btrfs_sysfs_add_device(device);
>>
>> - if (dev->bdev) {
>> - struct hd_struct *disk;
>> - struct kobject *disk_kobj;
>> -
>> - disk = dev->bdev->bd_part;
>> - disk_kobj = &part_to_dev(disk)->kobj;
>> -
>> - error = sysfs_create_link(fs_devices->devices_kobj,
>> - disk_kobj, disk_kobj->name);
>> - if (error)
>> - break;
>> - }
>> + list_for_each_entry(device, &fs_devices->devices, dev_list) {
>> + ret = btrfs_sysfs_add_device(device);
>> + if (ret)
>> + goto out;
>> + }
>>
>> - init_completion(&dev->kobj_unregister);
>> - error = kobject_init_and_add(&dev->devid_kobj, &devid_ktype,
>> - fs_devices->devinfo_kobj, "%llu",
>> - dev->devid);
>> - if (error) {
>> - kobject_put(&dev->devid_kobj);
>> - break;
>> + list_for_each_entry(seed_fs_devices, &fs_devices->seed_list, seed_list) {
>> + list_for_each_entry(device, &seed_fs_devices->devices, dev_list) {
>> + ret = btrfs_sysfs_add_device(device);
>> + if (ret)
>> + goto out;
>> }
>> }
>> +
>> +out:
>> memalloc_nofs_restore(nofs_flag);
>>
>> - return error;
>> + return ret;
>> }
>>
>> void btrfs_kobject_uevent(struct block_device *bdev, enum kobject_action action)
>>