Re: [UNIKRAFT PATCH 5/7] lib/vfscore: Fix double refcounting bug

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Thanks for fixing this, Cristi! We needed it for a long time, so it
helps us a great deal. I'll upstream this now and we shall skip it from
the v2.

Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>

On 5/2/20 6:13 PM, Cristian Banu wrote:
> This patch removes the `dref()` line in `sys_open` which would result
> in an unbounded growth of the refcount. open() would take two
> references: one via dentry_lookup() in namei(), and the other via
> dref(); close() would release only one reference via drele().
> 
> As to the comment also deleted in this patch, it is true that OSv uses
> intrusive_ptr, however it is created with the second argument
> add_ref = false, hence the "std::move()" comment below in the original
> OSv source code.
> 
> Now close() correctly releases the dentry if there are no open files
> left.
> 
> Signed-off-by: Cristian Banu <cristb@xxxxxxxxx>
> ---
>  lib/vfscore/syscalls.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vfscore/syscalls.c b/lib/vfscore/syscalls.c
> index 9a132b7..ce51274 100644
> --- a/lib/vfscore/syscalls.c
> +++ b/lib/vfscore/syscalls.c
> @@ -205,8 +205,10 @@ sys_open(char *path, int flags, mode_t mode, struct 
> vfscore_file **fpp)
>       fhold(fp);
>       fp->f_flags = flags;
>  
> -     // OSv was using a intrusive_ptr which was increasing the refcount
> -     dref(dp);
> +     /*
> +      * Don't need to increase refcount here, we already hold a reference
> +      * to dp from namei().
> +      */
>       // change to std::move once dp is a dentry_ref
>       fp->f_dentry = dp;
>       dp = NULL;
>