On 10/03/2016 10:05 PM, Jason Gunthorpe wrote: > On Mon, Oct 03, 2016 at 03:35:23PM +0300, Jarkko Sakkinen wrote: > >>>> The scheme you suggested is also way off the mark for how fops works, >>>> fops->close has no relation to the needed duration for 'data', the >>>> duration is related to securityfs_remove. >>> >>> Right, the above would not work because it's not linked to >>> securityfs_remove by any means. >>> > >> You have to provide something factors more concrete. Otherwise, >> I'm inclined to accept the approach in Naynas patch. It's an >> improvement. > > I said I haven't checked the patch yet to see if the lifetime model > for 'data' with securityfs is correct. Only that it matches the only > other user of this feature.. > > I looked more carefully, and I still can't find the right sort of > locking in securityfs_remove.. > >>> Are you trying to say that after securityfs_remove() there might be a >>> "grace period" when user space could still see the files visible and >>> open them? > > Sort of, the typical race is broadly > > CPU0 CPU1 > > fops->open() > securityfs_remove() > kref_put(chip) > kfree(chip) > kref_get(data->chip.kref)
I didn't understand which kref_get() are we referring here. I mean is it expected to happen somewhere during eventlog parsing, or exactly which code path ? > > This race should always be analyzed when working with user files. > > We deal with this situation in the other user interface: > - cdev uses 'chip->cdev.kobj.parent = &chip->dev.kobj;' and the cdev > core handles get/put of the chip at the proper time > - sysfs uses kernfs_drain which guarentees nothing is running in any > callback before returning > > I suspect securityfs_remove is defective in this regard. Eg debugfs is > built on the same libfs scheme as securityfs and it incorporates the > mechanism around 'debugfs_use_file_start/etc' to provide sensible > removal fencing. > > I don't know if there is a simple fix, so mabye the best thing is to > just leave it be with a comment saying it securityfs_remove probably > races with fops->open(), and that should be fixed inside securityfs > not tpm. > > The file operations are also missing '.owner = THIS_MODULE' which is > bad as well. Yeah, this I will fix. > > Jason > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
