| Message ID | 20180302215919.27207-1-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 2, 2018 at 10:59 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > The code has been missing a way for a ->get_acl method to not cache > a return value without risking invalidating a cached value > that was set while get_acl() was returning. > > Add that support by implementing to_uncachable_acl, to_cachable_acl, > is_uncacheable_acl, and dealing with uncachable acls in get_acl(). I don't like the pointer magic here. Can't the uncachable bit just be added to struct posix_acl? AFAICS that can be done even without increasing the size of that struct (e.g. by unioning it with the rcu_head). Thanks, Miklos
Miklos Szeredi <mszeredi@redhat.com> writes: > On Fri, Mar 2, 2018 at 10:59 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> The code has been missing a way for a ->get_acl method to not cache >> a return value without risking invalidating a cached value >> that was set while get_acl() was returning. >> >> Add that support by implementing to_uncachable_acl, to_cachable_acl, >> is_uncacheable_acl, and dealing with uncachable acls in get_acl(). > > I don't like the pointer magic here. Can't the uncachable bit just be > added to struct posix_acl? > > AFAICS that can be done even without increasing the size of that > struct (e.g. by unioning it with the rcu_head). Except that would: - add a possible cache line miss. - make it unusable for overlayfs. I am after very light-weight semantics that say don't cache this return value but don't have any effects elsewhere. We are already playing pointer magic games in this code. This just uses those games for the last piece of information to keep the logic clean. I see two possible implementation alternatives: - Make get_acl return a struct that returns the acl and cachability flag - Add a helper that does"cmpxchg(p, sentinel, ACL_NOT_CACHED)". Such a heleper function seems like a waste, it does side effect magic which is never particularly pleasant, and it is more code to execute in practice. Though honestly it is my second choice. void dont_cache_my_return_acl(struct inode *inode, int type) { /* Valid only inside ->get_acl implementations */ struct posix_acl **p = get_acl_type(inode, type); struct posix_acl *sentinel = uncached_acl_sentinel(current); cmpxchg(p, sentinel, ACL_NOT_CACHED); } EXPORT_SYMBOL(dont_cache_my_return_acl); It is just a few instructions more so I guess it isn't that bad. Especially for something that is not a common case. Do you think you could live with dont_cache_my_return_acl? Otherwise I think I will respin this patch set without the acl unification. There is plenty of evidence what it will look like now. We can deal with the rest of the patches. Then we can come back to exactly what acl unification in fuse should look like. Eric
diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 2fd0fde16fe1..00281bc30854 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -96,12 +96,16 @@ struct posix_acl *get_acl(struct inode *inode, int type) { void *sentinel; struct posix_acl **p; - struct posix_acl *acl; + struct posix_acl *acl, *to_cache; /* * The sentinel is used to detect when another operation like * set_cached_acl() or forget_cached_acl() races with get_acl(). * It is guaranteed that is_uncached_acl(sentinel) is true. + * + * This is sufficient to prevent races between ->set_acl + * calling set_cached_acl (outside of filesystem specific + * locking) and get_acl() caching the returned acl. */ acl = get_cached_acl(inode, type); @@ -126,12 +130,18 @@ 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. + * Normally, the ACL returned by ->get_acl() will be cached. + * + * A filesystem can prevent the acl returned by ->get_acl() + * from being cached by wrapping it with to_uncachable_acl(). * - * If the filesystem doesn't have a get_acl() function at all, we'll - * just create the negative cache entry. + * A filesystem can at anytime effect the cache directly and + * cause in process calls of get_acl() not to update the cache + * by calling forget_cache_acl(inode, type) or + * set_cached_acl(inode, type, acl). + * + * If the filesystem doesn't have a ->get_acl() function at + * all, we'll just create the negative cache entry. */ if (!inode->i_op->get_acl) { set_cached_acl(inode, type, NULL); @@ -140,20 +150,28 @@ struct posix_acl *get_acl(struct inode *inode, int type) acl = inode->i_op->get_acl(inode, type); if (IS_ERR(acl)) { - /* - * Remove our sentinel so that we don't block future attempts - * to cache the ACL. - */ - cmpxchg(p, sentinel, ACL_NOT_CACHED); - return acl; + /* Don't cache an acl just return an error. */ + to_cache = ACL_NOT_CACHED; + } + else if (is_uncacheable_acl(acl)) { + /* Don't cache an acl, but return one. */ + to_cache = ACL_NOT_CACHED; + acl = to_cacheable_acl(acl); + } + else { + /* Cache and return the acl. */ + to_cache = posix_acl_dup(acl); } /* - * Cache the result, but only if our sentinel is still in place. + * Remove the sentinel and replace it with the value to + * cache, but only if the sentinel is still in place. */ - posix_acl_dup(acl); - if (unlikely(cmpxchg(p, sentinel, acl) != sentinel)) - posix_acl_release(acl); + if (unlikely(cmpxchg(p, sentinel, to_cache) != sentinel)) { + if (!is_uncached_acl(to_cache)) + posix_acl_release(to_cache); + } + return acl; } EXPORT_SYMBOL(get_acl); diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h index 540595a321a7..3be8929b9f48 100644 --- a/include/linux/posix_acl.h +++ b/include/linux/posix_acl.h @@ -56,6 +56,23 @@ posix_acl_release(struct posix_acl *acl) kfree_rcu(acl, a_rcu); } +/* + * Allow for acls returned from ->get_acl() to not be cached. + */ +static inline bool is_uncacheable_acl(struct posix_acl *acl) +{ + return ((unsigned long)acl) & 1UL; +} + +static inline struct posix_acl *to_uncacheable_acl(struct posix_acl *acl) +{ + return (struct posix_acl *)(((unsigned long)acl) | 1UL); +} + +static inline struct posix_acl *to_cacheable_acl(struct posix_acl *acl) +{ + return (struct posix_acl *)(((unsigned long)acl) & ~1UL); +} /* posix_acl.c */
The code has been missing a way for a ->get_acl method to not cache a return value without risking invalidating a cached value that was set while get_acl() was returning. Add that support by implementing to_uncachable_acl, to_cachable_acl, is_uncacheable_acl, and dealing with uncachable acls in get_acl(). Update the comments so that they are a little clearer about what is going on in get_acl() Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/posix_acl.c | 50 ++++++++++++++++++++++++++++++++--------------- include/linux/posix_acl.h | 17 ++++++++++++++++ 2 files changed, 51 insertions(+), 16 deletions(-)