Re: [PATCH 1/2] efi: Don't use spinlocks for efi vars

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2016-01-08 12:08 GMT+01:00 Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>:
> On Wed, 06 Jan, at 04:22:40PM, Sylvain Chouleur wrote:
>> 2016-01-06 13:24 GMT+01:00 Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>:
>> > > @@ -582,7 +583,8 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
>> > >               return ret;
>> > >
>> > >       kobject_uevent(&new_var->kobj, KOBJ_ADD);
>> > > -     efivar_entry_add(new_var, &efivar_sysfs_list);
>> > > +     if (efivar_entry_add(new_var, &efivar_sysfs_list))
>> > > +             return -EINTR;
>> > >
>> > >       return 0;
>> > >  }
>> >
>> > This looks like it's missing a call to efivar_unregister() in the
>> > -EINTR case.
>>
>> I don't see why
>> It's returning an error code as other lines in the same function.
>> Can you develop your thoughts?
>
> kobject_uevent() just notified userspace that a new object was
> available. But because it hasn't been put onto 'efivar_sysfs_list'
> when efivar_entry_add() fails, we'll never ever call kobject_put().
>
> Before your changes efivar_entry_add() never failed and 'new_var' was
> always added to the list, and always removed in efivar_sysfs_destroy().
>
> That invariant no longer holds. Now we leak new_var->kobj.

Got it.

>
>> > > @@ -1055,12 +1087,16 @@ int efivars_register(struct efivars *efivars,
>> > >                    const struct efivar_operations *ops,
>> > >                    struct kobject *kobject)
>> > >  {
>> > > -     spin_lock_init(&efivars->lock);
>> > > +     if (down_trylock(&efivars_lock))
>> > > +             return -EBUSY;
>> > > +
>> >
>> > Is this correct? I would have assumed that you'd want to return -EINTR
>> > here, not -EBUSY since if an EFI variable operation is currently
>> > running we should spin waiting for the semaphore to be released.
>>
>> No because this is a trylock, it will not wait for the semaphore to be release,
>> it will return as soon as it sees that the semaphore is locked.
>> We don't want to use down_interruptible() here because we want to be able to
>> (un)register efivars operation in an interrupt context.
>
> Like I said in the other mail, I think supporting registration from
> within a panic context is a bad idea. Using down_interruptible() here
> would be much better.

Ok
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux