lib/vfscore: Fix lstat() infinite loop

[mariasfiraiala]

Prerequisite checklist

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

Base target

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

Additional configuration

Build a simple app (such as helloworld) on x86 and call lstat().

Description of changes

Including musl with even simple apps and attempting to call lstat() on x86 results in an infinite loop due to __lxstat(), a helper function, existing in musl as well.

This PR changes the function called by lstat() in order to stop referencing musl by adding an extra wrapper, used by __lxstat() too. The change was made in order to be consistent to other implementations found in vfscore, such as this one.

Fixes: #652

Signed-off-by: Maria Sfiraiala maria.sfiraiala@gmail.com

[StefanJum]

This works file @mariasfiraiala, thank you.

I think the __lxstat function is used since the actual implementation of lstat may differ on different systems, and the __lxstat function will be overwritten to provide the desired implementation.
However, I don't see any use for it in the unikraft lstat syscall (or in the unikraft core), since we want the syscall implementation to be well defined within the core, not dependant on any kind of external libraries (like musl).

This pr will make the __lxstat function useless anyway since it will never be called, no matter what library implements it. so I would go for just moving the code in the UK_SYSCALL_R_DEFINE lstat, since using another helper function makes things messy and makes the original __lxstat useless.

Any suggestions on this @razvand @eduardvintila?

[mariasfiraiala]

This pr will make the __lxstat function useless anyway since it will never be called, no matter what library implements it. so I would go for just moving the code in the UK_SYSCALL_R_DEFINE lstat, since using another helper function makes things messy and makes the original __lxstat useless.

@StefanJum Indeed that would be the natural approach, however, there are other instances in vfscore (that I mentioned in the message above) that follow the same path: have a another helper function, used by both the syscall and its helper: __xmknod_helper.

I tried to follow this implemetation in order to keep the consistency.

[StefanJum]

@StefanJum Indeed that would be the natural approach, however, there are other instances in vfscore (that I mentioned in the message above) that follow the same path: have a another helper function, used by both the syscall and its helper: __xmknod_helper.

I tried to follow this implemetation in order to keep the consistency.

Yes, those instances also don't make sense to me, I would change them too, but there could be something I'm missing.

[eduardvintila]

Hi @mariasfiraiala and @StefanJum. The PR is looking good from my side: it finally fixes that pesky infinite loop issue with app-sqlite on x86 and also brings lstat to be more consistent with other implementations in vfscore.

I agree with @StefanJum in that we should eventually consider to move the code from the helpers straight to the syscall implementations. However, I strongly propose that we postpone this refactoring effort for a seperate PR. There are at least four other syscalls currently present in vfscore that I know of which follow a similar pattern:

• mknod -> __xmknod, __xmknod_helper
• fstat -> __fxstat, __fxstat_helper
• fstatat -> __fxstatat, __fxstatat_helper
• stat ->__xstat, __xstat_helper

There may be even more. I find that the refactoring of all of these functions is outside the scope of this PR, which is to fix the infinite loop issue. I think it's good that this PR brings lstat to the existing pattern mentioned above, since it should make an eventual cleanup of vfscore more straightforward by having the functions consistent with each other.

[StefanJum]

Makes sense @eduardvintila. @mariasfiraiala if we go with this approach, please open an issue on refactoring these vfscore functions so we won't forget about them.

[unikraft-bot]

✅ Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA	commit	checkpatch
6ef510e	lib/vfscore: Fix `lstat()` infinite loop	✅

[razvand]

@mariasfiraiala , @StefanJum , @eduardvintila , as discussed on Discord, I agree we can merge this PR as it is for consistency.

Then, a new PR would be required that updates all these functions (there's 5 of them: stat, lstat, fstat, fstatat, mknod) with these steps:

• Move the contents of the __*xstat_helper() functions to *stat() functions.
• Remove the __*xstat_helper() functions.
• Have the __*xstat() functions call *stat() functions.
• Guard the __*xstat() functions with #if UK_LIBC_SYSCALLS. *stat() functions are already guarded via UK_SYSCALL_R_DEFINE.

[eduardvintila]

Alright, this should go in. Thanks, @mariasfiraiala.

Reviewed-by: Eduard Vintilă eduard.vintila47@gmail.com

[razvand]

@StefanJum, if this is OK with you, please approve this PR and add your message tag. I will then approve it as an assignee.

[StefanJum]

Thanks @mariasfiraiala.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

[razvand]

Reviewed-by: Razvan Deaconescu razvand@unikraft.io
Approved-by: Razvan Deaconescu razvand@unikraft.io