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

Josef Bacik
2020-08-31T16:22:23Z

On 8/30/20 10:40 AM, 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>
> ---

Ok again, this will not work.

Mount a seed fs, you get fs_info->fs_devices pointed at the seed device, and
fs_info->fs_devices->devices_kobj is what is initialized.

Now you sprout.

This does clone_fs_devices(fs_info->fs_devices), which doesn't copy over
fs_fdevices->devices_kobj. Now we take this clone device, and set
fs_info->fsdevices to the cloned fs_devices, and we add the original fs_devices,
which had the sysfs objects init'ed already, to fs_devices->seed_list.

It appears to me you are completely ignoring this aspect of sprout. Maybe I'm
missing something, but I've gone through this code twice now to see if what I
think is happening is actually happening, and I'm convinced this is wrong.

If it's _not_ wrong, and I _am_ missing something, then you need to explain why
I'm wrong, and then go back and fix what needs to be fixed to make this whole
process more clear, and _then_ you can do this patch series. Thanks,

Josef