linux-kernel - Re: [PATCH] fuse: check attributes staleness on fuse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJfpegsF9iYG04YkA0AOKvsrg0hua3JGw=Phq=qeOurgqk_OuA@mail.gmail.com>
Date: Fri, 15 Nov 2024 12:59:54 +0100
From: Miklos Szeredi <miklos@...redi.hu>
To: Zhang Tianci <zhangtianci.1997@...edance.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	xieyongji@...edance.com, Jiachen Zhang <zhangjiachen.jaycee@...edance.com>
Subject: Re: [PATCH] fuse: check attributes staleness on fuse_iget()

On Thu, 14 Nov 2024 at 08:12, Zhang Tianci
<zhangtianci.1997@...edance.com> wrote:
>
> Function fuse_direntplus_link() might call fuse_iget() to initialize a new
> fuse_inode and change its attributes. If fi->attr_version is always
> initialized with 0, even if the attributes returned by the FUSE_READDIR
> request is staled, as the new fi->attr_version is 0, fuse_change_attributes
> will still set the staled attributes to inode. This wrong behaviour may
> cause file size inconsistency even when there is no changes from
> server-side.

Thanks for working on this.

I have some comments, see below.

> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -173,6 +173,7 @@ static void fuse_evict_inode(struct inode *inode)
>                         fuse_cleanup_submount_lookup(fc, fi->submount_lookup);
>                         fi->submount_lookup = NULL;
>                 }
> +               atomic64_inc(&fc->evict_ctr);

I think this should only be done if (inode->i_nlink > 0), because if
the file/directory was removed, then the race between another lookup
cannot happen.

> @@ -426,7 +427,8 @@ static int fuse_inode_set(struct inode *inode, void *_nodeidp)
>
>  struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>                         int generation, struct fuse_attr *attr,
> -                       u64 attr_valid, u64 attr_version)
> +                       u64 attr_valid, u64 attr_version,
> +                       u64 evict_ctr)
>  {
>         struct inode *inode;
>         struct fuse_inode *fi;
> @@ -488,6 +490,10 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>         spin_unlock(&fi->lock);
>  done:
>         fuse_change_attributes(inode, attr, NULL, attr_valid, attr_version);
> +       spin_lock(&fi->lock);
> +       if (evict_ctr < fuse_get_evict_ctr(fc))
> +               fuse_invalidate_attr(inode);

Similarly, this should not be done on creation (attr_version == 0),
since in case of a brand new inode the previous inode state cannot
have any effect on it.

The other case that may be worth taking special care of is when the
inode is already in the cache.  This happens for example on lookup of
hard links.  This is the (fi->attr_version > 0) case where we can rely
on the validity of attr_version.  I.e. the attr_version comparison in
fuse_change_attributes() will make sure the attributes are only
updated if necessary, no need to check evict_ctr.

So the only case that needs evict_ctr verification is (attr_version !=
0 && fi->attr_version == 0).

One other thing: I don't like the fact that the invalid mask is first
cleared in fuse_change_attributes_common(), then reset in
fuse_invalidate_attr().  It would be cleaner to not clear the mask in
the first place.

Attaching an untested incremental patch.  Can you please review and test?

Thanks,
Miklos

View attachment "fuse-evict-race-incremental.patch" of type "text/x-patch" (5758 bytes)