From mboxrd@z Thu Jan 1 00:00:00 1970 From: Seth Forshee Subject: Re: [RFC v3 2/2] fuse: Add posix acl support Date: Sun, 7 Aug 2016 08:51:39 -0500 Message-ID: References: <1470086846-19844-1-git-send-email-seth.forshee@canonical.com> <1470086846-19844-3-git-send-email-seth.forshee@canonical.com> <20160804141159.GA28476@ubuntu-xps13> <87shuiu85b.fsf@x220.int.ebiederm.org> <20160806015250.GA90790@ubuntu-hedt> <20160807034631.GA135007@ubuntu-hedt> <87popkrazt.fsf@x220.int.ebiederm.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4829947793967069955==" Cc: Miklos Szeredi , fuse-devel , Nikolaus Rath , linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "Eric W. Biederman" Return-path: In-Reply-To: <87popkrazt.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: fuse-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org --===============4829947793967069955== Content-Type: multipart/alternative; boundary=001a114ab9c260327c05397b99e6 --001a114ab9c260327c05397b99e6 Content-Type: text/plain; charset=UTF-8 On Aug 7, 2016 8:12 AM, "Eric W. Biederman" wrote: > > Seth Forshee writes: > > > On Sat, Aug 06, 2016 at 11:09:54PM +0200, Miklos Szeredi wrote: > >> On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee < seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote: > >> > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote: > >> > What I'm not convinced of is that the userspace visible changes in > >> > behavior won't break someone's software, even if they aren't really > >> > getting acl enforcement. > >> > >> That's a key point. Backward compatibility is important, and not even > >> hard to do because fuse can negotiate supported features with the > >> userspace filesystem. > >> > >> So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if > >> "default_permissions" is on. > >> > >> If not set in INIT reply just pass all xattrs through to the > >> filesystem. Caching should not be done. Don't think about whether > >> it's logical or not, or if anyone could use it for anything sane. > >> Just do what we are doing currently. Translating uids still makes > >> sense, but that's another story. > > > > Translating uids is actually central to the differing positions that you > > and Eric have. What Eric wants is for the only path for supporting posix > > acls to be via the helpers, that way all concerns about translating uids > > can be addressed there. If fuse is to allow the xattrs to be passed > > directly through to the filesystem then there has to be a second > > mechanism which translates the uids for that case. > > I think I will agree with Miklos. Translating uids is another story and > worst case we can work on that in September when I get back from > vacation. Nothing of what you are talking about will make things worse > for translating uids, so it is perfectly fine to merge posix acl support > into fuse and then handle uid translation. > > Until we set FS_USER_NS fuse it is fine to not get the uid translation. > > Which yields a very simple implementation: > > In fuse_fill_super: > if (!fc->posix_acl) { > sb->s_xattr = fuse_xattr_handlers; > } else { > sb->s_xattr = fuse_acl_xattr_handlers; > } > > Where fuse_acl_xattr_handlers are a different array that > includes the posix acl interrcept (aka the array in patch 1 or the array > in patch 2). > > Then fuse_get_acl and fuse_set_acl can just test fc->posix_acl > and fail if that is not set. > > I think that is all you need to do, and we can worry about the other > details after posix acl support has landed in fuse. Ok. > >> If the flag is set in INIT reply, then that means userspace filesystem > >> wants handling of posix acl permission checking in kernel. It would > >> also mean that caching of posix acl are allowed (lifetime linked to > >> attribute lifetime). > >> > >> If filesystem wants to explicitly disable posix acl support, then it > >> can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*". > >> Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that > >> filesystem can return in reply to FUSE_POSIX_ACL. > >> > >> I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it, > >> just make any such code dependent on CONFIG_FS_POSIX_ACL. > > > > But CONFIG_FS_POSIX_ACL doesn't have an input prompt and thus isn't > > displayed in menuconfig, etc. If that's what you want, fine, but it > > seems like an unusual situation. > > Then in Kconfig I would have FUSE_FS "select FS_POSIX_ACL". That > simplifies the problem. Unless Miklos wants something more granular. Which is the the alternative I offered a couple of emails ago ;-) Thanks, Seth --001a114ab9c260327c05397b99e6 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Aug 7, 2016 8:12 AM, "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:=
>
> Seth Forshee <seth.fo= rshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
>
> > On Sat, Aug 06, 2016 at 11:09:54PM +0200, Miklos Szeredi wrote: > >> On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:=
> >> > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Bieder= man wrote:
> >> > What I'm not convinced of is that the userspace visi= ble changes in
> >> > behavior won't break someone's software, even if= they aren't really
> >> > getting acl enforcement.
> >>
> >> That's a key point.=C2=A0 Backward compatibility is impor= tant, and not even
> >> hard to do because fuse can negotiate supported features with= the
> >> userspace filesystem.
> >>
> >> So we can have a new FUSE_POSIX_ACL feature flag in INIT, sen= t if
> >> "default_permissions" is on.
> >>
> >> If not set in INIT reply just pass all xattrs through to the<= br> > >> filesystem.=C2=A0 Caching should not be done. Don't think= about whether
> >> it's logical or not, or if anyone could use it for anythi= ng sane.
> >> Just do what we are doing currently.=C2=A0 Translating uids s= till makes
> >> sense, but that's another story.
> >
> > Translating uids is actually central to the differing positions t= hat you
> > and Eric have. What Eric wants is for the only path for supportin= g posix
> > acls to be via the helpers, that way all concerns about translati= ng uids
> > can be addressed there. If fuse is to allow the xattrs to be pass= ed
> > directly through to the filesystem then there has to be a second<= br> > > mechanism which translates the uids for that case.
>
> I think I will agree with Miklos.=C2=A0 Translating uids is another st= ory and
> worst case we can work on that in September when I get back from
> vacation.=C2=A0 Nothing of what you are talking about will make things= worse
> for translating uids, so it is perfectly fine to merge posix acl suppo= rt
> into fuse and then handle uid translation.
>
> Until we set FS_USER_NS fuse it is fine to not get the uid translation= .
>
> Which yields a very simple implementation:
>
> In fuse_fill_super:
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!fc->posix_acl) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sb->s_xattr= =3D fuse_xattr_handlers;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sb->s_xattr= =3D fuse_acl_xattr_handlers;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
> Where fuse_acl_xattr_handlers are a different array that
> includes the posix acl interrcept (aka the array in patch 1 or the arr= ay
> in patch 2).
>
> Then fuse_get_acl and fuse_set_acl can just test fc->posix_acl
> and fail if that is not set.
>
> I think that is all you need to do, and we can worry about the other > details after posix acl support has landed in fuse.

