From: ebiederm@xmission.com (Eric W. Biederman)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Miklos Szeredi <mszeredi@redhat.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux Containers <containers@lists.linux-foundation.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Alban Crequy <alban@kinvolk.io>,
Seth Forshee <seth.forshee@canonical.com>,
Sargun Dhillon <sargun@sargun.me>,
Dongsu Park <dongsu@kinvolk.io>,
"Serge E. Hallyn" <serge@hallyn.com>
Subject: Re: [PATCH v7 3/7] fs/posix_acl: Document that get_acl respects ACL_DONT_CACHE
Date: Mon, 26 Feb 2018 21:14:52 -0600 [thread overview]
Message-ID: <87tvu3qg2b.fsf@xmission.com> (raw)
In-Reply-To: <87r2p7rvn5.fsf@xmission.com> (Eric W. Biederman's message of "Mon, 26 Feb 2018 20:53:02 -0600")
ebiederm@xmission.com (Eric W. Biederman) writes:
2> So the purpose for having a patch in the first place is that
> 2a3a2a3f3524 ("ovl: don't cache acl on overlay layer")
> which addded ACL_DONT_CACHED did not result in any comment updates
> to get_acl.
>
> Which mean that if you read the comments in get_acl() that you
> don't even think of ACL_DONT_CACHED.
>
> Which means that this comment:
> /*
> * If the ACL isn't being read yet, set our sentinel. Otherwise, the
> * current value of the ACL will not be ACL_NOT_CACHED and so our own
> * sentinel will not be set; another task will update the cache. We
> * could wait for that other task to complete its job, but it's easier
> * to just call ->get_acl to fetch the ACL ourself. (This is going to
> * be an unlikely race.)
> */
>
> Which presumes the only reason the acl could be anything other
> ACL_NOT_CACHED is because get_acl() is already being called upon it in
> another task.
>
> I wanted something to mention ACL_DONT_CACHED so someone would at least
> think about that case if they ever step up to modify the code.
>
> The code is perfectly clear, the comment is not. That scares me.
>
> And I had to read the code about a dozen times before I realized the
> ACL_DONT_CACHED case even exists. Not useful when I am need to use
> that to preserve historical fuse semantics.
>
> So something is missing here even if my wording does not improve things.
>
>
>
> Then we get this comment:
> /*
> * Normally, the ACL returned by ->get_acl will be cached.
> * A filesystem can prevent that by calling
> * forget_cached_acl(inode, type) in ->get_acl.
> */
>
> Which was added in b8a7a3a66747 ("posix_acl: Inode acl caching fixes")
> That comment is and always has been rubbish.
>
> I don't have a clue what it is trying to say but it is not something
> a person can use to write filesystem code with.
>
>
> Truths:
> - forget_cached_acl(inode, type) can be used to invalidate the acl
> cache.
>
> - Calling forget_cached_acl from within the filesystems ->get_acl
> method won't prevent a cached value from being returend because
> ->get_acl will be set.
>
> - Calling forget_cached_acl from within the filesystems ->get_acl
> method won't prevent a returned value from being cached
> because it the caching happens after ->get_acl returns.
Sigh. Yes it will because we set the special sentinel value,
and forget_cached_acl will replace the sentinel value with
ACL_NOT_CACHED.
It is a terribly brittle and racy thing to do, and it probably won't
work to say cache this acl but not this one on a case by case bases
in ->get_acl.
As such I believe that usage of forget_cached_acl should be subsumed by
using ACL_NOT_CACHED. If not we should really come up with a different
helper function name to call from ->get_acl. Preferably one that does
"cmpxchng(p, sentinel, ACL_NOT_CACHED)" so that we remove the races.
> - Setting inode->i_acl = ACL_DONT_CACHE is the only way to prevent
> a value from ->get_acl from being cached.
>
>
> In summary I only care about two things.
> 1) ACL_NOT_CACHED being mentioned somewhere in get_acl so people looking
> at the code, and people updating the code will have a hint that they
> need to consider that case.
>
> 2) That misleading completely bogus comment being removed/fixed.
>
>
> And yes I agree the code is clear. The comments are not.
>
>
> Does this look better as a comment updating patch?
>
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 2fd0fde16fe1..5453094b8828 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -98,6 +98,11 @@ struct posix_acl *get_acl(struct inode *inode, int type)
> struct posix_acl **p;
> struct posix_acl *acl;
>
> + /*
> + * To avoid caching the result of ->get_acl
> + * set inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE;
> + */
> +
> /*
> * The sentinel is used to detect when another operation like
> * set_cached_acl() or forget_cached_acl() races with get_acl().
> @@ -126,9 +131,7 @@ struct posix_acl *get_acl(struct inode *inode, int type)
> /* fall through */ ;
>
> /*
> - * Normally, the ACL returned by ->get_acl will be cached.
> - * A filesystem can prevent that by calling
> - * forget_cached_acl(inode, type) in ->get_acl.
> + * The ACL returned by ->get_acl will be cached.
> *
> * If the filesystem doesn't have a get_acl() function at all, we'll
> * just create the negative cache entry.
>
> Eric
next prev parent reply other threads:[~2018-02-27 3:15 UTC|newest]
Thread overview: 107+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-22 14:32 [PATCH v5 00/11] FUSE mounts from non-init user namespaces Dongsu Park
2017-12-22 14:32 ` [PATCH 01/11] block_dev: Support checking inode permissions in lookup_bdev() Dongsu Park
2017-12-22 18:59 ` Coly Li
2017-12-23 12:00 ` Dongsu Park
2017-12-23 3:03 ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 02/11] mtd: Check permissions towards mtd block device inode when mounting Dongsu Park
2017-12-22 21:06 ` Richard Weinberger
2017-12-23 12:18 ` Dongsu Park
2017-12-23 12:56 ` Richard Weinberger
2017-12-23 3:05 ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 03/11] fs: Allow superblock owner to change ownership of inodes Dongsu Park
2017-12-23 3:17 ` Serge E. Hallyn
2018-01-05 19:24 ` Luis R. Rodriguez
2018-01-09 15:10 ` Dongsu Park
2018-01-09 17:23 ` Luis R. Rodriguez
2018-02-13 13:18 ` Miklos Szeredi
2018-02-16 22:00 ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 04/11] fs: Don't remove suid for CAP_FSETID for userns root Dongsu Park
2017-12-23 3:26 ` Serge E. Hallyn
2017-12-23 12:38 ` Dongsu Park
2018-02-13 13:37 ` Miklos Szeredi
2017-12-22 14:32 ` [PATCH 05/11] fs: Allow superblock owner to access do_remount_sb() Dongsu Park
2017-12-23 3:30 ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 06/11] capabilities: Allow privileged user in s_user_ns to set security.* xattrs Dongsu Park
2017-12-23 3:33 ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 07/11] fs: Allow CAP_SYS_ADMIN in s_user_ns to freeze and thaw filesystems Dongsu Park
2017-12-23 3:39 ` Serge E. Hallyn
2018-02-14 12:28 ` Miklos Szeredi
2018-02-19 22:56 ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 08/11] fuse: Support fuse filesystems outside of init_user_ns Dongsu Park
2017-12-23 3:46 ` Serge E. Hallyn
2018-01-17 10:59 ` Alban Crequy
2018-01-17 14:29 ` Seth Forshee
2018-01-17 18:56 ` Alban Crequy
2018-01-17 19:31 ` Seth Forshee
2018-01-18 10:29 ` Alban Crequy
2018-02-12 15:57 ` Miklos Szeredi
2018-02-12 16:35 ` Eric W. Biederman
2018-02-13 10:20 ` Miklos Szeredi
2018-02-16 21:52 ` Eric W. Biederman
2018-02-20 2:12 ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 09/11] fuse: Restrict allow_other to the superblock's namespace or a descendant Dongsu Park
2017-12-23 3:50 ` Serge E. Hallyn
2018-02-19 23:16 ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 10/11] fuse: Allow user namespace mounts Dongsu Park
2017-12-23 3:51 ` Serge E. Hallyn
2018-02-14 13:44 ` Miklos Szeredi
2018-02-15 8:46 ` Miklos Szeredi
2017-12-22 14:32 ` [PATCH 11/11] evm: Don't update hmacs in user ns mounts Dongsu Park
2017-12-23 4:03 ` Serge E. Hallyn
2017-12-24 5:12 ` Mimi Zohar
2017-12-24 5:56 ` Mimi Zohar
2017-12-25 7:05 ` [PATCH v5 00/11] FUSE mounts from non-init user namespaces Eric W. Biederman
2018-01-09 15:05 ` Dongsu Park
2018-01-18 14:58 ` Alban Crequy
2018-02-19 23:09 ` Eric W. Biederman
2018-02-13 11:32 ` Miklos Szeredi
2018-02-16 21:53 ` Eric W. Biederman
2018-02-21 20:24 ` [PATCH v6 0/6] fuse: " Eric W. Biederman
2018-02-21 20:29 ` [PATCH v6 1/5] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-02-22 10:13 ` Miklos Szeredi
2018-02-22 19:04 ` Eric W. Biederman
2018-02-21 20:29 ` [PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-02-22 10:26 ` Miklos Szeredi
2018-02-22 18:15 ` Eric W. Biederman
2018-02-21 20:29 ` [PATCH v6 3/5] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-02-21 20:29 ` [PATCH v6 4/5] fuse: Ensure posix acls are translated " Eric W. Biederman
2018-02-22 11:40 ` Miklos Szeredi
2018-02-22 19:18 ` Eric W. Biederman
2018-02-22 22:50 ` Eric W. Biederman
2018-02-26 7:47 ` Miklos Szeredi
2018-02-26 16:35 ` Eric W. Biederman
2018-02-26 21:51 ` Eric W. Biederman
2018-02-21 20:29 ` [PATCH v6 5/5] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-02-26 23:52 ` [PATCH v7 0/7] fuse: mounts from non-init user namespaces Eric W. Biederman
2018-02-26 23:52 ` [PATCH v7 1/7] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-02-26 23:52 ` [PATCH v7 2/7] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-02-26 23:52 ` [PATCH v7 3/7] fs/posix_acl: Document that get_acl respects ACL_DONT_CACHE Eric W. Biederman
2018-02-27 1:13 ` Linus Torvalds
2018-02-27 2:53 ` Eric W. Biederman
2018-02-27 3:14 ` Eric W. Biederman [this message]
2018-02-27 3:41 ` Linus Torvalds
2018-03-02 19:53 ` [RFC][PATCH] fs/posix_acl: Update the comments and support lightweight cache skipping Eric W. Biederman
2018-02-27 3:36 ` [PATCH v7 3/7] fs/posix_acl: Document that get_acl respects ACL_DONT_CACHE Linus Torvalds
2018-02-26 23:52 ` [PATCH v7 4/7] fuse: Cache a NULL acl when FUSE_GETXATTR returns -ENOSYS Eric W. Biederman
2018-02-26 23:53 ` [PATCH v7 5/7] fuse: Simplfiy the posix acl handling logic Eric W. Biederman
2018-02-27 9:00 ` Miklos Szeredi
2018-03-02 21:49 ` Eric W. Biederman
2018-02-26 23:53 ` [PATCH v7 6/7] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-02-26 23:53 ` [PATCH v7 7/7] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-03-02 21:58 ` [PATCH v8 0/6] fuse: mounts from non-init user namespaces Eric W. Biederman
2018-03-02 21:59 ` [PATCH v8 1/6] fs/posix_acl: Update the comments and support lightweight cache skipping Eric W. Biederman
2018-03-05 9:53 ` Miklos Szeredi
2018-03-05 13:53 ` Eric W. Biederman
2018-03-02 21:59 ` [PATCH v8 2/6] fuse: Simplfiy the posix acl handling logic Eric W. Biederman
2018-03-02 21:59 ` [PATCH v8 3/6] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-03-02 21:59 ` [PATCH v8 4/6] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-03-02 21:59 ` [PATCH v8 5/6] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-03-02 21:59 ` [PATCH v8 6/6] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-03-08 21:23 ` [PATCH v9 0/4] fuse: mounts from non-init user namespaces Eric W. Biederman
2018-03-08 21:24 ` [PATCH v9 1/4] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-03-08 21:24 ` [PATCH v9 2/4] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-03-08 21:24 ` [PATCH v9 3/4] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-03-08 21:24 ` [PATCH v9 4/4] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-03-20 16:25 ` [PATCH v9 0/4] fuse: mounts from non-init user namespaces Miklos Szeredi
2018-03-20 18:27 ` Eric W. Biederman
2018-03-21 8:38 ` Miklos Szeredi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tvu3qg2b.fsf@xmission.com \
--to=ebiederm@xmission.com \
--cc=alban@kinvolk.io \
--cc=containers@lists.linux-foundation.org \
--cc=dongsu@kinvolk.io \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mszeredi@redhat.com \
--cc=sargun@sargun.me \
--cc=serge@hallyn.com \
--cc=seth.forshee@canonical.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).