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
