Re: [PATCH 4/4] btrfs: sysfs, add devid/dev_state kobject and attribute

David Sterba
2019-12-13T20:39:30Z

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?