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

lib/ramfs: Ensure stability of inode numbers #996

Closed
wants to merge 1 commit into from

Conversation

andreittr
Copy link
Contributor

Description of changes

Previously ramfs would assign a new unique inode number to a file on every successful filesystem lookup. This can lead to the same file being allocated multiple different inodes, breaking code that (correctly) uses inodes as reliable indicators of file identity.
This change makes inode numbers persistent, allocated along with the filesystem node, and stable across multiple lookups of the same file.

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

Test with:

int f = open("file", O_RDONLY);
fstat(f, &st1);
stat("file", &st2);
printf("Should be same: %ld %ld\n", st1.st_ino, st2.st_ino);

@andreittr andreittr requested a review from a team as a code owner July 24, 2023 21:21
@razvand razvand requested review from RaduNichita and mariasfiraiala and removed request for a team July 24, 2023 21:52
@razvand razvand self-assigned this Jul 24, 2023
@razvand razvand added area/lib Internal Unikraft Microlibrary kind/quick-fix Issue is a quick fix lib/ramfs lang/c Issues or PRs to do with C/C++ priority/medium labels Jul 24, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jul 24, 2023
Copy link
Contributor

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems OK, thanks @andreittr! I left some minor nitpicks

@@ -47,6 +47,7 @@ struct ramfs_node {
* else NULL
*/
struct ramfs_node *rn_child;
uint64_t rn_ino;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment about the need for this member in the structure.

@@ -93,6 +93,8 @@ ramfs_allocate_node(const char *name, int type)
strlcpy(np->rn_name, name, np->rn_namelen + 1);
np->rn_type = type;

np->rn_ino = inode_count++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have an empty line before?

@RaduNichita
Copy link
Contributor

RaduNichita commented Jul 30, 2023

@andreittr, is it intended to open the file without the O_CREAT flag?

I am asking this since for Linux, it creates a file, but the inode number differs at the second attempt or running (0 vs a large number).

@andreittr
Copy link
Contributor Author

@andreittr, is it intended to open the file without the O_CREAT flag?

I am asking this since for Linux, it creates a file, but the inode number differs at the second attempt or running (0 vs a large number).

Yes, the test snippet assumes "file" exists and is a regular file created ahead of time (e.g. already included in the ramfs/initrd)

@mariasfiraiala
Copy link
Contributor

@andreittr The changes look fine, however I wasn't able to reproduce the issue. Could you please share a bit more regarding your setup?

This is what I've tested:

int main(void)
{
	struct stat st1 = {0};
	struct stat st2 = {0};

	int f = open("file", O_CREAT);
	fstat(f, &st1);
	stat("file", &st2);
	printf("Should be same: %ld %ld\n", st1.st_ino, st2.st_ino);
}

And this is the output:

ttr-inode

Notice how the break point at which the static variable should be incremented is reached only once.

@andreittr
Copy link
Contributor Author

@andreittr The changes look fine, however I wasn't able to reproduce the issue. Could you please share a bit more regarding your setup?

This is what I've tested:

int main(void)
{
	struct stat st1 = {0};
	struct stat st2 = {0};

	int f = open("file", O_CREAT);
	fstat(f, &st1);
	stat("file", &st2);
	printf("Should be same: %ld %ld\n", st1.st_ino, st2.st_ino);
}

And this is the output:

ttr-inode

Notice how the break point at which the static variable should be incremented is reached only once.

Hi! See the above comment as well as the test snippet in the description. open() is not supposed to be called with O_CREAT, rather just with O_RDONLY and "file" needs to already exist. This is necessary to cause multiple FS lookups of the file, resulting in different inode numbers.

Previously ramfs would assign a new unique inode number to a file on
every successful filesystem lookup. This can lead to the same file being
allocated multiple different inodes, breaking code that (correctly) uses
inodes as reliable indicators of file identity.
This change makes inode numbers persistent, allocated along with the
filesystem node, and stable across multiple lookups of the same file.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr
Copy link
Contributor Author

Pushed changes addressing comments.

Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, all good on my side, then. Let's merge this!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

@RaduNichita
Copy link
Contributor

RaduNichita commented Aug 2, 2023

It works for me as well.

I used something more hackish:

int main(void)
{
        struct stat st1, st2;
        int f = open("file", O_CREAT);
        fstat(f, &st1);
        close(f);

        f = open("file", O_RDONLY);
        stat("file", &st2);
        printf("Should be same: %ld %ld\n", st1.st_ino, st2.st_ino);
        close(f);
        return 0;
}

but it does the trick

Thanks, @andreittr

Reviewed-by: Radu Nichita radunichita99@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 7, 2023
@andreittr andreittr deleted the ttr/ramfs-inodes branch August 7, 2023 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI kind/quick-fix Issue is a quick fix lang/c Issues or PRs to do with C/C++ lib/ramfs priority/medium
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants