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

[Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]

On Wed, 06 Jan, at 04:22:40PM, Sylvain Chouleur wrote:
> 2016-01-06 13:24 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
> > > @@ -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.

> > > @@ -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.