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