Ok.

> >> If the flag is set in INIT reply, then that me= ans userspace filesystem
> >> wants handling of posix acl permission checking in kernel.=C2= =A0 It would
> >> also mean that caching of posix acl are allowed (lifetime lin= ked to
> >> attribute lifetime).
> >>
> >> If filesystem wants to explicitly disable posix acl support, = then it
> >> can reply EOPNOTSUPP to getxattr and setxattr on "system= .posix_acl_*".
> >>=C2=A0 =C2=A0Alternatively we can add a FUSE_NO_POSIX_ACL feat= ure flag, that
> >> filesystem can return in reply to FUSE_POSIX_ACL.
> >>
> >> I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not = worth it,
> >> just make any such code dependent on CONFIG_FS_POSIX_ACL.
> >
> > But CONFIG_FS_POSIX_ACL doesn't have an input prompt and thus= isn't
> > displayed in menuconfig, etc. If that's what you want, fine, = but it
> > seems like an unusual situation.
>
> Then in Kconfig I would have FUSE_FS "select FS_POSIX_ACL".= =C2=A0 That
> simplifies the problem.=C2=A0 Unless Miklos wants something more granu= lar.

Which is the the alternative I offered a couple of emails ag= o ;-)

Thanks,
Seth

--001a114ab9c260327c05397b99e6-- --===============4829947793967069955== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------------ --===============4829947793967069955== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline -- fuse-devel mailing list To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel --===============4829947793967069955==--