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