From: Anand Jain <anand.jain@oracle.com>
To: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Cc: "dsterba@suse.com" <dsterba@suse.com>,
"josef@toxicpanda.com" <josef@toxicpanda.com>,
"nborisov@suse.com" <nborisov@suse.com>
Subject: Re: [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete
Date: Mon, 7 Sep 2020 19:57:10 +0800 [thread overview]
Message-ID: <a0f3b051-b9e6-e881-cdb1-0c75224f6760@oracle.com> (raw)
In-Reply-To: <SN4PR0401MB3598B2EC32C25D601E59BF879B280@SN4PR0401MB3598.namprd04.prod.outlook.com>
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;
> }
>
next prev parent reply other threads:[~2020-09-07 12:01 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-04 17:34 [PATCH v3 0/16] btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups Anand Jain
2020-09-04 17:34 ` [PATCH 01/16] btrfs: fix put of uninitialized kobject after seed device delete Anand Jain
2020-09-07 11:29 ` Johannes Thumshirn
2020-09-07 11:30 ` Johannes Thumshirn
2020-09-07 11:57 ` Anand Jain [this message]
2020-09-07 15:09 ` Johannes Thumshirn
2020-09-08 15:47 ` David Sterba
2020-09-08 15:55 ` David Sterba
2020-09-04 17:34 ` [PATCH 02/16] btrfs: fix replace of seed device Anand Jain
2020-09-04 17:34 ` [PATCH 03/16] btrfs: add btrfs_sysfs_add_device helper Anand Jain
2020-09-04 17:34 ` [PATCH 04/16] btrfs: add btrfs_sysfs_remove_device helper Anand Jain
2020-09-07 15:51 ` Johannes Thumshirn
2020-09-04 17:34 ` [PATCH 05/16] btrfs: btrfs_sysfs_remove_devices_dir drop return value Anand Jain
2020-09-04 17:34 ` [PATCH 06/16] btrfs: refactor btrfs_sysfs_add_devices_dir Anand Jain
2020-09-04 17:34 ` [PATCH 07/16] btrfs: refactor btrfs_sysfs_remove_devices_dir Anand Jain
2020-09-04 17:34 ` [PATCH 08/16] btrfs: initialize sysfs devid and device link for seed device Anand Jain
2020-09-04 17:34 ` [PATCH 09/16] btrfs: handle fail path for btrfs_sysfs_add_fs_devices Anand Jain
2020-09-04 17:34 ` [PATCH 10/16] btrfs: reada: use sprout device_list_mutex Anand Jain
2020-09-04 17:34 ` [PATCH 11/16] btrfs: btrfs_init_devices_late: " Anand Jain
2020-09-04 17:34 ` [PATCH 12/16] btrfs: open code list_head pointer in btrfs_init_dev_replace_tgtdev Anand Jain
2020-09-04 17:34 ` [PATCH 13/16] btrfs: cleanup btrfs_remove_chunk Anand Jain
2020-09-09 10:50 ` David Sterba
2020-09-09 11:15 ` Anand Jain
2020-09-04 17:34 ` [PATCH 14/16] btrfs: cleanup btrfs_assign_next_active_device() Anand Jain
2020-09-04 17:34 ` [PATCH 15/16] btrfs: cleanup unnecessary goto in open_seed_device Anand Jain
2020-09-04 17:34 ` [PATCH 16/16] btrfs: btrfs_dev_replace_update_device_in_mapping_tree drop file global declare Anand Jain
2020-09-04 23:25 ` [PATCH v2 0/2] fstests: btrfs seed device device operation tests Anand Jain
2020-09-04 23:25 ` [PATCH v2 1/2] btrfs: add a test case for btrfs seed device delete Anand Jain
2020-10-15 15:45 ` Filipe Manana
2020-10-20 11:21 ` Anand Jain
2020-09-04 23:25 ` [PATCH 2/2] btrfs/163: replace sprout instead of seed Anand Jain
2020-10-15 15:49 ` Filipe Manana
2020-10-20 12:20 ` Anand Jain
2020-09-09 13:14 ` [PATCH v3 0/16] btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups David Sterba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a0f3b051-b9e6-e881-cdb1-0c75224f6760@oracle.com \
--to=anand.jain@oracle.com \
--cc=Johannes.Thumshirn@wdc.com \
--cc=dsterba@suse.com \
--cc=josef@toxicpanda.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=nborisov@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox