linux-kernel - Re: [PATCH v8 1/6] fs/posix_acl: Update the comments and support lightweight cache skipping

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: <87lgf639xx.fsf@xmission.com>
Date:   Mon, 05 Mar 2018 07:53:46 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Miklos Szeredi <mszeredi@...hat.com>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Alban Crequy <alban@...volk.io>,
        Seth Forshee <seth.forshee@...onical.com>,
        Sargun Dhillon <sargun@...gun.me>,
        Dongsu Park <dongsu@...volk.io>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v8 1/6] fs/posix_acl: Update the comments and support lightweight cache skipping

Miklos Szeredi <mszeredi@...hat.com> writes:

> On Fri, Mar 2, 2018 at 10:59 PM, Eric W. Biederman
> <ebiederm@...ssion.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