On Mon, Dec 09, 2019 at 10:05:39PM +0800, Anand Jain wrote:
> On 12/6/19 9:49 PM, Anand Jain wrote:
> >
> >
> > On 5/12/19 11:14 PM, David Sterba wrote:
> >> On Thu, Dec 05, 2019 at 10:38:15PM +0800, Anand Jain wrote:
> >>>> So the values copy the device state macros, that's probably ok.
> >>> Yep.
> >>
> >> Although, sysfs files should print one value per file, which makes sense
> >> in many cases, so eg. missing should exist separately too for quick
> >> checks for the most common device states. The dev_state reflects the
> >> internal state and is likely useful only for debugging.
> >>
> >
> > I agree. Its better to create an individual attribute for each of the
> > device states. For instance..
> >
> > under the 'UUID/devinfo/<devid>' kobject
> > attributes will be:
> > missing
> > in_fs_metadata
> > replace_target
> >
> > cat missing
> > 0
> > cat in_fs_metadata
> > 1
> >
> > ..etc
> >
> > which seems to be more or less standard for block devices.
> >
> > Will fix it in v2.
>
> This is fixed in v2.
>
>
> >
> >>>>> +static ssize_t btrfs_sysfs_dev_state_show(struct kobject *kobj,
> >>>>> + struct kobj_attribute *a, char *buf)
> >>>>> +{
> >>>>> + struct btrfs_device *device = container_of(kobj, struct
> >>>>> btrfs_device,
> >>>>> + devid_kobj);
> >>>>> +
> >>>>> + btrfs_dev_state_to_str(device, buf, PAGE_SIZE);
> >>>>
> >>>> The device access is unprotected, you need at least RCU but that still
> >>>> does not prevent from the device being freed by deletion.
> >>>
> >>> We need RCU let me fix. Device being deleted is fine, there
> >>> is nothing to lose, another directory lookup will show that
> >>> UUID/devinfo/<devid> is gone is the device is deleted.
> >>
> >> The device can be gone from the list but the sysfs files can still
> >> exist.
> >>
> >> CPU1 CPU2
> >>
> >> btrfs_rm_device
> >> open file
> >> btrfs_sysfs_rm_device_link
> >> btrfs_free_device
> >> kfree(device)
> >> call read, access freed device
> >>
> >
> > I completely missed the sysfs synchronization with device delete.
> > As in the other email discussion, I think a new rwlock shall suffice.
> > And as its lock is only between device delete and sysfs so it shall
> > be light weight without affecting the other device_list_mutex holders.
>
> Looked into this further, actually we don't need any lock here
> the device delete thread which calls kobject_put() makes sure
> sysfs read is closed. So an existing sysfs read thread will have
> to complete before device free.
>
>
> CPU1 CPU2
>
> btrfs_rm_device
> open file
> btrfs_sysfs_rm_device_link
> call read, access freed device
> sysfs waits for the open file
> to close.
How exactly does sysfs wait for the device? Is it eg wait_event checking
number of references? If the file stays open by an evil process is it
going to block the device removal indefinitelly?
