On Fri, Jul 22, 2011 at 07:34:43PM -0700, Linus Torvalds wrote:
> Comments on this part? It's untested, but it's a very straightforward
> conversion of my previous patch (that I did test).
Looks fine and I'm OK with applying and throwing into the next pull request,
but...
> + * Under RCU walk, we cannot even do a "get_cached_acl()",
> + * because that involves locking and getting a refcount on
> + * a cached ACL.
... why is that a problem? Locking there is mere ->i_lock and getting
a refcount is atomic_inc(). Grabbing a reference might be Not Nice from
the cacheline bouncing POV, but...
IIRC, these suckers (in-core ones) are copy-on-write. If so we should be
reasonably safe here. We obviously have no business trying to get ACL
from fs if it's not cached, but other than that...
Mind you, if they *are* C-O-W, we could do freeing them via RCU and avoid
even bothering with reference...
<checks> oh, my... All callers of posix_acl_chmod_masq() have exactly the
same form:
clone = posix_acl_clone(acl, some_flags);
if (clone)
error = -ENOMEM;
sod off
posix_acl_release(acl);
error = posix_acl_chmod_masq(clone, mode);
if (error)
posix_acl_release(clone);
sod off
do something useful
Sounds like a really dumb API to me...
They are C-O-W, all right. With too low-level helpers exposed...
posix_acl_create_masq(): same story, apparently. Whee... on ocfs2:
clone = posix_acl_clone(acl, GFP_NOFS);
ret = -ENOMEM;
if (!clone)
goto cleanup;
mode = inode->i_mode;
ret = posix_acl_create_masq(clone, &mode);
if (ret >= 0) {
ret2 = ocfs2_acl_set_mode(inode, di_bh, handle, mode);
if (ret2) {
mlog_errno(ret2);
ret = ret2;
goto cleanup;
...
cleanup:
posix_acl_release(acl);
return ret;
Leaks'R'Us...
OK, unless there are serious objections, I
* apply Linus' patch as-is
* fix that ocfs2 leak
* replace posix_acl_..._masq() with saner helpers (take original
acl + other arguments, return modified clone or ERR_PTR) and kill open-coded
instances...
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
