Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

store: Avoid access to Forgotten() before configured #1599

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ktock
Copy link
Member

@ktock ktock commented Mar 11, 2024

Fixes #1594

Our Lookup() implementation returns a newly created *fusefs.Inode to go-fuse. Then that *fusefs.Inode is configured and managed by go-fuse lib.

Though *fusefs.Inode.Forgotten() allows status check of that object, it shouldn't be called until that *fusefs.Inode is fully configured by go-fuse. Otherwize we hit issue like #1594.

To avoid this race, this commit adds a lock for avoiding calling *fusefs.Inode.Forgotten() during configuration on-going in go-fuse.

Our `Lookup()` implementation returns a newly created `*fusefs.Inode` to
go-fuse. Then that `*fusefs.Inode` is configured and managed by go-fuse lib.

Though [`*fusefs.Inode.Forgotten()`](https://github.com/hanwen/go-fuse/blob/e9e7c22af17af4611b5783a16458647088cc8dec/fs/inode.go#L258)
allows status check of that object, it shouldn't be called until that
`*fusefs.Inode` is [fully configured by
go-fuse](https://github.com/hanwen/go-fuse/blob/e9e7c22af17af4611b5783a16458647088cc8dec/fs/bridge.go#L209-L210).

To avoid this race, this commit adds a lock for avoiding calling Forgotten API
during configuration on-going in go-fuse.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@ktock ktock changed the title Avoid access to Forgotten() before initialized Avoid access to Forgotten() before configured Mar 11, 2024
@ktock ktock changed the title Avoid access to Forgotten() before configured store: Avoid access to Forgotten() before configured Mar 11, 2024
@iain-macdonald
Copy link
Contributor

iain-macdonald commented Mar 11, 2024

My understanding of the interactions between the go-fuse and stargz-snapshotter parts of the code may be off here, but won't the RLock/RUnlock calls in fsLocker surround the Lock/Unlock calls nested in newNodeWithID, causing deadlock? And if not, then isn't the lock in releasable insufficient as it doesn't guard the entire period from creation to initialization?

I think a simpler fix would be to used Inode.changeCounter to detect when an Inode hasn't been initialized and pair that with the Forgotten() check that's already present. I asked about this possibility in hanwen/go-fuse#504, so what would you think about seeing what Han-Wen says and then coming up with a more elaborate solution if that doesn't work?

@ktock ktock marked this pull request as draft March 13, 2024 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ino reuse in newInodeWithID
2 participants