From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from out01.mta.xmission.com ([166.70.13.231]:40136 "EHLO
	out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752510AbcF3Qin (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Thu, 30 Jun 2016 12:38:43 -0400
From: ebiederm@xmission.com (Eric W. Biederman)
To: Seth Forshee <seth.forshee@canonical.com>
Cc: Michael j Theall <mtheall@us.ibm.com>,
	fuse-devel@lists.sourceforge.net, linux-fsdevel@vger.kernel.org,
	Miklos Szeredi <miklos@szeredi.hu>
References: <20160629190731.GF53123@ubuntu-hedt>
	<OFF8F0F486.DB2CEB73-ON86257FE1.006A1FF4-86257FE1.006A9703@notes.na.collabserv.com>
	<87vb0rhhpr.fsf@x220.int.ebiederm.org>
	<20160630130746.GA123267@ubuntu-hedt>
Date: Thu, 30 Jun 2016 11:25:32 -0500
In-Reply-To: <20160630130746.GA123267@ubuntu-hedt> (Seth Forshee's message of
	"Thu, 30 Jun 2016 08:07:46 -0500")
Message-ID: <871t3efxtv.fsf@x220.int.ebiederm.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

Seth Forshee <seth.forshee@canonical.com> writes:

> On Wed, Jun 29, 2016 at 03:18:24PM -0500, Eric W. Biederman wrote:
>> "Michael j Theall" <mtheall@us.ibm.com> writes:
>> 
>> > Going by the patch I posted a couple of years ago:
>> > https://sourceforge.net/p/fuse/mailman/message/33033653/
>> >
>> > The only hole I see in your patch is that in setattr() you are not
>> > updating the cached acl if the ATTR_MODE is updated. The other major
>> > difference is that my version uses the get_acl/set_acl inode
>> > operations but you use that plus the xattr handlers. I'm not
>> > up-to-speed on the kernel so I'm not sure if you actually need to
>> > implement both.
>> 
>> That makes an interesting question.  Is it desirable to keep
>> inode->i_mode in sync with the posix acls in fuse or should a filesystem
>> that supports posix acls worry about that?
>
> My first blush opinion is that the kernel should take care of this, not
> the filesystems. Then a fuse filesystem which supports xattrs gets acl
> support for free. Otherwise if a filesystem supports xattrs but not acls
> internally, we have no way of knowing that in the kernel and they get
> out of sync.
>
> However if some filesystems are already doing this internally then we
> have redundancy. Presumably this would be harmless aside from the wasted
> effort.

Which means that in set_acl we need to something like:

	if (type == ACL_TYPE_ACCESS) {
        	struct iattr attr;
                attr.ia_valid = ATTR_MODE;
                attr.ia_mode = inode->i_mode;
        	ret = posix_acl_equiv_mode(acl, &attr.ia_mode);
                if (ret < 0)
                	return ret;
                if (ret == 0)
                	acl = NULL;
		if (attr.ia_mode != inode->i_mode) {
			ret = fuse_do_setattr(inode, &attr, NULL);
	                if (ret < 0)
        	        	return ret;
                }
        }

In fuse_setattr should wind up looking something like:

static int fuse_setattr(struct dentry *entry, struct iattr *attr)
{
	struct inode *inode = d_inode(entry);
        int ret;

	if (!fuse_allow_current_process(get_fuse_conn(inode)))
		return -EACCES;

	if (attr->ia_valid & ATTR_FILE)
		ret = fuse_do_setattr(inode, attr, attr->ia_file);
	else
		ret = fuse_do_setattr(inode, attr, NULL);

	if (ret == 0 && attr->ia_valid & ATTR_MODE)
        	ret = posix_acl_chmod(inode, inode->i_mode);
	return ret;
}

That should be enough to keep everything in sync with the existing
fuse protocol.  And then fuse filesystems won't have to care in general
about the contents of acls (unless they choose to care).

Eric